[PATCH] gcov,x86: Mark GCOV broken for x86

[PATCH] gcov,x86: Mark GCOV broken for x86

[Peter Zijlstra]

As recently discovered, there is no function attribute to disable the
-fprofile-generate instrumentation. As such, GCOV is fundamentally
incompatible with architectures that rely on 'noinstr' for correctness.

Until such time as that compilers have added a function attribute to
disable this instrumentation, mark GCOV as broken.

Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
 arch/x86/Kconfig    | 2 +-
 kernel/gcov/Kconfig | 4 ++++
 2 files changed, 5 insertions(+), 1 deletion(-)

diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
index 86dae426798b..c0f8c9d4c31a 100644
--- a/arch/x86/Kconfig
+++ b/arch/x86/Kconfig
@@ -75,7 +75,7 @@ config X86
 	select ARCH_HAS_FAST_MULTIPLIER
 	select ARCH_HAS_FILTER_PGPROT
 	select ARCH_HAS_FORTIFY_SOURCE
-	select ARCH_HAS_GCOV_PROFILE_ALL
+	select ARCH_HAS_GCOV_BROKEN
 	select ARCH_HAS_KCOV			if X86_64 && STACK_VALIDATION
 	select ARCH_HAS_MEM_ENCRYPT
 	select ARCH_HAS_MEMBARRIER_SYNC_CORE
diff --git a/kernel/gcov/Kconfig b/kernel/gcov/Kconfig
index 58f87a3092f3..74b028a66ebe 100644
--- a/kernel/gcov/Kconfig
+++ b/kernel/gcov/Kconfig
@@ -1,10 +1,14 @@
 # SPDX-License-Identifier: GPL-2.0-only
 menu "GCOV-based kernel profiling"
 
+config ARCH_HAS_GCOV_BROKEN
+	def_bool n
+
 config GCOV_KERNEL
 	bool "Enable gcov-based kernel profiling"
 	depends on DEBUG_FS
 	depends on !CC_IS_CLANG || CLANG_VERSION >= 110000
+	depends on !ARCH_HAS_GCOV_BROKEN
 	select CONSTRUCTORS
 	default n
 	help

Re: [PATCH] gcov,x86: Mark GCOV broken for x86

[Marco Elver]

On Mon, 14 Jun 2021 at 12:17, Peter Zijlstra <peterz@infradead.org> wrote:
> As recently discovered, there is no function attribute to disable the
> -fprofile-generate instrumentation. As such, GCOV is fundamentally
> incompatible with architectures that rely on 'noinstr' for correctness.

GCOV today uses only -fprofile-arcs -ftest-coverage. But the problem
is the same: https://godbolt.org/z/fr9cs4sar

> Until such time as that compilers have added a function attribute to
> disable this instrumentation, mark GCOV as broken.
>
> Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> ---
>  arch/x86/Kconfig    | 2 +-
>  kernel/gcov/Kconfig | 4 ++++
>  2 files changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
> index 86dae426798b..c0f8c9d4c31a 100644
> --- a/arch/x86/Kconfig
> +++ b/arch/x86/Kconfig
> @@ -75,7 +75,7 @@ config X86
>         select ARCH_HAS_FAST_MULTIPLIER
>         select ARCH_HAS_FILTER_PGPROT
>         select ARCH_HAS_FORTIFY_SOURCE
> -       select ARCH_HAS_GCOV_PROFILE_ALL
> +       select ARCH_HAS_GCOV_BROKEN
>         select ARCH_HAS_KCOV                    if X86_64 && STACK_VALIDATION
>         select ARCH_HAS_MEM_ENCRYPT
>         select ARCH_HAS_MEMBARRIER_SYNC_CORE
> diff --git a/kernel/gcov/Kconfig b/kernel/gcov/Kconfig
> index 58f87a3092f3..74b028a66ebe 100644
> --- a/kernel/gcov/Kconfig
> +++ b/kernel/gcov/Kconfig
> @@ -1,10 +1,14 @@
>  # SPDX-License-Identifier: GPL-2.0-only
>  menu "GCOV-based kernel profiling"
>
> +config ARCH_HAS_GCOV_BROKEN
> +       def_bool n
> +
>  config GCOV_KERNEL
>         bool "Enable gcov-based kernel profiling"
>         depends on DEBUG_FS
>         depends on !CC_IS_CLANG || CLANG_VERSION >= 110000
> +       depends on !ARCH_HAS_GCOV_BROKEN
>         select CONSTRUCTORS
>         default n
>         help

