[PATCH] Fix undefined behavior in OS defines

[Stefan Tauner]

Found by clang (warning introduced in r258128), reported by Idwer.

Signed-off-by: Stefan Tauner 
---
 platform.h | 22 ++++++++++++++++++----
 1 file changed, 18 insertions(+), 4 deletions(-)

diff --git a/platform.h b/platform.h
index c5a52ef..b58c303 100644
--- a/platform.h
+++ b/platform.h
@@ -24,10 +24,24 @@
 #ifndef __PLATFORM_H__
 #define __PLATFORM_H__ 1

-// Helper defines for operating systems
-#define IS_LINUX	(defined(__gnu_linux__) || defined(__linux__))
-#define IS_MACOSX	(defined(__APPLE__) && defined(__MACH__)) /* yes, both. */
-#define IS_WINDOWS	(defined(_WIN32) || defined(_WIN64) || defined(__WIN32__) || defined(__WINDOWS__))
+/* Helper defines for operating systems.
+ * This may seem complicated but the following would be undefined behavior:
+ * #define IS_LINUX	(defined(__gnu_linux__) || defined(__linux__)) */
+#if defined(__gnu_linux__) || defined(__linux__)
+	#define IS_LINUX (1)
+#else
+	#define IS_LINUX (0)
+#endif
+#if defined(__APPLE__) && defined(__MACH__) /* yes, both. */
+	#define IS_MACOSX (1)
+#else
+	#define IS_MACOSX (0)
+#endif
+#if defined(_WIN32) || defined(_WIN64) || defined(__WIN32__) || defined(__WINDOWS__)
+	#define IS_WINDOWS (1)
+#else
+	#define IS_WINDOWS (0)
+#endif

// Likewise for target architectures
 #if defined (__i386__) || defined (__x86_64__) || defined(__amd64__)

[PATCH] Fix undefined behavior in some preprocessor define checks

[Stefan Tauner]

The macro below (as used it was used previously) would produce
undefined behavior when used as expression in an
#if preprocessor directive:
#define IS_LINUX (defined(__gnu_linux__) || defined(__linux__))

This patch replaces all such statements with a more verbose but
well-defined #if/#define x 1/#else/#define x 0/#endif

Found by clang (warning introduced in r258128), reported by Idwer.

Signed-off-by: Stefan Tauner 
---
 hwaccess.c | 18 +++++++++++++++---
 platform.h | 19 +++++++++++++++----
 2 files changed, 30 insertions(+), 7 deletions(-)

diff --git a/hwaccess.c b/hwaccess.c
index 1901ee6..80852e7 100644
--- a/hwaccess.c
+++ b/hwaccess.c
@@ -37,9 +37,21 @@
 #error "Unknown operating system"
 #endif

-#define USE_IOPL	(IS_LINUX || IS_MACOSX || defined(__NetBSD__) || defined(__OpenBSD__))
-#define USE_DEV_IO	(defined(__FreeBSD__) || defined(__FreeBSD_kernel__) || defined(__DragonFly__))
-#define USE_IOPERM	(defined(__gnu_hurd__))
+#if (IS_LINUX || IS_MACOSX || defined(__NetBSD__) || defined(__OpenBSD__))
+	#define USE_IOPL (1)
+#else
+	#define USE_IOPL (0)
+#endif
+#if (defined(__FreeBSD__) || defined(__FreeBSD_kernel__) || defined(__DragonFly__))
+	#define USE_DEV_IO (1)
+#else
+	#define USE_DEV_IO (0)
+#endif
+#if (defined(__gnu_hurd__))
+	#define USE_IOPERM (1)
+#else
+	#define USE_IOPERM (0)
+#endif

#if USE_IOPERM
 #include <sys/io.h>
diff --git a/platform.h b/platform.h
index c5a52ef..2785753 100644
--- a/platform.h
+++ b/platform.h
@@ -24,10 +24,21 @@
 #ifndef __PLATFORM_H__
 #define __PLATFORM_H__ 1

-// Helper defines for operating systems
-#define IS_LINUX	(defined(__gnu_linux__) || defined(__linux__))
-#define IS_MACOSX	(defined(__APPLE__) && defined(__MACH__)) /* yes, both. */
-#define IS_WINDOWS	(defined(_WIN32) || defined(_WIN64) || defined(__WIN32__) || defined(__WINDOWS__))
+#if (defined(__gnu_linux__) || defined(__linux__))
+	#define IS_LINUX (1)
+#else
+	#define IS_LINUX (0)
+#endif
+#if (defined(__APPLE__) && defined(__MACH__)) /* yes, both. */
+	#define IS_MACOSX (1)
+#else
+	#define IS_MACOSX (0)
+#endif
+#if (defined(_WIN32) || defined(_WIN64) || defined(__WIN32__) || defined(__WINDOWS__))
+	#define IS_WINDOWS (1)
+#else
+	#define IS_WINDOWS (0)
+#endif

// Likewise for target architectures
 #if defined (__i386__) || defined (__x86_64__) || defined(__amd64__)

[PATCH] Fix undefined behavior in some preprocessor define checks

[Carl-Daniel Hailfinger]

Hi Stefan,

this patch includes [flashrom] [PATCH] Fix undefined behavior in OS defines
Is that intentional?

