linux-toolchains.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/2] no_profile fn attr and Kconfig for GCOV+PGO
@ 2021-06-18 23:30 Nick Desaulniers
  2021-06-18 23:30 ` [PATCH 1/2] compiler_attributes.h: define __no_profile, add to noinstr Nick Desaulniers
                   ` (3 more replies)
  0 siblings, 4 replies; 20+ messages in thread
From: Nick Desaulniers @ 2021-06-18 23:30 UTC (permalink / raw)
  To: Kees Cook
  Cc: Peter Zijlstra, Bill Wendling, Sami Tolvanen,
	Peter Oberparleiter, Masahiro Yamada, Miguel Ojeda,
	Nathan Chancellor, Luc Van Oostenryck, Ard Biesheuvel,
	Will Deacon, Arnd Bergmann, Andrew Morton, Rasmus Villemoes,
	linux-kernel, clang-built-linux, x86, Borislav Petkov,
	Martin Liska, Marco Elver, Jonathan Corbet, Fangrui Song,
	linux-doc, linux-kbuild, Dmitry Vyukov, johannes.berg,
	linux-toolchains, Nick Desaulniers

When we say noinstr, we mean noinstr.  GCOV and PGO can both instrument
functions. Add a new function annotation __no_profile that expands to
__attribute__((__no_profile__)) and Kconfig value
CC_HAS_NO_PROFILE_FN_ATTR.

Base is
https://git.kernel.org/pub/scm/linux/kernel/git/kees/linux.git/log/?h=for-next/clang/pgo.

Nick Desaulniers (2):
  compiler_attributes.h: define __no_profile, add to noinstr
  Kconfig: CC_HAS_NO_PROFILE_FN_ATTR, depend on for GCOV and PGO

 include/linux/compiler_attributes.h | 12 ++++++++++++
 include/linux/compiler_types.h      |  2 +-
 init/Kconfig                        |  3 +++
 kernel/gcov/Kconfig                 |  1 +
 kernel/pgo/Kconfig                  |  3 ++-
 5 files changed, 19 insertions(+), 2 deletions(-)


base-commit: 4356bc4c0425c81e204f561acf4dd0095544a6cb
-- 
2.32.0.288.g62a8d224e6-goog


^ permalink raw reply	[flat|nested] 20+ messages in thread

* [PATCH 1/2] compiler_attributes.h: define __no_profile, add to noinstr
  2021-06-18 23:30 [PATCH 0/2] no_profile fn attr and Kconfig for GCOV+PGO Nick Desaulniers
@ 2021-06-18 23:30 ` Nick Desaulniers
  2021-06-19 11:26   ` Miguel Ojeda
  2021-06-18 23:30 ` [PATCH 2/2] Kconfig: CC_HAS_NO_PROFILE_FN_ATTR, depend on for GCOV and PGO Nick Desaulniers
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 20+ messages in thread
From: Nick Desaulniers @ 2021-06-18 23:30 UTC (permalink / raw)
  To: Kees Cook
  Cc: Peter Zijlstra, Bill Wendling, Sami Tolvanen,
	Peter Oberparleiter, Masahiro Yamada, Miguel Ojeda,
	Nathan Chancellor, Luc Van Oostenryck, Ard Biesheuvel,
	Will Deacon, Arnd Bergmann, Andrew Morton, Rasmus Villemoes,
	linux-kernel, clang-built-linux, x86, Borislav Petkov,
	Martin Liska, Marco Elver, Jonathan Corbet, Fangrui Song,
	linux-doc, linux-kbuild, Dmitry Vyukov, johannes.berg,
	linux-toolchains, Nick Desaulniers

noinstr implies that we would like the compiler to avoid instrumenting a
function.  Add support for the compiler attribute no_profile to
compiler_attributes.h, then add __no_profile to the definition of
noinstr.

Cc: Miguel Ojeda <miguel.ojeda.sandonis@gmail.com>
Link: https://lore.kernel.org/lkml/20210614162018.GD68749@worktop.programming.kicks-ass.net/
Link: https://reviews.llvm.org/D104475
Link: https://reviews.llvm.org/D104257
Link: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=80223
Suggested-by: Peter Zijlstra <peterz@infradead.org>
Signed-off-by: Nick Desaulniers <ndesaulniers@google.com>
---
 include/linux/compiler_attributes.h | 12 ++++++++++++
 include/linux/compiler_types.h      |  2 +-
 2 files changed, 13 insertions(+), 1 deletion(-)

diff --git a/include/linux/compiler_attributes.h b/include/linux/compiler_attributes.h
index c043b8d2b17b..cf584a1908b3 100644
--- a/include/linux/compiler_attributes.h
+++ b/include/linux/compiler_attributes.h
@@ -33,6 +33,7 @@
 # define __GCC4_has_attribute___externally_visible__  1
 # define __GCC4_has_attribute___no_caller_saved_registers__ 0
 # define __GCC4_has_attribute___noclone__             1
+# define __GCC4_has_attribute___no_profile            0
 # define __GCC4_has_attribute___nonstring__           0
 # define __GCC4_has_attribute___no_sanitize_address__ (__GNUC_MINOR__ >= 8)
 # define __GCC4_has_attribute___no_sanitize_undefined__ (__GNUC_MINOR__ >= 9)
@@ -237,6 +238,17 @@
 # define __nonstring
 #endif
 
+/*
+ * Optional: only supported since clang >= 13
+ *      gcc: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=80223
+ *    clang: https://clang.llvm.org/docs/AttributeReference.html#no_profile
+ */
+#if __has_attribute(__no_profile__)
+# define __no_profile                  __attribute__((__no_profile__))
+#else
+# define __no_profile
+#endif
+
 /*
  *   gcc: https://gcc.gnu.org/onlinedocs/gcc/Common-Function-Attributes.html#index-noreturn-function-attribute
  * clang: https://clang.llvm.org/docs/AttributeReference.html#noreturn
diff --git a/include/linux/compiler_types.h b/include/linux/compiler_types.h
index d29bda7f6ebd..d509169860f1 100644
--- a/include/linux/compiler_types.h
+++ b/include/linux/compiler_types.h
@@ -210,7 +210,7 @@ struct ftrace_likely_data {
 /* Section for code which can't be instrumented at all */
 #define noinstr								\
 	noinline notrace __attribute((__section__(".noinstr.text")))	\
-	__no_kcsan __no_sanitize_address
+	__no_kcsan __no_sanitize_address __no_profile
 
 #endif /* __KERNEL__ */
 
-- 
2.32.0.288.g62a8d224e6-goog


^ permalink raw reply	[flat|nested] 20+ messages in thread

* [PATCH 2/2] Kconfig: CC_HAS_NO_PROFILE_FN_ATTR, depend on for GCOV and PGO
  2021-06-18 23:30 [PATCH 0/2] no_profile fn attr and Kconfig for GCOV+PGO Nick Desaulniers
  2021-06-18 23:30 ` [PATCH 1/2] compiler_attributes.h: define __no_profile, add to noinstr Nick Desaulniers
@ 2021-06-18 23:30 ` Nick Desaulniers
  2021-06-19  6:23   ` Marco Elver
  2021-06-18 23:52 ` [PATCH 0/2] no_profile fn attr and Kconfig for GCOV+PGO Fangrui Song
  2021-06-19  2:45 ` Kees Cook
  3 siblings, 1 reply; 20+ messages in thread
From: Nick Desaulniers @ 2021-06-18 23:30 UTC (permalink / raw)
  To: Kees Cook
  Cc: Peter Zijlstra, Bill Wendling, Sami Tolvanen,
	Peter Oberparleiter, Masahiro Yamada, Miguel Ojeda,
	Nathan Chancellor, Luc Van Oostenryck, Ard Biesheuvel,
	Will Deacon, Arnd Bergmann, Andrew Morton, Rasmus Villemoes,
	linux-kernel, clang-built-linux, x86, Borislav Petkov,
	Martin Liska, Marco Elver, Jonathan Corbet, Fangrui Song,
	linux-doc, linux-kbuild, Dmitry Vyukov, johannes.berg,
	linux-toolchains, Nick Desaulniers

We don't want compiler instrumentation to touch noinstr functions, which
are annotated with the no_profile function attribute. Add a Kconfig test
for this and make PGO and GCOV depend on it.

Cc: Masahiro Yamada <masahiroy@kernel.org>
Cc: Peter Oberparleiter <oberpar@linux.ibm.com>
Link: https://lore.kernel.org/lkml/YMTn9yjuemKFLbws@hirez.programming.kicks-ass.net/
Link: https://lore.kernel.org/lkml/YMcssV%2Fn5IBGv4f0@hirez.programming.kicks-ass.net/
Suggested-by: Peter Zijlstra <peterz@infradead.org>
Signed-off-by: Nick Desaulniers <ndesaulniers@google.com>
---
 init/Kconfig        | 3 +++
 kernel/gcov/Kconfig | 1 +
 kernel/pgo/Kconfig  | 3 ++-
 3 files changed, 6 insertions(+), 1 deletion(-)

diff --git a/init/Kconfig b/init/Kconfig
index 1ea12c64e4c9..540f862b40c6 100644
--- a/init/Kconfig
+++ b/init/Kconfig
@@ -83,6 +83,9 @@ config TOOLS_SUPPORT_RELR
 config CC_HAS_ASM_INLINE
 	def_bool $(success,echo 'void foo(void) { asm inline (""); }' | $(CC) -x c - -c -o /dev/null)
 
+config CC_HAS_NO_PROFILE_FN_ATTR
+	def_bool $(success,echo '__attribute__((no_profile)) int x();' | $(CC) -x c - -c -o /dev/null -Werror)
+
 config CONSTRUCTORS
 	bool
 
