[PATCH v3] include/uapi/linux/swab.h: move explicit cast outside ternary

[PATCH v3] include/uapi/linux/swab.h: move explicit cast outside ternary

[Justin Stitt]

From: Justin Stitt <jstitt007@gmail.com>

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.

Additionally, if __HAVE_BUILTIN_BSWAP16__ was not defined then __swab16
is actually returning an `int` not a `u16` due to integer promotion.

As Al Viro notes:
You *can't* get smaller-than-int out of ? :, same as you can't get it
out of addition, etc.

This also fixes some clang -Wformat warnings involving default
argument promotion.

Link: https://github.com/ClangBuiltLinux/linux/issues/378
Suggested-by: Al Viro <viro@zeniv.linux.org.uk>
Suggested-by: Nathan Chancellor <nathan@kernel.org>
Suggested-by: Nick Desaulniers <ndesaulniers@google.com>
Signed-off-by: Justin Stitt <jstitt007@gmail.com>
---
 diff from v2 -> v3:
  * re-insert respective (u16, u32, u64) cast to __builtin_bswap as per
     Nick's suggestion
  * added note from Al Viro

 include/uapi/linux/swab.h | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

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
-- 
2.36.1.476.g0c4daa206d-goog

Re: [PATCH v3] include/uapi/linux/swab.h: move explicit cast outside ternary

[Nathan Chancellor]

On Wed, Jun 08, 2022 at 03:35:39PM -0700, Justin Stitt wrote:
> From: Justin Stitt <jstitt007@gmail.com>
> 
> 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.
> 
> Additionally, if __HAVE_BUILTIN_BSWAP16__ was not defined then __swab16
> is actually returning an `int` not a `u16` due to integer promotion.
> 
> As Al Viro notes:
> You *can't* get smaller-than-int out of ? :, same as you can't get it
> out of addition, etc.
> 
> This also fixes some clang -Wformat warnings involving default
> argument promotion.
> 
> Link: https://github.com/ClangBuiltLinux/linux/issues/378
> Suggested-by: Al Viro <viro@zeniv.linux.org.uk>
> Suggested-by: Nathan Chancellor <nathan@kernel.org>
> Suggested-by: Nick Desaulniers <ndesaulniers@google.com>
> Signed-off-by: Justin Stitt <jstitt007@gmail.com>

Thanks for the patch and sticking with it through all the review!

Reviewed-by: Nathan Chancellor <nathan@kernel.org>

> ---
>  diff from v2 -> v3:
>   * re-insert respective (u16, u32, u64) cast to __builtin_bswap as per
>      Nick's suggestion
>   * added note from Al Viro
> 
>  include/uapi/linux/swab.h | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> 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
> -- 
> 2.36.1.476.g0c4daa206d-goog
> 
> 

Re: [PATCH v3] include/uapi/linux/swab.h: move explicit cast outside ternary

[Justin Stitt]

Any chance a maintainer could take a look at this patch? I am trying
to get it through this cycle and we are so close to enabling the
-Wformat option for Clang. This patch fixes over 50 warnings and
there's only a handful of patches remaining until we can enable
-Wformat.

Re: [PATCH v3] include/uapi/linux/swab.h: move explicit cast outside ternary

[Nathan Chancellor]

On Mon, Jul 18, 2022 at 11:12:25AM -0700, Justin Stitt wrote:
> Any chance a maintainer could take a look at this patch? I am trying
> to get it through this cycle and we are so close to enabling the
> -Wformat option for Clang. This patch fixes over 50 warnings and
> there's only a handful of patches remaining until we can enable
> -Wformat.

I think this change is already picked up? It's in -next.

https://git.kernel.org/akpm/mm/c/d30dfd490f7dc4cb6a7c11a647bd1ff7a22139e7
https://git.kernel.org/next/linux-next/c/d30dfd490f7dc4cb6a7c11a647bd1ff7a22139e7

Cheers,
Nathan

Re: [PATCH v3] include/uapi/linux/swab.h: move explicit cast outside ternary

[Justin Stitt]

On Mon, Jul 18, 2022 at 11:22 AM Nathan Chancellor <nathan@kernel.org> wrote:
>
> On Mon, Jul 18, 2022 at 11:12:25AM -0700, Justin Stitt wrote:
> > Any chance a maintainer could take a look at this patch? I am trying
> > to get it through this cycle and we are so close to enabling the
> > -Wformat option for Clang. This patch fixes over 50 warnings and
> > there's only a handful of patches remaining until we can enable
> > -Wformat.
>
> I think this change is already picked up? It's in -next.

Oh, awesome!

> https://git.kernel.org/akpm/mm/c/d30dfd490f7dc4cb6a7c11a647bd1ff7a22139e7
> https://git.kernel.org/next/linux-next/c/d30dfd490f7dc4cb6a7c11a647bd1ff7a22139e7

How did you track this down? I wasn't able to find any references to
my patch in -next through kernel.org.

> Cheers,
> Nathan

Re: [PATCH v3] include/uapi/linux/swab.h: move explicit cast outside ternary

[Nathan Chancellor]

On Mon, Jul 18, 2022 at 11:25:27AM -0700, Justin Stitt wrote:
> On Mon, Jul 18, 2022 at 11:22 AM Nathan Chancellor <nathan@kernel.org> wrote:
> >
> > On Mon, Jul 18, 2022 at 11:12:25AM -0700, Justin Stitt wrote:
> > > Any chance a maintainer could take a look at this patch? I am trying
> > > to get it through this cycle and we are so close to enabling the
> > > -Wformat option for Clang. This patch fixes over 50 warnings and
> > > there's only a handful of patches remaining until we can enable
> > > -Wformat.
> >
> > I think this change is already picked up? It's in -next.
> 
> Oh, awesome!
> 
> > https://git.kernel.org/akpm/mm/c/d30dfd490f7dc4cb6a7c11a647bd1ff7a22139e7
> > https://git.kernel.org/next/linux-next/c/d30dfd490f7dc4cb6a7c11a647bd1ff7a22139e7
> 
> How did you track this down? I wasn't able to find any references to
> my patch in -next through kernel.org.

If you have a copy of the linux-next repo, you can just run:

  $ git log --author "Justin Stitt" --oneline

after updating it (or 'git log <file>', whichever works) to quickly
survey.

Through the cgit web interface on kernel.org, the easiest way would be
searching your name in the upper right search bar with the "Author"
option. Otherwise, I typically look through the git log of a particular
file by appending "log/" + the full path of the file to the repository
URL like so:

  https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/log/include/uapi/linux/swab.h

Sometimes maintainer trees might not funnel into -next (they should be)
but your patch might still be picked up. You can pass the '--scm'
argument to get_maintainer.pl to see if the MAINTAINERS file has a git
tree associated with it and do the same thing as above to see if the
patch has been accepted (you might need to analyze the different
branches).

Cheers,
Nathan