Re: [PATCH] Fix many compiler warnings

[Rich Felker <dalias@libc.org>]

On Sat, Nov 10, 2018 at 06:59:32PM +0100, Markus Wichmann wrote:
> On Sat, Nov 10, 2018 at 05:48:23PM +0100, Sedrubal wrote:
> > Hello,
> > 
> > I'm building musl for aarch64 with clang and get many compiler warnings
> > (mostly Wbitwise-op-parentheses, Wshift-op-parentheses and Wlogical-op-parentheses).
> > I tried to fix them (and I hope, I did not introduce new bugs).
> > 
> 
> Well, hope belongs in the church. Whether or not you introduced bugs is
> a thing you can test for. There is libc-test.
> 
> I don't know Rich's position on warnings, but my understanding was he
> only cares about some of them. Writing code warning-free on multiple

musl's configure's --enable-warnings specifically enables (for gcc;
other compilers may vary) enables all that are actual constraint
violations or which are considered style mistakes for musl. However
it does part of this via -Wno-* added on to the end of -Wall, meaning
on some (probably newer) versions there may be on-by-default warnings
if --enable-warnings is not passed to configure. I think the defaults
here can and should be changed: always using the -Wno-* for ones we
don't want, and perhaps also always adding the ones we do want, or at
least making --enable-warnings the default, so that people don't just
add -Wall manually.

> platforms with multiple compilers is damn near impossible, anyway. (E.g.

Yes. One problem is that firm/cparser, and possibly also clang, has
a number of warnings on by default, and needs -w to start with a clean
slate of "none enabled". But with gcc, -w permanently disables all
warnings, even if a subsequent -W occurs. So getting this right for
all compilers is a pain.

> > diff --git a/include/byteswap.h b/include/byteswap.h
> > index 00b9df3c..f2dbb912 100644
> > --- a/include/byteswap.h
> > +++ b/include/byteswap.h
> > @@ -11,12 +11,12 @@ static __inline uint16_t __bswap_16(uint16_t __x)
> >  
> >  static __inline uint32_t __bswap_32(uint32_t __x)
> >  {
> > -	return __x>>24 | __x>>8&0xff00 | __x<<8&0xff0000 | __x<<24;
> > +	return __x>>24 | __x>>(8&0xff00) | __x<<(8&0xff0000) | __x<<24;
> >  }
> >  
> 
> That's erroneous and you can actually see that pretty easily: The part
> in the parentheses must always be zero. Could that possibly have been
> the intention? No. (And this change fixed a warning? Shows what those
> are worth!)
> 
> I guess the following will do the trick:
> 
> return (__x>>24) | (__x>>8 & 0xff00) | (__x<<8 & 0xff0000) | (__x<<24);

Indeed the above is wrong, but this might be one of the few changes we
could actually consider doing (with it fixed to be right) since it's
in a public header and might impact building programs against musl.
Ideally compilers should not produce warnings for system headers,
though; when they are produced it usually indicates the user has
botched the install somehow or the build system has bugs like explicit
-I/usr/include (which badly breaks cross compiling and some other
setups).

> > diff --git a/src/locale/dcngettext.c b/src/locale/dcngettext.c
> > index 8b891d00..39c0b191 100644
> > --- a/src/locale/dcngettext.c
> > +++ b/src/locale/dcngettext.c
> > @@ -176,7 +176,7 @@ notrans:
> >  			snprintf(name, sizeof name, "%s/%.*s%.*s/%s/%s.mo\0",
> >  				dirname, (int)loclen, locname,
> >  				(int)alt_modlen, modname, catname, domainname);
> > -			if (map = __map_file(name, &map_size)) break;
> > +			if ((map = __map_file(name, &map_size))) break;
> >  
> 
> Actually, why is there an if-condition assignment here? I've done that
> in the past, but only in complex conditions, not simple ones like this.
> Wouldn't
> 
>     map = __map_file(name, &map_size);
>     if (map) break;
> 
> be better style here? I'm not against assigning in conditions in
> general, but it should serve a purpose. "while ((de = readdir(d)))" has
> such a purpose, but that is a while-condition.

Yeah. I'm fairly indifferent about this though.

> > diff --git a/src/locale/iconv.c b/src/locale/iconv.c
> > index 3047c27b..d189f598 100644
> > --- a/src/locale/iconv.c
> > +++ b/src/locale/iconv.c
> > @@ -181,7 +181,7 @@ static void put_16(unsigned char *s, unsigned c, int e)
> >  static unsigned get_32(const unsigned char *s, int e)
> >  {
> >  	e &= 3;
> > -	return s[e]+0U<<24 | s[e^1]<<16 | s[e^2]<<8 | s[e^3];
> > +	return (s[e]+0U)<<24 | s[e^1]<<16 | s[e^2]<<8 | s[e^3];
> >  }
> >  
> 
> Wait, I'm confused again. Why is it necessary to convert the first
> dereference of s to unsigned int, but not the others?

Because, with x in the range 0-255, x<<16 and x<<8 cannot overflow,
but x<<24 can when x>=128.

> 
> >  static void put_32(unsigned char *s, unsigned c, int e)
> > @@ -201,7 +201,7 @@ static unsigned legacy_map(const unsigned char *map, unsigned c)
> >  {
> >  	if (c < 4*map[-1]) return c;
> >  	unsigned x = c - 4*map[-1];
> > -	x = map[x*5/4]>>2*x%8 | map[x*5/4+1]<<8-2*x%8 & 1023;
> > +	x = (map[x*5/4]>>(2*x%8)) | ((map[x*5/4+1]<<(8-2*x%8)) & 1023);
> >  	return x < 256 ? x : legacy_chars[x-256];
> >  }
> >  
> 
> I've stared at the old line for five minutes now, and I still don't know
> what it does. As such I can't evaluate the consistency of the new
> version. What were you thinking when you wrote that line, Rich?

It extracts a 10-bit value from a packed array of 10-bit values that
span 2 consecutive bytes at positions x*5/4 and x*5/4+1. Their offsets
within the bytes rotate around with a period of 8, shifting by 2 for
each successive slot, thus 2*x%8.

> Let's see here... bitshift binds less strongly than arithmetic, and
> bitlogic binds even less strongly than that. So mechanically, the new
> line is correct. I still don't know what it does.

Mechanically it's correct, but if anything it makes it less readable.
There might be some intermediate form, like just parenthesizing the
RHS's of the <</>>, that would be more readable, but not by much. In
any case, changes like this should be motivated by "this change makes
something easier to understand", not by "the compiler printed a
warning".

Rich