diff --git a/kernel/gcov/Kconfig b/kernel/gcov/Kconfig
index 58f87a3092f3..19facd4289cd 100644
--- a/kernel/gcov/Kconfig
+++ b/kernel/gcov/Kconfig
@@ -5,6 +5,7 @@ config GCOV_KERNEL
 	bool "Enable gcov-based kernel profiling"
 	depends on DEBUG_FS
 	depends on !CC_IS_CLANG || CLANG_VERSION >= 110000
+	depends on !X86 || (X86 && CC_HAS_NO_PROFILE_FN_ATTR)
 	select CONSTRUCTORS
 	default n
 	help
diff --git a/kernel/pgo/Kconfig b/kernel/pgo/Kconfig
index d2053df1111c..26f75ac4c6c1 100644
--- a/kernel/pgo/Kconfig
+++ b/kernel/pgo/Kconfig
@@ -8,7 +8,8 @@ config PGO_CLANG
 	bool "Enable clang's PGO-based kernel profiling"
 	depends on DEBUG_FS
 	depends on ARCH_SUPPORTS_PGO_CLANG
-	depends on CC_IS_CLANG && CLANG_VERSION >= 120000
+	depends on CC_IS_CLANG
+	depends on CC_HAS_NO_PROFILE_FN_ATTR
 	help
 	  This option enables clang's PGO (Profile Guided Optimization) based
 	  code profiling to better optimize the kernel.
-- 
2.32.0.288.g62a8d224e6-goog


^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH 0/2] no_profile fn attr and Kconfig for GCOV+PGO
  2021-06-18 23:30 [PATCH 0/2] no_profile fn attr and Kconfig for GCOV+PGO Nick Desaulniers
  2021-06-18 23:30 ` [PATCH 1/2] compiler_attributes.h: define __no_profile, add to noinstr Nick Desaulniers
  2021-06-18 23:30 ` [PATCH 2/2] Kconfig: CC_HAS_NO_PROFILE_FN_ATTR, depend on for GCOV and PGO Nick Desaulniers
@ 2021-06-18 23:52 ` Fangrui Song
  2021-06-19  2:45 ` Kees Cook
  3 siblings, 0 replies; 20+ messages in thread
From: Fangrui Song @ 2021-06-18 23:52 UTC (permalink / raw)
  To: Nick Desaulniers
  Cc: Kees Cook, Peter Zijlstra, Bill Wendling, Sami Tolvanen,
	Peter Oberparleiter, Masahiro Yamada, Miguel Ojeda,
	Nathan Chancellor, Luc Van Oostenryck, Ard Biesheuvel,
	Will Deacon, Arnd Bergmann, Andrew Morton, Rasmus Villemoes,
	linux-kernel, clang-built-linux, x86, Borislav Petkov,
	Martin Liska, Marco Elver, Jonathan Corbet, linux-doc,
	linux-kbuild, Dmitry Vyukov, johannes.berg, linux-toolchains

On 2021-06-18, Nick Desaulniers wrote:
>When we say noinstr, we mean noinstr.  GCOV and PGO can both instrument
>functions. Add a new function annotation __no_profile that expands to
>__attribute__((__no_profile__)) and Kconfig value
>CC_HAS_NO_PROFILE_FN_ATTR.
>
>Base is
>https://git.kernel.org/pub/scm/linux/kernel/git/kees/linux.git/log/?h=for-next/clang/pgo.
>
>Nick Desaulniers (2):
>  compiler_attributes.h: define __no_profile, add to noinstr
>  Kconfig: CC_HAS_NO_PROFILE_FN_ATTR, depend on for GCOV and PGO
>
> include/linux/compiler_attributes.h | 12 ++++++++++++
> include/linux/compiler_types.h      |  2 +-
> init/Kconfig                        |  3 +++
> kernel/gcov/Kconfig                 |  1 +
> kernel/pgo/Kconfig                  |  3 ++-
> 5 files changed, 19 insertions(+), 2 deletions(-)
>
>
>base-commit: 4356bc4c0425c81e204f561acf4dd0095544a6cb
>-- 
>2.32.0.288.g62a8d224e6-goog
>

Thanks for the attribute work in clang and kernel! Hope we can use clang
PGO in 5.14...  (I am a casual contributor to clang PGO/coverage)

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH 0/2] no_profile fn attr and Kconfig for GCOV+PGO
  2021-06-18 23:30 [PATCH 0/2] no_profile fn attr and Kconfig for GCOV+PGO Nick Desaulniers
                   ` (2 preceding siblings ...)
  2021-06-18 23:52 ` [PATCH 0/2] no_profile fn attr and Kconfig for GCOV+PGO Fangrui Song
@ 2021-06-19  2:45 ` Kees Cook
  2021-06-20  8:08   ` Bill Wendling
  2021-06-20 14:53   ` Miguel Ojeda
  3 siblings, 2 replies; 20+ messages in thread
From: Kees Cook @ 2021-06-19  2:45 UTC (permalink / raw)
  To: Nick Desaulniers
  Cc: Peter Zijlstra, Bill Wendling, Sami Tolvanen,
	Peter Oberparleiter, Masahiro Yamada, Miguel Ojeda,
	Nathan Chancellor, Luc Van Oostenryck, Ard Biesheuvel,
	Will Deacon, Arnd Bergmann, Andrew Morton, Rasmus Villemoes,
	linux-kernel, clang-built-linux, x86, Borislav Petkov,
	Martin Liska, Marco Elver, Jonathan Corbet, Fangrui Song,
	linux-doc, linux-kbuild, Dmitry Vyukov, johannes.berg,
	linux-toolchains

On Fri, Jun 18, 2021 at 04:30:21PM -0700, Nick Desaulniers wrote:
> When we say noinstr, we mean noinstr.  GCOV and PGO can both instrument
> functions. Add a new function annotation __no_profile that expands to
> __attribute__((__no_profile__)) and Kconfig value
> CC_HAS_NO_PROFILE_FN_ATTR.
> 
> Base is
> https://git.kernel.org/pub/scm/linux/kernel/git/kees/linux.git/log/?h=for-next/clang/pgo.
> 
> Nick Desaulniers (2):
>   compiler_attributes.h: define __no_profile, add to noinstr
>   Kconfig: CC_HAS_NO_PROFILE_FN_ATTR, depend on for GCOV and PGO

Oh, awesome! Thanks for the fast work on this. If there are no objections,
I'll apply this in front of the PGO series and put it in -next.

-Kees

> 
>  include/linux/compiler_attributes.h | 12 ++++++++++++
>  include/linux/compiler_types.h      |  2 +-
>  init/Kconfig                        |  3 +++
>  kernel/gcov/Kconfig                 |  1 +
>  kernel/pgo/Kconfig                  |  3 ++-
>  5 files changed, 19 insertions(+), 2 deletions(-)
> 
> 
> base-commit: 4356bc4c0425c81e204f561acf4dd0095544a6cb
> -- 
> 2.32.0.288.g62a8d224e6-goog
> 

-- 
Kees Cook

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH 2/2] Kconfig: CC_HAS_NO_PROFILE_FN_ATTR, depend on for GCOV and PGO
  2021-06-18 23:30 ` [PATCH 2/2] Kconfig: CC_HAS_NO_PROFILE_FN_ATTR, depend on for GCOV and PGO Nick Desaulniers
@ 2021-06-19  6:23   ` Marco Elver
  2021-06-21 18:22     ` Nick Desaulniers
  0 siblings, 1 reply; 20+ messages in thread
From: Marco Elver @ 2021-06-19  6:23 UTC (permalink / raw)
  To: Nick Desaulniers
  Cc: Kees Cook, Peter Zijlstra, Bill Wendling, Sami Tolvanen,
	Peter Oberparleiter, Masahiro Yamada, Miguel Ojeda,
	Nathan Chancellor, Luc Van Oostenryck, Ard Biesheuvel,
	Will Deacon, Arnd Bergmann, Andrew Morton, Rasmus Villemoes,
	linux-kernel, clang-built-linux, x86, Borislav Petkov,
	Martin Liska, Jonathan Corbet, Fangrui Song, linux-doc,
	linux-kbuild, Dmitry Vyukov, johannes.berg, linux-toolchains,
	Mark Rutland

On Sat, 19 Jun 2021 at 01:30, Nick Desaulniers <ndesaulniers@google.com> wrote:
>
> We don't want compiler instrumentation to touch noinstr functions, which
> are annotated with the no_profile function attribute. Add a Kconfig test
> for this and make PGO and GCOV depend on it.
>
> Cc: Masahiro Yamada <masahiroy@kernel.org>
> Cc: Peter Oberparleiter <oberpar@linux.ibm.com>
> Link: https://lore.kernel.org/lkml/YMTn9yjuemKFLbws@hirez.programming.kicks-ass.net/
> Link: https://lore.kernel.org/lkml/YMcssV%2Fn5IBGv4f0@hirez.programming.kicks-ass.net/
> Suggested-by: Peter Zijlstra <peterz@infradead.org>
> Signed-off-by: Nick Desaulniers <ndesaulniers@google.com>
> ---
>  init/Kconfig        | 3 +++
>  kernel/gcov/Kconfig | 1 +
>  kernel/pgo/Kconfig  | 3 ++-
>  3 files changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/init/Kconfig b/init/Kconfig
> index 1ea12c64e4c9..540f862b40c6 100644
> --- a/init/Kconfig
> +++ b/init/Kconfig
> @@ -83,6 +83,9 @@ config TOOLS_SUPPORT_RELR
>  config CC_HAS_ASM_INLINE
>         def_bool $(success,echo 'void foo(void) { asm inline (""); }' | $(CC) -x c - -c -o /dev/null)
>
> +config CC_HAS_NO_PROFILE_FN_ATTR
> +       def_bool $(success,echo '__attribute__((no_profile)) int x();' | $(CC) -x c - -c -o /dev/null -Werror)
> +
>  config CONSTRUCTORS
>         bool
>
> diff --git a/kernel/gcov/Kconfig b/kernel/gcov/Kconfig
> index 58f87a3092f3..19facd4289cd 100644
> --- a/kernel/gcov/Kconfig
> +++ b/kernel/gcov/Kconfig
> @@ -5,6 +5,7 @@ config GCOV_KERNEL
>         bool "Enable gcov-based kernel profiling"
>         depends on DEBUG_FS
>         depends on !CC_IS_CLANG || CLANG_VERSION >= 110000
> +       depends on !X86 || (X86 && CC_HAS_NO_PROFILE_FN_ATTR)

