diff mbox series

features.h: Fix -Wundef problems

Message ID 20251129220736.377130-1-stefan.tauner@gmx.at
State New
Headers
Series features.h: Fix -Wundef problems |

Commit Message

Stefan Tauner Nov. 29, 2025, 10:07 p.m. UTC 
  -Wundef warns if an undefined identifier is evaluated in an #if
directive. This would be valid as they are replaced with 0. However, it
is often an early warning sign and not intentional. To allow for
enabling -Wundef even outside system directories (where compilers
ignore such problems unless -Wsystem-headers is enabled) this patch
adds the required defined() checks.

glibc also has been supporting this for 10 years now:
  https://sourceware.org/git/?p=glibc.git;a=commitdiff;h=f248238cf43bd751db29e6f151d6da7645337ff5

I have not exhaustively tested this but you can see the effect with
something like the following (+ using -stdc= and/or -D...):
  echo | gcc -include newlib/libc/include/sys/features.h -Inewlib/libc/include/ -E - -Wundef

Signed-off-by: Stefan Tauner <stefan.tauner@gmx.at>
---
 newlib/libc/include/sys/features.h | 51 +++++++++++++++++-------------
 1 file changed, 29 insertions(+), 22 deletions(-)

Comments

Corinna Vinschen Dec. 1, 2025, 3:49 p.m. UTC | #1 
Hi Stefan,

On Nov 29 23:07, Stefan Tauner wrote:
> -Wundef warns if an undefined identifier is evaluated in an #if
> directive. This would be valid as they are replaced with 0. However, it
> is often an early warning sign and not intentional. To allow for
> enabling -Wundef even outside system directories (where compilers
> ignore such problems unless -Wsystem-headers is enabled) this patch
> adds the required defined() checks.
> 
> glibc also has been supporting this for 10 years now:
>   https://sourceware.org/git/?p=glibc.git;a=commitdiff;h=f248238cf43bd751db29e6f151d6da7645337ff5
> 
> I have not exhaustively tested this but you can see the effect with
> something like the following (+ using -stdc= and/or -D...):
>   echo | gcc -include newlib/libc/include/sys/features.h -Inewlib/libc/include/ -E - -Wundef
> 
> Signed-off-by: Stefan Tauner <stefan.tauner@gmx.at>
> ---
>  newlib/libc/include/sys/features.h | 51 +++++++++++++++++-------------
>  1 file changed, 29 insertions(+), 22 deletions(-)
> 
> diff --git a/newlib/libc/include/sys/features.h b/newlib/libc/include/sys/features.h
> index f0f5286ac..9dbdd5739 100644
> --- a/newlib/libc/include/sys/features.h
> +++ b/newlib/libc/include/sys/features.h
> @@ -156,15 +156,15 @@ extern "C" {
>  
>  #if !defined(_POSIX_SOURCE) && !defined(_POSIX_C_SOURCE) && \
>    ((!defined(__STRICT_ANSI__) && !defined(_ANSI_SOURCE)) || \
> -   (_XOPEN_SOURCE - 0) >= 500)
> +  (defined(_XOPEN_SOURCE) && _XOPEN_SOURCE >= 500))
>  #define	_POSIX_SOURCE		1
> -#if !defined(_XOPEN_SOURCE) || (_XOPEN_SOURCE - 0) >= 700
> +#if !defined(_XOPEN_SOURCE) || _XOPEN_SOURCE >= 700

GLibc adds `defined', but retains the `(MACRO - 0)' expression, even
today, 10 years later.  I didn't test it, but shouldn't we do the same?


Corinna
Stefan Tauner Dec. 3, 2025, 2:48 a.m. UTC | #2 
On Mon, 1 Dec 2025 16:49:45 +0100
Corinna Vinschen <vinschen@redhat.com> wrote:

> GLibc adds `defined', but retains the `(MACRO - 0)' expression, even
> today, 10 years later.  I didn't test it, but shouldn't we do the same?

Thanks for bringing this up. Initially I thought that this idiom is
just for very old compilers that do not replace undefined macros with
0. However, the real reason seems to be to allow for comparing macros
that *are* defined but without value. In such cases (MACRO - 0) decays
to -0 which still can be used as operand in a comparison while just
MACRO would decay to nil leading to syntax errors.

Since defining some of the macros in question without value is actually
legal and useful the first version of my patch breaks those. This is
for example the case for -D_XOPEN_SOURCE= (NB: the = is important.
Without it gcc defines the macro to 1(!)).

I also found some embarrassing copy and paste mistakes that I also
fixed.

