Compiler warnings checks and fixes #316

kilograham · 2021-04-06T22:01:59Z

Make kitchen_sink check param assertions, and include all headers - fix sign-compare warnings

…ix sign-compare warnings

lurch · 2021-04-06T22:20:41Z

src/rp2_common/hardware_pwm/include/hardware/pwm.h

+    valid_params_if(PWM, mode == PWM_DIV_FREE_RUNNING ||
+            mode == PWM_DIV_B_RISING ||
+            mode == PWM_DIV_B_HIGH ||
+            mode == PWM_DIV_B_FALLING);


a) it probably makes sense to list these in the same order they're listed in the enum? (i.e. swap round PWM_DIV_B_RISING and PWM_DIV_B_HIGH)

b) as this is a multi-line chunk of code repeated in both pwm_config_set_clkdiv_mode and pwm_set_clkdiv_mode it's probably worth factoring it out to an(other) inline function?

a) don't care enough to change it.
b) thought about it at the time, but decided against because it is non functional code... and i was just working around a compiler limitation

lurch · 2021-04-06T22:23:38Z

test/kitchen_sink/CMakeLists.txt


 target_compile_definitions(kitchen_sink_libs INTERFACE
-        NDEBUG
+        PARAM_ASSERTIONS_ENABLE_ALL=1 # want to check all the assertions for compilation warnings


Haha, I'd never noticed that this file is manually specifying NDEBUG! That explains why I couldn't get the compiler to throw errors at me when I deliberately put syntax-errors inside valid_params_if / invalid_params_if macros 🤣 (even when I was building in Debug mode and had enabled PARAM_ASSERTIONS_ENABLE_ALL)

yup; i have no idea why that was like that :-)

lurch · 2021-04-06T22:26:02Z

test/kitchen_sink/kitchen_sink.c

 #include <stdio.h>
-#include "pico/stdlib.h"
-#include "pico/time.h"
+// Include all headers to check for compiler warnings


Is it worth having a build-time script do this automatically, as it's likely that additional headers will get added to the SDK in future?

Partially added in #319 🙂

lurch · 2021-04-06T22:34:27Z

BTW I'm fairly sure I've mentioned it before, but when I build with -DCMAKE_BUILD_TYPE=Debug I get:

[ 10%] Building C object test/pico_time_test/CMakeFiles/pico_time_test.dir/pico_time_test.c.obj
In file included from /home/andrew/github/raspberrypi/databooks/pico-sdk/src/common/pico_stdlib/include/pico/stdlib.h:12:0,
                 from /home/andrew/github/raspberrypi/databooks/pico-sdk/test/pico_time_test/pico_time_test.c:11:
