[PATCH] eliminate strict parentheses warnings for byteswaps

[PATCH] eliminate strict parentheses warnings for byteswaps

[Nick Bray]

[-- Attachment #1: Type: text/plain, Size: 837 bytes --]

Patch is attached.

The change was motivated by including Musl's headers from a project
with -Werror -Wall enabled. I believe Fucshia ran into this, too.  I
couldn't think of a good regression test, in part because warnings are
somewhat compiler specific.  One possible approach would be to enable
-Werror -Wall in the main build, but that runs into issues of which
compilers are supported and what the core developers prefer.  Another
approach would be to only lint the header files - generate a dummy .c
file that includes all the header files and compile it with -Werror.
This is complicated by the "redirection" header files that warn you
should use the canonical version.  Which header files should be
checked?  So for the moment I punted on regression testing.  I mention
this line of thinking in case anyone has some perspective.

[-- Attachment #2: 0001-eliminate-strict-parentheses-warnings-for-byteswaps.patch --]
[-- Type: text/x-patch, Size: 1673 bytes --]

From 347fdeccf9268cefa40b8389790e465a3e2e6587 Mon Sep 17 00:00:00 2001
From: Nick Bray <ncbray@google.com>
Date: Thu, 17 Jan 2019 15:53:46 -0800
Subject: [PATCH] eliminate strict parentheses warnings for byteswaps

adding these parentheses eliminates a warning from -Wparentheses, but
does not change the semantics of the code.
---
 include/byteswap.h | 4 ++--
 include/endian.h   | 4 ++--
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/include/byteswap.h b/include/byteswap.h
index 00b9df3c..928be2b3 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;
 }
 
 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..88c3347b 100644
--- a/include/endian.h
+++ b/include/endian.h
@@ -29,12 +29,12 @@ static __inline uint16_t __bswap16(uint16_t __x)
 
 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;
 }
 
 static __inline uint64_t __bswap64(uint64_t __x)
 {
-	return __bswap32(__x)+0ULL<<32 | __bswap32(__x>>32);
+	return (__bswap32(__x)+0ULL)<<32 | __bswap32(__x>>32);
 }
 
 #if __BYTE_ORDER == __LITTLE_ENDIAN
-- 
2.21.0.rc0.258.g878e2cd30e-goog

Re: [PATCH] eliminate strict parentheses warnings for byteswaps

[A. Wilcox]