[+Cc Mark]

arm64 is also starting to rely on noinstr working properly.

This should probably be a 'select ARCH_HAS_GCOV_PROFILE_ALL if
CC_HAS_NO_PROFILE_FN_ATTR' in the relevant arch/../Kconfig.

Alternatively, using:
https://lkml.kernel.org/r/YMcssV/n5IBGv4f0@hirez.programming.kicks-ass.net

But I'd probably not overcomplicate things at this point and just use
ARCH_HAS_GCOV_PROFILE_ALL, because GCOV seems to be a) rarely used,
and b) if someone decides to selectively instrument stuff like entry
code, we can just say it's user error.


>         select CONSTRUCTORS
>         default n
>         help
> diff --git a/kernel/pgo/Kconfig b/kernel/pgo/Kconfig
> index d2053df1111c..26f75ac4c6c1 100644
> --- a/kernel/pgo/Kconfig
> +++ b/kernel/pgo/Kconfig
> @@ -8,7 +8,8 @@ config PGO_CLANG
>         bool "Enable clang's PGO-based kernel profiling"
>         depends on DEBUG_FS
>         depends on ARCH_SUPPORTS_PGO_CLANG
> -       depends on CC_IS_CLANG && CLANG_VERSION >= 120000
> +       depends on CC_IS_CLANG
> +       depends on CC_HAS_NO_PROFILE_FN_ATTR
>         help
>           This option enables clang's PGO (Profile Guided Optimization) based
>           code profiling to better optimize the kernel.
> --
> 2.32.0.288.g62a8d224e6-goog
>

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH 1/2] compiler_attributes.h: define __no_profile, add to noinstr
  2021-06-18 23:30 ` [PATCH 1/2] compiler_attributes.h: define __no_profile, add to noinstr Nick Desaulniers
@ 2021-06-19 11:26   ` Miguel Ojeda
  2021-06-19 11:32     ` Miguel Ojeda
  0 siblings, 1 reply; 20+ messages in thread
From: Miguel Ojeda @ 2021-06-19 11:26 UTC (permalink / raw)
  To: Nick Desaulniers
  Cc: Kees Cook, Peter Zijlstra, Bill Wendling, Sami Tolvanen,
	Peter Oberparleiter, Masahiro Yamada, Nathan Chancellor,
	Luc Van Oostenryck, Ard Biesheuvel, Will Deacon, Arnd Bergmann,
	Andrew Morton, Rasmus Villemoes, linux-kernel, clang-built-linux,
	maintainer:X86 ARCHITECTURE (32-BIT AND 64-BIT),
	Borislav Petkov, Martin Liska, Marco Elver, Jonathan Corbet,
	Fangrui Song, Linux Doc Mailing List, Linux Kbuild mailing list,
	Dmitry Vyukov, johannes.berg, linux-toolchains

On Sat, Jun 19, 2021 at 1:30 AM Nick Desaulniers
<ndesaulniers@google.com> wrote:
>
> +/*
> + * Optional: only supported since clang >= 13
> + *      gcc: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=80223
> + *    clang: https://clang.llvm.org/docs/AttributeReference.html#no_profile
> + */

I am not sure if it is best or not to have the GCC link in order to be
consistent with the rest of the links (they are for the docs only). Do
we know if GCC going to implement it soon?

Otherwise, it looks good to me.

Cheers,
Miguel

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH 1/2] compiler_attributes.h: define __no_profile, add to noinstr
  2021-06-19 11:26   ` Miguel Ojeda
@ 2021-06-19 11:32     ` Miguel Ojeda
  2021-06-21 18:10       ` Nick Desaulniers
  0 siblings, 1 reply; 20+ messages in thread
From: Miguel Ojeda @ 2021-06-19 11:32 UTC (permalink / raw)
  To: Nick Desaulniers
  Cc: Kees Cook, Peter Zijlstra, Bill Wendling, Sami Tolvanen,
	Peter Oberparleiter, Masahiro Yamada, Nathan Chancellor,
	Luc Van Oostenryck, Ard Biesheuvel, Will Deacon, Arnd Bergmann,
	Andrew Morton, Rasmus Villemoes, linux-kernel, clang-built-linux,
	maintainer:X86 ARCHITECTURE (32-BIT AND 64-BIT),
	Borislav Petkov, Martin Liska, Marco Elver, Jonathan Corbet,
	Fangrui Song, Linux Doc Mailing List, Linux Kbuild mailing list,
	Dmitry Vyukov, johannes.berg, linux-toolchains

On Sat, Jun 19, 2021 at 1:26 PM Miguel Ojeda
<miguel.ojeda.sandonis@gmail.com> wrote:
>
> I am not sure if it is best or not to have the GCC link in order to be
> consistent with the rest of the links (they are for the docs only). Do
> we know if GCC going to implement it soon?

i.e. if GCC does not implement it yet we use elsewhere this kind of
marker instead:

     * Optional: not supported by gcc

The first of its kind, normally it is clang/icc there ;-)

We could nevertheless have the link there, something like:

    * Optional: not supported by GCC
                https://gcc.gnu.org/bugzilla/show_bug.cgi?id=80223

Cheers,
Miguel

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH 0/2] no_profile fn attr and Kconfig for GCOV+PGO
  2021-06-19  2:45 ` Kees Cook
@ 2021-06-20  8:08   ` Bill Wendling
  2021-06-20 14:53   ` Miguel Ojeda
  1 sibling, 0 replies; 20+ messages in thread
From: Bill Wendling @ 2021-06-20  8:08 UTC (permalink / raw)
  To: Kees Cook
  Cc: Nick Desaulniers, Peter Zijlstra, Bill Wendling, Sami Tolvanen,
	Peter Oberparleiter, Masahiro Yamada, Miguel Ojeda,
	Nathan Chancellor, Luc Van Oostenryck, Ard Biesheuvel,
	Will Deacon, Arnd Bergmann, Andrew Morton, Rasmus Villemoes,
	LKML, clang-built-linux,
	maintainer:X86 ARCHITECTURE (32-BIT AND 64-BIT),
	Borislav Petkov, Martin Liska, Marco Elver, Jonathan Corbet,
	Fangrui Song, Linux Doc Mailing List, Linux Kbuild mailing list,
	Dmitry Vyukov, johannes.berg, linux-toolchains

On Fri, Jun 18, 2021 at 7:45 PM Kees Cook <keescook@chromium.org> wrote:
>
> On Fri, Jun 18, 2021 at 04:30:21PM -0700, Nick Desaulniers wrote:
> > When we say noinstr, we mean noinstr.  GCOV and PGO can both instrument
> > functions. Add a new function annotation __no_profile that expands to
> > __attribute__((__no_profile__)) and Kconfig value
> > CC_HAS_NO_PROFILE_FN_ATTR.
> >
> > Base is
> > https://git.kernel.org/pub/scm/linux/kernel/git/kees/linux.git/log/?h=for-next/clang/pgo.
> >
> > Nick Desaulniers (2):
> >   compiler_attributes.h: define __no_profile, add to noinstr
> >   Kconfig: CC_HAS_NO_PROFILE_FN_ATTR, depend on for GCOV and PGO
>
> Oh, awesome! Thanks for the fast work on this. If there are no objections,
> I'll apply this in front of the PGO series and put it in -next.
>
That works for me! Thanks, Nick and Kees!

-bw

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH 0/2] no_profile fn attr and Kconfig for GCOV+PGO
  2021-06-19  2:45 ` Kees Cook
  2021-06-20  8:08   ` Bill Wendling
@ 2021-06-20 14:53   ` Miguel Ojeda
  1 sibling, 0 replies; 20+ messages in thread
From: Miguel Ojeda @ 2021-06-20 14:53 UTC (permalink / raw)
  To: Kees Cook
  Cc: Nick Desaulniers, Peter Zijlstra, Bill Wendling, Sami Tolvanen,
	Peter Oberparleiter, Masahiro Yamada, Nathan Chancellor,
	Luc Van Oostenryck, Ard Biesheuvel, Will Deacon, Arnd Bergmann,
	Andrew Morton, Rasmus Villemoes, linux-kernel, clang-built-linux,
	maintainer:X86 ARCHITECTURE (32-BIT AND 64-BIT),
	Borislav Petkov, Martin Liska, Marco Elver, Jonathan Corbet,
	Fangrui Song, Linux Doc Mailing List, Linux Kbuild mailing list,
	Dmitry Vyukov, johannes.berg, linux-toolchains

On Sat, Jun 19, 2021 at 4:45 AM Kees Cook <keescook@chromium.org> wrote:
>
> Oh, awesome! Thanks for the fast work on this. If there are no objections,
> I'll apply this in front of the PGO series and put it in -next.

If you are picking both patches on your tree, please see my comment on
the first commit.

With that solved, for the first commit:

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

