Re: i386 byteswap.h provoking ``may be undefined'' warnings in gcc3

[Paul Haahr]

Andreas wrote, replying to me:
> > Gcc 3.0's new ``warning: operation on `p' may be undefined'' is
> > (spuriously?) provoked by glibc's <bits/byteswap.h> code for the x86.
> >
> > [...]
> > 
> > The bswap_16 and bswap_32 macros expand into horrible creatures, where a
> > reference to n++ appears several times in one expression.  This generates
> > one warning for the call to bswap_16 and three for the call to bswap_32.
> > However, evaluation of the problematic expressions is gated by an
> > ``if (__builtin_constant_p(x))'' test which always evaluates to false if
> > there are internal side effects, so the ``may be undefined'' path is
> > never actually taken.
> > 
> > I can't quite tell if this is a gcc bug or a glibc bug, but I've
> > attached a workaround that lives in glibc.  It appears to still
> > constant fold properly, and generate identical code to the old
> > version, for a few small test cases.
> 
> But might fail with GCC 3.1 if that one has a more sophisticated
> analysis.

I don't think so, because my workaround uses an additional local
variable to avoid repeated evaluation of the macro argument, so the
internal multiple evaluations in the macro are not a problem.

I've reattached my proposed patch -- take a look at the new variable __x
for why the problem doesn't exist in my version.

> > Fixing this in gcc would require not issuing this kind of warning
> > (and perhaps others) in code that isn't evaluated due to a
> > __builtin_constant_p test.
> 
> And that's the way to go.

That's still not clear -- it's hard to argue that, just because some
code is in a provably untaken branch of an if, it shouldn't be checked
for warnings.  And special casing __builtin_constant_p seems problematic.

Any gcc folk want to weigh in with an opinion?

--p

--- /usr/include/bits/byteswap.h        Sun Nov 19 06:24:03 2000
+++ bits/byteswap.h     Wed Jul 11 15:22:48 2001
@@ -29,9 +29,10 @@
 # define __bswap_16(x) \
      (__extension__                                                          \
       ({ register unsigned short int __v;                                    \
-        if (__builtin_constant_p (x))                                        \
-          __v = __bswap_constant_16 (x);                                     \
-        else                                                                 \
+        if (__builtin_constant_p (x)) {                                      \
+          register unsigned short int __x = x;                               \
+          __v = __bswap_constant_16 (__x);                                   \
+        } else                                                               \
           __asm__ __volatile__ ("rorw $8, %w0"                               \
                                 : "=r" (__v)                                 \
                                 : "0" ((unsigned short int) (x))             \
@@ -55,9 +56,10 @@
 #  define __bswap_32(x) \
      (__extension__                                                          \
       ({ register unsigned int __v;                                          \
-        if (__builtin_constant_p (x))                                        \
-          __v = __bswap_constant_32 (x);                                     \
-        else                                                                 \
+        if (__builtin_constant_p (x)) {                                      \
+          register unsigned int __x = x;                                     \
+          __v = __bswap_constant_32 (__x);                                   \
+        } else                                                               \
           __asm__ __volatile__ ("rorw $8, %w0;"                              \
                                 "rorl $16, %0;"                              \
                                 "rorw $8, %w0"                               \