/home/andrew/github/raspberrypi/databooks/pico-sdk/test/pico_time_test/pico_time_test.c: In function 'main':
/home/andrew/github/raspberrypi/databooks/pico-sdk/src/common/pico_time/include/pico/time.h:61:31: warning: inlining failed in call to 'get_absolute_time': call is unlikely and code size would grow [-Winline]
 static inline absolute_time_t get_absolute_time(void) {
                               ^~~~~~~~~~~~~~~~~
/home/andrew/github/raspberrypi/databooks/pico-sdk/test/pico_time_test/pico_time_test.c:87:21: note: called from here
     absolute_time_t time_base = get_absolute_time();
                     ^~~~~~~~~
In file included from /home/andrew/github/raspberrypi/databooks/pico-sdk/src/common/pico_stdlib/include/pico/stdlib.h:12:0,
                 from /home/andrew/github/raspberrypi/databooks/pico-sdk/test/pico_time_test/pico_time_test.c:11:
/home/andrew/github/raspberrypi/databooks/pico-sdk/src/common/pico_time/include/pico/time.h:61:31: warning: inlining failed in call to 'get_absolute_time': call is unlikely and code size would grow [-Winline]
 static inline absolute_time_t get_absolute_time(void) {
                               ^~~~~~~~~~~~~~~~~
In file included from /home/andrew/github/raspberrypi/databooks/pico-sdk/test/pico_time_test/pico_time_test.c:12:0:
/home/andrew/github/raspberrypi/databooks/pico-sdk/test/pico_time_test/pico_time_test.c:106:20: note: called from here
     PICOTEST_CHECK(absolute_time_diff_us(time_base, get_absolute_time()) < init_ms * 1000, "This is a flaky test :-(");
                    ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/home/andrew/github/raspberrypi/databooks/pico-sdk/test/pico_test/include/pico/test.h:31:45: note: in definition of macro 'PICOTEST_CHECK'
 #define PICOTEST_CHECK(COND, MESSAGE) if (!(COND)) {                                               \
                                             ^~~~
In file included from /home/andrew/github/raspberrypi/databooks/pico-sdk/src/common/pico_stdlib/include/pico/stdlib.h:12:0,
                 from /home/andrew/github/raspberrypi/databooks/pico-sdk/test/pico_time_test/pico_time_test.c:11:
/home/andrew/github/raspberrypi/databooks/pico-sdk/src/common/pico_time/include/pico/time.h:61:31: warning: inlining failed in call to 'get_absolute_time': call is unlikely and code size would grow [-Winline]
 static inline absolute_time_t get_absolute_time(void) {
                               ^~~~~~~~~~~~~~~~~
/home/andrew/github/raspberrypi/databooks/pico-sdk/test/pico_time_test/pico_time_test.c:144:21: note: called from here
     absolute_time_t time_base = get_absolute_time();
                     ^~~~~~~~~
In file included from /home/andrew/github/raspberrypi/databooks/pico-sdk/src/common/pico_stdlib/include/pico/stdlib.h:12:0,
                 from /home/andrew/github/raspberrypi/databooks/pico-sdk/test/pico_time_test/pico_time_test.c:11:
/home/andrew/github/raspberrypi/databooks/pico-sdk/src/common/pico_time/include/pico/time.h:61:31: warning: inlining failed in call to 'get_absolute_time': call is unlikely and code size would grow [-Winline]
 static inline absolute_time_t get_absolute_time(void) {
                               ^~~~~~~~~~~~~~~~~
In file included from /home/andrew/github/raspberrypi/databooks/pico-sdk/test/pico_time_test/pico_time_test.c:12:0:
/home/andrew/github/raspberrypi/databooks/pico-sdk/test/pico_time_test/pico_time_test.c:211:20: note: called from here
     PICOTEST_CHECK(absolute_time_diff_us(at_the_end_of_time, get_absolute_time()) < 0, "now should be before the end of time")
                    ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/home/andrew/github/raspberrypi/databooks/pico-sdk/test/pico_test/include/pico/test.h:31:45: note: in definition of macro 'PICOTEST_CHECK'
 #define PICOTEST_CHECK(COND, MESSAGE) if (!(COND)) {                                               \
                                             ^~~~
In file included from /home/andrew/github/raspberrypi/databooks/pico-sdk/src/common/pico_stdlib/include/pico/stdlib.h:12:0,
                 from /home/andrew/github/raspberrypi/databooks/pico-sdk/test/pico_time_test/pico_time_test.c:11:
/home/andrew/github/raspberrypi/databooks/pico-sdk/src/common/pico_time/include/pico/time.h:61:31: warning: inlining failed in call to 'get_absolute_time': call is unlikely and code size would grow [-Winline]
 static inline absolute_time_t get_absolute_time(void) {
                               ^~~~~~~~~~~~~~~~~
In file included from /home/andrew/github/raspberrypi/databooks/pico-sdk/test/pico_time_test/pico_time_test.c:12:0:
/home/andrew/github/raspberrypi/databooks/pico-sdk/test/pico_time_test/pico_time_test.c:212:20: note: called from here
     PICOTEST_CHECK(absolute_time_diff_us(get_absolute_time(), at_the_end_of_time) > 0, "the end of time should be after now")
                    ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/home/andrew/github/raspberrypi/databooks/pico-sdk/test/pico_test/include/pico/test.h:31:45: note: in definition of macro 'PICOTEST_CHECK'
 #define PICOTEST_CHECK(COND, MESSAGE) if (!(COND)) {                                               \
                                             ^~~~

Is that something it'd be possible to squeeze a fix for into 1.1.2 ?

lurch

Approving, in case Graham wants to merge it as-is without addressing my comments.

kilograham · 2021-04-07T01:49:40Z

BTW I'm fairly sure I've mentioned it before, but when I build with -DCMAKE_BUILD_TYPE=Debug I get:

[ 10%] Building C object test/pico_time_test/CMakeFiles/pico_time_test.dir/pico_time_test.c.obj
In file included from /home/andrew/github/raspberrypi/databooks/pico-sdk/src/common/pico_stdlib/include/pico/stdlib.h:12:0,
                 from /home/andrew/github/raspberrypi/databooks/pico-sdk/test/pico_time_test/pico_time_test.c:11:
/home/andrew/github/raspberrypi/databooks/pico-sdk/test/pico_time_test/pico_time_test.c: In function 'main':
/home/andrew/github/raspberrypi/databooks/pico-sdk/src/common/pico_time/include/pico/time.h:61:31: warning: inlining failed in call to 'get_absolute_time': call is unlikely and code size would grow [-Winline]
 static inline absolute_time_t get_absolute_time(void) {
                               ^~~~~~~~~~~~~~~~~
/home/andrew/github/raspberrypi/databooks/pico-sdk/test/pico_time_test/pico_time_test.c:87:21: note: called from here
     absolute_time_t time_base = get_absolute_time();
                     ^~~~~~~~~
In file included from /home/andrew/github/raspberrypi/databooks/pico-sdk/src/common/pico_stdlib/include/pico/stdlib.h:12:0,
                 from /home/andrew/github/raspberrypi/databooks/pico-sdk/test/pico_time_test/pico_time_test.c:11:
/home/andrew/github/raspberrypi/databooks/pico-sdk/src/common/pico_time/include/pico/time.h:61:31: warning: inlining failed in call to 'get_absolute_time': call is unlikely and code size would grow [-Winline]
 static inline absolute_time_t get_absolute_time(void) {
                               ^~~~~~~~~~~~~~~~~
In file included from /home/andrew/github/raspberrypi/databooks/pico-sdk/test/pico_time_test/pico_time_test.c:12:0:
/home/andrew/github/raspberrypi/databooks/pico-sdk/test/pico_time_test/pico_time_test.c:106:20: note: called from here
     PICOTEST_CHECK(absolute_time_diff_us(time_base, get_absolute_time()) < init_ms * 1000, "This is a flaky test :-(");
                    ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/home/andrew/github/raspberrypi/databooks/pico-sdk/test/pico_test/include/pico/test.h:31:45: note: in definition of macro 'PICOTEST_CHECK'
 #define PICOTEST_CHECK(COND, MESSAGE) if (!(COND)) {                                               \
                                             ^~~~
In file included from /home/andrew/github/raspberrypi/databooks/pico-sdk/src/common/pico_stdlib/include/pico/stdlib.h:12:0,
                 from /home/andrew/github/raspberrypi/databooks/pico-sdk/test/pico_time_test/pico_time_test.c:11:
/home/andrew/github/raspberrypi/databooks/pico-sdk/src/common/pico_time/include/pico/time.h:61:31: warning: inlining failed in call to 'get_absolute_time': call is unlikely and code size would grow [-Winline]
 static inline absolute_time_t get_absolute_time(void) {
                               ^~~~~~~~~~~~~~~~~
/home/andrew/github/raspberrypi/databooks/pico-sdk/test/pico_time_test/pico_time_test.c:144:21: note: called from here
     absolute_time_t time_base = get_absolute_time();
                     ^~~~~~~~~
In file included from /home/andrew/github/raspberrypi/databooks/pico-sdk/src/common/pico_stdlib/include/pico/stdlib.h:12:0,
                 from /home/andrew/github/raspberrypi/databooks/pico-sdk/test/pico_time_test/pico_time_test.c:11:
/home/andrew/github/raspberrypi/databooks/pico-sdk/src/common/pico_time/include/pico/time.h:61:31: warning: inlining failed in call to 'get_absolute_time': call is unlikely and code size would grow [-Winline]
 static inline absolute_time_t get_absolute_time(void) {
                               ^~~~~~~~~~~~~~~~~
In file included from /home/andrew/github/raspberrypi/databooks/pico-sdk/test/pico_time_test/pico_time_test.c:12:0:
/home/andrew/github/raspberrypi/databooks/pico-sdk/test/pico_time_test/pico_time_test.c:211:20: note: called from here
     PICOTEST_CHECK(absolute_time_diff_us(at_the_end_of_time, get_absolute_time()) < 0, "now should be before the end of time")
                    ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/home/andrew/github/raspberrypi/databooks/pico-sdk/test/pico_test/include/pico/test.h:31:45: note: in definition of macro 'PICOTEST_CHECK'
 #define PICOTEST_CHECK(COND, MESSAGE) if (!(COND)) {                                               \
                                             ^~~~
In file included from /home/andrew/github/raspberrypi/databooks/pico-sdk/src/common/pico_stdlib/include/pico/stdlib.h:12:0,
                 from /home/andrew/github/raspberrypi/databooks/pico-sdk/test/pico_time_test/pico_time_test.c:11:
/home/andrew/github/raspberrypi/databooks/pico-sdk/src/common/pico_time/include/pico/time.h:61:31: warning: inlining failed in call to 'get_absolute_time': call is unlikely and code size would grow [-Winline]
 static inline absolute_time_t get_absolute_time(void) {
                               ^~~~~~~~~~~~~~~~~
In file included from /home/andrew/github/raspberrypi/databooks/pico-sdk/test/pico_time_test/pico_time_test.c:12:0:
/home/andrew/github/raspberrypi/databooks/pico-sdk/test/pico_time_test/pico_time_test.c:212:20: note: called from here
     PICOTEST_CHECK(absolute_time_diff_us(get_absolute_time(), at_the_end_of_time) > 0, "the end of time should be after now")
                    ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/home/andrew/github/raspberrypi/databooks/pico-sdk/test/pico_test/include/pico/test.h:31:45: note: in definition of macro 'PICOTEST_CHECK'
 #define PICOTEST_CHECK(COND, MESSAGE) if (!(COND)) {                                               \
                                             ^~~~

Is that something it'd be possible to squeeze a fix for into 1.1.2 ?

nothing much i can do about that - it only happens on older gcc ( I think <8, <=8?))

lurch · 2021-04-07T06:44:24Z

nothing much i can do about that

Fair enough.

it only happens on older gcc ( I think <8, <=8?))

I just checked, and with gcc 10.2.1 it actually complains (with a similar error) about more parts 😉

/home/andrew/github/raspberrypi/databooks/pico-sdk/src/rp2_common/hardware_dma/include/hardware/dma.h:288:34: warning: inlining failed in call to 'dma_channel_get_default_config': call is unlikely and code size would grow [-Winline]
  288 | static inline dma_channel_config dma_channel_get_default_config(uint channel) {
      |                                  ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~


/home/andrew/github/raspberrypi/databooks/pico-sdk/src/rp2_common/pico_bootrom/include/pico/bootrom.h:82:46: warning: inlining failed in call to 'reset_usb_boot': call is unlikely and code size would grow [-Winline]
   82 | static inline void __attribute__((noreturn)) reset_usb_boot(uint32_t usb_activity_gpio_pin_mask,
      |                                              ^~~~~~~~~~~~~~


/home/andrew/github/raspberrypi/databooks/pico-sdk/src/rp2_common/hardware_pwm/include/hardware/pwm.h:209:26: warning: inlining failed in call to 'pwm_get_default_config': call is unlikely and code size would grow [-Winline]
  209 | static inline pwm_config pwm_get_default_config(void) {
      |                          ^~~~~~~~~~~~~~~~~~~~~~

Make kitchen_sink check param assertions, and include all headers - f…

0f7b1c1

…ix sign-compare warnings

kilograham requested a review from lurch April 6, 2021 22:01

lurch reviewed Apr 6, 2021

View reviewed changes

lurch approved these changes Apr 6, 2021

View reviewed changes

kilograham merged commit 92f948c into develop-1.1.2 Apr 7, 2021

kilograham deleted the fix-sign-compare branch April 7, 2021 01:50

kilograham added this to the 1.1.2 milestone Apr 9, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Compiler warnings checks and fixes #316

Compiler warnings checks and fixes #316

Uh oh!

kilograham commented Apr 6, 2021

Uh oh!

lurch Apr 6, 2021

Uh oh!

kilograham Apr 7, 2021

Uh oh!

lurch Apr 6, 2021 •

edited

Loading

Uh oh!

kilograham Apr 7, 2021

Uh oh!

lurch Apr 6, 2021

Uh oh!

lurch Apr 7, 2021

Uh oh!

lurch commented Apr 6, 2021

Uh oh!

lurch left a comment

Uh oh!

kilograham commented Apr 7, 2021

Uh oh!

lurch commented Apr 7, 2021

Uh oh!

Uh oh!

Compiler warnings checks and fixes #316

Compiler warnings checks and fixes #316

Uh oh!

Conversation

kilograham commented Apr 6, 2021

Uh oh!

lurch Apr 6, 2021

Choose a reason for hiding this comment

Uh oh!

kilograham Apr 7, 2021

Choose a reason for hiding this comment

Uh oh!

lurch Apr 6, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kilograham Apr 7, 2021

Choose a reason for hiding this comment

Uh oh!

lurch Apr 6, 2021

Choose a reason for hiding this comment

Uh oh!

lurch Apr 7, 2021

Choose a reason for hiding this comment

Uh oh!

lurch commented Apr 6, 2021

Uh oh!

lurch left a comment

Choose a reason for hiding this comment

Uh oh!

kilograham commented Apr 7, 2021

Uh oh!

lurch commented Apr 7, 2021

Uh oh!

Uh oh!

lurch Apr 6, 2021 •

edited

Loading