Cheers,
Miguel

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH 1/2] compiler_attributes.h: define __no_profile, add to noinstr
  2021-06-19 11:32     ` Miguel Ojeda
@ 2021-06-21 18:10       ` Nick Desaulniers
  2021-06-21 18:24         ` Fangrui Song
  0 siblings, 1 reply; 20+ messages in thread
From: Nick Desaulniers @ 2021-06-21 18:10 UTC (permalink / raw)
  To: Miguel Ojeda
  Cc: Kees Cook, Peter Zijlstra, Bill Wendling, Sami Tolvanen,
	Peter Oberparleiter, Masahiro Yamada, Nathan Chancellor,
	Luc Van Oostenryck, Ard Biesheuvel, Will Deacon, Arnd Bergmann,
	Andrew Morton, Rasmus Villemoes, linux-kernel, clang-built-linux,
	maintainer:X86 ARCHITECTURE (32-BIT AND 64-BIT),
	Borislav Petkov, Martin Liska, Marco Elver, Jonathan Corbet,
	Fangrui Song, Linux Doc Mailing List, Linux Kbuild mailing list,
	Dmitry Vyukov, Johannes Berg, linux-toolchains

On Sat, Jun 19, 2021 at 4:32 AM Miguel Ojeda
<miguel.ojeda.sandonis@gmail.com> wrote:
>
> On Sat, Jun 19, 2021 at 1:26 PM Miguel Ojeda
> <miguel.ojeda.sandonis@gmail.com> wrote:
> >
> > I am not sure if it is best or not to have the GCC link in order to be
> > consistent with the rest of the links (they are for the docs only). Do
> > we know if GCC going to implement it soon?
>
> i.e. if GCC does not implement it yet we use elsewhere this kind of
> marker instead:
>
>      * Optional: not supported by gcc
>
> The first of its kind, normally it is clang/icc there ;-)

:^) GCC does have an attribute since GCC 7.1 for this.
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=80223#c11
I'm moving Clang over to use that in
https://reviews.llvm.org/D104658
Once that lands, I'll send a v2 (without carrying any reviewed by tags).

>
> We could nevertheless have the link there, something like:
>
>     * Optional: not supported by GCC
>                 https://gcc.gnu.org/bugzilla/show_bug.cgi?id=80223

-- 
Thanks,
~Nick Desaulniers

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH 2/2] Kconfig: CC_HAS_NO_PROFILE_FN_ATTR, depend on for GCOV and PGO
  2021-06-19  6:23   ` Marco Elver
@ 2021-06-21 18:22     ` Nick Desaulniers
  2021-06-21 18:50       ` Bill Wendling
  0 siblings, 1 reply; 20+ messages in thread
From: Nick Desaulniers @ 2021-06-21 18:22 UTC (permalink / raw)
  To: Will Deacon, Catalin Marinas, Mark Rutland, Ard Biesheuvel,
	Heiko Carstens, Vasily Gorbik, Christian Borntraeger,
	Peter Oberparleiter
  Cc: Kees Cook, Peter Zijlstra, Bill Wendling, Sami Tolvanen,
	Masahiro Yamada, Miguel Ojeda, Nathan Chancellor,
	Luc Van Oostenryck, Arnd Bergmann, Andrew Morton,
	Rasmus Villemoes, LKML, clang-built-linux,
	maintainer:X86 ARCHITECTURE (32-BIT AND 64-BIT),
	Borislav Petkov, Martin Liska, Jonathan Corbet, Fangrui Song,
	Linux Doc Mailing List, Linux Kbuild mailing list, Dmitry Vyukov,
	Johannes Berg, linux-toolchains, Marco Elver, Linux ARM,
	linux-s390

On Fri, Jun 18, 2021 at 11:23 PM Marco Elver <elver@google.com> wrote:
>
> On Sat, 19 Jun 2021 at 01:30, Nick Desaulniers <ndesaulniers@google.com> wrote:
> >
> > We don't want compiler instrumentation to touch noinstr functions, which
> > are annotated with the no_profile function attribute. Add a Kconfig test
> > for this and make PGO and GCOV depend on it.
> >
> > Cc: Masahiro Yamada <masahiroy@kernel.org>
> > Cc: Peter Oberparleiter <oberpar@linux.ibm.com>
> > Link: https://lore.kernel.org/lkml/YMTn9yjuemKFLbws@hirez.programming.kicks-ass.net/
> > Link: https://lore.kernel.org/lkml/YMcssV%2Fn5IBGv4f0@hirez.programming.kicks-ass.net/
> > Suggested-by: Peter Zijlstra <peterz@infradead.org>
> > Signed-off-by: Nick Desaulniers <ndesaulniers@google.com>
> > ---
> >  init/Kconfig        | 3 +++
> >  kernel/gcov/Kconfig | 1 +
> >  kernel/pgo/Kconfig  | 3 ++-
> >  3 files changed, 6 insertions(+), 1 deletion(-)
> >
> > diff --git a/init/Kconfig b/init/Kconfig
> > index 1ea12c64e4c9..540f862b40c6 100644
> > --- a/init/Kconfig
> > +++ b/init/Kconfig
> > @@ -83,6 +83,9 @@ config TOOLS_SUPPORT_RELR
> >  config CC_HAS_ASM_INLINE
> >         def_bool $(success,echo 'void foo(void) { asm inline (""); }' | $(CC) -x c - -c -o /dev/null)
> >
> > +config CC_HAS_NO_PROFILE_FN_ATTR
> > +       def_bool $(success,echo '__attribute__((no_profile)) int x();' | $(CC) -x c - -c -o /dev/null -Werror)
> > +
> >  config CONSTRUCTORS
> >         bool
> >
> > diff --git a/kernel/gcov/Kconfig b/kernel/gcov/Kconfig
> > index 58f87a3092f3..19facd4289cd 100644
> > --- a/kernel/gcov/Kconfig
> > +++ b/kernel/gcov/Kconfig
> > @@ -5,6 +5,7 @@ config GCOV_KERNEL
> >         bool "Enable gcov-based kernel profiling"
> >         depends on DEBUG_FS
> >         depends on !CC_IS_CLANG || CLANG_VERSION >= 110000
> > +       depends on !X86 || (X86 && CC_HAS_NO_PROFILE_FN_ATTR)
>
> [+Cc Mark]
>
> arm64 is also starting to rely on noinstr working properly.

Sure,
Will, Catalin, other arm64 folks:
Any thoughts on requiring GCC 7.1+/Clang 13.0+ for GCOV support?  That
way we can better guarantee that GCOV (and eventually, PGO) don't
touch noinstr functions?

If that's ok, I'll add modify the above like:

+ depends on !ARM64 || (ARM64 && CC_HAS_NO_PROFILE_FN_ATTR)

to the above hunk in v2.  Oh, looks like arch/s390 also uses noinstr.
Same question applies then:

+ depends on !S390 || (S390 && CC_HAS_NO_PROFILE_FN_ATTR)

Or, we could just do

+ depends on CC_HAS_NO_PROFILE_FN_ATTR

Though that will penalize architectures not using noinstr, that still
would like to use GCOV with versions of GCC older than 7.1.  Perhaps
there are no such such users, or they should consider upgrading their
tools to we can stick with the simpler Kconfig? Thoughts?

>
> This should probably be a 'select ARCH_HAS_GCOV_PROFILE_ALL if
> CC_HAS_NO_PROFILE_FN_ATTR' in the relevant arch/../Kconfig.
>
> Alternatively, using:
> https://lkml.kernel.org/r/YMcssV/n5IBGv4f0@hirez.programming.kicks-ass.net
>
> But I'd probably not overcomplicate things at this point and just use
> ARCH_HAS_GCOV_PROFILE_ALL, because GCOV seems to be a) rarely used,
> and b) if someone decides to selectively instrument stuff like entry
> code, we can just say it's user error.
>
>
> >         select CONSTRUCTORS
> >         default n
> >         help
> > diff --git a/kernel/pgo/Kconfig b/kernel/pgo/Kconfig
> > index d2053df1111c..26f75ac4c6c1 100644
> > --- a/kernel/pgo/Kconfig
> > +++ b/kernel/pgo/Kconfig
> > @@ -8,7 +8,8 @@ config PGO_CLANG
> >         bool "Enable clang's PGO-based kernel profiling"
> >         depends on DEBUG_FS
> >         depends on ARCH_SUPPORTS_PGO_CLANG
> > -       depends on CC_IS_CLANG && CLANG_VERSION >= 120000
> > +       depends on CC_IS_CLANG
> > +       depends on CC_HAS_NO_PROFILE_FN_ATTR
> >         help
> >           This option enables clang's PGO (Profile Guided Optimization) based
> >           code profiling to better optimize the kernel.
> > --
> > 2.32.0.288.g62a8d224e6-goog
> >



-- 
Thanks,
~Nick Desaulniers

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH 1/2] compiler_attributes.h: define __no_profile, add to noinstr
  2021-06-21 18:10       ` Nick Desaulniers
@ 2021-06-21 18:24         ` Fangrui Song
  2021-06-21 19:09           ` Miguel Ojeda
  0 siblings, 1 reply; 20+ messages in thread
From: Fangrui Song @ 2021-06-21 18:24 UTC (permalink / raw)
  To: Nick Desaulniers
  Cc: Miguel Ojeda, Kees Cook, Peter Zijlstra, Bill Wendling,
	Sami Tolvanen, Peter Oberparleiter, Masahiro Yamada,
	Nathan Chancellor, Luc Van Oostenryck, Ard Biesheuvel,
	Will Deacon, Arnd Bergmann, Andrew Morton, Rasmus Villemoes,
	linux-kernel, clang-built-linux,
	maintainer:X86 ARCHITECTURE (32-BIT AND 64-BIT),
	Borislav Petkov, Martin Liska, Marco Elver, Jonathan Corbet,
	Linux Doc Mailing List, Linux Kbuild mailing list, Dmitry Vyukov,
	Johannes Berg, linux-toolchains

On 2021-06-21, Nick Desaulniers wrote:
>On Sat, Jun 19, 2021 at 4:32 AM Miguel Ojeda
><miguel.ojeda.sandonis@gmail.com> wrote:
>>
>> On Sat, Jun 19, 2021 at 1:26 PM Miguel Ojeda
>> <miguel.ojeda.sandonis@gmail.com> wrote:
>> >
>> > I am not sure if it is best or not to have the GCC link in order to be
>> > consistent with the rest of the links (they are for the docs only). Do
>> > we know if GCC going to implement it soon?
>>
>> i.e. if GCC does not implement it yet we use elsewhere this kind of
>> marker instead:
>>
>>      * Optional: not supported by gcc
>>
>> The first of its kind, normally it is clang/icc there ;-)
>
>:^) GCC does have an attribute since GCC 7.1 for this.
>https://gcc.gnu.org/bugzilla/show_bug.cgi?id=80223#c11
>I'm moving Clang over to use that in
>https://reviews.llvm.org/D104658
>Once that lands, I'll send a v2 (without carrying any reviewed by tags).

Thanks! __attribute__((no_profile_instrument_function)) looks good to me.

Also a reminder that __GCC4_has_attribute___no_profile in v1 misses two
underscores. v2 no_profile_instrument_function may need to fix this.


Reviewed-by: Fangrui Song <maskray@google.com>

>>
>> We could nevertheless have the link there, something like:
>>
>>     * Optional: not supported by GCC
>>                 https://gcc.gnu.org/bugzilla/show_bug.cgi?id=80223
>
>-- 
>Thanks,
>~Nick Desaulniers

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH 2/2] Kconfig: CC_HAS_NO_PROFILE_FN_ATTR, depend on for GCOV and PGO
  2021-06-21 18:22     ` Nick Desaulniers