Re: [PATCH] gcov,x86: Mark GCOV broken for x86

[Peter Oberparleiter]

On 14.06.2021 12:17, Peter Zijlstra wrote:
> As recently discovered, there is no function attribute to disable the
> -fprofile-generate instrumentation. As such, GCOV is fundamentally
> incompatible with architectures that rely on 'noinstr' for correctness.

Does this problem affect all code or just those pieces that use
'noinstr'? Doing a quick grep over kernel source shows me ~40 source
files containing 'noinstr' vs. ~30000 that don't.

It seems to me like an extreme measure to disable gcov-based profiling
for all files on an architecture when only a small fraction of code
would actually be affected.

I'll gladly admit that I haven't followed the full discussion that lead
to your patch, so maybe some of the following suggestions may already
have been proposed.

What about marking source files that contain 'noinstr' using the

  GCOV_PROFILE_<filename.o> := n

directive that gcov-kernel profiling provides to exclude those files
from being compiled with the corresponding profiling flags? If that's
too much effort there's also a directive for excluding all files in a
directory.

If there was a way to automatically identify 'noinstr'-afflicted source
files (e.g. by grepping the pre-processed source files), one could also
automate this process by adjusting the kbuild-code that adds profiling
flags to automatically exclude such files.

> 
> Until such time as that compilers have added a function attribute to
> disable this instrumentation, mark GCOV as broken.
> 
> Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> ---
>  arch/x86/Kconfig    | 2 +-
>  kernel/gcov/Kconfig | 4 ++++
>  2 files changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
> index 86dae426798b..c0f8c9d4c31a 100644
> --- a/arch/x86/Kconfig
> +++ b/arch/x86/Kconfig
> @@ -75,7 +75,7 @@ config X86
>  	select ARCH_HAS_FAST_MULTIPLIER
>  	select ARCH_HAS_FILTER_PGPROT
>  	select ARCH_HAS_FORTIFY_SOURCE
> -	select ARCH_HAS_GCOV_PROFILE_ALL
> +	select ARCH_HAS_GCOV_BROKEN

Assuming none of the above mentioned alternatives are viable, removing
ARCH_HAS_GCOV_PROFILE_ALL should be enough for your purpose. This way
you are already excluding all source files from automatic profiling on x86.

Users that are absolutely sure that their code can work with
gcov-profiling can manually edit their sub-Makefiles to list those files
that should be instrumented. In my opinion your introduction of
ARCH_HAS_GCOV_BROKEN unnecessarily takes away this capability.


Regards,
  Peter Oberparleiter

-- 
Peter Oberparleiter
Linux on Z Development - IBM Germany

Re: [PATCH] gcov,x86: Mark GCOV broken for x86

[Peter Zijlstra]

On Mon, Jun 14, 2021 at 04:43:27PM +0200, Peter Oberparleiter wrote:
> On 14.06.2021 12:17, Peter Zijlstra wrote:
> > As recently discovered, there is no function attribute to disable the
> > -fprofile-generate instrumentation. As such, GCOV is fundamentally
> > incompatible with architectures that rely on 'noinstr' for correctness.
> 
> Does this problem affect all code or just those pieces that use
> 'noinstr'? Doing a quick grep over kernel source shows me ~40 source
> files containing 'noinstr' vs. ~30000 that don't.

I count 82, but yeah.

> It seems to me like an extreme measure to disable gcov-based profiling
> for all files on an architecture when only a small fraction of code
> would actually be affected.
> 
> I'll gladly admit that I haven't followed the full discussion that lead
> to your patch, so maybe some of the following suggestions may already
> have been proposed.
> 
> What about marking source files that contain 'noinstr' using the
> 
>   GCOV_PROFILE_<filename.o> := n
> 
> directive that gcov-kernel profiling provides to exclude those files
> from being compiled with the corresponding profiling flags? If that's
> too much effort there's also a directive for excluding all files in a
> directory.

It's just not scalable and super fragile. Forget one and you have a
potentially dead kernel. At the same time, we'll end up excluding
significant chunks of the core kernel that way, also limit the use of
GCOV.

> If there was a way to automatically identify 'noinstr'-afflicted source
> files (e.g. by grepping the pre-processed source files), one could also
> automate this process by adjusting the kbuild-code that adds profiling
> flags to automatically exclude such files.