One minor nitpick: Using "#ifdef IS_LINUX" will be true on all 
platforms with that change. Admittedly it probably was undefined
behaviour before, so this is an improvement. I wonder if we should add a
comment before the IS_ and USE_ #defines to tell people to always use
them with #if and never with if defined() or #ifdef.

Acked-by: Carl-Daniel Hailfinger

Regards,
Carl-Daniel

On 02.12.2016 01:49, Stefan Tauner wrote:
> The macro below (as used it was used previously) would produce
> undefined behavior when used as expression in an
> #if preprocessor directive:
> #define IS_LINUX (defined(__gnu_linux__) || defined(__linux__))
>
> This patch replaces all such statements with a more verbose but
> well-defined #if/#define x 1/#else/#define x 0/#endif
>
> Found by clang (warning introduced in r258128), reported by Idwer.
> Signed-off-by: Stefan Tauner
>
> hwaccess.c | 18 +++++++++++++++---
>  platform.h | 19 +++++++++++++++----
>  2 files changed, 30 insertions(+), 7 deletions(-)
>
> diff --git a/hwaccess.c b/hwaccess.c
> index 1901ee6..80852e7 100644
> --- a/hwaccess.c
> +++ b/hwaccess.c
> @@ -37,9 +37,21 @@
>  #error "Unknown operating system"
>  #endif
>
> -#define USE_IOPL	(IS_LINUX || IS_MACOSX || defined(__NetBSD__) || defined(__OpenBSD__))
> -#define USE_DEV_IO	(defined(__FreeBSD__) || defined(__FreeBSD_kernel__) || defined(__DragonFly__))
> -#define USE_IOPERM	(defined(__gnu_hurd__))
> +#if (IS_LINUX || IS_MACOSX || defined(__NetBSD__) || defined(__OpenBSD__))
> #define USE_IOPL (1)
>
> +#else
> #define USE_IOPL (0)
>
> +#endif
> +#if (defined(__FreeBSD__) || defined(__FreeBSD_kernel__) || defined(__DragonFly__))
> #define USE_DEV_IO (1)
>
> +#else
> #define USE_DEV_IO (0)
>
> +#endif
> +#if (defined(__gnu_hurd__))
> #define USE_IOPERM (1)
>
> +#else
> #define USE_IOPERM (0)
>
> +#endif
>
> #if USE_IOPERM
>  #include <sys/io.h>
> diff --git a/platform.h b/platform.h
> index c5a52ef..2785753 100644
> --- a/platform.h
> +++ b/platform.h
> @@ -24,10 +24,21 @@
>  #ifndef __PLATFORM_H__
>  #define __PLATFORM_H__ 1
>
> -// Helper defines for operating systems
> -#define IS_LINUX	(defined(__gnu_linux__) || defined(__linux__))
> -#define IS_MACOSX	(defined(__APPLE__) && defined(__MACH__)) /* yes, both. */
> -#define IS_WINDOWS	(defined(_WIN32) || defined(_WIN64) || defined(__WIN32__) || defined(__WINDOWS__))
> +#if (defined(__gnu_linux__) || defined(__linux__))
> #define IS_LINUX (1)
>
> +#else
> #define IS_LINUX (0)
>
> +#endif
> +#if (defined(__APPLE__) && defined(__MACH__)) /* yes, both. */
> #define IS_MACOSX (1)
>
> +#else
> #define IS_MACOSX (0)
>
> +#endif
> +#if (defined(_WIN32) || defined(_WIN64) || defined(__WIN32__) || defined(__WINDOWS__))
> #define IS_WINDOWS (1)
>
> +#else
> #define IS_WINDOWS (0)
>
> +#endif
>
> // Likewise for target architectures
>  #if defined (__i386__) || defined (__x86_64__) || defined(__amd64__)

[PATCH] Fix undefined behavior in some preprocessor define checks

[Stefan Tauner]

On Fri, 2 Dec 2016 08:28:11 +0100
Carl-Daniel Hailfinger  wrote:
> Hi Stefan,
>
> this patch includes [flashrom] [PATCH] Fix undefined behavior in OS defines
> Is that intentional?

Yes, of course: for the second one I checked the source code more
thoroughly (looking for #define.*defined IIRC)and it replaces the first
patch.
> One minor nitpick: Using "#ifdef IS_LINUX" will be true on all 
> platforms with that change. Admittedly it probably was undefined
> behaviour before, so this is an improvement. I wonder if we should add a
> comment before the IS_ and USE_ #defines to tell people to always use
> them with #if and never with if defined() or #ifdef.

#ifdef IS_LINUX reads wrong already so I don't think this is a real
issue. That's the point of using the prefixes IS_ (and USE_): to convey
the information that these are already boolean values.
> Acked-by: Carl-Daniel Hailfinger

Now we just need a proper repo to push it to... :)

[PATCH] Fix undefined behavior in some preprocessor define checks

[Stefan Tauner]

On Fri, 2 Dec 2016 11:06:06 +0100
Stefan Tauner  wrote:
> On Fri, 2 Dec 2016 08:28:11 +0100
> Carl-Daniel Hailfinger  wrote:
> > Acked-by: Carl-Daniel Hailfinger
>
> Now we just need a proper repo to push it to... :)

Yay, took less than a year! But I have finally committed this to our
new stable branch in 51e43039. There is another version of this change
in staging though because Patrick did not bother to look or ask before
rewriting it months later *shrug*. I still took this one not only
because it is older but because obviously moar parentheses is better!