Use of #if DEBUG in reent.h

Use of #if DEBUG in reent.h

[Joe Seymour]

Users can enable assertions in newlib/libc/include/sys/reent.h by
defining the macro DEBUG. As DEBUG is a relatively obvious and general
purpose name, it is also used by other projects. I've seen it used by
the Atmel Software Framework (ASF) and some TI Energia projects.  A web
search finds a few more examples:

https://forum.pjrc.com/threads/19916-reent-h-space-problems-or-just-my-pc
http://forum.arduino.cc/index.php?topic=151655.0

Actually, these are all examples of projects that have been seen to fail
to build with the following error:

> include\sys\reent.h:458:10: error: #if with no expression

This occurs when DEBUG is defined but has no value. Which seems to
happen with the following test case:

> #define DEBUG
> #include <sys/reent.h>

Notably, I don't see an error if I remove the #define and instead pass
-DDEBUG (with no value) to my gcc build [msp430-elf-gcc (GCC) 7.0.0
20161220].

My initial reaction to seeing the error message was that this was a bug
in reent.h, which should use #ifdef DEBUG instead. Unfortunately if you
do that then "#define DEBUG 0" and -DDEBUG=0 result in debug being
turned on, which seems undesirable. This leads me to believe that the
use of #if DEBUG is deliberate and not considered a bug?

I mention this here because I couldn't find any previous discussion of
it on this list, and as it has been discussed elsewhere I think there's
value in having a definitive statement about it here.

Again for the record, I'll ask whether "#if DEBUG" could be changed to
"#if _SYS_REENT_H_DEBUG", or similar? That would avoid the error and the
naming conflict.  I'm assuming that the examples listed above aren't
trying to turn newlib assertions on. I suspect that at the very least
DEBUG has been used for long enough that there's a stronger argument for
keeping it the same than for changing it to something else?

Thanks,

Joe Seymour

Re: Use of #if DEBUG in reent.h

[Freddie Chopin]

Hi!

Maybe it would be ok to change the offending line to:

#if defined(DEBUG) && DEBUG != 0

This way the old behaviour should be retained in most of the cases.
From what I've read, if you add "-DDEBUG" to GCC flags, it is
equivalent to "-DDEBUG=1". I also think that any undefined macro in a
"#if whatever" statement is considered to be 0, but this is also only a
GCC extensions.

Regards,
FCh

Re: Use of #if DEBUG in reent.h

[Eric Blake]