@ 2021-06-21 18:50       ` Bill Wendling
  2021-06-21 20:43         ` Nick Desaulniers
  0 siblings, 1 reply; 20+ messages in thread
From: Bill Wendling @ 2021-06-21 18:50 UTC (permalink / raw)
  To: Nick Desaulniers
  Cc: Will Deacon, Catalin Marinas, Mark Rutland, Ard Biesheuvel,
	Heiko Carstens, Vasily Gorbik, Christian Borntraeger,
	Peter Oberparleiter, Kees Cook, Peter Zijlstra, Bill Wendling,
	Sami Tolvanen, Masahiro Yamada, Miguel Ojeda, Nathan Chancellor,
	Luc Van Oostenryck, Arnd Bergmann, Andrew Morton,
	Rasmus Villemoes, LKML, clang-built-linux,
	maintainer:X86 ARCHITECTURE (32-BIT AND 64-BIT),
	Borislav Petkov, Martin Liska, Jonathan Corbet, Fangrui Song,
	Linux Doc Mailing List, Linux Kbuild mailing list, Dmitry Vyukov,
	Johannes Berg, linux-toolchains, Marco Elver, Linux ARM,
	linux-s390

On Mon, Jun 21, 2021 at 11:22 AM Nick Desaulniers
<ndesaulniers@google.com> wrote:
>
> On Fri, Jun 18, 2021 at 11:23 PM Marco Elver <elver@google.com> wrote:
> >
> > On Sat, 19 Jun 2021 at 01:30, Nick Desaulniers <ndesaulniers@google.com> wrote:
> > >
> > > We don't want compiler instrumentation to touch noinstr functions, which
> > > are annotated with the no_profile function attribute. Add a Kconfig test
> > > for this and make PGO and GCOV depend on it.
> > >
> > > Cc: Masahiro Yamada <masahiroy@kernel.org>
> > > Cc: Peter Oberparleiter <oberpar@linux.ibm.com>
> > > Link: https://lore.kernel.org/lkml/YMTn9yjuemKFLbws@hirez.programming.kicks-ass.net/
> > > Link: https://lore.kernel.org/lkml/YMcssV%2Fn5IBGv4f0@hirez.programming.kicks-ass.net/
> > > Suggested-by: Peter Zijlstra <peterz@infradead.org>
> > > Signed-off-by: Nick Desaulniers <ndesaulniers@google.com>
> > > ---
> > >  init/Kconfig        | 3 +++
> > >  kernel/gcov/Kconfig | 1 +
> > >  kernel/pgo/Kconfig  | 3 ++-
> > >  3 files changed, 6 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/init/Kconfig b/init/Kconfig
> > > index 1ea12c64e4c9..540f862b40c6 100644
> > > --- a/init/Kconfig
> > > +++ b/init/Kconfig
> > > @@ -83,6 +83,9 @@ config TOOLS_SUPPORT_RELR
> > >  config CC_HAS_ASM_INLINE
> > >         def_bool $(success,echo 'void foo(void) { asm inline (""); }' | $(CC) -x c - -c -o /dev/null)
> > >
> > > +config CC_HAS_NO_PROFILE_FN_ATTR
> > > +       def_bool $(success,echo '__attribute__((no_profile)) int x();' | $(CC) -x c - -c -o /dev/null -Werror)
> > > +
> > >  config CONSTRUCTORS
> > >         bool
> > >
> > > diff --git a/kernel/gcov/Kconfig b/kernel/gcov/Kconfig
> > > index 58f87a3092f3..19facd4289cd 100644
> > > --- a/kernel/gcov/Kconfig
> > > +++ b/kernel/gcov/Kconfig
> > > @@ -5,6 +5,7 @@ config GCOV_KERNEL
> > >         bool "Enable gcov-based kernel profiling"
> > >         depends on DEBUG_FS
> > >         depends on !CC_IS_CLANG || CLANG_VERSION >= 110000
> > > +       depends on !X86 || (X86 && CC_HAS_NO_PROFILE_FN_ATTR)
> >
> > [+Cc Mark]
> >
> > arm64 is also starting to rely on noinstr working properly.
>
> Sure,
> Will, Catalin, other arm64 folks:
> Any thoughts on requiring GCC 7.1+/Clang 13.0+ for GCOV support?  That
> way we can better guarantee that GCOV (and eventually, PGO) don't
> touch noinstr functions?
>
> If that's ok, I'll add modify the above like:
>
> + depends on !ARM64 || (ARM64 && CC_HAS_NO_PROFILE_FN_ATTR)
>
Wouldn't "!ARM64 || CC_HAS_NO_PROFILE_FN_ATTR" be more succinct?

> to the above hunk in v2.  Oh, looks like arch/s390 also uses noinstr.
> Same question applies then:
>
> + depends on !S390 || (S390 && CC_HAS_NO_PROFILE_FN_ATTR)
>
> Or, we could just do
>
> + depends on CC_HAS_NO_PROFILE_FN_ATTR
>
> Though that will penalize architectures not using noinstr, that still
> would like to use GCOV with versions of GCC older than 7.1.  Perhaps
> there are no such such users, or they should consider upgrading their
> tools to we can stick with the simpler Kconfig? Thoughts?
>
> >
> > This should probably be a 'select ARCH_HAS_GCOV_PROFILE_ALL if
> > CC_HAS_NO_PROFILE_FN_ATTR' in the relevant arch/../Kconfig.
> >
> > Alternatively, using:
> > https://lkml.kernel.org/r/YMcssV/n5IBGv4f0@hirez.programming.kicks-ass.net
> >
> > But I'd probably not overcomplicate things at this point and just use
> > ARCH_HAS_GCOV_PROFILE_ALL, because GCOV seems to be a) rarely used,
> > and b) if someone decides to selectively instrument stuff like entry
> > code, we can just say it's user error.
> >
> >
> > >         select CONSTRUCTORS
> > >         default n
> > >         help
> > > diff --git a/kernel/pgo/Kconfig b/kernel/pgo/Kconfig
> > > index d2053df1111c..26f75ac4c6c1 100644
> > > --- a/kernel/pgo/Kconfig
> > > +++ b/kernel/pgo/Kconfig
> > > @@ -8,7 +8,8 @@ config PGO_CLANG
> > >         bool "Enable clang's PGO-based kernel profiling"
> > >         depends on DEBUG_FS
> > >         depends on ARCH_SUPPORTS_PGO_CLANG
> > > -       depends on CC_IS_CLANG && CLANG_VERSION >= 120000
> > > +       depends on CC_IS_CLANG
> > > +       depends on CC_HAS_NO_PROFILE_FN_ATTR
> > >         help
> > >           This option enables clang's PGO (Profile Guided Optimization) based
> > >           code profiling to better optimize the kernel.
> > > --
> > > 2.32.0.288.g62a8d224e6-goog
> > >
>
>
>
> --
> Thanks,
> ~Nick Desaulniers

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH 1/2] compiler_attributes.h: define __no_profile, add to noinstr
  2021-06-21 18:24         ` Fangrui Song
@ 2021-06-21 19:09           ` Miguel Ojeda
  0 siblings, 0 replies; 20+ messages in thread
From: Miguel Ojeda @ 2021-06-21 19:09 UTC (permalink / raw)
  To: Fangrui Song
  Cc: Nick Desaulniers, Kees Cook, Peter Zijlstra, Bill Wendling,
	Sami Tolvanen, Peter Oberparleiter, Masahiro Yamada,
	Nathan Chancellor, Luc Van Oostenryck, Ard Biesheuvel,
	Will Deacon, Arnd Bergmann, Andrew Morton, Rasmus Villemoes,
	linux-kernel, clang-built-linux,
	maintainer:X86 ARCHITECTURE (32-BIT AND 64-BIT),
	Borislav Petkov, Martin Liska, Marco Elver, Jonathan Corbet,
	Linux Doc Mailing List, Linux Kbuild mailing list, Dmitry Vyukov,
	Johannes Berg, linux-toolchains

On Mon, Jun 21, 2021 at 8:24 PM Fangrui Song <maskray@google.com> wrote:
>
> Also a reminder that __GCC4_has_attribute___no_profile in v1 misses two
> underscores. v2 no_profile_instrument_function may need to fix this.

Good catch! Yes, it is missing the last two.

Cheers,
Miguel

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH 2/2] Kconfig: CC_HAS_NO_PROFILE_FN_ATTR, depend on for GCOV and PGO
  2021-06-21 18:50       ` Bill Wendling
@ 2021-06-21 20:43         ` Nick Desaulniers
  2021-06-21 21:15           ` Nathan Chancellor
  2021-06-22  9:25           ` Catalin Marinas
  0 siblings, 2 replies; 20+ messages in thread
