[PATCH] linux/compiler-clang.h: define HAVE_BUILTIN_BSWAP*

[PATCH] linux/compiler-clang.h: define HAVE_BUILTIN_BSWAP*

[Arnd Bergmann]

From: Arnd Bergmann <arnd@arndb.de>

Separating compiler-clang.h from compiler-gcc.h inadventently dropped the
definitions of the three HAVE_BUILTIN_BSWAP macros, which requires falling
back to the open-coded version and hoping that the compiler detects it.

Since all versions of clang support the __builtin_bswap interfaces,
add back the flags and have the headers pick these up automatically.

This results in a 4% improvement of compilation speed for arm defconfig.

Fixes: 815f0ddb346c ("include/linux/compiler*.h: make compiler-*.h mutually exclusive")
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
 include/linux/compiler-clang.h | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/include/linux/compiler-clang.h b/include/linux/compiler-clang.h
index 6478bff6fcc2..bbfa9ff6a2ec 100644
--- a/include/linux/compiler-clang.h
+++ b/include/linux/compiler-clang.h
@@ -33,6 +33,16 @@
 #define __no_sanitize_thread
 #endif
 
+/*
+ * sparse (__CHECKER__) pretends to be gcc, but can't do constant
+ * folding in __builtin_bswap*() (yet), so don't set these for it.
+ */
+#if defined(CONFIG_ARCH_USE_BUILTIN_BSWAP) && !defined(__CHECKER__)
+#define __HAVE_BUILTIN_BSWAP32__
+#define __HAVE_BUILTIN_BSWAP64__
+#define __HAVE_BUILTIN_BSWAP16__
+#endif /* CONFIG_ARCH_USE_BUILTIN_BSWAP && !__CHECKER__ */
+
 #if __has_feature(undefined_behavior_sanitizer)
 /* GCC does not have __SANITIZE_UNDEFINED__ */
 #define __no_sanitize_undefined \
-- 
2.29.2

Re: [PATCH] linux/compiler-clang.h: define HAVE_BUILTIN_BSWAP*

[Nathan Chancellor]

On Thu, Feb 25, 2021 at 05:45:09PM +0100, Arnd Bergmann wrote:
> From: Arnd Bergmann <arnd@arndb.de>
> 
> Separating compiler-clang.h from compiler-gcc.h inadventently dropped the
> definitions of the three HAVE_BUILTIN_BSWAP macros, which requires falling
> back to the open-coded version and hoping that the compiler detects it.
> 
> Since all versions of clang support the __builtin_bswap interfaces,
> add back the flags and have the headers pick these up automatically.
> 
> This results in a 4% improvement of compilation speed for arm defconfig.
> 
> Fixes: 815f0ddb346c ("include/linux/compiler*.h: make compiler-*.h mutually exclusive")
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>

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

> ---
>  include/linux/compiler-clang.h | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/include/linux/compiler-clang.h b/include/linux/compiler-clang.h
> index 6478bff6fcc2..bbfa9ff6a2ec 100644
> --- a/include/linux/compiler-clang.h
> +++ b/include/linux/compiler-clang.h
> @@ -33,6 +33,16 @@
>  #define __no_sanitize_thread
>  #endif
>  
> +/*
> + * sparse (__CHECKER__) pretends to be gcc, but can't do constant
> + * folding in __builtin_bswap*() (yet), so don't set these for it.
> + */
> +#if defined(CONFIG_ARCH_USE_BUILTIN_BSWAP) && !defined(__CHECKER__)
> +#define __HAVE_BUILTIN_BSWAP32__
> +#define __HAVE_BUILTIN_BSWAP64__
> +#define __HAVE_BUILTIN_BSWAP16__
> +#endif /* CONFIG_ARCH_USE_BUILTIN_BSWAP && !__CHECKER__ */
> +
>  #if __has_feature(undefined_behavior_sanitizer)
>  /* GCC does not have __SANITIZE_UNDEFINED__ */
>  #define __no_sanitize_undefined \
> -- 
> 2.29.2
> 

Re: [PATCH] linux/compiler-clang.h: define HAVE_BUILTIN_BSWAP*

[Kees Cook]

On Thu, Feb 25, 2021 at 05:45:09PM +0100, Arnd Bergmann wrote:
> From: Arnd Bergmann <arnd@arndb.de>
> 
> Separating compiler-clang.h from compiler-gcc.h inadventently dropped the
> definitions of the three HAVE_BUILTIN_BSWAP macros, which requires falling
> back to the open-coded version and hoping that the compiler detects it.
> 
> Since all versions of clang support the __builtin_bswap interfaces,
> add back the flags and have the headers pick these up automatically.
> 
> This results in a 4% improvement of compilation speed for arm defconfig.
> 
> Fixes: 815f0ddb346c ("include/linux/compiler*.h: make compiler-*.h mutually exclusive")
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>

