From mboxrd@z Thu Jan 1 00:00:00 1970 X-Msuck: nntp://news.gmane.org/gmane.linux.lib.musl.general/13426 Path: news.gmane.org!.POSTED!not-for-mail From: Rich Felker Newsgroups: gmane.linux.lib.musl.general Subject: Re: [PATCH] Fix many compiler warnings Date: Sat, 10 Nov 2018 18:37:38 -0500 Message-ID: <20181110233738.GG5150@brightrain.aerifal.cx> References: <1636016.5PCnkOXM1K@prometheus.sedrubal.de> <20181110175932.GA15979@voyager> Reply-To: musl@lists.openwall.com NNTP-Posting-Host: blaine.gmane.org Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii X-Trace: blaine.gmane.org 1541892947 24665 195.159.176.226 (10 Nov 2018 23:35:47 GMT) X-Complaints-To: usenet@blaine.gmane.org NNTP-Posting-Date: Sat, 10 Nov 2018 23:35:47 +0000 (UTC) User-Agent: Mutt/1.5.21 (2010-09-15) To: musl@lists.openwall.com Original-X-From: musl-return-13442-gllmg-musl=m.gmane.org@lists.openwall.com Sun Nov 11 00:35:43 2018 Return-path: Envelope-to: gllmg-musl@m.gmane.org Original-Received: from mother.openwall.net ([195.42.179.200]) by blaine.gmane.org with smtp (Exim 4.84_2) (envelope-from ) id 1gLcn1-0006K6-0R for gllmg-musl@m.gmane.org; Sun, 11 Nov 2018 00:35:43 +0100 Original-Received: (qmail 16048 invoked by uid 550); 10 Nov 2018 23:37:52 -0000 Mailing-List: contact musl-help@lists.openwall.com; run by ezmlm Precedence: bulk List-Post: List-Help: List-Unsubscribe: List-Subscribe: List-ID: Original-Received: (qmail 16030 invoked from network); 10 Nov 2018 23:37:51 -0000 Content-Disposition: inline In-Reply-To: <20181110175932.GA15979@voyager> Original-Sender: Rich Felker Xref: news.gmane.org gmane.linux.lib.musl.general:13426 Archived-At: 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