From: Nick Desaulniers @ 2021-06-21 20:43 UTC (permalink / raw)
  To: Bill Wendling, Masahiro Yamada
  Cc: Will Deacon, Catalin Marinas, Mark Rutland, Ard Biesheuvel,
	Heiko Carstens, Vasily Gorbik, Christian Borntraeger,
	Peter Oberparleiter, Kees Cook, Peter Zijlstra, Bill Wendling,
	Sami Tolvanen, Miguel Ojeda, Nathan Chancellor,
	Luc Van Oostenryck, Arnd Bergmann, Andrew Morton,
	Rasmus Villemoes, LKML, clang-built-linux,
	maintainer:X86 ARCHITECTURE (32-BIT AND 64-BIT),
	Borislav Petkov, Martin Liska, Jonathan Corbet, Fangrui Song,
	Linux Doc Mailing List, Linux Kbuild mailing list, Dmitry Vyukov,
	Johannes Berg, linux-toolchains, Marco Elver, Linux ARM,
	linux-s390

On Mon, Jun 21, 2021 at 11:50 AM Bill Wendling <morbo@google.com> wrote:
>
> On Mon, Jun 21, 2021 at 11:22 AM Nick Desaulniers
> <ndesaulniers@google.com> wrote:
> >
> > On Fri, Jun 18, 2021 at 11:23 PM Marco Elver <elver@google.com> wrote:
> > >
> > > On Sat, 19 Jun 2021 at 01:30, Nick Desaulniers <ndesaulniers@google.com> wrote:
> > > >
> > > > We don't want compiler instrumentation to touch noinstr functions, which
> > > > are annotated with the no_profile function attribute. Add a Kconfig test
> > > > for this and make PGO and GCOV depend on it.
> > > >
> > > > Cc: Masahiro Yamada <masahiroy@kernel.org>
> > > > Cc: Peter Oberparleiter <oberpar@linux.ibm.com>
> > > > Link: https://lore.kernel.org/lkml/YMTn9yjuemKFLbws@hirez.programming.kicks-ass.net/
> > > > Link: https://lore.kernel.org/lkml/YMcssV%2Fn5IBGv4f0@hirez.programming.kicks-ass.net/
> > > > Suggested-by: Peter Zijlstra <peterz@infradead.org>
> > > > Signed-off-by: Nick Desaulniers <ndesaulniers@google.com>
> > > > ---
> > > >  init/Kconfig        | 3 +++
> > > >  kernel/gcov/Kconfig | 1 +
> > > >  kernel/pgo/Kconfig  | 3 ++-
> > > >  3 files changed, 6 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/init/Kconfig b/init/Kconfig
> > > > index 1ea12c64e4c9..540f862b40c6 100644
> > > > --- a/init/Kconfig
> > > > +++ b/init/Kconfig
> > > > @@ -83,6 +83,9 @@ config TOOLS_SUPPORT_RELR
> > > >  config CC_HAS_ASM_INLINE
> > > >         def_bool $(success,echo 'void foo(void) { asm inline (""); }' | $(CC) -x c - -c -o /dev/null)
> > > >
> > > > +config CC_HAS_NO_PROFILE_FN_ATTR
> > > > +       def_bool $(success,echo '__attribute__((no_profile)) int x();' | $(CC) -x c - -c -o /dev/null -Werror)
> > > > +
> > > >  config CONSTRUCTORS
> > > >         bool
> > > >
> > > > diff --git a/kernel/gcov/Kconfig b/kernel/gcov/Kconfig
> > > > index 58f87a3092f3..19facd4289cd 100644
> > > > --- a/kernel/gcov/Kconfig
> > > > +++ b/kernel/gcov/Kconfig
> > > > @@ -5,6 +5,7 @@ config GCOV_KERNEL
> > > >         bool "Enable gcov-based kernel profiling"
> > > >         depends on DEBUG_FS
> > > >         depends on !CC_IS_CLANG || CLANG_VERSION >= 110000
> > > > +       depends on !X86 || (X86 && CC_HAS_NO_PROFILE_FN_ATTR)
> > >
> > > [+Cc Mark]
> > >
> > > arm64 is also starting to rely on noinstr working properly.
> >
> > Sure,
> > Will, Catalin, other arm64 folks:
> > Any thoughts on requiring GCC 7.1+/Clang 13.0+ for GCOV support?  That
> > way we can better guarantee that GCOV (and eventually, PGO) don't
> > touch noinstr functions?
> >
> > If that's ok, I'll add modify the above like:
> >
> > + depends on !ARM64 || (ARM64 && CC_HAS_NO_PROFILE_FN_ATTR)
> >
> Wouldn't "!ARM64 || CC_HAS_NO_PROFILE_FN_ATTR" be more succinct?

We need to be able to express via Kconfig "GCOV should not be enabled
for architectures that use noinstr when the toolchain does not support
__attribute__((no_profile_instrument_function))."

Where "architectures that use noinstr" are currently arm64, s390, and
x86.  So I guess we could do:

+ depends on !ARM64 || !S390 || !X86 || CC_HAS_NO_PROFILE_FN_ATTR

(We could add a Kconfig for ARCH_WANTS_NO_INSTR, which might be more
informative than listed out architectures which might be non-obvious
to passers-by).

It would be most succinct to raise the requirements to: "GCOV should
not be enabled when the toolchain does not support
__attribute__((no_profile_instrument_function))." Then we could do:

+ depends on CC_HAS_NO_PROFILE_FN_ATTR

Assuming no one has the requirement to support GCOV on PPC with GCC <
7.1, for example.

>
> > to the above hunk in v2.  Oh, looks like arch/s390 also uses noinstr.
> > Same question applies then:
> >
> > + depends on !S390 || (S390 && CC_HAS_NO_PROFILE_FN_ATTR)
> >
> > Or, we could just do
> >
> > + depends on CC_HAS_NO_PROFILE_FN_ATTR
> >
> > Though that will penalize architectures not using noinstr, that still
> > would like to use GCOV with versions of GCC older than 7.1.  Perhaps
> > there are no such such users, or they should consider upgrading their
> > tools to we can stick with the simpler Kconfig? Thoughts?
> >
> > >
> > > This should probably be a 'select ARCH_HAS_GCOV_PROFILE_ALL if
> > > CC_HAS_NO_PROFILE_FN_ATTR' in the relevant arch/../Kconfig.
> > >
> > > Alternatively, using:
> > > https://lkml.kernel.org/r/YMcssV/n5IBGv4f0@hirez.programming.kicks-ass.net
> > >
> > > But I'd probably not overcomplicate things at this point and just use
> > > ARCH_HAS_GCOV_PROFILE_ALL, because GCOV seems to be a) rarely used,
> > > and b) if someone decides to selectively instrument stuff like entry
> > > code, we can just say it's user error.
> > >
> > >
> > > >         select CONSTRUCTORS
> > > >         default n
> > > >         help
> > > > diff --git a/kernel/pgo/Kconfig b/kernel/pgo/Kconfig
> > > > index d2053df1111c..26f75ac4c6c1 100644
> > > > --- a/kernel/pgo/Kconfig
> > > > +++ b/kernel/pgo/Kconfig
> > > > @@ -8,7 +8,8 @@ config PGO_CLANG
> > > >         bool "Enable clang's PGO-based kernel profiling"
> > > >         depends on DEBUG_FS
> > > >         depends on ARCH_SUPPORTS_PGO_CLANG
> > > > -       depends on CC_IS_CLANG && CLANG_VERSION >= 120000
> > > > +       depends on CC_IS_CLANG
> > > > +       depends on CC_HAS_NO_PROFILE_FN_ATTR
> > > >         help
> > > >           This option enables clang's PGO (Profile Guided Optimization) based
> > > >           code profiling to better optimize the kernel.
> > > > --
> > > > 2.32.0.288.g62a8d224e6-goog
> > > >
> >
> >
> >
> > --
> > Thanks,
> > ~Nick Desaulniers



