From mboxrd@z Thu Jan 1 00:00:00 1970 X-Msuck: nntp://news.gmane.org/gmane.linux.lib.musl.general/13423 Path: news.gmane.org!.POSTED!not-for-mail From: Markus Wichmann Newsgroups: gmane.linux.lib.musl.general Subject: Re: [PATCH] Fix many compiler warnings Date: Sat, 10 Nov 2018 18:59:32 +0100 Message-ID: <20181110175932.GA15979@voyager> References: <1636016.5PCnkOXM1K@prometheus.sedrubal.de> 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 1541872674 26146 195.159.176.226 (10 Nov 2018 17:57:54 GMT) X-Complaints-To: usenet@blaine.gmane.org NNTP-Posting-Date: Sat, 10 Nov 2018 17:57:54 +0000 (UTC) User-Agent: Mutt/1.10.1 (2018-07-13) To: musl@lists.openwall.com Original-X-From: musl-return-13439-gllmg-musl=m.gmane.org@lists.openwall.com Sat Nov 10 18:57:50 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 1gLXVw-0006Wa-Lv for gllmg-musl@m.gmane.org; Sat, 10 Nov 2018 18:57:44 +0100 Original-Received: (qmail 24452 invoked by uid 550); 10 Nov 2018 17:59:53 -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 24430 invoked from network); 10 Nov 2018 17:59:53 -0000 Content-Disposition: inline In-Reply-To: <1636016.5PCnkOXM1K@prometheus.sedrubal.de> X-Provags-ID: V03:K1:H2H21TCf+1dEgKZGDgLtXdSfuIuK9YTnE2CJbnQoerryJRXA6E8 9IzJaO+8u0npJPO9Vl/QV1uWMOV66yjWfxlBPDDIOSfS/bfW9X7GcxN4JAiuCDHSKgqGSRL u9HqnEEbwC5muGzXID7kInWwwRlUH6wkAjBYlmcDLy4CWxf9T1yJzbGJuN8uLCxAXFIv7cC TEqpqoKenemmVOAOpIhKg== X-UI-Out-Filterresults: notjunk:1;V01:K0:2DDkK4e4ZYw=:y052u7Q/+smfAkH/n7877J 1eN2P3z8sSXB03p2nUed3AT3bO4NsLmfRB2LP18ZI1L1KgixGwFFL6p0rHIzENdaTmeHHH0fw pNCYkS1od7cdXKzy0UaLyUCjMjiOTfQfZjYRqaJnfeJ84fwGwr5S7luV+wsryWGDgE745EBxZ fbW5PR1t59CpLONbzDfsFkq7PfUv2SK75Ur6BX1xEuwCOT3dunTdRmOYPTAeoBdYECK4rftCP cCwoZCbici/QsKItGCP1OiaOjTaUfa/9BAKBTofMW8YY6aVAEFEJqmGq9uXbWwjcnI+qnhthC 6mlv7NaajWoYEV40ymyU1p2+F6VajGWQpTzYrZnQHf3IucH0JS7V5nw4fB+eM/wDy5KmsOcET JtUudEdm1YYQYtMiF/DmwTz5axnnqc1hlbrXRugizLzuwdSXyYUJ+gwgTAvhgKGVwzZrjDMK0 4r+gHZ5DL1HwtbUf7+fi21cPQc+3zYdnhbpMcGx19VgL6VgwUH9bswmqewsakWGzX/7tvIBs1 GU8YffVW5493Bj684/3H7eerC2mSjTcJolUycTHTWBfak5rPA+mOm4fSImCCTo8A/ZSeY3icz kgwnHE5+JFstStcYdFv9bWUbKCK1wJJ6WTJDVp2fp3p0RFKLteO2yy0XgEL8hWfs5WJtudsE1 ry1rz+fN/FxmbPZXp3ER4ObR54OwuUnUB/xUT/eITOyM6ZYwCYf3tsT9SkXbf+SNGE7ugzoak ILpt0T5fRKwUFVdRS7vf16Sha/ri2GECRWZRt3kYJhAWXp48YKp+xFoBM1dTsnLQNKEW3Jsc Xref: news.gmane.org gmane.linux.lib.musl.general:13423 Archived-At: 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 platforms with multiple compilers is damn near impossible, anyway. (E.g. take the following snippet: switch (x) { case foo: return other_func(); break; } At work I have a compiler that will complain about the unreachable break. If I deleted it I have another compiler (and a company directive) warning that I have a case-statement that is not terminated with a break.) > > 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); > static __inline uint64_t __bswap_64(uint64_t __x) > { > - return __bswap_32(__x)+0ULL<<32 | __bswap_32(__x>>32); > + return (__bswap_32(__x)+0ULL)<<32 | __bswap_32(__x>>32); > } > > #define bswap_16(x) __bswap_16(x) > diff --git a/include/endian.h b/include/endian.h > index 1bd44451..6957fad3 100644 > --- a/include/endian.h > +++ b/include/endian.h > @@ -24,17 +24,17 @@ > > static __inline uint16_t __bswap16(uint16_t __x) > { > - return __x<<8 | __x>>8; > + return (__x << 8) | (__x >> 8); > } > > static __inline uint32_t __bswap32(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); > } > You did it correctly here. The why the error above? > diff --git a/src/locale/__mo_lookup.c b/src/locale/__mo_lookup.c > index d18ab774..8750fd2c 100644 > --- a/src/locale/__mo_lookup.c > +++ b/src/locale/__mo_lookup.c > @@ -3,7 +3,7 @@ > > static inline uint32_t swapc(uint32_t x, int c) > { > - return c ? x>>24 | x>>8&0xff00 | x<<8&0xff0000 | x<<24 : x; > + return c ? x>>24 | x>>(8&0xff00) | x<<(8&0xff0000) | x<<24 : x; > } > Again, as above. > 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. > 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? > 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? 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. Ciao, Markus