[-- Attachment #1.1: Type: text/plain, Size: 1503 bytes --]

On 01/04/2017 04:14 AM, Freddie Chopin wrote:
> Hi!
> 
> Maybe it would be ok to change the offending line to:
> 
> #if defined(DEBUG) && DEBUG != 0
> 
> This way the old behaviour should be retained in most of the cases.
>From what I've read, if you add "-DDEBUG" to GCC flags, it is
> equivalent to "-DDEBUG=1".

Not by my experience.  There's a difference to being defined to the
empty string (-DDEBUG on the command line corresponds to '#define DEBUG'
in source) vs. an explicit string (-DDEBUG=1 corresponds to '#define
DEBUG 1').

' I also think that any undefined macro in a
> "#if whatever" statement is considered to be 0, but this is also only a
> GCC extensions.

No, it is required by the C standard.  Any otherwise-unknown
preprocessor token is required to be treated as 0 in arithmetic context.
 However, based on your choice of warning flags, gcc can treat such
usage as a preprocessor error.  Therefore, it is indeed safest to check
whether a macro is defined before coercing it into an arithmetic value,
when writing headers that must be robust to various choices of compiler
warning flags.  Gcc also has a pragma for marking a particular header as
a system header (which, among other things, silences warnings about
constructs in that header that would normally provoke warnings, such as
the implicit use of an undefined macro name meaning 0).

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: Use of #if DEBUG in reent.h

[Christian Groessler]

On 01/04/17 15:05, Eric Blake wrote:
> On 01/04/2017 04:14 AM, Freddie Chopin wrote:
>> Hi!
>>
>> Maybe it would be ok to change the offending line to:
>>
>> #if defined(DEBUG) && DEBUG != 0
>>
>> This way the old behaviour should be retained in most of the cases.
>> >From what I've read, if you add "-DDEBUG" to GCC flags, it is
>> equivalent to "-DDEBUG=1".
> Not by my experience.  There's a difference to being defined to the
> empty string (-DDEBUG on the command line corresponds to '#define DEBUG'
> in source) vs. an explicit string (-DDEBUG=1 corresponds to '#define
> DEBUG 1').


No.

$ gcc -dM -E - < /dev/null  | grep DEBUG
$ gcc -DDEBUG -dM -E - < /dev/null  | grep DEBUG
#define DEBUG 1
$


regards,
chris

Re: Use of #if DEBUG in reent.h

[Corinna Vinschen]

[-- Attachment #1: Type: text/plain, Size: 2132 bytes --]

Jeff?

Thanks,
Corinna

On Jan  3 16:41, Joe Seymour wrote:
> Users can enable assertions in newlib/libc/include/sys/reent.h by
> defining the macro DEBUG. As DEBUG is a relatively obvious and general
> purpose name, it is also used by other projects. I've seen it used by
> the Atmel Software Framework (ASF) and some TI Energia projects.  A web
> search finds a few more examples:
> 
> https://forum.pjrc.com/threads/19916-reent-h-space-problems-or-just-my-pc
> http://forum.arduino.cc/index.php?topic=151655.0
> 
> Actually, these are all examples of projects that have been seen to fail
> to build with the following error:
> 
> > include\sys\reent.h:458:10: error: #if with no expression
> 
> This occurs when DEBUG is defined but has no value. Which seems to
> happen with the following test case:
> 
> > #define DEBUG
> > #include <sys/reent.h>
> 
> Notably, I don't see an error if I remove the #define and instead pass
> -DDEBUG (with no value) to my gcc build [msp430-elf-gcc (GCC) 7.0.0
> 20161220].
> 
> My initial reaction to seeing the error message was that this was a bug
> in reent.h, which should use #ifdef DEBUG instead. Unfortunately if you
> do that then "#define DEBUG 0" and -DDEBUG=0 result in debug being
> turned on, which seems undesirable. This leads me to believe that the
> use of #if DEBUG is deliberate and not considered a bug?
> 
> I mention this here because I couldn't find any previous discussion of
> it on this list, and as it has been discussed elsewhere I think there's
> value in having a definitive statement about it here.
> 
> Again for the record, I'll ask whether "#if DEBUG" could be changed to
> "#if _SYS_REENT_H_DEBUG", or similar? That would avoid the error and the
> naming conflict.  I'm assuming that the examples listed above aren't
> trying to turn newlib assertions on. I suspect that at the very least
> DEBUG has been used for long enough that there's a stronger argument for
> keeping it the same than for changing it to something else?
> 
> Thanks,
> 
> Joe Seymour

-- 
Corinna Vinschen
Cygwin Maintainer
Red Hat

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: Use of #if DEBUG in reent.h

[Jeff Johnston]

Hmm, I appear to have put that code in ages ago (2008) because it originally had asserts in
place by default from code changes in 2002.  Not sure anyone ever used this.

I think the choice of DEBUG was unfortunate and non-standard so, yes,
I think renaming it makes sense with at least an under-score.  The flag was
meant to be a switch, opposite to NDEBUG to turn on use of the asserts so
#ifdef should be used.  I will post a patch shortly.

-- Jeff J.

----- Original Message -----
> Jeff?
> 
> Thanks,
> Corinna
> 
> On Jan  3 16:41, Joe Seymour wrote:
> > Users can enable assertions in newlib/libc/include/sys/reent.h by
> > defining the macro DEBUG. As DEBUG is a relatively obvious and general
> > purpose name, it is also used by other projects. I've seen it used by
> > the Atmel Software Framework (ASF) and some TI Energia projects.  A web
> > search finds a few more examples:
> > 
> > https://forum.pjrc.com/threads/19916-reent-h-space-problems-or-just-my-pc
> > http://forum.arduino.cc/index.php?topic=151655.0
> > 
> > Actually, these are all examples of projects that have been seen to fail
> > to build with the following error:
> > 
> > > include\sys\reent.h:458:10: error: #if with no expression
> > 
> > This occurs when DEBUG is defined but has no value. Which seems to
> > happen with the following test case:
> > 
> > > #define DEBUG
> > > #include <sys/reent.h>
> > 
> > Notably, I don't see an error if I remove the #define and instead pass
> > -DDEBUG (with no value) to my gcc build [msp430-elf-gcc (GCC) 7.0.0
> > 20161220].
> > 
> > My initial reaction to seeing the error message was that this was a bug
> > in reent.h, which should use #ifdef DEBUG instead. Unfortunately if you
> > do that then "#define DEBUG 0" and -DDEBUG=0 result in debug being
> > turned on, which seems undesirable. This leads me to believe that the
> > use of #if DEBUG is deliberate and not considered a bug?
> > 
> > I mention this here because I couldn't find any previous discussion of
> > it on this list, and as it has been discussed elsewhere I think there's
> > value in having a definitive statement about it here.
> > 
> > Again for the record, I'll ask whether "#if DEBUG" could be changed to
> > "#if _SYS_REENT_H_DEBUG", or similar? That would avoid the error and the
> > naming conflict.  I'm assuming that the examples listed above aren't
> > trying to turn newlib assertions on. I suspect that at the very least
> > DEBUG has been used for long enough that there's a stronger argument for
> > keeping it the same than for changing it to something else?
> > 
> > Thanks,
> > 
> > Joe Seymour
> 
> --
> Corinna Vinschen
> Cygwin Maintainer
> Red Hat
> 

Re: Use of #if DEBUG in reent.h

[Jeff Johnston]

[-- Attachment #1: Type: text/plain, Size: 2998 bytes --]

Patch attached.  I will check in if no objections.  It was only used for
debugging the _REENT_CHECK macros so I renamed it _REENT_CHECK_DEBUG.

-- Jeff J.

----- Original Message -----
> Hmm, I appear to have put that code in ages ago (2008) because it originally
> had asserts in
> place by default from code changes in 2002.  Not sure anyone ever used this.
> 
> I think the choice of DEBUG was unfortunate and non-standard so, yes,
> I think renaming it makes sense with at least an under-score.  The flag was
> meant to be a switch, opposite to NDEBUG to turn on use of the asserts so
> #ifdef should be used.  I will post a patch shortly.
> 
> -- Jeff J.
> 
> ----- Original Message -----
> > Jeff?
> > 
> > Thanks,
> > Corinna
> > 
> > On Jan  3 16:41, Joe Seymour wrote:
> > > Users can enable assertions in newlib/libc/include/sys/reent.h by
> > > defining the macro DEBUG. As DEBUG is a relatively obvious and general
> > > purpose name, it is also used by other projects. I've seen it used by
> > > the Atmel Software Framework (ASF) and some TI Energia projects.  A web
> > > search finds a few more examples:
> > > 
> > > https://forum.pjrc.com/threads/19916-reent-h-space-problems-or-just-my-pc
> > > http://forum.arduino.cc/index.php?topic=151655.0
> > > 
> > > Actually, these are all examples of projects that have been seen to fail
> > > to build with the following error:
> > > 
> > > > include\sys\reent.h:458:10: error: #if with no expression
> > > 
> > > This occurs when DEBUG is defined but has no value. Which seems to
> > > happen with the following test case:
> > > 
> > > > #define DEBUG
> > > > #include <sys/reent.h>
> > > 
> > > Notably, I don't see an error if I remove the #define and instead pass
> > > -DDEBUG (with no value) to my gcc build [msp430-elf-gcc (GCC) 7.0.0
> > > 20161220].
> > > 
> > > My initial reaction to seeing the error message was that this was a bug
> > > in reent.h, which should use #ifdef DEBUG instead. Unfortunately if you
> > > do that then "#define DEBUG 0" and -DDEBUG=0 result in debug being
> > > turned on, which seems undesirable. This leads me to believe that the
> > > use of #if DEBUG is deliberate and not considered a bug?
> > > 
> > > I mention this here because I couldn't find any previous discussion of
> > > it on this list, and as it has been discussed elsewhere I think there's
> > > value in having a definitive statement about it here.
> > > 
> > > Again for the record, I'll ask whether "#if DEBUG" could be changed to
> > > "#if _SYS_REENT_H_DEBUG", or similar? That would avoid the error and the
> > > naming conflict.  I'm assuming that the examples listed above aren't
> > > trying to turn newlib assertions on. I suspect that at the very least
> > > DEBUG has been used for long enough that there's a stronger argument for
> > > keeping it the same than for changing it to something else?
> > > 
> > > Thanks,
> > > 
> > > Joe Seymour
> > 
> > --
> > Corinna Vinschen
> > Cygwin Maintainer
> > Red Hat
> > 
> 

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Fix-sys-reent.h-to-remove-use-of-DEBUG-flag.patch --]
[-- Type: text/x-patch; name=0001-Fix-sys-reent.h-to-remove-use-of-DEBUG-flag.patch, Size: 1091 bytes --]

From 81b770557b3390a4cac631a9662e56a553b97305 Mon Sep 17 00:00:00 2001
From: Jeff Johnston <jjohnstn@redhat.com>
Date: Mon, 9 Jan 2017 18:21:19 -0500
Subject: [PATCH] Fix sys/reent.h to remove use of DEBUG flag.

- use of DEBUG flag is non-standard and interferes with other
  project's using same flag
- change to be _REENT_CHECK_DEBUG which means the flag is
  allowing debugging of _REENT_CHECK macros
- use #ifdef instead of #if
---
 newlib/libc/include/sys/reent.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/newlib/libc/include/sys/reent.h b/newlib/libc/include/sys/reent.h
index e1ed8b4..8b67889 100644
--- a/newlib/libc/include/sys/reent.h
+++ b/newlib/libc/include/sys/reent.h
@@ -454,8 +454,8 @@ extern const struct __sFILE_fake __sf_fake_stderr;
     (var)->_stderr = (__FILE *)&__sf_fake_stderr; \
   }
 
-/* Only built the assert() calls if we are built with debugging.  */
-#if DEBUG
+/* Only add assert() calls if we are specified to debug.  */
+#ifdef _REENT_CHECK_DEBUG
 #include <assert.h>
 #define __reent_assert(x) assert(x)
 #else
-- 
2.5.5

Re: Use of #if DEBUG in reent.h

[Corinna Vinschen]

[-- Attachment #1: Type: text/plain, Size: 284 bytes --]

On Jan  9 18:25, Jeff Johnston wrote:
> Patch attached.  I will check in if no objections.  It was only used for
> debugging the _REENT_CHECK macros so I renamed it _REENT_CHECK_DEBUG.

Looks good to me.


Thanks,
Corinna

-- 
Corinna Vinschen
Cygwin Maintainer
Red Hat

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: Use of #if DEBUG in reent.h

[Jeff Johnston]

Ok, done.

-- Jeff J.

----- Original Message -----
> On Jan  9 18:25, Jeff Johnston wrote:
> > Patch attached.  I will check in if no objections.  It was only used for
> > debugging the _REENT_CHECK macros so I renamed it _REENT_CHECK_DEBUG.
> 
> Looks good to me.
> 
> 
> Thanks,
> Corinna
> 
> --
> Corinna Vinschen
> Cygwin Maintainer
> Red Hat
>