-- 
Thanks,
~Nick Desaulniers

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH 2/2] Kconfig: CC_HAS_NO_PROFILE_FN_ATTR, depend on for GCOV and PGO
  2021-06-21 20:43         ` Nick Desaulniers
@ 2021-06-21 21:15           ` Nathan Chancellor
  2021-06-22  9:25           ` Catalin Marinas
  1 sibling, 0 replies; 20+ messages in thread
From: Nathan Chancellor @ 2021-06-21 21:15 UTC (permalink / raw)
  To: Nick Desaulniers, Bill Wendling, Masahiro Yamada
  Cc: Will Deacon, Catalin Marinas, Mark Rutland, Ard Biesheuvel,
	Heiko Carstens, Vasily Gorbik, Christian Borntraeger,
	Peter Oberparleiter, Kees Cook, Peter Zijlstra, Bill Wendling,
	Sami Tolvanen, Miguel Ojeda, Luc Van Oostenryck, Arnd Bergmann,
	Andrew Morton, Rasmus Villemoes, LKML, clang-built-linux,
	maintainer:X86 ARCHITECTURE (32-BIT AND 64-BIT),
	Borislav Petkov, Martin Liska, Jonathan Corbet, Fangrui Song,
	Linux Doc Mailing List, Linux Kbuild mailing list, Dmitry Vyukov,
	Johannes Berg, linux-toolchains, Marco Elver, Linux ARM,
	linux-s390

On 6/21/2021 1:43 PM, Nick Desaulniers wrote:
> On Mon, Jun 21, 2021 at 11:50 AM Bill Wendling <morbo@google.com> wrote:
>>
>> On Mon, Jun 21, 2021 at 11:22 AM Nick Desaulniers
>> <ndesaulniers@google.com> wrote:
>>>
>>> On Fri, Jun 18, 2021 at 11:23 PM Marco Elver <elver@google.com> wrote:
>>>>
>>>> On Sat, 19 Jun 2021 at 01:30, Nick Desaulniers <ndesaulniers@google.com> wrote:
>>>>>
>>>>> We don't want compiler instrumentation to touch noinstr functions, which
>>>>> are annotated with the no_profile function attribute. Add a Kconfig test
>>>>> for this and make PGO and GCOV depend on it.
>>>>>
>>>>> Cc: Masahiro Yamada <masahiroy@kernel.org>
>>>>> Cc: Peter Oberparleiter <oberpar@linux.ibm.com>
>>>>> Link: https://lore.kernel.org/lkml/YMTn9yjuemKFLbws@hirez.programming.kicks-ass.net/
>>>>> Link: https://lore.kernel.org/lkml/YMcssV%2Fn5IBGv4f0@hirez.programming.kicks-ass.net/
>>>>> Suggested-by: Peter Zijlstra <peterz@infradead.org>
>>>>> Signed-off-by: Nick Desaulniers <ndesaulniers@google.com>
>>>>> ---
>>>>>   init/Kconfig        | 3 +++
>>>>>   kernel/gcov/Kconfig | 1 +
>>>>>   kernel/pgo/Kconfig  | 3 ++-
>>>>>   3 files changed, 6 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/init/Kconfig b/init/Kconfig
>>>>> index 1ea12c64e4c9..540f862b40c6 100644
>>>>> --- a/init/Kconfig
>>>>> +++ b/init/Kconfig
>>>>> @@ -83,6 +83,9 @@ config TOOLS_SUPPORT_RELR
>>>>>   config CC_HAS_ASM_INLINE
>>>>>          def_bool $(success,echo 'void foo(void) { asm inline (""); }' | $(CC) -x c - -c -o /dev/null)
>>>>>
>>>>> +config CC_HAS_NO_PROFILE_FN_ATTR
>>>>> +       def_bool $(success,echo '__attribute__((no_profile)) int x();' | $(CC) -x c - -c -o /dev/null -Werror)
>>>>> +
>>>>>   config CONSTRUCTORS
>>>>>          bool
>>>>>
>>>>> diff --git a/kernel/gcov/Kconfig b/kernel/gcov/Kconfig
>>>>> index 58f87a3092f3..19facd4289cd 100644
>>>>> --- a/kernel/gcov/Kconfig
>>>>> +++ b/kernel/gcov/Kconfig
>>>>> @@ -5,6 +5,7 @@ config GCOV_KERNEL
>>>>>          bool "Enable gcov-based kernel profiling"
>>>>>          depends on DEBUG_FS
>>>>>          depends on !CC_IS_CLANG || CLANG_VERSION >= 110000
>>>>> +       depends on !X86 || (X86 && CC_HAS_NO_PROFILE_FN_ATTR)
>>>>
>>>> [+Cc Mark]
>>>>
>>>> arm64 is also starting to rely on noinstr working properly.
>>>
>>> Sure,
>>> Will, Catalin, other arm64 folks:
>>> Any thoughts on requiring GCC 7.1+/Clang 13.0+ for GCOV support?  That
>>> way we can better guarantee that GCOV (and eventually, PGO) don't
>>> touch noinstr functions?
>>>
>>> If that's ok, I'll add modify the above like:
>>>
>>> + depends on !ARM64 || (ARM64 && CC_HAS_NO_PROFILE_FN_ATTR)
>>>
>> Wouldn't "!ARM64 || CC_HAS_NO_PROFILE_FN_ATTR" be more succinct?
> 
> We need to be able to express via Kconfig "GCOV should not be enabled
> for architectures that use noinstr when the toolchain does not support
> __attribute__((no_profile_instrument_function))."
> 
> Where "architectures that use noinstr" are currently arm64, s390, and
> x86.  So I guess we could do:
> 
> + depends on !ARM64 || !S390 || !X86 || CC_HAS_NO_PROFILE_FN_ATTR
> 
> (We could add a Kconfig for ARCH_WANTS_NO_INSTR, which might be more
> informative than listed out architectures which might be non-obvious
> to passers-by).

I agree that spelling this out might be nicer for the future, in case 
instances like this crop up again. ARCH_REQUIRES_NO_INSTR might be a 
better wording?

> It would be most succinct to raise the requirements to: "GCOV should
> not be enabled when the toolchain does not support
> __attribute__((no_profile_instrument_function))." Then we could do:
> 
> + depends on CC_HAS_NO_PROFILE_FN_ATTR

Then this could become

depends on !ARCH_REQUIRES_NO_INSTR || (ARCH_REQUIRES_NO_INSTR && 
CC_HAS_NO_PROFILE_FN_ATTR)

(sorry for the potential wrap).

Cheers,
Nathan

> Assuming no one has the requirement to support GCOV on PPC with GCC <
> 7.1, for example.
> 
>>
>>> to the above hunk in v2.  Oh, looks like arch/s390 also uses noinstr.
>>> Same question applies then:
>>>
>>> + depends on !S390 || (S390 && CC_HAS_NO_PROFILE_FN_ATTR)
>>>
>>> Or, we could just do
>>>
>>> + depends on CC_HAS_NO_PROFILE_FN_ATTR
>>>
>>> Though that will penalize architectures not using noinstr, that still
>>> would like to use GCOV with versions of GCC older than 7.1.  Perhaps
>>> there are no such such users, or they should consider upgrading their
>>> tools to we can stick with the simpler Kconfig? Thoughts?
>>>
>>>>
>>>> This should probably be a 'select ARCH_HAS_GCOV_PROFILE_ALL if
>>>> CC_HAS_NO_PROFILE_FN_ATTR' in the relevant arch/../Kconfig.
>>>>
>>>> Alternatively, using:
>>>> https://lkml.kernel.org/r/YMcssV/n5IBGv4f0@hirez.programming.kicks-ass.net
>>>>
>>>> But I'd probably not overcomplicate things at this point and just use
>>>> ARCH_HAS_GCOV_PROFILE_ALL, because GCOV seems to be a) rarely used,
>>>> and b) if someone decides to selectively instrument stuff like entry
>>>> code, we can just say it's user error.
>>>>
>>>>
>>>>>          select CONSTRUCTORS
>>>>>          default n
>>>>>          help
>>>>> diff --git a/kernel/pgo/Kconfig b/kernel/pgo/Kconfig
>>>>> index d2053df1111c..26f75ac4c6c1 100644
>>>>> --- a/kernel/pgo/Kconfig
>>>>> +++ b/kernel/pgo/Kconfig
>>>>> @@ -8,7 +8,8 @@ config PGO_CLANG
>>>>>          bool "Enable clang's PGO-based kernel profiling"
>>>>>          depends on DEBUG_FS
>>>>>          depends on ARCH_SUPPORTS_PGO_CLANG
>>>>> -       depends on CC_IS_CLANG && CLANG_VERSION >= 120000
>>>>> +       depends on CC_IS_CLANG
>>>>> +       depends on CC_HAS_NO_PROFILE_FN_ATTR
>>>>>          help
>>>>>            This option enables clang's PGO (Profile Guided Optimization) based
>>>>>            code profiling to better optimize the kernel.
>>>>> --
>>>>> 2.32.0.288.g62a8d224e6-goog
>>>>>
>>>
>>>
>>>
>>> --
>>> Thanks,
>>> ~Nick Desaulniers
> 
> 
> 

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH 2/2] Kconfig: CC_HAS_NO_PROFILE_FN_ATTR, depend on for GCOV and PGO
  2021-06-21 20:43         ` Nick Desaulniers
  2021-06-21 21:15           ` Nathan Chancellor
@ 2021-06-22  9:25           ` Catalin Marinas
  2021-06-22  9:29             ` Mark Rutland
  1 sibling, 1 reply; 20+ messages in thread
From: Catalin Marinas @ 2021-06-22  9:25 UTC (permalink / raw)
  To: Nick Desaulniers
  Cc: Bill Wendling, Masahiro Yamada, Will Deacon, Mark Rutland,
	Ard Biesheuvel, Heiko Carstens, Vasily Gorbik,
	Christian Borntraeger, Peter Oberparleiter, Kees Cook,
	Peter Zijlstra, Bill Wendling, Sami Tolvanen, Miguel Ojeda,
	Nathan Chancellor, Luc Van Oostenryck, Arnd Bergmann,
	Andrew Morton, Rasmus Villemoes, LKML, clang-built-linux,
	maintainer:X86 ARCHITECTURE (32-BIT AND 64-BIT),
	Borislav Petkov, Martin Liska, Jonathan Corbet, Fangrui Song,
	Linux Doc Mailing List, Linux Kbuild mailing list, Dmitry Vyukov,
	Johannes Berg, linux-toolchains, Marco Elver, Linux ARM,
	linux-s390

On Mon, Jun 21, 2021 at 01:43:54PM -0700, Nick Desaulniers wrote:
> On Mon, Jun 21, 2021 at 11:50 AM Bill Wendling <morbo@google.com> wrote:
> > On Mon, Jun 21, 2021 at 11:22 AM Nick Desaulniers
> > <ndesaulniers@google.com> wrote:
> > > On Fri, Jun 18, 2021 at 11:23 PM Marco Elver <elver@google.com> wrote:
> > > > On Sat, 19 Jun 2021 at 01:30, Nick Desaulniers <ndesaulniers@google.com> wrote:
> > > > > We don't want compiler instrumentation to touch noinstr functions, which
> > > > > are annotated with the no_profile function attribute. Add a Kconfig test
> > > > > for this and make PGO and GCOV depend on it.
> > > > >
> > > > > Cc: Masahiro Yamada <masahiroy@kernel.org>
> > > > > Cc: Peter Oberparleiter <oberpar@linux.ibm.com>
> > > > > Link: https://lore.kernel.org/lkml/YMTn9yjuemKFLbws@hirez.programming.kicks-ass.net/
> > > > > Link: https://lore.kernel.org/lkml/YMcssV%2Fn5IBGv4f0@hirez.programming.kicks-ass.net/
> > > > > Suggested-by: Peter Zijlstra <peterz@infradead.org>
> > > > > Signed-off-by: Nick Desaulniers <ndesaulniers@google.com>
> > > > > ---
> > > > >  init/Kconfig        | 3 +++
> > > > >  kernel/gcov/Kconfig | 1 +
> > > > >  kernel/pgo/Kconfig  | 3 ++-
> > > > >  3 files changed, 6 insertions(+), 1 deletion(-)
> > > > >
> > > > > diff --git a/init/Kconfig b/init/Kconfig
> > > > > index 1ea12c64e4c9..540f862b40c6 100644
> > > > > --- a/init/Kconfig
> > > > > +++ b/init/Kconfig
> > > > > @@ -83,6 +83,9 @@ config TOOLS_SUPPORT_RELR
> > > > >  config CC_HAS_ASM_INLINE
> > > > >         def_bool $(success,echo 'void foo(void) { asm inline (""); }' | $(CC) -x c - -c -o /dev/null)
> > > > >
> > > > > +config CC_HAS_NO_PROFILE_FN_ATTR
> > > > > +       def_bool $(success,echo '__attribute__((no_profile)) int x();' | $(CC) -x c - -c -o /dev/null -Werror)
> > > > > +
> > > > >  config CONSTRUCTORS
> > > > >         bool
> > > > >
> > > > > diff --git a/kernel/gcov/Kconfig b/kernel/gcov/Kconfig
> > > > > index 58f87a3092f3..19facd4289cd 100644
> > > > > --- a/kernel/gcov/Kconfig
> > > > > +++ b/kernel/gcov/Kconfig
> > > > > @@ -5,6 +5,7 @@ config GCOV_KERNEL
> > > > >         bool "Enable gcov-based kernel profiling"
> > > > >         depends on DEBUG_FS
> > > > >         depends on !CC_IS_CLANG || CLANG_VERSION >= 110000
> > > > > +       depends on !X86 || (X86 && CC_HAS_NO_PROFILE_FN_ATTR)
> > > >
> > > > [+Cc Mark]
> > > >
> > > > arm64 is also starting to rely on noinstr working properly.
> > >
> > > Sure,
> > > Will, Catalin, other arm64 folks:
> > > Any thoughts on requiring GCC 7.1+/Clang 13.0+ for GCOV support?  That
> > > way we can better guarantee that GCOV (and eventually, PGO) don't
> > > touch noinstr functions?
> > >
> > > If that's ok, I'll add modify the above like:
> > >
> > > + depends on !ARM64 || (ARM64 && CC_HAS_NO_PROFILE_FN_ATTR)
> >
> > Wouldn't "!ARM64 || CC_HAS_NO_PROFILE_FN_ATTR" be more succinct?
> 
> We need to be able to express via Kconfig "GCOV should not be enabled
> for architectures that use noinstr when the toolchain does not support
> __attribute__((no_profile_instrument_function))."
> 
> Where "architectures that use noinstr" are currently arm64, s390, and
> x86.  So I guess we could do:
> 
> + depends on !ARM64 || !S390 || !X86 || CC_HAS_NO_PROFILE_FN_ATTR

I think you want:

  depends on !(ARM64 || S390 || X86) || CC_HAS_NO_PROFILE_FN_ATTR

> (We could add a Kconfig for ARCH_WANTS_NO_INSTR, which might be more
> informative than listed out architectures which might be non-obvious
> to passers-by).

That would probably look better.

-- 
Catalin

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH 2/2] Kconfig: CC_HAS_NO_PROFILE_FN_ATTR, depend on for GCOV and PGO
  2021-06-22  9:25           ` Catalin Marinas
