[Clang][Codegen] Add GNU function attribute 'no_profile' and lower it to noprofile

[nickdesaulniers]

Summary

noprofile IR attribute already exists to prevent profiling with PGO;
emit that when a function uses the newly added no_profile function
attribute.

The Linux kernel would like to avoid compiler generated code in
functions annotated with such attribute. We already respect this for
libcalls to fentry() and mcount().

Alternate implementation to: https://reviews.llvm.org/D104253
Link: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=80223
Link: https://lore.kernel.org/lkml/[e-mail]/

[MaskRay]

Should no_profile specify the inlining behavior? Though no_sanitize_* don't specify that, either.

As I commented on https://gcc.gnu.org/bugzilla/show_bug.cgi?id=80223 , the Linux kernel specifies noinline so the exact inlining behavior doesn't matter.

#define noinstr                                                         \
        noinline notrace __attribute((__section__(".noinstr.text")))    \
        __no_kcsan __no_sanitize_address

clang/test/CodeGen/fprofile.c

19

// CHECK:     attributes [[ATTR2]] = {
// CHECK-NOT: noprofile
// CHECK:     }

otherwise the test may become stale if the order of ATTR and ATTR2 shuffles.

[void]

clang/include/clang/Basic/Attr.td

1974

Is this an attribute that exists in GCC? I couldn't find it...

clang/include/clang/Basic/AttrDocs.td

2565

s/function to declaration/function declaration/

[melver]

In D104475#2825297, @MaskRay wrote:

Should no_profile specify the inlining behavior? Though no_sanitize_* don't specify that, either.

I think it's somehow implicit, but I can't quite say how. There are some tests that check this, e.g.
compiler-rt/test/asan/TestCases/inline.cpp

As I commented on https://gcc.gnu.org/bugzilla/show_bug.cgi?id=80223 , the Linux kernel specifies noinline so the exact inlining behavior doesn't matter.

#define noinstr                                                         \
        noinline notrace __attribute((__section__(".noinstr.text")))    \
        __no_kcsan __no_sanitize_address

noinstr does add noinline, but other uses of the attribute alone might not. But in the end inlining is a red herring, it just seems to be the easiest way to enforce the promised contract. See https://lore.kernel.org/lkml/[e-mail]/

In particular:

But the contract of
"no_sanitize" is "that a particular instrumentation or set of
instrumentations should not be applied". That contract is broken if a
function is instrumented, however that may happen.

Also always_inline needs checking, because it's a little special:

• An always_inline function inlined into a no_sanitize function is not instrumented
• An __always_inline function inlined into an instrumented function is instrumented

This was also previously discussed here: https://lore.kernel.org/lkml/CANpmjNMTsY_8241bS7=[e-mail]/

[nickdesaulniers]

• fix td files and test CHECKs

[MaskRay]

In D104475#2825666, @melver wrote:

In D104475#2825297, @MaskRay wrote:

Should no_profile specify the inlining behavior? Though no_sanitize_* don't specify that, either.

I think it's somehow implicit, but I can't quite say how. There are some tests that check this, e.g.
compiler-rt/test/asan/TestCases/inline.cpp
[...]
noinstr does add noinline, but other uses of the attribute alone might not. But in the end inlining is a red herring, it just seems to be the easiest way to enforce the promised contract. See https://lore.kernel.org/lkml/[e-mail]/
[...]

Thanks for the comments. Recently there is a discussion about LLVM IR function attribute nointeropt (similar to a debugging-only GCC function attribute noipa).
People tend to think attributes should be orthogonal. I think not suppressing inlining for the IR attribute no_profile is nice.
Ideally the GNU function attribute (C/C++) does not diverge much from the IR semantics.
So if we don't want to offer guarantee for IR/C/C++ attributes, we can document that users may need to additionally specify __attribute__((noinline)) to suppress inlining.

[melver]

In D104475#2825711, @MaskRay wrote:

In D104475#2825666, @melver wrote:

In D104475#2825297, @MaskRay wrote:

Should no_profile specify the inlining behavior? Though no_sanitize_* don't specify that, either.

I think it's somehow implicit, but I can't quite say how. There are some tests that check this, e.g.
compiler-rt/test/asan/TestCases/inline.cpp
[...]
noinstr does add noinline, but other uses of the attribute alone might not. But in the end inlining is a red herring, it just seems to be the easiest way to enforce the promised contract. See https://lore.kernel.org/lkml/[e-mail]/
[...]