Cc: stable@vger.kernel.org
Reviewed-by: Kees Cook <keescook@chromium.org>

-- 
Kees Cook

Re: [PATCH] linux/compiler-clang.h: define HAVE_BUILTIN_BSWAP*

[Andrew Morton]

On Thu, 25 Feb 2021 12:03:48 -0800 Kees Cook <keescook@chromium.org> wrote:

> On Thu, Feb 25, 2021 at 05:45:09PM +0100, Arnd Bergmann wrote:
> > From: Arnd Bergmann <arnd@arndb.de>
> > 
> > Separating compiler-clang.h from compiler-gcc.h inadventently dropped the
> > definitions of the three HAVE_BUILTIN_BSWAP macros, which requires falling
> > back to the open-coded version and hoping that the compiler detects it.
> > 
> > Since all versions of clang support the __builtin_bswap interfaces,
> > add back the flags and have the headers pick these up automatically.
> > 
> > This results in a 4% improvement of compilation speed for arm defconfig.
> > 
> > Fixes: 815f0ddb346c ("include/linux/compiler*.h: make compiler-*.h mutually exclusive")
> > Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> 
> Cc: stable@vger.kernel.org

I figured 4% better compile time isn't significant enough to justify a
backport.  Thoughts?

> Reviewed-by: Kees Cook <keescook@chromium.org>

Re: [PATCH] linux/compiler-clang.h: define HAVE_BUILTIN_BSWAP*

[Kees Cook]

On Thu, Feb 25, 2021 at 12:06:37PM -0800, Andrew Morton wrote:
> On Thu, 25 Feb 2021 12:03:48 -0800 Kees Cook <keescook@chromium.org> wrote:
> 
> > On Thu, Feb 25, 2021 at 05:45:09PM +0100, Arnd Bergmann wrote:
> > > From: Arnd Bergmann <arnd@arndb.de>
> > > 
> > > Separating compiler-clang.h from compiler-gcc.h inadventently dropped the
> > > definitions of the three HAVE_BUILTIN_BSWAP macros, which requires falling
> > > back to the open-coded version and hoping that the compiler detects it.
> > > 
> > > Since all versions of clang support the __builtin_bswap interfaces,
> > > add back the flags and have the headers pick these up automatically.
> > > 
> > > This results in a 4% improvement of compilation speed for arm defconfig.
> > > 
> > > Fixes: 815f0ddb346c ("include/linux/compiler*.h: make compiler-*.h mutually exclusive")
> > > Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> > 
> > Cc: stable@vger.kernel.org
> 
> I figured 4% better compile time isn't significant enough to justify a
> backport.  Thoughts?

It's a trivial change, so I think it's worth it?

> 
> > Reviewed-by: Kees Cook <keescook@chromium.org>
> 

-- 
Kees Cook

Re: [PATCH] linux/compiler-clang.h: define HAVE_BUILTIN_BSWAP*

[Nathan Chancellor]

On Thu, Feb 25, 2021 at 12:14:17PM -0800, Kees Cook wrote:
> On Thu, Feb 25, 2021 at 12:06:37PM -0800, Andrew Morton wrote:
> > On Thu, 25 Feb 2021 12:03:48 -0800 Kees Cook <keescook@chromium.org> wrote:
> > 
> > > On Thu, Feb 25, 2021 at 05:45:09PM +0100, Arnd Bergmann wrote:
> > > > From: Arnd Bergmann <arnd@arndb.de>
> > > > 
> > > > Separating compiler-clang.h from compiler-gcc.h inadventently dropped the
> > > > definitions of the three HAVE_BUILTIN_BSWAP macros, which requires falling
> > > > back to the open-coded version and hoping that the compiler detects it.
> > > > 
> > > > Since all versions of clang support the __builtin_bswap interfaces,
> > > > add back the flags and have the headers pick these up automatically.
> > > > 
> > > > This results in a 4% improvement of compilation speed for arm defconfig.
> > > > 
> > > > Fixes: 815f0ddb346c ("include/linux/compiler*.h: make compiler-*.h mutually exclusive")
> > > > Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> > > 
> > > Cc: stable@vger.kernel.org
> > 
> > I figured 4% better compile time isn't significant enough to justify a
> > backport.  Thoughts?
> 
> It's a trivial change, so I think it's worth it?

Indeed. Any wins that we can get with compile time, we should take.
Clang is being widely used in production systems now so I feel like with
a trivial change plus user visible impact, it should be backported.

