Use of NDEBUG define in "pico/types.h" seems to be the wrong way round #255

New issue

Closed

opened 2021-03-13 17:31:36 -06:00 by bpinball · 6 comments

bpinball commented

2021-03-13 17:31:36 -06:00

(Migrated from github.com)

The use of the NDEBUG (Not DEBUG) compiler definition in pico/types.h seems to be the wrong way round. E.g. in the code in types.h:

#ifndef NDEBUG
typedef uint64_t absolute_time_t;
#else
typedef struct {
    uint64_t _private_us_since_boot;
} absolute_time_t;
#endif

the first section defined as not NDEBUG (i.e. is debug code) seems to be code that should be used in release builds, and the NDEBUG section (marked as not debug code) is the code that should be used in debug builds to detect misuse of the time types.

This causes the release build usage of time/clock functions to run more slowly than in debug builds, as well as causing the incorrect usage of the absolute_time_t type to be detected only in a release build.

On a side note, the overhead of having the time functions have to dereference a structure (when this issue is fixed) in debug code is considerable e.g. in tight audio PWM loops.

The use of the NDEBUG (Not DEBUG) compiler definition in pico/types.h seems to be the wrong way round. E.g. in the code in types.h: ``` #ifndef NDEBUG typedef uint64_t absolute_time_t; #else typedef struct { uint64_t _private_us_since_boot; } absolute_time_t; #endif ``` the first section defined as not NDEBUG (i.e. is debug code) seems to be code that should be used in release builds, and the NDEBUG section (marked as not debug code) is the code that should be used in debug builds to detect misuse of the time types. This causes the release build usage of time/clock functions to run more slowly than in debug builds, as well as causing the incorrect usage of the absolute_time_t type to be detected only in a release build. On a side note, the overhead of having the time functions have to dereference a structure (when this issue is fixed) in debug code is considerable e.g. in tight audio PWM loops.

lurch commented

2021-03-13 18:48:16 -06:00

(Migrated from github.com)

Hmmm, that header file also says "It is equivalent to uint64_t in release builds." so maybe that implies that all the #ifndef NDEBUG lines in that file ought to be #ifdef NDEBUG ? 🤷‍♂️ 🤷

Hmmm, that [header file](https://github.com/raspberrypi/pico-sdk/blob/develop/src/common/pico_base/include/pico/types.h) also says "It is equivalent to uint64_t in release builds." so _maybe_ that implies that all the `#ifndef NDEBUG` lines in that file ought to be `#ifdef NDEBUG` ? :man_shrugging: :shrug:

kilograham commented

2021-03-13 21:01:26 -06:00

(Migrated from github.com)

oops; that is backwards...

on a side note, why on earth would you do PWM audio in a timing loop; there are perfectly good timer paced DMAs :-)

oops; that is backwards... on a side note, why on earth would you do PWM audio in a timing loop; there are perfectly good timer paced DMAs :-)

bpinball commented

2021-03-14 07:07:56 -05:00

(Migrated from github.com)

Thanks everyone. I know it's the gcc way, but that double negative of #ifndef NDEBUG confused me for a while. Don't worry Graham, it's just experimental PWM code, honest! Will refactor to a DMA based solution.

Thanks everyone. I know it's the gcc way, but that double negative of `#ifndef NDEBUG` confused me for a while. Don't worry Graham, it's just experimental PWM code, honest! Will refactor to a DMA based solution.

lurch commented

2021-03-14 07:42:34 -05:00

(Migrated from github.com)

@kilograham This simple patch results in a clean Release build

diff --git a/src/common/pico_base/include/pico/types.h b/src/common/pico_base/include/pico/types.h
index 51debf2..6b8e66f 100644
--- a/src/common/pico_base/include/pico/types.h
+++ b/src/common/pico_base/include/pico/types.h
@@ -25,7 +25,7 @@ typedef unsigned int uint;
     \see update_us_since_boot()
     \ingroup timestamp
 */
