* [PATCH] include/uapi/linux/swab.h: add __u16 cast to __swab16 conditional @ 2022-06-07 22:20 Justin Stitt 2022-06-07 22:27 ` Andrew Morton 2022-06-07 23:20 ` Nick Desaulniers 0 siblings, 2 replies; 13+ messages in thread From: Justin Stitt @ 2022-06-07 22:20 UTC (permalink / raw) To: Andrew Morton Cc: Nathan Chancellor, Nick Desaulniers, Tom Rix, linux-kernel, llvm, Justin Stitt 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 <jstitt007@gmail.com> --- 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 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH] include/uapi/linux/swab.h: add __u16 cast to __swab16 conditional 2022-06-07 22:20 [PATCH] include/uapi/linux/swab.h: add __u16 cast to __swab16 conditional Justin Stitt @ 2022-06-07 22:27 ` Andrew Morton 2022-06-07 22:42 ` Nick Desaulniers 2022-06-07 23:20 ` Nick Desaulniers 1 sibling, 1 reply; 13+ messages in thread From: Andrew Morton @ 2022-06-07 22:27 UTC (permalink / raw) To: Justin Stitt Cc: Nathan Chancellor, Nick Desaulniers, Tom Rix, linux-kernel, llvm On Tue, 7 Jun 2022 15:20:06 -0700 Justin Stitt <jstitt007@gmail.com> 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? ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] include/uapi/linux/swab.h: add __u16 cast to __swab16 conditional 2022-06-07 22:27 ` Andrew Morton @ 2022-06-07 22:42 ` Nick Desaulniers 2022-06-07 23:21 ` Andrew Morton 0 siblings, 1 reply; 13+ messages in thread From: Nick Desaulniers @ 2022-06-07 22:42 UTC (permalink / raw) To: Andrew Morton Cc: Justin Stitt, Nathan Chancellor, Tom Rix, linux-kernel, llvm, Richard Smith On Tue, Jun 7, 2022 at 3:27 PM Andrew Morton <akpm@linux-foundation.org> wrote: > > On Tue, 7 Jun 2022 15:20:06 -0700 Justin Stitt <jstitt007@gmail.com> 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. -- Thanks, ~Nick Desaulniers ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] include/uapi/linux/swab.h: add __u16 cast to __swab16 conditional 2022-06-07 22:42 ` Nick Desaulniers @ 2022-06-07 23:21 ` Andrew Morton 2022-06-07 23:43 ` Nick Desaulniers 2022-06-08 4:54 ` Al Viro 0 siblings, 2 replies; 13+ messages in thread From: Andrew Morton @ 2022-06-07 23:21 UTC (permalink / raw) To: Nick Desaulniers Cc: Justin Stitt, Nathan Chancellor, Tom Rix, linux-kernel, llvm, Richard Smith On Tue, 7 Jun 2022 15:42:56 -0700 Nick Desaulniers <ndesaulniers@google.com> wrote: > On Tue, Jun 7, 2022 at 3:27 PM Andrew Morton <akpm@linux-foundation.org> wrote: > > > > On Tue, 7 Jun 2022 15:20:06 -0700 Justin Stitt <jstitt007@gmail.com> 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? Or something else? What did we do wrong here and is it possible to correct our types rather than adding a cast? ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] include/uapi/linux/swab.h: add __u16 cast to __swab16 conditional 2022-06-07 23:21 ` Andrew Morton @ 2022-06-07 23:43 ` Nick Desaulniers 2022-06-08 4:54 ` Al Viro 1 sibling, 0 replies; 13+ messages in thread From: Nick Desaulniers @ 2022-06-07 23:43 UTC (permalink / raw) To: Andrew Morton, Justin Stitt Cc: Nathan Chancellor, Tom Rix, LKML, clang-built-linux, Richard Smith On Tue, Jun 7, 2022 at 4:21 PM Andrew Morton <akpm@linux-foundation.org> wrote: > > On Tue, 7 Jun 2022 15:42:56 -0700 Nick Desaulniers <ndesaulniers@google.com> wrote: > > > On Tue, Jun 7, 2022 at 3:27 PM Andrew Morton <akpm@linux-foundation.org> wrote: > > > > > > On Tue, 7 Jun 2022 15:20:06 -0700 Justin Stitt <jstitt007@gmail.com> 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] include/uapi/linux/swab.h: add __u16 cast to __swab16 conditional 2022-06-07 23:21 ` Andrew Morton 2022-06-07 23:43 ` Nick Desaulniers @ 2022-06-08 4:54 ` Al Viro 2022-06-08 19:35 ` Nick Desaulniers 1 sibling, 1 reply; 13+ messages in thread From: Al Viro @ 2022-06-08 4:54 UTC (permalink / raw) To: Andrew Morton Cc: Nick Desaulniers, Justin Stitt, Nathan Chancellor, Tom Rix, linux-kernel, llvm, Richard Smith On Tue, Jun 07, 2022 at 04:21:28PM -0700, Andrew Morton wrote: > > 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? Or something else? What did we do wrong here and is it > possible to correct our types rather than adding a cast? Not quite. Same rules as u16 + u16 - on architectures where int is wider than 16 bits it's (int)u16 + (int)u16 and yields int, on 16bit ones it's (unsigned int)u16 + (unsigned int)u16 and yields unsigned int. You *can't* get smaller-than-int out of ? :, same as you can't get it out of addition, etc. __builtin_choose_expr() would do it, but I would take a cast over that ugliness. FWIW, it might make sense for clang to keep track of the following property: expression has the same value as it would if integer promotions in it had been replaced with integer promotion of result. Example: with unsigned short x, y, mask; expresion "x & y" is interpreted as and_int((int)x, (int)y), which is equal to (int)and_u16(x, y), so that expression has the property in question. "x != 12 ? x : y" has the same property. "x + y", OTOH, doesn't - if x and y are both 32768, x + y is add_int((int)x, (int)y), i.e. 65536, while (int)add_u16(x, y) would be 0. For a somewhat more subtle example, (x & ~mask) | (y & mask) is interpreted as or_int(and_int((int)x, not_int((int)mask)), and_int((int)y, (int)mask)) which is equal to (int)or_u16(and_u16(x,not_u16(mask)), and_u16(y, mask)) IOW, the property in question holds for that one, despite having a subexpression (~mask) that does *NOT* have that property. (int)not_u16(0) is 0xffff and not_int((int)0) is (assuming 32bit int) 0xffffffff. Upper 16 bits get fouled; applying & with known-16bit launders them off... That predicate is behind the handling of small bitwise types in sparse; otherwise all operations on __be16 would trigger warnings due to promotions from __be16 to int. And aforementioned subtle example is common enough, so we had to deal with it. See commit d24967cb847b "[PATCH] handle fouled-bitwise" in sparse git... ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] include/uapi/linux/swab.h: add __u16 cast to __swab16 conditional 2022-06-08 4:54 ` Al Viro @ 2022-06-08 19:35 ` Nick Desaulniers 2022-06-10 7:19 ` David Laight 0 siblings, 1 reply; 13+ messages in thread From: Nick Desaulniers @ 2022-06-08 19:35 UTC (permalink / raw) To: Al Viro Cc: Andrew Morton, Justin Stitt, Nathan Chancellor, Tom Rix, linux-kernel, llvm, Richard Smith On Tue, Jun 7, 2022 at 9:54 PM Al Viro <viro@zeniv.linux.org.uk> wrote: > > On Tue, Jun 07, 2022 at 04:21:28PM -0700, Andrew Morton wrote: > > > > 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? Or something else? What did we do wrong here and is it > > possible to correct our types rather than adding a cast? > > Not quite. Same rules as u16 + u16 - on architectures where int is wider > than 16 bits it's (int)u16 + (int)u16 and yields int, on 16bit ones it's > (unsigned int)u16 + (unsigned int)u16 and yields unsigned int. > > You *can't* get smaller-than-int out of ? :, same as you can't get it > out of addition, etc. Exactly, and well put. More concise than I was able to express. I think that description will satisfy Andrew's request for additional context, so I'll recommend Justin add a blurb derived from what you said when sending a v3. > > __builtin_choose_expr() would do it, but I would take a cast over that > ugliness. > > FWIW, it might make sense for clang to keep track of the following > property: expression has the same value as it would if integer promotions > in it had been replaced with integer promotion of result. I'm not sure that's precisely the same issue here. The issue we're facing is more so that `ntohs` is being used in printf-like expressions; clang's -Wformat warns about default argument promotion so we need to clean up cases where smaller-than-int format flags are being used for promoted-to-int params. While looking at that, Nathan noticed that __swab16 will return either a __u16 or an int based on whether __HAVE_BUILTIN_BSWAP16__ is defined, which depends on BOTH the compiler being used and target architecture. This patch from Justin just cleans that up. > > Example: with > unsigned short x, y, mask; > > expresion "x & y" is interpreted as and_int((int)x, (int)y), which is equal > to (int)and_u16(x, y), so that expression has the property in question. > "x != 12 ? x : y" has the same property. "x + y", OTOH, doesn't - if x and y > are both 32768, x + y is add_int((int)x, (int)y), i.e. 65536, while > (int)add_u16(x, y) would be 0. > > For a somewhat more subtle example, > (x & ~mask) | (y & mask) > is interpreted as > or_int(and_int((int)x, not_int((int)mask)), and_int((int)y, (int)mask)) > which is equal to > (int)or_u16(and_u16(x,not_u16(mask)), and_u16(y, mask)) > IOW, the property in question holds for that one, despite having a subexpression > (~mask) that does *NOT* have that property. (int)not_u16(0) is 0xffff and > not_int((int)0) is (assuming 32bit int) 0xffffffff. Upper 16 bits get fouled; > applying & with known-16bit launders them off... > > That predicate is behind the handling of small bitwise types in sparse; > otherwise all operations on __be16 would trigger warnings due to promotions > from __be16 to int. And aforementioned subtle example is common enough, so we > had to deal with it. See commit d24967cb847b "[PATCH] handle fouled-bitwise" > in sparse git... https://git.kernel.org/pub/scm/devel/sparse/sparse.git/commit/?id=d24967cb847b7a04920698a9053ea8195046a831 (For others' reference) -- Thanks, ~Nick Desaulniers ^ permalink raw reply [flat|nested] 13+ messages in thread
* RE: [PATCH] include/uapi/linux/swab.h: add __u16 cast to __swab16 conditional 2022-06-08 19:35 ` Nick Desaulniers @ 2022-06-10 7:19 ` David Laight 0 siblings, 0 replies; 13+ messages in thread From: David Laight @ 2022-06-10 7:19 UTC (permalink / raw) To: 'Nick Desaulniers', Al Viro Cc: Andrew Morton, Justin Stitt, Nathan Chancellor, Tom Rix, linux-kernel, llvm, Richard Smith From: Nick Desaulniers > Sent: 08 June 2022 20:35 .... > The issue we're facing is more so that `ntohs` is being used in > printf-like expressions; clang's -Wformat warns about default argument > promotion so we need to clean up cases where smaller-than-int format > flags are being used for promoted-to-int params. While looking at > that, Nathan noticed that __swab16 will return either a __u16 or an > int based on whether __HAVE_BUILTIN_BSWAP16__ is defined, which > depends on BOTH the compiler being used and target architecture. This > patch from Justin just cleans that up. The 'problem' is that the (__u16) cast is likely to add an extra '& 0xffff' instruction that is almost certainly not required. OTOH the lack of this masking has always been a difference between htons() on BE and LE systems. But clang is also being over-enthusiastic with its warnings. IIRC varargs parameters always get integer promotion. So if %hd ever makes sense for printf then it implies a mask inside printf. David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales) ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] include/uapi/linux/swab.h: add __u16 cast to __swab16 conditional 2022-06-07 22:20 [PATCH] include/uapi/linux/swab.h: add __u16 cast to __swab16 conditional Justin Stitt 2022-06-07 22:27 ` Andrew Morton @ 2022-06-07 23:20 ` Nick Desaulniers 2022-06-08 0:14 ` [PATCH v2] include/uapi/linux/swab.h: move explicit cast outside ternary Justin Stitt 1 sibling, 1 reply; 13+ messages in thread From: Nick Desaulniers @ 2022-06-07 23:20 UTC (permalink / raw) To: Justin Stitt, Andrew Morton Cc: Nathan Chancellor, Tom Rix, linux-kernel, llvm On Tue, Jun 7, 2022 at 3:20 PM Justin Stitt <jstitt007@gmail.com> 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 <jstitt007@gmail.com> 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 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH v2] include/uapi/linux/swab.h: move explicit cast outside ternary 2022-06-07 23:20 ` Nick Desaulniers @ 2022-06-08 0:14 ` Justin Stitt 2022-06-08 20:09 ` Andrew Morton 2022-06-08 21:19 ` Nick Desaulniers 0 siblings, 2 replies; 13+ messages in thread From: Justin Stitt @ 2022-06-08 0:14 UTC (permalink / raw) To: ndesaulniers, akpm; +Cc: jstitt007, linux-kernel, llvm, nathan, trix 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 <nathan@kernel.org> Suggested-by: Nick Desaulniers <ndesaulniers@google.com> Signed-off-by: Justin Stitt <jstitt007@gmail.com> --- 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 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH v2] include/uapi/linux/swab.h: move explicit cast outside ternary 2022-06-08 0:14 ` [PATCH v2] include/uapi/linux/swab.h: move explicit cast outside ternary Justin Stitt @ 2022-06-08 20:09 ` Andrew Morton 2022-06-08 21:16 ` Nick Desaulniers 2022-06-08 21:19 ` Nick Desaulniers 1 sibling, 1 reply; 13+ messages in thread From: Andrew Morton @ 2022-06-08 20:09 UTC (permalink / raw) To: Justin Stitt; +Cc: ndesaulniers, linux-kernel, llvm, nathan, trix On Tue, 7 Jun 2022 17:14:22 -0700 Justin Stitt <jstitt007@gmail.com> 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2] include/uapi/linux/swab.h: move explicit cast outside ternary 2022-06-08 20:09 ` Andrew Morton @ 2022-06-08 21:16 ` Nick Desaulniers 0 siblings, 0 replies; 13+ messages in thread From: Nick Desaulniers @ 2022-06-08 21:16 UTC (permalink / raw) To: Andrew Morton, Justin Stitt Cc: linux-kernel, llvm, nathan, trix, Alexander Viro On Wed, Jun 8, 2022 at 1:09 PM Andrew Morton <akpm@linux-foundation.org> wrote: > > On Tue, 7 Jun 2022 17:14:22 -0700 Justin Stitt <jstitt007@gmail.com> 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/20220608001422.26383-1-jstitt007@gmail.com/ 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2] include/uapi/linux/swab.h: move explicit cast outside ternary 2022-06-08 0:14 ` [PATCH v2] include/uapi/linux/swab.h: move explicit cast outside ternary Justin Stitt 2022-06-08 20:09 ` Andrew Morton @ 2022-06-08 21:19 ` Nick Desaulniers 1 sibling, 0 replies; 13+ messages in thread From: Nick Desaulniers @ 2022-06-08 21:19 UTC (permalink / raw) To: Justin Stitt; +Cc: akpm, linux-kernel, llvm, nathan, trix On Tue, Jun 7, 2022 at 5:14 PM Justin Stitt <jstitt007@gmail.com> 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2022-06-10 7:19 UTC | newest] Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-06-07 22:20 [PATCH] include/uapi/linux/swab.h: add __u16 cast to __swab16 conditional Justin Stitt 2022-06-07 22:27 ` Andrew Morton 2022-06-07 22:42 ` Nick Desaulniers 2022-06-07 23:21 ` Andrew Morton 2022-06-07 23:43 ` Nick Desaulniers 2022-06-08 4:54 ` Al Viro 2022-06-08 19:35 ` Nick Desaulniers 2022-06-10 7:19 ` David Laight 2022-06-07 23:20 ` Nick Desaulniers 2022-06-08 0:14 ` [PATCH v2] include/uapi/linux/swab.h: move explicit cast outside ternary Justin Stitt 2022-06-08 20:09 ` Andrew Morton 2022-06-08 21:16 ` Nick Desaulniers 2022-06-08 21:19 ` Nick Desaulniers
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.