Thanks for the comments. Recently there is a discussion about LLVM IR function attribute nointeropt (similar to a debugging-only GCC function attribute noipa).
People tend to think attributes should be orthogonal. I think not suppressing inlining for the IR attribute no_profile is nice.
Ideally the GNU function attribute (C/C++) does not diverge much from the IR semantics.
So if we don't want to offer guarantee for IR/C/C++ attributes, we can document that users may need to additionally specify __attribute__((noinline)) to suppress inlining.

As long as it remains practical. I don't want us ending up with a mess of an attribute like we once had for KASAN: __no_kasan_or_inline -- because static inline __no_sanitize_address foo(void) { ... } did the wrong thing, because a) attribute was incompatible with 'inline', and b) caused instrumentation if it was inlined (AFAIK that behaviour is fixed and was a GCC-only bug).

Essentially, as long as no_foo means that code in function where no_foo is specified is never instrumented by foo, then we're good. If that code is inlined or not doesn't matter, as long as the contract is honored. If that's not the case we have a problem because we need ugly workarounds for what are essentially compiler bugs (contract not honored).

[nickdesaulniers]

In D104475#2825711, @MaskRay wrote:

So if we don't want to offer guarantee for IR/C/C++ attributes, we can document that users may need to additionally specify __attribute__((noinline)) to suppress inlining.

I don't generally like that approach:

1. it's not easy for developers to validate their call call chains to ensure that a caller with a restrictive function attribute doesn't have unrestricted callers inlined into the restricted caller.
2. that behavior opposes the principle of least surprise.
3. We don't have a statement attribute for call sites to say "please don't inline this call" which would be fine grain. noinline is a course-grain hammer; what if we want to inline a callee into most callers but not this one caller that has such a restricted function attribute?

See also D91816 where I took the conservative approach for no_stack_protector and simply chose not to perform such inline substitution on caller and callee function attribute mismatch. I find this behavior to be the least surprising, and the developer is provided with introspection as to why the compile did not perform such inlining via -Rpass=inline flag.

[melver]

In D104475#2825772, @nickdesaulniers wrote:

In D104475#2825711, @MaskRay wrote:

So if we don't want to offer guarantee for IR/C/C++ attributes, we can document that users may need to additionally specify __attribute__((noinline)) to suppress inlining.

I don't generally like that approach:

1. it's not easy for developers to validate their call call chains to ensure that a caller with a restrictive function attribute doesn't have unrestricted callers inlined into the restricted caller.
2. that behavior opposes the principle of least surprise.
3. We don't have a statement attribute for call sites to say "please don't inline this call" which would be fine grain. noinline is a course-grain hammer; what if we want to inline a callee into most callers but not this one caller that has such a restricted function attribute?

See also D91816 where I took the conservative approach for no_stack_protector and simply chose not to perform such inline substitution on caller and callee function attribute mismatch. I find this behavior to be the least surprising, and the developer is provided with introspection as to why the compile did not perform such inlining via -Rpass=inline flag.

Agree. I think we just raced to say the same thing. ;-)

[MaskRay]

In D104475#2825772, @nickdesaulniers wrote:

In D104475#2825711, @MaskRay wrote:

So if we don't want to offer guarantee for IR/C/C++ attributes, we can document that users may need to additionally specify __attribute__((noinline)) to suppress inlining.

I don't generally like that approach:

If a guarantee would cause inferior optimization or we cannot think of all the complication/fallout initially, I'd prefer that we keep the initial semantics narrow.
If we cannot make a promise, don't provide a promise.
No-suppressing inlining makes the attribute freely composable with other attributes (https://lists.llvm.org/pipermail/llvm-dev/2021-April/150062.html )

1. it's not easy for developers to validate their call call chains to ensure that a caller with a restrictive function attribute doesn't have unrestricted callers inlined into the restricted caller.
2. that behavior opposes the principle of least surprise.

Disagree. If the user wants a function not to be inlined, let them add __attribute__((noinline)).

3. We don't have a statement attribute for call sites to say "please don't inline this call" which would be fine grain. noinline is a course-grain hammer; what if we want to inline a callee into most callers but not this one caller that has such a restricted function attribute?

See also D91816 where I took the conservative approach for no_stack_protector and simply chose not to perform such inline substitution on caller and callee function attribute mismatch. I find this behavior to be the least surprising, and the developer is provided with introspection as to why the compile did not perform such inlining via -Rpass=inline flag.

I think D91816 was an unfortunate case. That would require the inliner to understand every possible attribute. That was the practical approach back then because it is not easy for the kernel to cope.
For new things we should strive for making the kernel do the right thing.

For example, if a kernel function can sometimes be inlinable, sometimes not: add a alias and add __attribute__((noinline)) to that alias.

[nickdesaulniers]

In D104475#2825795, @MaskRay wrote:

In D104475#2825772, @nickdesaulniers wrote:

In D104475#2825711, @MaskRay wrote:

So if we don't want to offer guarantee for IR/C/C++ attributes, we can document that users may need to additionally specify __attribute__((noinline)) to suppress inlining.

I don't generally like that approach:

If a guarantee would cause inferior optimization or we cannot think of all the complication/fallout initially, I'd prefer that we keep the initial semantics narrow.

The guarantee is more important than the optimization.

If we cannot make a promise, don't provide a promise.
No-suppressing inlining makes the attribute freely composable with other attributes (https://lists.llvm.org/pipermail/llvm-dev/2021-April/150062.html )

IIUC, isn't @jdoerfert arguing that optnone and noinline being composable is _bad_?

1. it's not easy for developers to validate their call call chains to ensure that a caller with a restrictive function attribute doesn't have unrestricted callers inlined into the restricted caller.
2. that behavior opposes the principle of least surprise.

Disagree. If the user wants a function not to be inlined, let them add __attribute__((noinline)).

3. We don't have a statement attribute for call sites to say "please don't inline this call" which would be fine grain. noinline is a course-grain hammer; what if we want to inline a callee into most callers but not this one caller that has such a restricted function attribute?

See also D91816 where I took the conservative approach for no_stack_protector and simply chose not to perform such inline substitution on caller and callee function attribute mismatch. I find this behavior to be the least surprising, and the developer is provided with introspection as to why the compile did not perform such inlining via -Rpass=inline flag.

I think D91816 was an unfortunate case. That would require the inliner to understand every possible attribute.

Hyperbole. Only a few such function attributes would need such restrictions.

That was the practical approach back then because it is not easy for the kernel to cope.
For new things we should strive for making the kernel do the right thing.

For example, if a kernel function can sometimes be inlinable, sometimes not: add a alias and add __attribute__((noinline)) to that alias.

Interesting idea, but noinline is straight up broken in LLVM: https://godbolt.org/z/MoE1a7c3v. The Linux kernel relies on noinline meaning don't perform inline substitution. That violates the principal of least surprise. When a developer uses noinline, no_stack_protector, or no_profile, they mean DO NOT inline, insert a stack protector, or insert profiling/coverage instrumentation EVER.

Also debugging to find out that inlining was the cause of such violated expectations sucks.

[aaron.ballman]

This is missing all of the usual semantic tests (attribute accepts no args, applies only to functions, etc).

clang/include/clang/Basic/Attr.td

1975

Should this also be supported on ObjC methods?

clang/include/clang/Basic/AttrDocs.td

2566

[nickdesaulniers]

• add hyphen to docs, add boilerplate test to check Subjects.

clang/include/clang/Basic/Attr.td

1975

Wouldn't that be the case for many surrounding attributes defined near by? I don't see why not, but I also know literally nothing about ObjC. Would you like me to extend this attribute to ObjC methods?

[aaron.ballman]

LGTM!

clang/include/clang/Basic/Attr.td

1975

It would be, and I think many of them are in the same state for the same reason (not knowing about ObjC).

I don't insist on the change. I ask whenever I see Function in the subject list because it's pretty rare that ObjC methods work differently than a function call in ways that matter for attributes. We can always add the functionality when a user requests it, later.

[MaskRay]

In D104475#2825841, @nickdesaulniers wrote:

In D104475#2825795, @MaskRay wrote:

In D104475#2825772, @nickdesaulniers wrote:

In D104475#2825711, @MaskRay wrote:

So if we don't want to offer guarantee for IR/C/C++ attributes, we can document that users may need to additionally specify __attribute__((noinline)) to suppress inlining.

I don't generally like that approach:

If a guarantee would cause inferior optimization or we cannot think of all the complication/fallout initially, I'd prefer that we keep the initial semantics narrow.

The guarantee is more important than the optimization.

If we cannot make a promise, don't provide a promise.
No-suppressing inlining makes the attribute freely composable with other attributes (https://lists.llvm.org/pipermail/llvm-dev/2021-April/150062.html )

IIUC, isn't @jdoerfert arguing that optnone and noinline being composable is _bad_?

No, the opposite. @jdoerfert is arguing that attributes should be composable.
I agree and I think the optnone design has some orthogonality problems.

1. it's not easy for developers to validate their call call chains to ensure that a caller with a restrictive function attribute doesn't have unrestricted callers inlined into the restricted caller.
2. that behavior opposes the principle of least surprise.

Disagree. If the user wants a function not to be inlined, let them add __attribute__((noinline)).

3. We don't have a statement attribute for call sites to say "please don't inline this call" which would be fine grain. noinline is a course-grain hammer; what if we want to inline a callee into most callers but not this one caller that has such a restricted function attribute?

See also D91816 where I took the conservative approach for no_stack_protector and simply chose not to perform such inline substitution on caller and callee function attribute mismatch. I find this behavior to be the least surprising, and the developer is provided with introspection as to why the compile did not perform such inlining via -Rpass=inline flag.

I think D91816 was an unfortunate case. That would require the inliner to understand every possible attribute.

Hyperbole. Only a few such function attributes would need such restrictions.

That was the practical approach back then because it is not easy for the kernel to cope.
For new things we should strive for making the kernel do the right thing.

For example, if a kernel function can sometimes be inlinable, sometimes not: add a alias and add __attribute__((noinline)) to that alias.

Interesting idea, but noinline is straight up broken in LLVM: https://godbolt.org/z/MoE1a7c3v. The Linux kernel relies on noinline meaning don't perform inline substitution. That violates the principal of least surprise. When a developer uses noinline, no_stack_protector, or no_profile, they mean DO NOT inline, insert a stack protector, or insert profiling/coverage instrumentation EVER.

Also debugging to find out that inlining was the cause of such violated expectations sucks.

It is not broken. That is constant propagation in early-cse. A function call can be optimized out if it has no side effect and its value can be determined. The behavior may look like inlining. That is the nointeropt IR attribute proposal intends to address.
I guess for most(all?) non-trivial noinline usage in the Linux kernel, we can assume noinline is fine.

[MaskRay]

LG.

@marxin FYI the GNU-style function attribute no_profile

[MaskRay]

clang/test/CodeGen/fprofile.c

2

no_profile may be a better test name.

[nickdesaulniers]

• rename test, add __has_attribute unit test

[phosek]

LGTM I'm happy to see that the noprofile LLVM attribute is useful in other contexts.

[nickdesaulniers]

rG817218336aa3e3c0ca422ae00f8d8ca41b8fbd6d follow up based on reports of test failure on mac.

[nickdesaulniers]

It looks like GCC has had no_profile_instrument_function since GCC 7.1 for this purpose.

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

Follow up: https://reviews.llvm.org/D104658.

clang/include/clang/Basic/Attr.td

 
 def NoInstrumentFunction : InheritableAttr {
   let Spellings = [GCC<"no_instrument_function">];
   let Subjects = SubjectList<[Function]>;
   let Documentation = [Undocumented];
   let SimpleHandler = 1;
 }
 
+def NoProfileFunction : InheritableAttr {
+  let Spellings = [GCC<"no_profile">];
+  let Subjects = SubjectList<[Function]>;
+  let Documentation = [NoProfileDocs];
+  let SimpleHandler = 1;
+}
+
 def NotTailCalled : InheritableAttr {
   let Spellings = [Clang<"not_tail_called">];
   let Subjects = SubjectList<[Function]>;
   let Documentation = [NotTailCalledDocs];
   let SimpleHandler = 1;
 }
 def : MutualExclusions<[AlwaysInline, NotTailCalled]>;
 

clang/include/clang/Basic/AttrDocs.td

 Each ``class`` that uses any of the typestate annotations must first be marked
 using the ``consumable`` attribute. Failure to do so will result in a warning.
 
 This attribute accepts a single parameter that must be one of the following:
 ``unknown``, ``consumed``, or ``unconsumed``.
   }];
 }
 