Not to mention that the generated code in theory should be better
because it is the compiler's builtin, rather than a hand rolled one, AND
this is technically a regression, given that it worked before compiler.h
was split.

Cheers,
Nathan

> > 
> > > Reviewed-by: Kees Cook <keescook@chromium.org>
> > 
> 
> -- 
> Kees Cook

Re: [PATCH] linux/compiler-clang.h: define HAVE_BUILTIN_BSWAP*

[Nick Desaulniers]

On Thu, Feb 25, 2021 at 12:06 PM Andrew Morton
<akpm@linux-foundation.org> wrote:
>
> On Thu, 25 Feb 2021 12:03:48 -0800 Kees Cook <keescook@chromium.org> wrote:
>
> > On Thu, Feb 25, 2021 at 05:45:09PM +0100, Arnd Bergmann wrote:
> > > From: Arnd Bergmann <arnd@arndb.de>
> > >
> > > Separating compiler-clang.h from compiler-gcc.h inadventently dropped the
> > > definitions of the three HAVE_BUILTIN_BSWAP macros, which requires falling
> > > back to the open-coded version and hoping that the compiler detects it.
> > >
> > > Since all versions of clang support the __builtin_bswap interfaces,
> > > add back the flags and have the headers pick these up automatically.
> > >
> > > This results in a 4% improvement of compilation speed for arm defconfig.
> > >
> > > Fixes: 815f0ddb346c ("include/linux/compiler*.h: make compiler-*.h mutually exclusive")
> > > Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> >
> > Cc: stable@vger.kernel.org
>
> I figured 4% better compile time isn't significant enough to justify a
> backport.  Thoughts?

If I made a mistake in 815f0ddb346c, then it would be important to
correct it since 815f0ddb346c has existed for a few stable branches
(first landed in v4.19-rc1).
Acked-by: Nick Desaulniers <ndesaulniers@google.com>

>
> > Reviewed-by: Kees Cook <keescook@chromium.org>
>


-- 
Thanks,
~Nick Desaulniers

Re: [PATCH] linux/compiler-clang.h: define HAVE_BUILTIN_BSWAP*

[Miguel Ojeda]

On Thu, Feb 25, 2021 at 9:14 PM Kees Cook <keescook@chromium.org> wrote:
>
> It's a trivial change, so I think it's worth it?

It is simple, but it is not trivial since it may change codegen.

Acked-by: Miguel Ojeda <ojeda@kernel.org>

Cheers,
Miguel

Re: [PATCH] linux/compiler-clang.h: define HAVE_BUILTIN_BSWAP*

[Miguel Ojeda]

On Thu, Feb 25, 2021 at 9:18 PM Nathan Chancellor <nathan@kernel.org> wrote:
>
> Indeed. Any wins that we can get with compile time, we should take.
> Clang is being widely used in production systems now so I feel like with
> a trivial change plus user visible impact, it should be backported.
>
> Not to mention that the generated code in theory should be better
> because it is the compiler's builtin, rather than a hand rolled one, AND
> this is technically a regression, given that it worked before compiler.h
> was split.

Compilation speed shouldn't be an argument for a stable change unless
it is something egregious like a 50% that may affect users or tightly
timed CIs.

Fixing an important runtime regression is a stronger argument, but the
patch doesn't show what the effects are, so it isn't justified (yet).

Please note that this kind of change touches a lot of code all over
the place, which could always trigger other runtime regressions or
even bad codegen (yes, very unlikely, but it happens).

Cheers,
Miguel

Re: [PATCH] linux/compiler-clang.h: define HAVE_BUILTIN_BSWAP*

[Luc Van Oostenryck]

On Thu, Feb 25, 2021 at 05:45:09PM +0100, Arnd Bergmann wrote:
> From: Arnd Bergmann <arnd@arndb.de>
> 
> Fixes: 815f0ddb346c ("include/linux/compiler*.h: make compiler-*.h mutually exclusive")
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> ---
>  include/linux/compiler-clang.h | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/include/linux/compiler-clang.h b/include/linux/compiler-clang.h
> index 6478bff6fcc2..bbfa9ff6a2ec 100644
> --- a/include/linux/compiler-clang.h
> +++ b/include/linux/compiler-clang.h
> @@ -33,6 +33,16 @@
>  #define __no_sanitize_thread
>  #endif
>  
> +/*
> + * sparse (__CHECKER__) pretends to be gcc, but can't do constant
> + * folding in __builtin_bswap*() (yet), so don't set these for it.
> + */

This is not true anymore since 2017. Also, a much recent version of
Sparse is needed for _Generic(), for example).

Can you remove the comment and the test for __CHECKER__?

Best regards,
-- Luc