Or we just wait for the compilers to implement the required function
attribute and then make the whole thing depend on having a recent enough
compiler, which is what I'm hoping for.

Developers should use recent compilers anyway...

> > Until such time as that compilers have added a function attribute to
> > disable this instrumentation, mark GCOV as broken.
> > 
> > Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> > ---
> >  arch/x86/Kconfig    | 2 +-
> >  kernel/gcov/Kconfig | 4 ++++
> >  2 files changed, 5 insertions(+), 1 deletion(-)
> > 
> > diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
> > index 86dae426798b..c0f8c9d4c31a 100644
> > --- a/arch/x86/Kconfig
> > +++ b/arch/x86/Kconfig
> > @@ -75,7 +75,7 @@ config X86
> >  	select ARCH_HAS_FAST_MULTIPLIER
> >  	select ARCH_HAS_FILTER_PGPROT
> >  	select ARCH_HAS_FORTIFY_SOURCE
> > -	select ARCH_HAS_GCOV_PROFILE_ALL
> > +	select ARCH_HAS_GCOV_BROKEN
> 
> Assuming none of the above mentioned alternatives are viable, removing
> ARCH_HAS_GCOV_PROFILE_ALL should be enough for your purpose. This way
> you are already excluding all source files from automatic profiling on x86.

But you can still select one manually, which is not safe.

> Users that are absolutely sure that their code can work with
> gcov-profiling can manually edit their sub-Makefiles to list those files
> that should be instrumented. In my opinion your introduction of
> ARCH_HAS_GCOV_BROKEN unnecessarily takes away this capability.

Are there any users? Who uses this GCOV stuff, and should we migrate
them to KCOV?

The thing is, I got dead kernel reports from KCOV users really quickly
after all this landed, I've never even heard of a GCOV user, let alone
had a problem report from one.

Given all this seems mostly unused, I suppose we can wait for the
compilers to implement the attribute and simply ignore any and all
problems stemming from the use of GCOV -- telling them to go use KCOV
instead.

At the same time; since there are no users (that I know of), I don't see
the problem with killing the entire thing for x86 either.

Re: [PATCH] gcov,x86: Mark GCOV broken for x86

[Peter Oberparleiter]

On 18.06.2021 13:13, Peter Zijlstra wrote:
> On Mon, Jun 14, 2021 at 04:43:27PM +0200, Peter Oberparleiter wrote:
>> On 14.06.2021 12:17, Peter Zijlstra wrote:
>> If there was a way to automatically identify 'noinstr'-afflicted source
>> files (e.g. by grepping the pre-processed source files), one could also
>> automate this process by adjusting the kbuild-code that adds profiling
>> flags to automatically exclude such files.
> 
> Or we just wait for the compilers to implement the required function
> attribute and then make the whole thing depend on having a recent enough
> compiler, which is what I'm hoping for.

Sounds like the best approach given the current situation.

> Developers should use recent compilers anyway...
> 
>>> Until such time as that compilers have added a function attribute to
>>> disable this instrumentation, mark GCOV as broken.
>>>
>>> Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>

[...]

>> Users that are absolutely sure that their code can work with
>> gcov-profiling can manually edit their sub-Makefiles to list those files
>> that should be instrumented. In my opinion your introduction of
>> ARCH_HAS_GCOV_BROKEN unnecessarily takes away this capability.
> 
> Are there any users?

I honestly cannot tell how many people are using the gcov-kernel
facility, but I guess that is true for most kernel functions. I do get
regular bug reports if things break due to GCC changes though, so there
are definitely some users.

> Who uses this GCOV stuff, and should we migrate them to KCOV?

I have not used KCOV myself, but based on the documentation available
about the mechanism itself and some of its users it appears that GCOV
and KCOV are not feature-equivalent:

KCOV provides an opt-in capability for user-space processes to record a
history of kernel code executed on their behalf. Opt-in means the
process must include code to start recording. There's also some limited
support for collecting data for kernel background tasks.

The output is a sequence of kernel addresses relating to executed basic
blocks - great for consumption in automated mechanisms like fuzzers and
the like, but tooling for post-processing the data into human-readable
format for general purpose review seems to be scarce.

GCOV provides a build-time configurable option to instrument almost all
kernel code or, if requested, only specific parts. There's no need to
opt-in at run-time, all execution of instrumented code running since
boot will be recorded, including interrupt handlers, background tasks,
etc. Of course this also means there's no means to filter by the process
causing the kernel code execution.

