From: Arnd Bergmann <[email protected]>
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 <[email protected]>
---
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
On Thu, Feb 25, 2021 at 05:45:09PM +0100, Arnd Bergmann wrote:
> From: Arnd Bergmann <[email protected]>
>
> 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 <[email protected]>
Reviewed-by: Nathan Chancellor <[email protected]>
> ---
> 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
>
On Thu, Feb 25, 2021 at 05:45:09PM +0100, Arnd Bergmann wrote:
> From: Arnd Bergmann <[email protected]>
>
> 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 <[email protected]>
Cc: [email protected]
Reviewed-by: Kees Cook <[email protected]>
--
Kees Cook
On Thu, 25 Feb 2021 12:03:48 -0800 Kees Cook <[email protected]> wrote:
> On Thu, Feb 25, 2021 at 05:45:09PM +0100, Arnd Bergmann wrote:
> > From: Arnd Bergmann <[email protected]>
> >
> > 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 <[email protected]>
>
> Cc: [email protected]
I figured 4% better compile time isn't significant enough to justify a
backport. Thoughts?
> Reviewed-by: Kees Cook <[email protected]>
On Thu, Feb 25, 2021 at 12:06:37PM -0800, Andrew Morton wrote:
> On Thu, 25 Feb 2021 12:03:48 -0800 Kees Cook <[email protected]> wrote:
>
> > On Thu, Feb 25, 2021 at 05:45:09PM +0100, Arnd Bergmann wrote:
> > > From: Arnd Bergmann <[email protected]>
> > >
> > > 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 <[email protected]>
> >
> > Cc: [email protected]
>
> 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 <[email protected]>
>
--
Kees Cook
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 <[email protected]> wrote:
> >
> > > On Thu, Feb 25, 2021 at 05:45:09PM +0100, Arnd Bergmann wrote:
> > > > From: Arnd Bergmann <[email protected]>
> > > >
> > > > 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 <[email protected]>
> > >
> > > Cc: [email protected]
> >
> > 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 <[email protected]>
> >
>
> --
> Kees Cook
On Thu, Feb 25, 2021 at 12:06 PM Andrew Morton
<[email protected]> wrote:
>
> On Thu, 25 Feb 2021 12:03:48 -0800 Kees Cook <[email protected]> wrote:
>
> > On Thu, Feb 25, 2021 at 05:45:09PM +0100, Arnd Bergmann wrote:
> > > From: Arnd Bergmann <[email protected]>
> > >
> > > 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 <[email protected]>
> >
> > Cc: [email protected]
>
> 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 <[email protected]>
>
> > Reviewed-by: Kees Cook <[email protected]>
>
--
Thanks,
~Nick Desaulniers
On Thu, Feb 25, 2021 at 9:14 PM Kees Cook <[email protected]> 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 <[email protected]>
Cheers,
Miguel
On Thu, Feb 25, 2021 at 9:18 PM Nathan Chancellor <[email protected]> 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
On Thu, Feb 25, 2021 at 05:45:09PM +0100, Arnd Bergmann wrote:
> From: Arnd Bergmann <[email protected]>
>
> Fixes: 815f0ddb346c ("include/linux/compiler*.h: make compiler-*.h mutually exclusive")
> Signed-off-by: Arnd Bergmann <[email protected]>
> ---
> 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