On Mon, Jun 21, 2021 at 11:50 AM Bill Wendling [off-list ref] wrote:

On Mon, Jun 21, 2021 at 11:22 AM Nick Desaulniers
[off-list ref] wrote:

quoted

On Fri, Jun 18, 2021 at 11:23 PM Marco Elver [off-list ref] wrote:

quoted

On Sat, 19 Jun 2021 at 01:30, Nick Desaulniers [off-list ref] wrote:

quoted

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/ (local)
Link: https://lore.kernel.org/lkml/YMcssV%2Fn5IBGv4f0@hirez.programming.kicks-ass.net/ (local)
Suggested-by: Peter Zijlstra <peterz@infradead.org>
Signed-off-by: Nick Desaulniers <redacted>
---
 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.

quoted

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?

quoted

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.

quoted

        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