if __HAVE_BUILTIN_BSWAP16__ is defined then __swab16 utilizes a __u16 cast.
This same cast should be used if __HAVE_BUILTIN_BSWAP16__ is not defined as
well. This should fix loads (at least a few) clang -Wformat warnings
specifically with `ntohs()`
Link: https://github.com/ClangBuiltLinux/linux/issues/378
Signed-off-by: Justin Stitt <[email protected]>
---
include/uapi/linux/swab.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/include/uapi/linux/swab.h b/include/uapi/linux/swab.h
index 7272f85d6d6a..f6be3f2e6fee 100644
--- a/include/uapi/linux/swab.h
+++ b/include/uapi/linux/swab.h
@@ -102,7 +102,7 @@ static inline __attribute_const__ __u32 __fswahb32(__u32 val)
#define __swab16(x) (__u16)__builtin_bswap16((__u16)(x))
#else
#define __swab16(x) \
- (__builtin_constant_p((__u16)(x)) ? \
+ (__u16)(__builtin_constant_p((__u16)(x)) ? \
___constant_swab16(x) : \
__fswab16(x))
#endif
--
2.30.2
On Tue, Jun 7, 2022 at 4:21 PM Andrew Morton <[email protected]> wrote:
>
> On Tue, 7 Jun 2022 15:42:56 -0700 Nick Desaulniers <[email protected]> wrote:
>
> > On Tue, Jun 7, 2022 at 3:27 PM Andrew Morton <[email protected]> wrote:
> > >
> > > On Tue, 7 Jun 2022 15:20:06 -0700 Justin Stitt <[email protected]> wrote:
> > >
> > > > if __HAVE_BUILTIN_BSWAP16__ is defined then __swab16 utilizes a __u16 cast.
> > > > This same cast should be used if __HAVE_BUILTIN_BSWAP16__ is not defined as
> > > > well. This should fix loads (at least a few) clang -Wformat warnings
> > > > specifically with `ntohs()`
> > > >
> > > > ...
> > > >
> > > > --- a/include/uapi/linux/swab.h
> > > > +++ b/include/uapi/linux/swab.h
> > > > @@ -102,7 +102,7 @@ static inline __attribute_const__ __u32 __fswahb32(__u32 val)
> > > > #define __swab16(x) (__u16)__builtin_bswap16((__u16)(x))
> > > > #else
> > > > #define __swab16(x) \
> > > > - (__builtin_constant_p((__u16)(x)) ? \
> > > > + (__u16)(__builtin_constant_p((__u16)(x)) ? \
> > > > ___constant_swab16(x) : \
> > > > __fswab16(x))
> > > > #endif
> > >
> > > More explanation, please? Both ___constant_swab16() and __fswab16()
> > > return __u16, so why does this patch have any effect?
> > >
> >
> > See this example:
> > https://godbolt.org/z/fzE73jn13
> > And the ImplicitCastExpr nodes adding to the AST:
> > https://godbolt.org/z/oYeYxYdKW
> >
> > Both the second and third operand are promoted to int.
> >
> > C11: https://www.open-std.org/jtc1/sc22/wg14/www/docs/n1548.pdf
> >
> > 6.5.15/5
> > >> If both the second and third operands have arithmetic type, the result type that would be determined by the usual arithmetic conversions, were they applied to those two operands, is the type of the result.
> > 6.3.1.8/1
> > >> Otherwise, the integer promotions are performed on both operands.
> > 6.3.1.1/2
> > >> If an int can represent all values of the original type (as restricted by the width, for a bit-field), the value is converted to an int; otherwise, it is converted to an unsigned int. These are called the integer promotions.
>
> Geeze. Can we please turn this into English and add it to the changelog?
>
> Is it saying that an expression
>
> int ? u16 : u16
>
> has type int?
Yep.
> Or something else?
Technically, the `int` in your example (the first operand) doesn't
matter. Could be a `long long` or a `char` and it would not matter.
> What did we do wrong here and is it
Perhaps the simplest English explanation would be "ternary expressions
with then/else clauses with types smaller than int undergo implicit
promotion to int."
> possible to correct our types rather than adding a cast?
I think the cast is the explicit cast back to __u16 way to go here,
IMO. I don't think anything within the ternary could be changed to
avoid implicit promotions.
Justin, can you please send a v2 removing the casts withing
__builtin_constant_p (as in the diff I posted previously in this
thread) and with the below text added to the commit message:
Ternary expressions with then/else clauses with types smaller than int
undergo implicit promotion to int. Cast the result of the ternary back
to the expected __u16 to match the type when __HAVE_BUILTIN_BSWAP16__
is defined.
Also remove pointless casts within __builtin_constant_p argument lists.
--
Thanks,
~Nick Desaulniers
On Tue, Jun 7, 2022 at 3:20 PM Justin Stitt <[email protected]> wrote:
>
> if __HAVE_BUILTIN_BSWAP16__ is defined then __swab16 utilizes a __u16 cast.
> This same cast should be used if __HAVE_BUILTIN_BSWAP16__ is not defined as
> well. This should fix loads (at least a few) clang -Wformat warnings
> specifically with `ntohs()`
>
> Link: https://github.com/ClangBuiltLinux/linux/issues/378
> Signed-off-by: Justin Stitt <[email protected]>
I wonder why there's a cast inside the __builtin_constant_p parameter
list? Can't imagine that changing anything. I wonder if this should
be:
diff --git a/include/uapi/linux/swab.h b/include/uapi/linux/swab.h
index 7272f85d6d6a..0723a9cce747 100644
--- a/include/uapi/linux/swab.h
+++ b/include/uapi/linux/swab.h
@@ -102,7 +102,7 @@ static inline __attribute_const__ __u32
__fswahb32(__u32 val)
#define __swab16(x) (__u16)__builtin_bswap16((__u16)(x))
#else
#define __swab16(x) \
- (__builtin_constant_p((__u16)(x)) ? \
+ (__u16)(__builtin_constant_p(x) ? \
___constant_swab16(x) : \
__fswab16(x))
#endif
@@ -115,7 +115,7 @@ static inline __attribute_const__ __u32
__fswahb32(__u32 val)
#define __swab32(x) (__u32)__builtin_bswap32((__u32)(x))
#else
#define __swab32(x) \
- (__builtin_constant_p((__u32)(x)) ? \
+ (__u32)(__builtin_constant_p(x) ? \
___constant_swab32(x) : \
__fswab32(x))
#endif
@@ -128,7 +128,7 @@ static inline __attribute_const__ __u32
__fswahb32(__u32 val)
#define __swab64(x) (__u64)__builtin_bswap64((__u64)(x))
#else
#define __swab64(x) \
- (__builtin_constant_p((__u64)(x)) ? \
+ (__u64)(__builtin_constant_p(x) ? \
___constant_swab64(x) : \
__fswab64(x))
#endif
--
Thanks,
~Nick Desaulniers
On Tue, 7 Jun 2022 15:20:06 -0700 Justin Stitt <[email protected]> wrote:
> if __HAVE_BUILTIN_BSWAP16__ is defined then __swab16 utilizes a __u16 cast.
> This same cast should be used if __HAVE_BUILTIN_BSWAP16__ is not defined as
> well. This should fix loads (at least a few) clang -Wformat warnings
> specifically with `ntohs()`
>
> ...
>
> --- a/include/uapi/linux/swab.h
> +++ b/include/uapi/linux/swab.h
> @@ -102,7 +102,7 @@ static inline __attribute_const__ __u32 __fswahb32(__u32 val)
> #define __swab16(x) (__u16)__builtin_bswap16((__u16)(x))
> #else
> #define __swab16(x) \
> - (__builtin_constant_p((__u16)(x)) ? \
> + (__u16)(__builtin_constant_p((__u16)(x)) ? \
> ___constant_swab16(x) : \
> __fswab16(x))
> #endif
More explanation, please? Both ___constant_swab16() and __fswab16()
return __u16, so why does this patch have any effect?
A cast inside __builtin_constant_p doesn't do anything since it should evaluate
as constant at compile time irrespective of this cast. Instead, I moved this
cast outside the ternary to ensure the return type is as expected.
For instance, if __HAVE_BUILTIN_BSWAP16__ was not defined then __swab16 is
actually returning an `int` not a `u16` due to integer promotion as described
by Nick in this thread. This has repercussions when building with clang
-Wformat. This fix should solve many of these warnings.
Link: https://github.com/ClangBuiltLinux/linux/issues/378
Suggested-by: Nathan Chancellor <[email protected]>
Suggested-by: Nick Desaulniers <[email protected]>
Signed-off-by: Justin Stitt <[email protected]>
---
include/uapi/linux/swab.h | 12 ++++++------
1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/include/uapi/linux/swab.h b/include/uapi/linux/swab.h
index f6be3f2e6fee..ab5a1283800c 100644
--- a/include/uapi/linux/swab.h
+++ b/include/uapi/linux/swab.h
@@ -99,10 +99,10 @@ static inline __attribute_const__ __u32 __fswahb32(__u32 val)
* @x: value to byteswap
*/
#ifdef __HAVE_BUILTIN_BSWAP16__
-#define __swab16(x) (__u16)__builtin_bswap16((__u16)(x))
+#define __swab16(x) (__u16)__builtin_bswap16(x)
#else
#define __swab16(x) \
- (__u16)(__builtin_constant_p((__u16)(x)) ? \
+ (__u16)(__builtin_constant_p(x) ? \
___constant_swab16(x) : \
__fswab16(x))
#endif
@@ -112,10 +112,10 @@ static inline __attribute_const__ __u32 __fswahb32(__u32 val)
* @x: value to byteswap
*/
#ifdef __HAVE_BUILTIN_BSWAP32__
-#define __swab32(x) (__u32)__builtin_bswap32((__u32)(x))
+#define __swab32(x) (__u32)__builtin_bswap32(x)
#else
#define __swab32(x) \
- (__builtin_constant_p((__u32)(x)) ? \
+ (__u32)(__builtin_constant_p(x) ? \
___constant_swab32(x) : \
__fswab32(x))
#endif
@@ -125,10 +125,10 @@ static inline __attribute_const__ __u32 __fswahb32(__u32 val)
* @x: value to byteswap
*/
#ifdef __HAVE_BUILTIN_BSWAP64__
-#define __swab64(x) (__u64)__builtin_bswap64((__u64)(x))
+#define __swab64(x) (__u64)__builtin_bswap64(x)
#else
#define __swab64(x) \
- (__builtin_constant_p((__u64)(x)) ? \
+ (__u64)(__builtin_constant_p(x) ? \
___constant_swab64(x) : \
__fswab64(x))
#endif
--
2.30.2
On Wed, Jun 8, 2022 at 1:09 PM Andrew Morton <[email protected]> wrote:
>
> On Tue, 7 Jun 2022 17:14:22 -0700 Justin Stitt <[email protected]> wrote:
>
> > A cast inside __builtin_constant_p doesn't do anything since it should evaluate
> > as constant at compile time irrespective of this cast. Instead, I moved this
> > cast outside the ternary to ensure the return type is as expected.
> >
> > For instance, if __HAVE_BUILTIN_BSWAP16__ was not defined then __swab16 is
> > actually returning an `int` not a `u16` due to integer promotion as described
> > by Nick in this thread. This has repercussions when building with clang
> > -Wformat. This fix should solve many of these warnings.
> >
>
> ARM allmodconfig:
>
> In file included from ./include/linux/swab.h:5,
> from ./arch/arm/include/asm/opcodes.h:86,
> from ./arch/arm/include/asm/bug.h:7,
> from ./include/linux/bug.h:5,
> from ./include/linux/mmdebug.h:5,
> from ./include/linux/gfp.h:5,
> from ./include/linux/slab.h:15,
> from ./fs/xfs/kmem.h:9,
> from ./fs/xfs/xfs_linux.h:24,
> from ./fs/xfs/xfs.h:22,
> from fs/xfs/scrub/agheader.c:6:
> fs/xfs/scrub/agheader.c: In function 'xchk_superblock':
> ./include/uapi/linux/byteorder/little_endian.h:42:52: error: unsigned conversion from 'int' to 'short unsigned int' changes value from '-49265' to '16271' [-Werror=overflow]
> 42 | #define __cpu_to_be16(x) ((__force __be16)__swab16((x)))
> | ^~~
> ./include/uapi/linux/swab.h:102:46: note: in definition of macro '__swab16'
> 102 | #define __swab16(x) (__u16)__builtin_bswap16(x)
> | ^
> ./include/linux/byteorder/generic.h:96:21: note: in expansion of macro '__cpu_to_be16'
> 96 | #define cpu_to_be16 __cpu_to_be16
> | ^~~~~~~~~~~~~
> fs/xfs/scrub/agheader.c:158:23: note: in expansion of macro 'cpu_to_be16'
> 158 | vernum_mask = cpu_to_be16(~XFS_SB_VERSION_OKBITS |
> | ^~~~~~~~~~~
> cc1: all warnings being treated as errors
> make[2]: *** [scripts/Makefile.build:249: fs/xfs/scrub/agheader.o] Error 1
> make[1]: *** [scripts/Makefile.build:466: fs/xfs] Error 2
> make: *** [Makefile:1839: fs] Error 2
>
Ah, I see what went wrong.
Justin added more than I requested.
Compare the diffstat between:
https://lore.kernel.org/llvm/CAKwvOd=-NXR5HoHwwEUZMsCt90oaADH6XHifje9n-8S8rj9SFw@mail.gmail.com/
with the v2:
https://lore.kernel.org/llvm/[email protected]/
Justin, please send a v3 dropping the changes made to the
"__HAVE_BUILTIN_BSWAP*__ is defined" case; just make the changes I
suggested.
In addition, please add to the commit message this snippet from Al:
```
As Al Viro notes:
You *can't* get smaller-than-int out of ? :, same as you can't get it
out of addition, etc.
```
Finally, Justin, you can retest this by running:
$ ARCH=arm make LLVM=1 -j72 allmodconfig fs/xfs/scrub/agheader.o
with your patch applied to linux-next. (It wouldn't hurt to build the
whole thing...but it's next, which is red today for ARCH=arm so nvm:
https://github.com/ClangBuiltLinux/continuous-integration2/runs/6796317443?check_suite_focus=true).
https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git
--
Thanks,
~Nick Desaulniers
On Tue, 7 Jun 2022 17:14:22 -0700 Justin Stitt <[email protected]> wrote:
> A cast inside __builtin_constant_p doesn't do anything since it should evaluate
> as constant at compile time irrespective of this cast. Instead, I moved this
> cast outside the ternary to ensure the return type is as expected.
>
> For instance, if __HAVE_BUILTIN_BSWAP16__ was not defined then __swab16 is
> actually returning an `int` not a `u16` due to integer promotion as described
> by Nick in this thread. This has repercussions when building with clang
> -Wformat. This fix should solve many of these warnings.
>
ARM allmodconfig:
In file included from ./include/linux/swab.h:5,
from ./arch/arm/include/asm/opcodes.h:86,
from ./arch/arm/include/asm/bug.h:7,
from ./include/linux/bug.h:5,
from ./include/linux/mmdebug.h:5,
from ./include/linux/gfp.h:5,
from ./include/linux/slab.h:15,
from ./fs/xfs/kmem.h:9,
from ./fs/xfs/xfs_linux.h:24,
from ./fs/xfs/xfs.h:22,
from fs/xfs/scrub/agheader.c:6:
fs/xfs/scrub/agheader.c: In function 'xchk_superblock':
./include/uapi/linux/byteorder/little_endian.h:42:52: error: unsigned conversion from 'int' to 'short unsigned int' changes value from '-49265' to '16271' [-Werror=overflow]
42 | #define __cpu_to_be16(x) ((__force __be16)__swab16((x)))
| ^~~
./include/uapi/linux/swab.h:102:46: note: in definition of macro '__swab16'
102 | #define __swab16(x) (__u16)__builtin_bswap16(x)
| ^
./include/linux/byteorder/generic.h:96:21: note: in expansion of macro '__cpu_to_be16'
96 | #define cpu_to_be16 __cpu_to_be16
| ^~~~~~~~~~~~~
fs/xfs/scrub/agheader.c:158:23: note: in expansion of macro 'cpu_to_be16'
158 | vernum_mask = cpu_to_be16(~XFS_SB_VERSION_OKBITS |
| ^~~~~~~~~~~
cc1: all warnings being treated as errors
make[2]: *** [scripts/Makefile.build:249: fs/xfs/scrub/agheader.o] Error 1
make[1]: *** [scripts/Makefile.build:466: fs/xfs] Error 2
make: *** [Makefile:1839: fs] Error 2
On Tue, Jun 7, 2022 at 5:14 PM Justin Stitt <[email protected]> wrote:
>
> A cast inside __builtin_constant_p doesn't do anything since it should evaluate
> as constant at compile time irrespective of this cast. Instead, I moved this
> cast outside the ternary to ensure the return type is as expected.
>
> For instance, if __HAVE_BUILTIN_BSWAP16__ was not defined then __swab16 is
> actually returning an `int` not a `u16` due to integer promotion as described
> by Nick in this thread. This has repercussions when building with clang
Also, "this thread" won't make much sense when applied if someone is
looking at git log. Consider phrasing this instead as "in the lore
link below" then include another link tag to
Link: https://lore.kernel.org/llvm/CAKwvOdmXeRbFjkHgFXps4pLH6Q6pGWRNOqA85=h2aFnR=uaggg@mail.gmail.com/
Though, I think it's simply more concise to just include what Al said,
and drop this sentence altogether. You can send me v3 privately as an
RFC and I'll greenlight it before you resend to the list.
--
Thanks,
~Nick Desaulniers