The output is GCC's .gcda data format. Using GCC's gcov tool this can be
directly converted into annotated source code containing statement,
function, and branch coverage data. Also there are tools building on
gcov output to create overview pages for the thousands of kernel source
files and detailed graphical output based on this information. This kind
of representation is great for manual review to answer questions like
"all my tests ran, why is there still an uncovered piece of kernel code?"

So to summarize:

KCOV => Great for automated processing (e.g. fuzzing)

GCOV => Great for manual review (e.g. improving test coverage)

> The thing is, I got dead kernel reports from KCOV users really quickly
> after all this landed, I've never even heard of a GCOV user, let alone
> had a problem report from one.

KCOV is used 24/7 by automated fuzzing code - I would assume that those
catch fatal errors rather quickly. GCOV is used when a developer/tester
has a need to review their test coverage. And even then they may only
instrument portions of the kernel that may not affected by the noinstr
problem.

> Given all this seems mostly unused, I suppose we can wait for the
> compilers to implement the attribute and simply ignore any and all
> problems stemming from the use of GCOV -- telling them to go use KCOV
> instead.

It seems this might be resolved for GCC rather quickly...

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=80223#c11

> At the same time; since there are no users (that I know of), I don't see
> the problem with killing the entire thing for x86 either.

If there really were no more GCOV users and KCOV would provide the same
functionality and level of tool support I would not object. But IMO none
of these requirements are met today.


Regards,
  Peter

-- 
Peter Oberparleiter
Linux on Z Development - IBM Germany

Re: [PATCH] gcov,x86: Mark GCOV broken for x86

[Nick Desaulniers]

On Mon, Jun 14, 2021 at 3:17 AM Peter Zijlstra <peterz@infradead.org> wrote:
>
>
> As recently discovered, there is no function attribute to disable the
> -fprofile-generate instrumentation. As such, GCOV is fundamentally
> incompatible with architectures that rely on 'noinstr' for correctness.

Is there context for comment, or is this patch meant as a joke?

>
> Until such time as that compilers have added a function attribute to
> disable this instrumentation, mark GCOV as broken.
>
> Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> ---
>  arch/x86/Kconfig    | 2 +-
>  kernel/gcov/Kconfig | 4 ++++
>  2 files changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
> index 86dae426798b..c0f8c9d4c31a 100644
> --- a/arch/x86/Kconfig
> +++ b/arch/x86/Kconfig
> @@ -75,7 +75,7 @@ config X86
>         select ARCH_HAS_FAST_MULTIPLIER
>         select ARCH_HAS_FILTER_PGPROT
>         select ARCH_HAS_FORTIFY_SOURCE
> -       select ARCH_HAS_GCOV_PROFILE_ALL
> +       select ARCH_HAS_GCOV_BROKEN
>         select ARCH_HAS_KCOV                    if X86_64 && STACK_VALIDATION
>         select ARCH_HAS_MEM_ENCRYPT
>         select ARCH_HAS_MEMBARRIER_SYNC_CORE
> diff --git a/kernel/gcov/Kconfig b/kernel/gcov/Kconfig
> index 58f87a3092f3..74b028a66ebe 100644
> --- a/kernel/gcov/Kconfig
> +++ b/kernel/gcov/Kconfig
> @@ -1,10 +1,14 @@
>  # SPDX-License-Identifier: GPL-2.0-only
>  menu "GCOV-based kernel profiling"
>
> +config ARCH_HAS_GCOV_BROKEN
> +       def_bool n
> +
>  config GCOV_KERNEL
>         bool "Enable gcov-based kernel profiling"
>         depends on DEBUG_FS
>         depends on !CC_IS_CLANG || CLANG_VERSION >= 110000
> +       depends on !ARCH_HAS_GCOV_BROKEN
>         select CONSTRUCTORS
>         default n
>         help



-- 
Thanks,
~Nick Desaulniers

Re: [PATCH] gcov,x86: Mark GCOV broken for x86

[Peter Zijlstra]

On Mon, Jun 14, 2021 at 09:05:04AM -0700, Nick Desaulniers wrote:
> On Mon, Jun 14, 2021 at 3:17 AM Peter Zijlstra <peterz@infradead.org> wrote:
> >
> >
> > As recently discovered, there is no function attribute to disable the
> > -fprofile-generate instrumentation. As such, GCOV is fundamentally
> > incompatible with architectures that rely on 'noinstr' for correctness.
> 
> Is there context for comment, or is this patch meant as a joke?