Additionally, I have used the idiom for the usages of _FORTIFY_SOURCE
and __OPTIMIZE__ that did not have used it previously. __OPTIMIZE__ is
set by the compilers if optimization is enabled. gcc and clang set it
to 1 in that case regardless of the optimization level (including -Os
and -Og). The GCC documentation explicitly states that it is defined
with a value of 1.
_FORTIFY_SOURCE on the other hand is set by the user. GCC documentation
defines the legal values 1 to 3, inclusive. The current code would bail
out for "-D_FORTIFY_SOURCE=".
So arguably the idiom is overkill for those two but OTOH I don't see
any disadvantage either so I left them in for now.

V2 is attached.
Corinna Vinschen Dec. 4, 2025, 10:10 a.m. UTC | #3 
On Dec  3 03:48, Stefan Tauner wrote:
> [...]
> From 4667f7d3f13cfc051db048efbf1636ecc25d2532 Mon Sep 17 00:00:00 2001
> From: Stefan Tauner <stefan.tauner@gmx.at>
> Date: Sat, 29 Nov 2025 22:52:26 +0100
> Subject: [PATCH] features.h: Fix -Wundef problems
> 
> -Wundef warns if an undefined identifier is evaluated in an #if
> directive. This would be valid as they are replaced with 0. However, it
> is often an early warning sign and not intentional. To allow for
> enabling -Wundef even outside system directories (where compilers
> ignore such problems unless -Wsystem-headers is enabled) this patch
> adds the required defined() checks.
> 
> glibc also has been supporting this for 10 years now:
>   https://sourceware.org/git/?p=glibc.git;a=commitdiff;h=f248238cf43bd751db29e6f151d6da7645337ff5
> 
> I have not exhaustively tested this but you can see the effect with
> something like the following (+ using -stdc= and/or -D...):
>   echo | gcc -include newlib/libc/include/sys/features.h -Inewlib/libc/include/ -E - -Wundef
> 
> Signed-off-by: Stefan Tauner <stefan.tauner@gmx.at>
> ---
>  newlib/libc/include/sys/features.h | 49 +++++++++++++++++-------------
>  1 file changed, 28 insertions(+), 21 deletions(-)

Pushed.

Thanks,
Corinna
diff mbox series

Patch

diff --git a/newlib/libc/include/sys/features.h b/newlib/libc/include/sys/features.h
index f0f5286ac..9dbdd5739 100644
--- a/newlib/libc/include/sys/features.h
+++ b/newlib/libc/include/sys/features.h
@@ -156,15 +156,15 @@  extern "C" {
 
 #if !defined(_POSIX_SOURCE) && !defined(_POSIX_C_SOURCE) && \
   ((!defined(__STRICT_ANSI__) && !defined(_ANSI_SOURCE)) || \
-   (_XOPEN_SOURCE - 0) >= 500)
+  (defined(_XOPEN_SOURCE) && _XOPEN_SOURCE >= 500))
 #define	_POSIX_SOURCE		1
-#if !defined(_XOPEN_SOURCE) || (_XOPEN_SOURCE - 0) >= 700
+#if !defined(_XOPEN_SOURCE) || _XOPEN_SOURCE >= 700
 #define	_POSIX_C_SOURCE		200809L
-#elif (_XOPEN_SOURCE - 0) >= 600
+#elif defined(_XOPEN_SOURCE) && _XOPEN_SOURCE >= 600
 #define	_POSIX_C_SOURCE		200112L
-#elif (_XOPEN_SOURCE - 0) >= 500
+#elif defined(_XOPEN_SOURCE) && _XOPEN_SOURCE >= 500
 #define	_POSIX_C_SOURCE		199506L
-#elif (_XOPEN_SOURCE - 0) < 500
+#elif defined(_XOPEN_SOURCE) && _XOPEN_SOURCE < 500
 #define	_POSIX_C_SOURCE		2
 #endif
 #endif
