[Bug gcov-profile/80223] RFE: Exclude functions from profile instrumentation

[Bug gcov-profile/80223] RFE: Exclude functions from profile instrumentation

[ndesaulniers at google dot com]

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

Nick Desaulniers <ndesaulniers at google dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |elver at google dot com,
                   |                            |isanbard at gmail dot com,
                   |                            |kees at outflux dot net,
                   |                            |maskray at google dot com,
                   |                            |ndesaulniers at google dot com

--- Comment #6 from Nick Desaulniers <ndesaulniers at google dot com> ---
We had a request for something like this today on LKML, see the thread.
https://lore.kernel.org/lkml/CAKwvOdmPTi93n2L0_yQkrzLdmpxzrOR7zggSzonyaw2PGshApw@mail.gmail.com/

And more specific use case:
https://lore.kernel.org/lkml/20210614190700.GF68749@worktop.programming.kicks-ass.net/

I've implemented this in LLVM; no_instrument_function function attribute in C
can be used to disable coverage of -fprofile-generate (instrumentation based
profiling; "PGO") and -fprofile-arcs (coverage; "GCOV").

PGO: https://reviews.llvm.org/D104253
GCOV: https://reviews.llvm.org/D104257

Inlining is a good point and something I'll need to check; generally when
caller's and callee's function attributes don't match, we block inline
substitution (though we permit it for always_inline; developer be damned).

One question Fangrui had made was whether no_instrument_function is the
appropriate function attribute to re-use.
https://reviews.llvm.org/D104253#2817695

It looks like both -finstrument-functions and -pg are affected by attribute
no_instrument_function; I decided to reuse no_instrument_function in LLVM
because:
1. it already exists; implementation is barely more than 1 LoC.
2. it already affects code gen of 2 different flags.
3. its name perfectly describes developer intent.
4. the Linux kernel is already wired up to make use of no_instrument_function
attribute (though the kernel's configuration step (KConfig) will need changes
to detect support for this issue probably).

I haven't landed the changes in LLVM yet, and don't particularly care what the
attribute used ultimately is even if that means revisting our approach in LLVM.

But without a solution to this problem, it's likely to block PGO and regress
GCOV for x86 Linux kernels.

[Bug gcov-profile/80223] RFE: Exclude functions from profile instrumentation

[i at maskray dot me]

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

Fangrui Song <i at maskray dot me> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |i at maskray dot me

--- Comment #7 from Fangrui Song <i at maskray dot me> ---
Some notes.

{gcc,clang} -fsanitize-coverage={trace-pc,trace-cmp} is another coverage
feature. It uses no_sanitize_coverage instead of no_instrument_function. The
GCC support for no_sanitize_coverage is very new (by Martin, in 2021-05-25).
(In Clang, the feature has more modes, e.g. you can control func/bb/edge.)

The Linux kernel use case (include/linux/compiler_types.h ) uses 'noinline' so
inlining is not a concern.

/* Section for code which can't be instrumented at all */
#define noinstr                                                         \
        noinline notrace __attribute((__section__(".noinstr.text")))    \
        __no_kcsan __no_sanitize_address

