From: Justin Stitt <[email protected]>
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 <[email protected]>
Suggested-by: Nathan Chancellor <[email protected]>
Suggested-by: Nick Desaulniers <[email protected]>
Signed-off-by: Justin Stitt <[email protected]>
---
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
On Wed, Jun 08, 2022 at 03:35:39PM -0700, Justin Stitt wrote:
> From: Justin Stitt <[email protected]>
>
> 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 <[email protected]>
> Suggested-by: Nathan Chancellor <[email protected]>
> Suggested-by: Nick Desaulniers <[email protected]>
> Signed-off-by: Justin Stitt <[email protected]>
Thanks for the patch and sticking with it through all the review!
Reviewed-by: Nathan Chancellor <[email protected]>
> ---
> 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
>
>
On Mon, Jul 18, 2022 at 11:22 AM Nathan Chancellor <[email protected]> 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
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
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.
On Mon, Jul 18, 2022 at 11:25:27AM -0700, Justin Stitt wrote:
> On Mon, Jul 18, 2022 at 11:22 AM Nathan Chancellor <[email protected]> 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