Message ID | 20161202004932.8102-1-stefan.tauner@alumni.tuwien.ac.at |
---|---|
State | New |
Headers | show |
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__)
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... :)
On Fri, 2 Dec 2016 11:06:06 +0100 Stefan Tauner <stefan.tauner@alumni.tuwien.ac.at> wrote: > On Fri, 2 Dec 2016 08:28:11 +0100 > Carl-Daniel Hailfinger <c-d.hailfinger.devel.2006@gmx.net> wrote: > > > Acked-by: Carl-Daniel Hailfinger <c-d.hailfinger.devel.2006@gmx.net> > > 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!
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__)
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(-)