Clang supports another filtering mechanism, -fprofile-list=
(https://reviews.llvm.org/D94820). But the kernel use case seems to prefer a
function attribute.

[Bug gcov-profile/80223] RFE: Exclude functions from profile instrumentation

[i at maskray dot me]

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

--- Comment #8 from Fangrui Song <i at maskray dot me> ---
I am thinking of __attribute__((no_profile)).

In Clang,
-fprofile-generate(-fcs-profile-generate)/-fprofile-instr-generate/-fprofile-arcs
are all different. It will make sense to have a attribute disabling all such
profiling related features.

I am not sure an umbrella __attribute__((no_instrument_function)) is suitable.
The Linux kernel wanting noinstr to exclude -fprofile-* is a very specific
characteristic, not suitable for other applications.

[Bug gcov-profile/80223] RFE: Exclude functions from profile instrumentation

[ndesaulniers at google dot com]

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

--- Comment #9 from Nick Desaulniers <ndesaulniers at google dot com> ---
(In reply to Fangrui Song from comment #8)
> I am thinking of __attribute__((no_profile)).
> 
> In Clang,
> -fprofile-generate(-fcs-profile-generate)/-fprofile-instr-generate/-fprofile-
> arcs are all different. It will make sense to have a attribute disabling all
> such profiling related features.
> 
> I am not sure an umbrella __attribute__((no_instrument_function)) is
> suitable.
> The Linux kernel wanting noinstr to exclude -fprofile-* is a very specific
> characteristic, not suitable for other applications.

Sure, though now we'll need to add use of that to the Linux kernel. v2:
https://reviews.llvm.org/D104475.

[Bug gcov-profile/80223] RFE: Exclude functions from profile instrumentation

[ndesaulniers at google dot com]

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

--- Comment #10 from Nick Desaulniers <ndesaulniers at google dot com> ---
Link to kernel patches:
https://lore.kernel.org/lkml/20210618233023.1360185-1-ndesaulniers@google.com/

[Bug gcov-profile/80223] RFE: Exclude functions from profile instrumentation

[marxin at gcc dot gnu.org]

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

--- Comment #11 from Martin Liška <marxin at gcc dot gnu.org> ---
Apparently, we already have an attribute that prevents function instrumentation
regarding PGO:

no_profile_instrument_function

The no_profile_instrument_function attribute on functions is used to inform the
compiler that it should not process any profile feedback based optimization
code instrumentation.

It's there for quite some releases.
@Nick: Do you need anything more?

[Bug gcov-profile/80223] RFE: Exclude functions from profile instrumentation

[ndesaulniers at google dot com]

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

--- Comment #12 from Nick Desaulniers <ndesaulniers at google dot com> ---
Ah, perfect!

commit 1225d6b1134b ("Introduce no_profile_instrument_function attribute")

LGTM: https://godbolt.org/z/779xzndY6

Looks like it landed in GCC 7.1.

Let me change over the attribute identifier in Clang to match, then I'll resend
the kernel patches, and close this out.

[Bug gcov-profile/80223] RFE: Exclude functions from profile instrumentation

[marxin at gcc dot gnu.org]

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

Martin Liška <marxin at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |ASSIGNED
           Assignee|unassigned at gcc dot gnu.org      |marxin at gcc dot gnu.org

--- Comment #13 from Martin Liška <marxin at gcc dot gnu.org> ---
What's likely missing is that the attribute should prevent inlining. I'm going
to test how it behaves right now. Then, the issue can be closed.

[Bug gcov-profile/80223] RFE: Exclude functions from profile instrumentation

[i at maskray dot me]

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

--- Comment #14 from Fangrui Song <i at maskray dot me> ---
(In reply to Martin Liška from comment #13)
> What's likely missing is that the attribute should prevent inlining. I'm
> going to test how it behaves right now. Then, the issue can be closed.

It's not clear to me that no_profile_instrument_function should prevent
inlining. I'll argue that attributes should be orthogonal.
https://lists.llvm.org/pipermail/llvm-dev/2021-April/150062.html
https://reviews.llvm.org/D101011#2715555

If the user wants to suppress inlining, add noinline.

Can a no_profile_instrument_function function be inlined to another
no_profile_instrument_function function? Why not.

Can a no_profile_instrument_function function be inlined into a function
without the attribute? This may be controversial but I'd argue that it can. GCC
no_stack_protector behaves this way. no_profile_instrument_function can mean
that user does not want profiling when the function is called with its entity,
not via another entity.

[Bug gcov-profile/80223] RFE: Exclude functions from profile instrumentation

[ndesaulniers at google dot com]

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

--- Comment #15 from Nick Desaulniers <ndesaulniers at google dot com> ---
(In reply to Fangrui Song from comment #14)
> Can a no_profile_instrument_function function be inlined into a function
> without the attribute? This may be controversial but I'd argue that it can.
> GCC no_stack_protector behaves this way. no_profile_instrument_function can
> mean that user does not want profiling when the function is called with its
> entity, not via another entity.

I respectfully but strongly disagree. It's surprising to developers when they
ask for no stack protector, or no profiling instrumentation, then get one
anyways.  For long call chains, it's hard for developers to diagnose on their
own which function they called that missed such function attribute.

This reminds me of "what color is your function?"
https://journal.stuffwithstuff.com/2015/02/01/what-color-is-your-function/
As suddenly a developer would need to verify for a no_* attributed function
that they only call no_* attributed functions, or add noinline (which is a big
hammer to all call sites, and games with aliases that have the noinline
attribute are kind of ridiculous).

It's less surprising to prevent inline substitution upon function attribute
mismatch. Then a developer can self diagnose with -Rpass=inline. Either way,
some form of diagnostics would be helpful for these kinds of issues, and has
been requested by Android platform developers working on Zygote.

For no_stack_protector in LLVM, I implemented the rules: upon mismatch, prevent
inline substitution unless the user specified always_inline.  This fixed
suspend/resume bugs in x86 Linux kernels when built with LTO.

Though, I'm happy to revisit that behavior in LLVM; we could add

#define noinline_for_lto __attribute__((__noinline__))

then use that in the Linux kernel instead.

[Bug gcov-profile/80223] RFE: Exclude functions from profile instrumentation

[ndesaulniers at google dot com]

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

--- Comment #16 from Nick Desaulniers <ndesaulniers at google dot com> ---
Clang patch (no_profile -> no_profile_instrument_function):
https://reviews.llvm.org/D104658
Kernel patches v2:
https://lore.kernel.org/lkml/20210621231822.2848305-1-ndesaulniers@google.com/

[Bug gcov-profile/80223] RFE: Exclude functions from profile instrumentation

[marxin at gcc dot gnu.org]

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

--- Comment #17 from Martin Liška <marxin at gcc dot gnu.org> ---
All right, similarly to sanitizer flags, I sent a patch that prevent inlining
when -fprofile-generate is used:
https://gcc.gnu.org/pipermail/gcc-patches/2021-June/573511.html

Note that one typically uses the attribute when a function is hot and would
delay instrumentation a lot. That's why we don't want the function be inlined.

Moreover, each hot function excluded from instrumentation should be likely
decorated with 'hot' attribute.

[Bug gcov-profile/80223] RFE: Exclude functions from profile instrumentation

[i at maskray dot me]

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

--- Comment #18 from Fangrui Song <i at maskray dot me> ---
(In reply to Nick Desaulniers from comment #15)
> (In reply to Fangrui Song from comment #14)
> > Can a no_profile_instrument_function function be inlined into a function
> > without the attribute? This may be controversial but I'd argue that it can.
> > GCC no_stack_protector behaves this way. no_profile_instrument_function can
> > mean that user does not want profiling when the function is called with its
> > entity, not via another entity.
> 
> I respectfully but strongly disagree. It's surprising to developers when
> they ask for no stack protector, or no profiling instrumentation, then get
> one anyways.  For long call chains, it's hard for developers to diagnose on
> their own which function they called that missed such function attribute.
> 
> This reminds me of "what color is your function?"
> https://journal.stuffwithstuff.com/2015/02/01/what-color-is-your-function/
> As suddenly a developer would need to verify for a no_* attributed function
> that they only call no_* attributed functions, or add noinline (which is a
> big hammer to all call sites, and games with aliases that have the noinline
> attribute are kind of ridiculous).
> 
> It's less surprising to prevent inline substitution upon function attribute
> mismatch. Then a developer can self diagnose with -Rpass=inline. Either way,
> some form of diagnostics would be helpful for these kinds of issues, and has
> been requested by Android platform developers working on Zygote.
> 
> For no_stack_protector in LLVM, I implemented the rules: upon mismatch,
> prevent inline substitution unless the user specified always_inline.  This
> fixed suspend/resume bugs in x86 Linux kernels when built with LTO.
> 
> Though, I'm happy to revisit that behavior in LLVM; we could add
> 
> #define noinline_for_lto __attribute__((__noinline__))
> 
> then use that in the Linux kernel instead.

Our problem is that a boolean attribute with 1 bit information cannot express
whether a neg attribute function can be inlined into a pos attribute function.

Let's agree to disagree. I don't see why a no_profile_instrument_function
function suppress inlining into a function without the attribute. For the use
cases where users want to suppress inlining, they can add noinline. What I
worry about is that now GCC has an attitude and if the LLVM side doesn't follow
it is like diverging. However, the GCC patch is still in review. I think a
similar topic may need to be raided on llvm-dev side as I feel this is the tip
of the iceberg - more attributes can be similarly leveraged. So, how about a
llvm-dev discussion?

[Bug gcov-profile/80223] RFE: Exclude functions from profile instrumentation

[elver at google dot com]

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

--- Comment #19 from Marco Elver <elver at google dot com> ---
(In reply to Fangrui Song from comment #18)
[...] 
> Our problem is that a boolean attribute with 1 bit information cannot
> express whether a neg attribute function can be inlined into a pos attribute
> function.
> 
> Let's agree to disagree. I don't see why a no_profile_instrument_function
> function suppress inlining into a function without the attribute. For the
> use cases where users want to suppress inlining, they can add noinline. What
> I worry about is that now GCC has an attitude and if the LLVM side doesn't
> follow it is like diverging. However, the GCC patch is still in review. I
> think a similar topic may need to be raided on llvm-dev side as I feel this
> is the tip of the iceberg - more attributes can be similarly leveraged. So,
> how about a llvm-dev discussion?

I have mentioned this several times now, but it seems nobody is listening: It's
_not_ about inlining -- the inlining discussion is about a particular
implementation strategy.

It's about the _contract an attribute promises_, which is treating the code in
the function a certain way (e.g. do not instrument). That can be done by
either: a) even if the code is inlined, respect the original attribute for the
inlined code (e.g. do not instrument), or b) just don't inline.

It looks like (b) is easier to do. I probably do not understand how hard (a)
is.

If you break the contract because it's too hard to do (a), then that's your
problem. Just don't break the contract. Because that's how we get
impossible-to-diagnose bugs.

Correctness comes first: if it is impossible for a user to reason about the
behaviour of their code due to unspecified behaviour (viz. breaking the
contract) of an attribute, then the code is doomed to be incorrect. Therefore,
do _not_ implement attributes with either unspecified or ridiculously specified
behaviour.

Ridiculous in this case is saying "this attribute only does what it promises if
you also add noinline". It's ridiculous, because the user will then rightfully
wonder "?!?!? Why doesn't it imply noinline then?!?!?!".

Thanks.

[Bug gcov-profile/80223] RFE: Exclude functions from profile instrumentation

[i at maskray dot me]

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

--- Comment #20 from Fangrui Song <i at maskray dot me> ---
(In reply to Marco Elver from comment #19)

I am ok with "inlining suppression" as an implementation strategy and I agree
that it should be useful. What I objected strongly is "promised inlining
suppression".

For example, if an inlining pass happens after instrumentation, then the
function attribute doesn't necessarily need to suppress inlining. After
instrumentation is done, we can even treat the noprofile attribute as a no-op.

The example applies to the non-LTO case -fsanitize-coverage= . (We don't
actually use the noprofile function attribute for -fsanitize-coverage=, but I
cannot find a better example in LLVM; I think all other noprofile affected
instrumentations happen before the inliner pipeline).

So in a documentation, it can be said that the inlined copy (if any) will not
get instrumentation, but it **should not** say that a noprofile function cannot
be inlined into a function without the attribute.

[Bug gcov-profile/80223] RFE: Exclude functions from profile instrumentation

[i at maskray dot me]

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

--- Comment #21 from Fangrui Song <i at maskray dot me> ---
(In reply to Fangrui Song from comment #20)
> For example, if an inlining pass happens after instrumentation, then the
> function attribute doesn't necessarily need to suppress inlining. After
> instrumentation is done, we can even treat the noprofile attribute as a
> no-op.

Sent too early:)

Amendment: a smart inliner can inline the noprofile callee and then drop
instrumentation code. That will also be an approach which does not break the
"no instrumenting my code" contract. Other approaches can be (probably more
relevant to function specialization/clones): the instrumentation pass can leave
an un-instrumented copy which can be used by a subsequent inliner.

As we can see, all these approaches are much more complex than simply
"suppressing inlining". So I agree that "suppressing inlining" is a good
implementation detail here.

[Bug gcov-profile/80223] RFE: Exclude functions from profile instrumentation

[marxin at gcc dot gnu.org]

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

--- Comment #22 from Martin Liška <marxin at gcc dot gnu.org> ---
> For example, if an inlining pass happens after instrumentation, then the
> function attribute doesn't necessarily need to suppress inlining. After
> instrumentation is done, we can even treat the noprofile attribute as a
> no-op.

In the case of GCC, the instrumentation happens before inlining, but after
early inlining. The patch reviewer noticed that and we would implement it that
way.
So yes, in order to preserve the attribute contract, we have to block inlining
for functions decorated with the attribute.

[Bug gcov-profile/80223] RFE: Exclude functions from profile instrumentation

[ndesaulniers at google dot com]

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

--- Comment #23 from Nick Desaulniers <ndesaulniers at google dot com> ---
(In reply to Fangrui Song from comment #18)
> I
> think a similar topic may need to be raided on llvm-dev side as I feel this
> is the tip of the iceberg - more attributes can be similarly leveraged. So,
> how about a llvm-dev discussion?

Sure.
llvm-dev RFC: https://lists.llvm.org/pipermail/llvm-dev/2021-June/151509.html
llvm patch: https://reviews.llvm.org/D104810

[Bug gcov-profile/80223] RFE: Exclude functions from profile instrumentation

[cvs-commit at gcc dot gnu.org]

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

--- Comment #24 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The master branch has been updated by Martin Liska <marxin@gcc.gnu.org>:

https://gcc.gnu.org/g:aad72d2ea8378e1a56c00d15daa4bdcac8a5ae39

commit r12-3379-gaad72d2ea8378e1a56c00d15daa4bdcac8a5ae39
Author: Martin Liska <mliska@suse.cz>
Date:   Tue Jun 22 10:09:01 2021 +0200

    inline: do not einline when no_profile_instrument_function is different

            PR gcov-profile/80223

    gcc/ChangeLog:

            * ipa-inline.c (can_inline_edge_p): Similarly to sanitizer
            options, do not inline when no_profile_instrument_function
            attributes are different in early inliner. It's fine to inline
            it after PGO instrumentation.

    gcc/testsuite/ChangeLog:

            * gcc.dg/no_profile_instrument_function-attr-2.c: New test.

[Bug gcov-profile/80223] RFE: Exclude functions from profile instrumentation

[marxin at gcc dot gnu.org]

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

Martin Liška <marxin at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
         Resolution|---                         |FIXED
             Status|ASSIGNED                    |RESOLVED

--- Comment #25 from Martin Liška <marxin at gcc dot gnu.org> ---
Should be implemented now.