Fix MIPS build failure caused by -Werror=undef

Message ID 1424196067.27855.80.camel@ubuntu-sellcey
State Committed
Headers

Commit Message

Steve Ellcey Feb. 17, 2015, 6:01 p.m. UTC
  On Sat, 2015-02-14 at 14:33 +0000, Maciej W. Rozycki wrote:

> > diff --git a/sysdeps/mips/bits/endian.h b/sysdeps/mips/bits/endian.h
> > index 9586104..43ce009 100644
> > --- a/sysdeps/mips/bits/endian.h
> > +++ b/sysdeps/mips/bits/endian.h
> > @@ -5,7 +5,7 @@
> >  # error "Never use <bits/endian.h> directly; include <endian.h> instead."
> >  #endif
> >  
> > -#if __MIPSEB
> > +#ifdef __MIPSEB
> >  # define __BYTE_ORDER __BIG_ENDIAN
> >  #endif
> >  #if __MIPSEL
> 
>    ^^^^^^^^^^^^
>  Don't we need a complementing change for __MIPSEL here then too?
> 
>   Maciej

You are right.  Apparently, this header is not actually used in the
glibc build since it didn't cause a build failure but we should fix it
so it doesn't cause an error/warning for users who do include the header
file.  I will check this in as an obvious fix:


2015-02-17  Steve Ellcey  <sellcey@imgtec.com>

	* sysdeps/mips/bits/endian.h (__MIPSEL): Use #ifdef instead of #if.
  

Comments

Joseph Myers Feb. 17, 2015, 6:11 p.m. UTC | #1
On Tue, 17 Feb 2015, Steve Ellcey wrote:

> You are right.  Apparently, this header is not actually used in the
> glibc build since it didn't cause a build failure but we should fix it

There are both sysdeps/mips/bits/endian.h (not used by any current 
configuration) and sysdeps/unix/sysv/linux/mips/bits/endian.h - clearly 
not both needed since one bits/endian.h file could be generic.  They 
should be unified.  (I'd say prefer sysdeps/mips/bits/endian.h as 
symmetric between the endiannesses, but with the comment corrected as 
"This file is for a machine using big-endian mode." is simply wrong.)
  
Maciej W. Rozycki Feb. 17, 2015, 7:58 p.m. UTC | #2
On Tue, 17 Feb 2015, Joseph Myers wrote:

> > You are right.  Apparently, this header is not actually used in the
> > glibc build since it didn't cause a build failure but we should fix it
> 
> There are both sysdeps/mips/bits/endian.h (not used by any current 
> configuration) and sysdeps/unix/sysv/linux/mips/bits/endian.h - clearly 
> not both needed since one bits/endian.h file could be generic.  They 
> should be unified.  (I'd say prefer sysdeps/mips/bits/endian.h as 
> symmetric between the endiannesses, but with the comment corrected as 
> "This file is for a machine using big-endian mode." is simply wrong.)

 There's clearly nothing Linux specific there although one might ask which 
of the two of __MIPSEB vs __MIPSEB__ is the "traditional" macro we might 
want to keep.  Or maybe _MIPSEB even.  I'm not sure offhand and clearly 
current GCC defines all at a time.

 Also I wonder if sanity checks like:

#if defined (__MIPSEB) && defined (__MIPSEL)
# error "Both __MIPSEB and __MIPSEL, endianness configuration problem?"
#elif defined (__MIPSEB)
# define __BYTE_ORDER __BIG_ENDIAN
#elif defined (__MIPSEL)
# define __BYTE_ORDER __LITTLE_ENDIAN
#elif
# error "Neither __MIPSEB nor __MIPSEL, endianness configuration problem?"
#endif

would make sense here too, to catch issues early on in case someone does 
something silly with `-D' or `-U'.

  Maciej
  
Steve Ellcey Feb. 17, 2015, 9:37 p.m. UTC | #3
On Tue, 2015-02-17 at 19:58 +0000, Maciej W. Rozycki wrote:

>  There's clearly nothing Linux specific there although one might ask which 
> of the two of __MIPSEB vs __MIPSEB__ is the "traditional" macro we might 
> want to keep.  Or maybe _MIPSEB even.  I'm not sure offhand and clearly 
> current GCC defines all at a time.
> 
>  Also I wonder if sanity checks like:
> 
> #if defined (__MIPSEB) && defined (__MIPSEL)
> # error "Both __MIPSEB and __MIPSEL, endianness configuration problem?"
> #elif defined (__MIPSEB)
> # define __BYTE_ORDER __BIG_ENDIAN
> #elif defined (__MIPSEL)
> # define __BYTE_ORDER __LITTLE_ENDIAN
> #elif
> # error "Neither __MIPSEB nor __MIPSEL, endianness configuration problem?"
> #endif
> 
> would make sense here too, to catch issues early on in case someone does 
> something silly with `-D' or `-U'.
> 
>   Maciej

I wonder if we should check here or if we should check in the shared
src/glibc/string/endian.h file.  We could verify that after including a
system specific bits/endian.h, __BYTE_ORDER is defined to either
__BIG_ENDIAN or __LITTLE_ENDIAN.

Maybe change this check in src/glibc/string/endian.h to have a '#else
#error' clause.  Is it ever legal to not define __BYTE_ORDER?

#if __BYTE_ORDER == __LITTLE_ENDIAN
# define __LONG_LONG_PAIR(HI, LO) LO, HI
#elif __BYTE_ORDER == __BIG_ENDIAN
# define __LONG_LONG_PAIR(HI, LO) HI, LO
#endif


Steve Ellcey
  
Roland McGrath Feb. 18, 2015, 12:09 a.m. UTC | #4
The purpose of <bits/endian.h> is to define __BYTE_ORDER,
so a sanity check that it indeed does so is certainly fine.
  

Patch

diff --git a/sysdeps/mips/bits/endian.h b/sysdeps/mips/bits/endian.h
index 43ce009..92e97c7 100644
--- a/sysdeps/mips/bits/endian.h
+++ b/sysdeps/mips/bits/endian.h
@@ -8,6 +8,6 @@ 
 #ifdef __MIPSEB
 # define __BYTE_ORDER __BIG_ENDIAN
 #endif
-#if __MIPSEL
+#ifdef __MIPSEL
 # define __BYTE_ORDER __LITTLE_ENDIAN
 #endif