Only if you think recursion in exception entry code is funny.

noinstr *MUST* disable any and all compiler generated instrumentation,
currently it that isn't the case for -fprofile-gnerate, nor
-fprofile-arc.

Look for all the fun we had with KCOV back then. Luckily KCOV
instrumentation was trivial to patch out using objtool, so that's what
x86 is currently doing.

Luckily both compilers grew a __no_sanitize_coverage recently and we no
longer have to rely on objtool fixing up the compiler output for much
longer.

  https://lkml.kernel.org/r/20210527194448.3470080-1-elver@google.com

Now all we need is one more such attribute to kill -fprofile-* stuff.

Re: [PATCH] gcov,x86: Mark GCOV broken for x86

[Nick Desaulniers]

On Mon, Jun 14, 2021 at 9:20 AM Peter Zijlstra <peterz@infradead.org> wrote:
>
> On Mon, Jun 14, 2021 at 09:05:04AM -0700, Nick Desaulniers wrote:
> > On Mon, Jun 14, 2021 at 3:17 AM Peter Zijlstra <peterz@infradead.org> wrote:
> > >
> > >
> > > As recently discovered, there is no function attribute to disable the
> > > -fprofile-generate instrumentation. As such, GCOV is fundamentally
> > > incompatible with architectures that rely on 'noinstr' for correctness.
> >
> > Is there context for comment, or is this patch meant as a joke?
>
> Only if you think recursion in exception entry code is funny.
>
> noinstr *MUST* disable any and all compiler generated instrumentation,
> currently it that isn't the case for -fprofile-gnerate, nor
> -fprofile-arc.
>
> Look for all the fun we had with KCOV back then. Luckily KCOV
> instrumentation was trivial to patch out using objtool, so that's what
> x86 is currently doing.
>
> Luckily both compilers grew a __no_sanitize_coverage recently and we no
> longer have to rely on objtool fixing up the compiler output for much
> longer.
>
>   https://lkml.kernel.org/r/20210527194448.3470080-1-elver@google.com
>
> Now all we need is one more such attribute to kill -fprofile-* stuff.

__attribute__((no_instrument_function)) is already wired to not emit
calls to mcount()/fentry().  I think extending it to also apply to
coverage (-fprofile-arcs) and instrumentation based profiling
(-fprofile-generate) is reasonable.

-- 
Thanks,
~Nick Desaulniers

Re: [PATCH] gcov,x86: Mark GCOV broken for x86

[Borislav Petkov]

On Mon, Jun 14, 2021 at 11:05:35AM -0700, Nick Desaulniers wrote:
> __attribute__((no_instrument_function)) is already wired to not emit
> calls to mcount()/fentry().  I think extending it to also apply to
> coverage (-fprofile-arcs) and instrumentation based profiling
> (-fprofile-generate) is reasonable.

Is anyone going to ping the gcc folks so that they do it too or should
we open a bug over there simply?

Thx.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Re: [PATCH] gcov,x86: Mark GCOV broken for x86

[Nick Desaulniers]

On Mon, Jun 14, 2021 at 11:21 AM Borislav Petkov <bp@alien8.de> wrote:
>
> On Mon, Jun 14, 2021 at 11:05:35AM -0700, Nick Desaulniers wrote:
> > __attribute__((no_instrument_function)) is already wired to not emit
> > calls to mcount()/fentry().  I think extending it to also apply to
> > coverage (-fprofile-arcs) and instrumentation based profiling
> > (-fprofile-generate) is reasonable.
>
> Is anyone going to ping the gcc folks so that they do it too or should
> we open a bug over there simply?

Yes, I will file a bug (or two) against GCC and then post the links on
the other thread (re: pgo) since that already has the toolchains
mailing list cc'ed.
-- 
Thanks,
~Nick Desaulniers

Re: [PATCH] gcov,x86: Mark GCOV broken for x86

[Borislav Petkov]

On Mon, Jun 14, 2021 at 12:03:15PM -0700, Nick Desaulniers wrote:
> Yes, I will file a bug (or two) against GCC and then post the links on
> the other thread (re: pgo) since that already has the toolchains
> mailing list cc'ed.