+def NoProfileDocs : Documentation {
+  let Category = DocCatFunction;
+  let Content = [{
+Use the ``no_profile`` attribute on a function to declaration to denote that
+the compiler should not instrument the function with profile related
+instrumentation, such as via the
+``-fprofile-generate`` / ``-fprofile-instr-generate`` /
+``-fcs-profile-generate`` / ``-fprofile-arcs`` flags.
+}];
+}
+
 def NoSanitizeDocs : Documentation {
   let Category = DocCatFunction;
   let Content = [{
 Use the ``no_sanitize`` attribute on a function or a global variable
 declaration to specify that a particular instrumentation or set of
 instrumentations should not be applied.
 
 The attribute takes a list of string literals with the following accepted

clang/lib/CodeGen/CodeGenFunction.cpp

if (CGM.getCodeGenOpts().ProfileSampleAccurate)
     Fn->addFnAttr("profile-sample-accurate");
 
   if (!CGM.getCodeGenOpts().SampleProfileFile.empty())
     Fn->addFnAttr("use-sample-profile");
 
   if (D && D->hasAttr<CFICanonicalJumpTableAttr>())
     Fn->addFnAttr("cfi-canonical-jump-table");
 
+  if (D && D->hasAttr<NoProfileFunctionAttr>())
+    Fn->addFnAttr(llvm::Attribute::NoProfile);
+
   if (getLangOpts().OpenCL) {
     // Add metadata for a kernel function.
     if (const FunctionDecl *FD = dyn_cast_or_null<FunctionDecl>(D))
       EmitOpenCLKernelMetadata(FD, Fn);
   }
 
   // If we are checking function types, emit a function type signature as
   // prologue data.

clang/test/CodeGen/fprofile.c

+// RUN: %clang_cc1 -fprofile-instrument=llvm -disable-llvm-passes \
+// RUN:   -emit-llvm -o - %s | FileCheck %s
+// RUN: %clang_cc1 -fprofile-instrument=csllvm -disable-llvm-passes \
+// RUN:   -emit-llvm -o - %s | FileCheck %s
+// RUN: %clang_cc1 -fprofile-instrument=clang -disable-llvm-passes \
+// RUN:   -emit-llvm -o - %s | FileCheck %s
+// RUN: %clang_cc1 -fprofile-arcs -disable-llvm-passes \
+// RUN:   -emit-llvm -o - %s | FileCheck %s
+int g(int);
+
+void __attribute__((no_profile)) no_instr() {
+// CHECK: define {{.*}} void @no_instr() [[ATTR:#[0-9]+]]
+}
+
+void instr(void) {
+// CHECK: define {{.*}} void @instr() [[ATTR2:#[0-9]+]]
+}
+// CHECK: attributes [[ATTR]] = {{.*}} noprofile
+// CHECK-NOT: attributes [[ATTR2]] = {{.*}} noprofile

clang/test/Misc/pragma-attribute-supported-attributes-list.test

 // CHECK-NEXT: NoDestroy (SubjectMatchRule_variable)
 // CHECK-NEXT: NoDuplicate (SubjectMatchRule_function)
 // CHECK-NEXT: NoEscape (SubjectMatchRule_variable_is_parameter)
 // CHECK-NEXT: NoInline (SubjectMatchRule_function)
 // CHECK-NEXT: NoInstrumentFunction (SubjectMatchRule_function)
 // CHECK-NEXT: NoMerge (SubjectMatchRule_function)
 // CHECK-NEXT: NoMicroMips (SubjectMatchRule_function)
 // CHECK-NEXT: NoMips16 (SubjectMatchRule_function)
+// CHECK-NEXT: NoProfileFunction (SubjectMatchRule_function)
 // CHECK-NEXT: NoSanitize (SubjectMatchRule_function, SubjectMatchRule_objc_method, SubjectMatchRule_variable_is_global)
 // CHECK-NEXT: NoSanitizeSpecific (SubjectMatchRule_function, SubjectMatchRule_variable_is_global)
 // CHECK-NEXT: NoSpeculativeLoadHardening (SubjectMatchRule_function, SubjectMatchRule_objc_method)
 // CHECK-NEXT: NoSplitStack (SubjectMatchRule_function)
 // CHECK-NEXT: NoStackProtector (SubjectMatchRule_function)
 // CHECK-NEXT: NoThreadSafetyAnalysis (SubjectMatchRule_function)
 // CHECK-NEXT: NoThrow (SubjectMatchRule_hasType_functionType)
 // CHECK-NEXT: NotTailCalled (SubjectMatchRule_function)