include/uapi/linux/swab.h | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-)
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.2On 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
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
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
© 2016 - 2026 Red Hat, Inc.