Thanks!

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Re: [PATCH] gcov,x86: Mark GCOV broken for x86

[Fangrui Song]

> On Mon, Jun 14, 2021 at 9:20 AM Peter Zijlstra <peterz@infradead.org> wrote:
> >
> > On Mon, Jun 14, 2021 at 09:05:04AM -0700, Nick Desaulniers wrote:
> > > On Mon, Jun 14, 2021 at 3:17 AM Peter Zijlstra <peterz@infradead.org> wrote:
> > > >
> > > >
> > > > As recently discovered, there is no function attribute to disable the
> > > > -fprofile-generate instrumentation. As such, GCOV is fundamentally
> > > > incompatible with architectures that rely on 'noinstr' for correctness.
> > >
> > > Is there context for comment, or is this patch meant as a joke?
> >
> > Only if you think recursion in exception entry code is funny.
> >
> > noinstr *MUST* disable any and all compiler generated instrumentation,
> > currently it that isn't the case for -fprofile-gnerate, nor
> > -fprofile-arc.
> >
> > Look for all the fun we had with KCOV back then. Luckily KCOV
> > instrumentation was trivial to patch out using objtool, so that's what
> > x86 is currently doing.
> >
> > Luckily both compilers grew a __no_sanitize_coverage recently and we no
> > longer have to rely on objtool fixing up the compiler output for much
> > longer.
> >
> >   https://lkml.kernel.org/r/20210527194448.3470080-1-elver@google.com
> >
> > Now all we need is one more such attribute to kill -fprofile-* stuff.
> 
> __attribute__((no_instrument_function)) is already wired to not emit
> calls to mcount()/fentry().  I think extending it to also apply to
> coverage (-fprofile-arcs) and instrumentation based profiling
> (-fprofile-generate) is reasonable.

__attribute__((no_instrument_function)) seems specific to
-finstrument-functions.  Somehow -pg uses it as well. The name may not be
generic, so it may be odd to exclude various instrumentations (there are a ton)
under this generic attribute.

I'd like to understand the definition of notrace and noinstr.

With value profiling disabled, clang -fprofile-generate/gcc -fprofile-arcs
don't add function calls. They just increment a counter in a writable section.
Why isn't that allowed for noinstr functions?

I can understand why -fpatchable-function-entry= is excluded: -fpatchable-function-entry=
causes the section __patchable_function_entries and the kernel may change the nops into call
instructions. And a function call may not be suitable for certain functions.
But I don't understand why incrementing a counter should be disallowed.

Re: [PATCH] gcov,x86: Mark GCOV broken for x86

[Peter Zijlstra]

On Mon, Jun 14, 2021 at 11:31:35AM -0700, Fangrui Song wrote:
> __attribute__((no_instrument_function)) seems specific to
> -finstrument-functions.  Somehow -pg uses it as well. The name may not be
> generic, so it may be odd to exclude various instrumentations (there are a ton)
> under this generic attribute.
> 
> I'd like to understand the definition of notrace and noinstr.

notrace came first; it disallows tracing (fentry/mcount) on specific
functions where tracing is unsafe, like inside the tracer itself.

noinstr is fairly recent, we lifted a whole bunch of code from asm to C
(because it's something that every arch ends up having to do and pretty
much every arch did it wrong, so we're slowly lifting it into arch
neutral code).

We now call into C before the full (kernel) C runtime is properly setup.
Consider it crt0 if you will. But it's sprinkled throughout arch code
and not nicely isolated to a few .c files.

For this we require that noinstr avoids any and all compiler
instrumentation; this really is C as C was intended, portable assembler
style usage (/me runs).

> With value profiling disabled, clang -fprofile-generate/gcc
> -fprofile-arcs don't add function calls. They just increment a counter
> in a writable section.  Why isn't that allowed for noinstr functions?

Because you can get exceptions from memory accesses just fine, which, if
you're inside an exception handler and haven't dealt with recursion
conditions yet, can be fatal.

> I can understand why -fpatchable-function-entry= is excluded:
> -fpatchable-function-entry= causes the section
> __patchable_function_entries and the kernel may change the nops into
> call instructions. And a function call may not be suitable for certain
> functions.  But I don't understand why incrementing a counter should
> be disallowed.

A memory access can generate #PF, #DB, #VE, #VC, #MC from the top of my
head.

Also that's what you do today, GCC emits calls, and those can cause even
more 'fun'.