[-- Attachment #1.1: Type: text/plain, Size: 1406 bytes --]

On 02/14/19 18:24, Nick Bray wrote:
> Patch is attached.
> 
> The change was motivated by including Musl's headers from a project
> with -Werror -Wall enabled. I believe Fucshia ran into this, too.  I
> couldn't think of a good regression test, in part because warnings are
> somewhat compiler specific.  One possible approach would be to enable
> -Werror -Wall in the main build, but that runs into issues of which
> compilers are supported and what the core developers prefer.  Another
> approach would be to only lint the header files - generate a dummy .c
> file that includes all the header files and compile it with -Werror.
> This is complicated by the "redirection" header files that warn you
> should use the canonical version.  Which header files should be
> checked?  So for the moment I punted on regression testing.  I mention
> this line of thinking in case anyone has some perspective. 


This has a strong +1 from me, but changes like this have already been
rejected at least twice on this list because "compilers shouldn't warn
about system headers".

Perhaps at attempt #3, they will just accept that this happens regularly
enough that a few parens is not the end of the world.  (It also makes
more components of musl reusable in other libraries, if desired.)

Best,
--arw

-- 
A. Wilcox (awilfox)
Project Lead, Adélie Linux
https://www.adelielinux.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] eliminate strict parentheses warnings for byteswaps

[Nick Bray]

To give a little more context, I am targeting a small non-Linux OS.
We are not at a point where maintaining our own toolchain make sense.
Instead we are using Android's prebuilt toolchains, providing an
alternate sysroot (empty for now), and treating Musl as a library
built from source.  It's a flat playing field and there is no such
thing as system headers or system libraries.  We can dial the
strictness down when compiling Musl itself, but header files touch
everything.  This isn't the traditional use case, but Musl seems like
a pretty good choice for embedded systems-ish type work when you need
to scale down into a smaller memory footprint.

On Thu, Feb 14, 2019 at 5:55 PM A. Wilcox <awilfox@adelielinux.org> wrote:
>
> On 02/14/19 18:24, Nick Bray wrote:
> > Patch is attached.
> >
> > The change was motivated by including Musl's headers from a project
> > with -Werror -Wall enabled. I believe Fucshia ran into this, too.  I
> > couldn't think of a good regression test, in part because warnings are
> > somewhat compiler specific.  One possible approach would be to enable
> > -Werror -Wall in the main build, but that runs into issues of which
> > compilers are supported and what the core developers prefer.  Another
> > approach would be to only lint the header files - generate a dummy .c
> > file that includes all the header files and compile it with -Werror.
> > This is complicated by the "redirection" header files that warn you
> > should use the canonical version.  Which header files should be
> > checked?  So for the moment I punted on regression testing.  I mention
> > this line of thinking in case anyone has some perspective.
>
>
> This has a strong +1 from me, but changes like this have already been
> rejected at least twice on this list because "compilers shouldn't warn
> about system headers".
>
> Perhaps at attempt #3, they will just accept that this happens regularly
> enough that a few parens is not the end of the world.  (It also makes
> more components of musl reusable in other libraries, if desired.)
>
> Best,
> --arw
>
> --
> A. Wilcox (awilfox)
> Project Lead, Adélie Linux
> https://www.adelielinux.org
>

Re: [PATCH] eliminate strict parentheses warnings for byteswaps

[Rich Felker]

On Thu, Feb 14, 2019 at 06:27:26PM -0800, Nick Bray wrote:
> To give a little more context, I am targeting a small non-Linux OS.
> We are not at a point where maintaining our own toolchain make sense.
> Instead we are using Android's prebuilt toolchains, providing an
> alternate sysroot (empty for now), and treating Musl as a library
> built from source.  It's a flat playing field and there is no such
> thing as system headers or system libraries.  We can dial the
> strictness down when compiling Musl itself, but header files touch
> everything.  This isn't the traditional use case, but Musl seems like
> a pretty good choice for embedded systems-ish type work when you need
> to scale down into a smaller memory footprint.

This has come up enough that there's some chance it may be worth
revisiting, but I wonder if using -isystem wouldn't solve your
problem.

The policy aspect here is that coding style in the libc headers should
not be governed by a possibly changing/unbounded set of compiler style
warnings. But maybe we should look at whether there's some small set
of stricter style rules we should adopt for the headers.

Also it's worth noting that, in the past when this has come up, it's
brought light to incorrect usage by the person raising the issue --
things like using a glibc-targeting toolchain and just adding -I for
the musl include dir.

Rich


> On Thu, Feb 14, 2019 at 5:55 PM A. Wilcox <awilfox@adelielinux.org> wrote:
> >
> > On 02/14/19 18:24, Nick Bray wrote:
> > > Patch is attached.
> > >
> > > The change was motivated by including Musl's headers from a project
> > > with -Werror -Wall enabled. I believe Fucshia ran into this, too.  I
> > > couldn't think of a good regression test, in part because warnings are
> > > somewhat compiler specific.  One possible approach would be to enable
> > > -Werror -Wall in the main build, but that runs into issues of which
> > > compilers are supported and what the core developers prefer.  Another
> > > approach would be to only lint the header files - generate a dummy .c
> > > file that includes all the header files and compile it with -Werror.
> > > This is complicated by the "redirection" header files that warn you
> > > should use the canonical version.  Which header files should be
> > > checked?  So for the moment I punted on regression testing.  I mention
> > > this line of thinking in case anyone has some perspective.
> >
> >
> > This has a strong +1 from me, but changes like this have already been
> > rejected at least twice on this list because "compilers shouldn't warn
> > about system headers".
> >
> > Perhaps at attempt #3, they will just accept that this happens regularly
> > enough that a few parens is not the end of the world.  (It also makes
> > more components of musl reusable in other libraries, if desired.)
> >
> > Best,
> > --arw

Re: [PATCH] eliminate strict parentheses warnings for byteswaps

[Nick Bray]

On Thu, Feb 14, 2019 at 6:39 PM Rich Felker <dalias@libc.org> wrote:
>
> On Thu, Feb 14, 2019 at 06:27:26PM -0800, Nick Bray wrote:
> > To give a little more context, I am targeting a small non-Linux OS.
> > We are not at a point where maintaining our own toolchain make sense.
> > Instead we are using Android's prebuilt toolchains, providing an
> > alternate sysroot (empty for now), and treating Musl as a library
> > built from source.  It's a flat playing field and there is no such
> > thing as system headers or system libraries.  We can dial the
> > strictness down when compiling Musl itself, but header files touch
> > everything.  This isn't the traditional use case, but Musl seems like
> > a pretty good choice for embedded systems-ish type work when you need
> > to scale down into a smaller memory footprint.
>
> This has come up enough that there's some chance it may be worth
> revisiting, but I wonder if using -isystem wouldn't solve your
> problem.

It looks like -isystem could work for my use case.  All things equal,
I'd prefer to have stricter compiler settings when modifying Musl and
also prefer not to rely on the esoteric details of -isystem.  But I'd
prefer -isystem to adding local patches.  I leave the question of
future direction to y'all.



> The policy aspect here is that coding style in the libc headers should
> not be governed by a possibly changing/unbounded set of compiler style
> warnings. But maybe we should look at whether there's some small set
> of stricter style rules we should adopt for the headers.
>
> Also it's worth noting that, in the past when this has come up, it's
> brought light to incorrect usage by the person raising the issue --
> things like using a glibc-targeting toolchain and just adding -I for
> the musl include dir.
>
> Rich
>
>
> > On Thu, Feb 14, 2019 at 5:55 PM A. Wilcox <awilfox@adelielinux.org> wrote:
> > >
> > > On 02/14/19 18:24, Nick Bray wrote:
> > > > Patch is attached.
> > > >
> > > > The change was motivated by including Musl's headers from a project
> > > > with -Werror -Wall enabled. I believe Fucshia ran into this, too.  I
> > > > couldn't think of a good regression test, in part because warnings are
> > > > somewhat compiler specific.  One possible approach would be to enable
> > > > -Werror -Wall in the main build, but that runs into issues of which
> > > > compilers are supported and what the core developers prefer.  Another
> > > > approach would be to only lint the header files - generate a dummy .c
> > > > file that includes all the header files and compile it with -Werror.
> > > > This is complicated by the "redirection" header files that warn you
> > > > should use the canonical version.  Which header files should be
> > > > checked?  So for the moment I punted on regression testing.  I mention
> > > > this line of thinking in case anyone has some perspective.
> > >
> > >
> > > This has a strong +1 from me, but changes like this have already been
> > > rejected at least twice on this list because "compilers shouldn't warn
> > > about system headers".
> > >
> > > Perhaps at attempt #3, they will just accept that this happens regularly
> > > enough that a few parens is not the end of the world.  (It also makes
> > > more components of musl reusable in other libraries, if desired.)
> > >
> > > Best,
> > > --arw