@@ -286,19 +286,24 @@  extern "C" {
 #endif
 
 #if defined(_ISOC23_SOURCE) || \
-  (__STDC_VERSION__ - 0) > 201710L || (__cplusplus - 0) >= 202002L
+    (defined(__STDC_VERSION__) && __STDC_VERSION__ > 201710L) || \
+    (defined(__cplusplus) && __cplusplus >= 202002L)
 #define __ISO_C_VISIBLE		2020
 #elif defined(_ISOC11_SOURCE) || \
-  (__STDC_VERSION__ - 0) >= 201112L || (__cplusplus - 0) >= 201103L
+    (defined(__STDC_VERSION__) && __STDC_VERSION__ > 201112L) || \
+    (defined(__cplusplus) && __cplusplus >= 201103L)
 #define	__ISO_C_VISIBLE		2011
-#elif defined(_ISOC99_SOURCE) || (_POSIX_C_SOURCE - 0) >= 200112L || \
-  (__STDC_VERSION__ - 0) >= 199901L || defined(__cplusplus)
+#elif defined(_ISOC99_SOURCE) || \
+    (defined(_POSIX_C_SOURCE) && _POSIX_C_SOURCE >= 200112L) || \
+    (defined(__STDC_VERSION__) && __STDC_VERSION__ > 199901L) || \
+    defined(__cplusplus)
 #define	__ISO_C_VISIBLE		1999
 #else
 #define	__ISO_C_VISIBLE		1990
 #endif
 
-#if defined(_LARGEFILE_SOURCE) || (_XOPEN_SOURCE - 0) >= 500
+#if defined(_LARGEFILE_SOURCE) || \
+    (defined(_XOPEN_SOURCE) && _XOPEN_SOURCE >= 500)
 #define	__LARGEFILE_VISIBLE	1
 #else
 #define	__LARGEFILE_VISIBLE	0
@@ -310,17 +315,18 @@  extern "C" {
 #define	__MISC_VISIBLE		0
 #endif
 
-#if (_POSIX_C_SOURCE - 0) >= 202405L
+#if defined(_POSIX_C_SOURCE) && _POSIX_C_SOURCE >= 202405L
 #define	__POSIX_VISIBLE		202405
-#elif (_POSIX_C_SOURCE - 0) >= 200809L
+#elif defined(_POSIX_C_SOURCE) && _POSIX_C_SOURCE >= 200809L
 #define	__POSIX_VISIBLE		200809
-#elif (_POSIX_C_SOURCE - 0) >= 200112L
+#elif defined(_POSIX_C_SOURCE) && _POSIX_C_SOURCE >= 200112L
 #define	__POSIX_VISIBLE		200112
-#elif (_POSIX_C_SOURCE - 0) >= 199506L
+#elif defined(_POSIX_C_SOURCE) && _POSIX_C_SOURCE >= 199506L
 #define	__POSIX_VISIBLE		199506
-#elif (_POSIX_C_SOURCE - 0) >= 199309L
+#elif defined(_POSIX_C_SOURCE) && _POSIX_C_SOURCE >= 199309L
 #define	__POSIX_VISIBLE		199309
-#elif (_POSIX_C_SOURCE - 0) >= 2 || defined(_XOPEN_SOURCE)
+#elif (defined(_POSIX_C_SOURCE) && _POSIX_C_SOURCE >= 2) || \
+    defined(_XOPEN_SOURCE)
 #define	__POSIX_VISIBLE		199209
 #elif defined(_POSIX_SOURCE) || defined(_POSIX_C_SOURCE)
 #define	__POSIX_VISIBLE		199009
@@ -334,11 +340,11 @@  extern "C" {
 #define	__SVID_VISIBLE		0
 #endif
 
-#if (_XOPEN_SOURCE - 0) >= 700
+#if defined(_XOPEN_SOURCE) && _XOPEN_SOURCE >= 700
 #define	__XSI_VISIBLE		700
-#elif (_XOPEN_SOURCE - 0) >= 600
+#elif defined(_XOPEN_SOURCE) && _XOPEN_SOURCE >= 600
 #define	__XSI_VISIBLE		600
-#elif (_XOPEN_SOURCE - 0) >= 500
+#elif defined(_XOPEN_SOURCE) && _XOPEN_SOURCE >= 500
 #define	__XSI_VISIBLE		500
 #elif defined(_XOPEN_SOURCE) && defined(_XOPEN_SOURCE_EXTENDED)
 #define	__XSI_VISIBLE		4
@@ -348,9 +354,10 @@  extern "C" {
 #define	__XSI_VISIBLE		0
 #endif
 
-#if _FORTIFY_SOURCE > 0 && !defined(__cplusplus) && !defined(__lint__) && \
-   (__OPTIMIZE__ > 0 || defined(__clang__)) && __GNUC_PREREQ__(4, 1) && \
-   !defined(_LIBC)
+#if defined(_FORTIFY_SOURCE) && _FORTIFY_SOURCE > 0 && \
+    !defined(__cplusplus) && !defined(__lint__) && \
+   (defined(__OPTIMIZE__) && __OPTIMIZE__ > 0 || defined(__clang__)) && \
+   __GNUC_PREREQ__(4, 1) && !defined(_LIBC)
 #  if _FORTIFY_SOURCE > 2 && defined(__has_builtin)
 #    if __has_builtin(__builtin_dynamic_object_size)
 #      define __SSP_FORTIFY_LEVEL 3