* Re: [PATCH] linux/compiler-clang.h: define HAVE_BUILTIN_BSWAP*
2021-02-25 16:45 [PATCH] linux/compiler-clang.h: define HAVE_BUILTIN_BSWAP* Arnd Bergmann
@ 2021-02-25 19:29 ` Nathan Chancellor
2021-02-25 20:03 ` Kees Cook
2021-02-26 7:47 ` Luc Van Oostenryck
2 siblings, 0 replies; 10+ messages in thread
From: Nathan Chancellor @ 2021-02-25 19:29 UTC (permalink / raw)
To: Arnd Bergmann
Cc: Masahiro Yamada, Nick Desaulniers, Arnd Bergmann, Miguel Ojeda,
Kees Cook, Andrew Morton, Marco Elver, Sami Tolvanen,
Arvind Sankar, Randy Dunlap, clang-built-linux, linux-kernel
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
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] linux/compiler-clang.h: define HAVE_BUILTIN_BSWAP*
2021-02-25 16:45 [PATCH] linux/compiler-clang.h: define HAVE_BUILTIN_BSWAP* Arnd Bergmann
2021-02-25 19:29 ` Nathan Chancellor
@ 2021-02-25 20:03 ` Kees Cook
2021-02-25 20:06 ` Andrew Morton
2021-02-26 7:47 ` Luc Van Oostenryck
2 siblings, 1 reply; 10+ messages in thread
From: Kees Cook @ 2021-02-25 20:03 UTC (permalink / raw)
To: Arnd Bergmann
Cc: Masahiro Yamada, Nathan Chancellor, Nick Desaulniers,
Arnd Bergmann, Miguel Ojeda, Andrew Morton, Marco Elver,
Sami Tolvanen, Arvind Sankar, Randy Dunlap, clang-built-linux,
linux-kernel
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] linux/compiler-clang.h: define HAVE_BUILTIN_BSWAP*
2021-02-25 20:03 ` Kees Cook
@ 2021-02-25 20:06 ` Andrew Morton
2021-02-25 20:14 ` Kees Cook
2021-02-25 21:01 ` Nick Desaulniers
0 siblings, 2 replies; 10+ messages in thread
From: Andrew Morton @ 2021-02-25 20:06 UTC (permalink / raw)
To: Kees Cook
Cc: Arnd Bergmann, Masahiro Yamada, Nathan Chancellor,
Nick Desaulniers, Arnd Bergmann, Miguel Ojeda, Marco Elver,
Sami Tolvanen, Arvind Sankar, Randy Dunlap, clang-built-linux,
linux-kernel
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>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] linux/compiler-clang.h: define HAVE_BUILTIN_BSWAP*
2021-02-25 20:06 ` Andrew Morton
@ 2021-02-25 20:14 ` Kees Cook
2021-02-25 20:18 ` Nathan Chancellor
2021-02-26 3:34 ` Miguel Ojeda
2021-02-25 21:01 ` Nick Desaulniers
1 sibling, 2 replies; 10+ messages in thread
From: Kees Cook @ 2021-02-25 20:14 UTC (permalink / raw)
To: Andrew Morton
Cc: Arnd Bergmann, Masahiro Yamada, Nathan Chancellor,
Nick Desaulniers, Arnd Bergmann, Miguel Ojeda, Marco Elver,
Sami Tolvanen, Arvind Sankar, Randy Dunlap, clang-built-linux,
linux-kernel
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] linux/compiler-clang.h: define HAVE_BUILTIN_BSWAP*
2021-02-25 20:14 ` Kees Cook
@ 2021-02-25 20:18 ` Nathan Chancellor
2021-02-26 3:58 ` Miguel Ojeda
2021-02-26 3:34 ` Miguel Ojeda
1 sibling, 1 reply; 10+ messages in thread
From: Nathan Chancellor @ 2021-02-25 20:18 UTC (permalink / raw)
To: Kees Cook
Cc: Andrew Morton, Arnd Bergmann, Masahiro Yamada, Nick Desaulniers,
Arnd Bergmann, Miguel Ojeda, Marco Elver, Sami Tolvanen,
Arvind Sankar, Randy Dunlap, clang-built-linux, linux-kernel
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] linux/compiler-clang.h: define HAVE_BUILTIN_BSWAP*
2021-02-25 20:18 ` Nathan Chancellor
@ 2021-02-26 3:58 ` Miguel Ojeda
0 siblings, 0 replies; 10+ messages in thread
From: Miguel Ojeda @ 2021-02-26 3:58 UTC (permalink / raw)
To: Nathan Chancellor
Cc: Kees Cook, Andrew Morton, Arnd Bergmann, Masahiro Yamada,
Nick Desaulniers, Arnd Bergmann, Miguel Ojeda, Marco Elver,
Sami Tolvanen, Arvind Sankar, Randy Dunlap, clang-built-linux,
linux-kernel
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] linux/compiler-clang.h: define HAVE_BUILTIN_BSWAP*
2021-02-25 20:14 ` Kees Cook
2021-02-25 20:18 ` Nathan Chancellor
@ 2021-02-26 3:34 ` Miguel Ojeda
1 sibling, 0 replies; 10+ messages in thread
From: Miguel Ojeda @ 2021-02-26 3:34 UTC (permalink / raw)
To: Kees Cook
Cc: Andrew Morton, Arnd Bergmann, Masahiro Yamada, Nathan Chancellor,
Nick Desaulniers, Arnd Bergmann, Miguel Ojeda, Marco Elver,
Sami Tolvanen, Arvind Sankar, Randy Dunlap, clang-built-linux,
linux-kernel
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] linux/compiler-clang.h: define HAVE_BUILTIN_BSWAP*
2021-02-25 20:06 ` Andrew Morton
2021-02-25 20:14 ` Kees Cook
@ 2021-02-25 21:01 ` Nick Desaulniers
1 sibling, 0 replies; 10+ messages in thread
From: Nick Desaulniers @ 2021-02-25 21:01 UTC (permalink / raw)
To: Andrew Morton
Cc: Kees Cook, Arnd Bergmann, Masahiro Yamada, Nathan Chancellor,
Arnd Bergmann, Miguel Ojeda, Marco Elver, Sami Tolvanen,
Arvind Sankar, Randy Dunlap, clang-built-linux, LKML
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] linux/compiler-clang.h: define HAVE_BUILTIN_BSWAP*
2021-02-25 16:45 [PATCH] linux/compiler-clang.h: define HAVE_BUILTIN_BSWAP* Arnd Bergmann
2021-02-25 19:29 ` Nathan Chancellor
2021-02-25 20:03 ` Kees Cook
@ 2021-02-26 7:47 ` Luc Van Oostenryck
2 siblings, 0 replies; 10+ messages in thread
From: Luc Van Oostenryck @ 2021-02-26 7:47 UTC (permalink / raw)
To: Arnd Bergmann
Cc: Masahiro Yamada, Nathan Chancellor, Nick Desaulniers,
Arnd Bergmann, Miguel Ojeda, Kees Cook, Andrew Morton,
Marco Elver, Sami Tolvanen, Arvind Sankar, Randy Dunlap,
clang-built-linux, linux-kernel
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
^ permalink raw reply [flat|nested] 10+ messages in thread