Fix undefined behavior in some preprocessor define checks

Submitted by Stefan Tauner on Dec. 2, 2016, 12:49 a.m.

Details

Message ID 20161202004932.8102-1-stefan.tauner@alumni.tuwien.ac.at
State New
Headers show

Commit Message

Stefan Tauner Dec. 2, 2016, 12:49 a.m.
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 <stefan.tauner@alumni.tuwien.ac.at>
---
 hwaccess.c | 18 +++++++++++++++---
 platform.h | 19 +++++++++++++++----
 2 files changed, 30 insertions(+), 7 deletions(-)

Comments

Carl-Daniel Hailfinger Dec. 2, 2016, 7:28 a.m.
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 <c-d.hailfinger.devel.2006@gmx.net>

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 <stefan.tauner@alumni.tuwien.ac.at>
> ---
>  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__)
Stefan Tauner Dec. 2, 2016, 10:06 a.m.
On Fri, 2 Dec 2016 08:28:11 +0100
Carl-Daniel Hailfinger <c-d.hailfinger.devel.2006@gmx.net> 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 <c-d.hailfinger.devel.2006@gmx.net>

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

Patch hide | download patch | download mbox

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__)