@ 2021-06-22  9:29             ` Mark Rutland
  2021-06-22  9:32               ` Catalin Marinas
  0 siblings, 1 reply; 20+ messages in thread
From: Mark Rutland @ 2021-06-22  9:29 UTC (permalink / raw)
  To: Catalin Marinas
  Cc: Nick Desaulniers, Bill Wendling, Masahiro Yamada, Will Deacon,
	Ard Biesheuvel, Heiko Carstens, Vasily Gorbik,
	Christian Borntraeger, Peter Oberparleiter, Kees Cook,
	Peter Zijlstra, Bill Wendling, Sami Tolvanen, Miguel Ojeda,
	Nathan Chancellor, Luc Van Oostenryck, Arnd Bergmann,
	Andrew Morton, Rasmus Villemoes, LKML, clang-built-linux,
	maintainer:X86 ARCHITECTURE (32-BIT AND 64-BIT),
	Borislav Petkov, Martin Liska, Jonathan Corbet, Fangrui Song,
	Linux Doc Mailing List, Linux Kbuild mailing list, Dmitry Vyukov,
	Johannes Berg, linux-toolchains, Marco Elver, Linux ARM,
	linux-s390

On Tue, Jun 22, 2021 at 10:25:34AM +0100, Catalin Marinas wrote:
> On Mon, Jun 21, 2021 at 01:43:54PM -0700, Nick Desaulniers wrote:
> > We need to be able to express via Kconfig "GCOV should not be enabled
> > for architectures that use noinstr when the toolchain does not support
> > __attribute__((no_profile_instrument_function))."
> > 
> > Where "architectures that use noinstr" are currently arm64, s390, and
> > x86.  So I guess we could do:
> > 
> > + depends on !ARM64 || !S390 || !X86 || CC_HAS_NO_PROFILE_FN_ATTR
> 
> I think you want:
> 
>   depends on !(ARM64 || S390 || X86) || CC_HAS_NO_PROFILE_FN_ATTR
> 
> > (We could add a Kconfig for ARCH_WANTS_NO_INSTR, which might be more
> > informative than listed out architectures which might be non-obvious
> > to passers-by).
> 
> That would probably look better.

It does; see:

https://lore.kernel.org/r/20210621231822.2848305-1-ndesaulniers@google.com

:)

Thanks,
Mark.

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH 2/2] Kconfig: CC_HAS_NO_PROFILE_FN_ATTR, depend on for GCOV and PGO
  2021-06-22  9:29             ` Mark Rutland
@ 2021-06-22  9:32               ` Catalin Marinas
  0 siblings, 0 replies; 20+ messages in thread
From: Catalin Marinas @ 2021-06-22  9:32 UTC (permalink / raw)
  To: Mark Rutland
  Cc: Nick Desaulniers, Bill Wendling, Masahiro Yamada, Will Deacon,
	Ard Biesheuvel, Heiko Carstens, Vasily Gorbik,
	Christian Borntraeger, Peter Oberparleiter, Kees Cook,
	Peter Zijlstra, Bill Wendling, Sami Tolvanen, Miguel Ojeda,
	Nathan Chancellor, Luc Van Oostenryck, Arnd Bergmann,
	Andrew Morton, Rasmus Villemoes, LKML, clang-built-linux,
	maintainer:X86 ARCHITECTURE (32-BIT AND 64-BIT),
	Borislav Petkov, Martin Liska, Jonathan Corbet, Fangrui Song,
	Linux Doc Mailing List, Linux Kbuild mailing list, Dmitry Vyukov,
	Johannes Berg, linux-toolchains, Marco Elver, Linux ARM,
	linux-s390

On Tue, Jun 22, 2021 at 10:29:37AM +0100, Mark Rutland wrote:
> On Tue, Jun 22, 2021 at 10:25:34AM +0100, Catalin Marinas wrote:
> > On Mon, Jun 21, 2021 at 01:43:54PM -0700, Nick Desaulniers wrote:
> > > We need to be able to express via Kconfig "GCOV should not be enabled
> > > for architectures that use noinstr when the toolchain does not support
> > > __attribute__((no_profile_instrument_function))."
> > > 
> > > Where "architectures that use noinstr" are currently arm64, s390, and
> > > x86.  So I guess we could do:
> > > 
> > > + depends on !ARM64 || !S390 || !X86 || CC_HAS_NO_PROFILE_FN_ATTR
> > 
> > I think you want:
> > 
> >   depends on !(ARM64 || S390 || X86) || CC_HAS_NO_PROFILE_FN_ATTR
> > 
> > > (We could add a Kconfig for ARCH_WANTS_NO_INSTR, which might be more
> > > informative than listed out architectures which might be non-obvious
> > > to passers-by).
> > 
> > That would probably look better.
> 
> It does; see:
> 
> https://lore.kernel.org/r/20210621231822.2848305-1-ndesaulniers@google.com

I'm getting there, eventually ;).

-- 
Catalin

^ permalink raw reply	[flat|nested] 20+ messages in thread

end of thread, other threads:[~2021-06-22  9:32 UTC | newest]

Thread overview: 20+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-06-18 23:30 [PATCH 0/2] no_profile fn attr and Kconfig for GCOV+PGO Nick Desaulniers
2021-06-18 23:30 ` [PATCH 1/2] compiler_attributes.h: define __no_profile, add to noinstr Nick Desaulniers
2021-06-19 11:26   ` Miguel Ojeda
2021-06-19 11:32     ` Miguel Ojeda
2021-06-21 18:10       ` Nick Desaulniers
2021-06-21 18:24         ` Fangrui Song
2021-06-21 19:09           ` Miguel Ojeda
2021-06-18 23:30 ` [PATCH 2/2] Kconfig: CC_HAS_NO_PROFILE_FN_ATTR, depend on for GCOV and PGO Nick Desaulniers
2021-06-19  6:23   ` Marco Elver
2021-06-21 18:22     ` Nick Desaulniers
2021-06-21 18:50       ` Bill Wendling
2021-06-21 20:43         ` Nick Desaulniers
2021-06-21 21:15           ` Nathan Chancellor
2021-06-22  9:25           ` Catalin Marinas
2021-06-22  9:29             ` Mark Rutland
2021-06-22  9:32               ` Catalin Marinas
2021-06-18 23:52 ` [PATCH 0/2] no_profile fn attr and Kconfig for GCOV+PGO Fangrui Song
2021-06-19  2:45 ` Kees Cook
2021-06-20  8:08   ` Bill Wendling
2021-06-20 14:53   ` Miguel Ojeda

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).