-#ifndef NDEBUG
+#ifdef NDEBUG
 typedef uint64_t absolute_time_t;
 #else
 typedef struct {
@@ -40,7 +40,7 @@ typedef struct {
  * \ingroup timestamp
  */
 static inline uint64_t to_us_since_boot(absolute_time_t t) {
-#ifndef NDEBUG
+#ifdef NDEBUG
     return t;
 #else
     return t._private_us_since_boot;
@@ -55,7 +55,7 @@ static inline uint64_t to_us_since_boot(absolute_time_t t) {
  * \ingroup timestamp
  */
 static inline void update_us_since_boot(absolute_time_t *t, uint64_t us_since_boot) {
-#ifndef NDEBUG
+#ifdef NDEBUG
     *t = us_since_boot;
 #else
     assert(us_since_boot <= INT64_MAX);
@@ -63,7 +63,7 @@ static inline void update_us_since_boot(absolute_time_t *t, uint64_t us_since_bo
 #endif
 }
 
-#ifndef NDEBUG
+#ifdef NDEBUG
 #define ABSOLUTE_TIME_INITIALIZED_VAR(name, value) name = value
 #else
 #define ABSOLUTE_TIME_INITIALIZED_VAR(name, value) name = {value}

but results in a large swath of warnings when doing a Debug build:

[ 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/pico-sdk.tmp/src/common/pico_stdlib/include/pico/stdlib.h:12:0,
                 from /home/andrew/github/raspberrypi/pico-sdk.tmp/test/pico_time_test/pico_time_test.c:11:
/home/andrew/github/raspberrypi/pico-sdk.tmp/test/pico_time_test/pico_time_test.c: In function 'main':
/home/andrew/github/raspberrypi/pico-sdk.tmp/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/pico-sdk.tmp/test/pico_time_test/pico_time_test.c:86:21: note: called from here
     absolute_time_t time_base = get_absolute_time();
                     ^~~~~~~~~
In file included from /home/andrew/github/raspberrypi/pico-sdk.tmp/src/common/pico_stdlib/include/pico/stdlib.h:12:0,
                 from /home/andrew/github/raspberrypi/pico-sdk.tmp/test/pico_time_test/pico_time_test.c:11:
/home/andrew/github/raspberrypi/pico-sdk.tmp/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/pico-sdk.tmp/test/pico_time_test/pico_time_test.c:12:0:
/home/andrew/github/raspberrypi/pico-sdk.tmp/test/pico_time_test/pico_time_test.c:105: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/pico-sdk.tmp/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/pico-sdk.tmp/src/common/pico_stdlib/include/pico/stdlib.h:12:0,
                 from /home/andrew/github/raspberrypi/pico-sdk.tmp/test/pico_time_test/pico_time_test.c:11:
/home/andrew/github/raspberrypi/pico-sdk.tmp/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/pico-sdk.tmp/test/pico_time_test/pico_time_test.c:143:21: note: called from here
     absolute_time_t time_base = get_absolute_time();
                     ^~~~~~~~~
In file included from /home/andrew/github/raspberrypi/pico-sdk.tmp/src/common/pico_stdlib/include/pico/stdlib.h:12:0,
                 from /home/andrew/github/raspberrypi/pico-sdk.tmp/test/pico_time_test/pico_time_test.c:11:
/home/andrew/github/raspberrypi/pico-sdk.tmp/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/pico-sdk.tmp/test/pico_time_test/pico_time_test.c:12:0:
/home/andrew/github/raspberrypi/pico-sdk.tmp/test/pico_time_test/pico_time_test.c:210: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/pico-sdk.tmp/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/pico-sdk.tmp/src/common/pico_stdlib/include/pico/stdlib.h:12:0,
                 from /home/andrew/github/raspberrypi/pico-sdk.tmp/test/pico_time_test/pico_time_test.c:11:
/home/andrew/github/raspberrypi/pico-sdk.tmp/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/pico-sdk.tmp/test/pico_time_test/pico_time_test.c:12:0:
/home/andrew/github/raspberrypi/pico-sdk.tmp/test/pico_time_test/pico_time_test.c:211: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/pico-sdk.tmp/test/pico_test/include/pico/test.h:31:45: note: in definition of macro 'PICOTEST_CHECK'
 #define PICOTEST_CHECK(COND, MESSAGE) if (!(COND)) {                                               \
                                             ^~~~

@kilograham This simple patch results in a clean `Release` build ```patch diff --git a/src/common/pico_base/include/pico/types.h b/src/common/pico_base/include/pico/types.h index 51debf2..6b8e66f 100644 --- a/src/common/pico_base/include/pico/types.h +++ b/src/common/pico_base/include/pico/types.h @@ -25,7 +25,7 @@ typedef unsigned int uint; \see update_us_since_boot() \ingroup timestamp */ -#ifndef NDEBUG +#ifdef NDEBUG typedef uint64_t absolute_time_t; #else typedef struct { @@ -40,7 +40,7 @@ typedef struct { * \ingroup timestamp */ static inline uint64_t to_us_since_boot(absolute_time_t t) { -#ifndef NDEBUG +#ifdef NDEBUG return t; #else return t._private_us_since_boot; @@ -55,7 +55,7 @@ static inline uint64_t to_us_since_boot(absolute_time_t t) { * \ingroup timestamp */ static inline void update_us_since_boot(absolute_time_t *t, uint64_t us_since_boot) { -#ifndef NDEBUG +#ifdef NDEBUG *t = us_since_boot; #else assert(us_since_boot <= INT64_MAX); @@ -63,7 +63,7 @@ static inline void update_us_since_boot(absolute_time_t *t, uint64_t us_since_bo #endif } -#ifndef NDEBUG +#ifdef NDEBUG #define ABSOLUTE_TIME_INITIALIZED_VAR(name, value) name = value #else #define ABSOLUTE_TIME_INITIALIZED_VAR(name, value) name = {value} ``` but results in a large swath of warnings when doing a `Debug` build: ``` [ 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/pico-sdk.tmp/src/common/pico_stdlib/include/pico/stdlib.h:12:0, from /home/andrew/github/raspberrypi/pico-sdk.tmp/test/pico_time_test/pico_time_test.c:11: /home/andrew/github/raspberrypi/pico-sdk.tmp/test/pico_time_test/pico_time_test.c: In function 'main': /home/andrew/github/raspberrypi/pico-sdk.tmp/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/pico-sdk.tmp/test/pico_time_test/pico_time_test.c:86:21: note: called from here absolute_time_t time_base = get_absolute_time(); ^~~~~~~~~ In file included from /home/andrew/github/raspberrypi/pico-sdk.tmp/src/common/pico_stdlib/include/pico/stdlib.h:12:0, from /home/andrew/github/raspberrypi/pico-sdk.tmp/test/pico_time_test/pico_time_test.c:11: /home/andrew/github/raspberrypi/pico-sdk.tmp/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/pico-sdk.tmp/test/pico_time_test/pico_time_test.c:12:0: /home/andrew/github/raspberrypi/pico-sdk.tmp/test/pico_time_test/pico_time_test.c:105: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/pico-sdk.tmp/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/pico-sdk.tmp/src/common/pico_stdlib/include/pico/stdlib.h:12:0, from /home/andrew/github/raspberrypi/pico-sdk.tmp/test/pico_time_test/pico_time_test.c:11: /home/andrew/github/raspberrypi/pico-sdk.tmp/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/pico-sdk.tmp/test/pico_time_test/pico_time_test.c:143:21: note: called from here absolute_time_t time_base = get_absolute_time(); ^~~~~~~~~ In file included from /home/andrew/github/raspberrypi/pico-sdk.tmp/src/common/pico_stdlib/include/pico/stdlib.h:12:0, from /home/andrew/github/raspberrypi/pico-sdk.tmp/test/pico_time_test/pico_time_test.c:11: /home/andrew/github/raspberrypi/pico-sdk.tmp/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/pico-sdk.tmp/test/pico_time_test/pico_time_test.c:12:0: /home/andrew/github/raspberrypi/pico-sdk.tmp/test/pico_time_test/pico_time_test.c:210: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/pico-sdk.tmp/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/pico-sdk.tmp/src/common/pico_stdlib/include/pico/stdlib.h:12:0, from /home/andrew/github/raspberrypi/pico-sdk.tmp/test/pico_time_test/pico_time_test.c:11: /home/andrew/github/raspberrypi/pico-sdk.tmp/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/pico-sdk.tmp/test/pico_time_test/pico_time_test.c:12:0: /home/andrew/github/raspberrypi/pico-sdk.tmp/test/pico_time_test/pico_time_test.c:211: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/pico-sdk.tmp/test/pico_test/include/pico/test.h:31:45: note: in definition of macro 'PICOTEST_CHECK' #define PICOTEST_CHECK(COND, MESSAGE) if (!(COND)) { \ ^~~~ ```

kilograham commented

2021-03-14 11:46:08 -05:00

(Migrated from github.com)

Thanks everyone. I know it's the gcc way, but that double negative of #ifndef NDEBUG confused me for a while. Don't worry Graham, it's just experimental PWM code, honest! Will refactor to a DMA based solution.

Ha ha! Btw thanks for the good catch.. that was wasting precious time :-)

> Thanks everyone. I know it's the gcc way, but that double negative of `#ifndef NDEBUG` confused me for a while. Don't worry Graham, it's just experimental PWM code, honest! Will refactor to a DMA based solution. Ha ha! Btw thanks for the good catch.. that was wasting precious time :-)

Wren6991 commented

2021-03-17 13:06:42 -05:00

(Migrated from github.com)