Support function attribute patchable_function_entry

[MaskRay]

Summary

This feature is generic. Make it applicable for AArch64 and X86 because
the backend has only implemented NOP insertion for AArch64 and X86.

[MaskRay]

Move a line from D72222 to D72221

[merge_guards_bot]

Unit tests: pass. 61253 tests passed, 0 failed and 736 were skipped.

clang-tidy: fail. Please fix clang-tidy findings.

clang-format: fail. Please format your changes with clang-format by running git-clang-format HEAD^ or applying this patch.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

[MaskRay]

clang-format

[merge_guards_bot]

Unit tests: pass. 61253 tests passed, 0 failed and 736 were skipped.

clang-tidy: fail. Please fix clang-tidy findings.

clang-format: fail. Please format your changes with clang-format by running git-clang-format HEAD^ or applying this patch.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

[merge_guards_bot]

Unit tests: pass. 61253 tests passed, 0 failed and 736 were skipped.

clang-tidy: fail. Please fix clang-tidy findings.

clang-format: fail. Please format your changes with clang-format by running git-clang-format HEAD^ or applying this patch.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

[ostannard]

clang/include/clang/Basic/AttrDocs.td

3995

Grammar nits:
s/attributes/attribute/
over _the_ command line option

clang/lib/CodeGen/CodeGenFunction.cpp

826

I think using two function attributes here would be better, to avoid needing to parse this again later.

[nickdesaulniers]

clang/lib/Sema/SemaDeclAttr.cpp

4923

Why is the target arch also checked in clang/lib/Driver/ToolChains/Clang.cpp in https://reviews.llvm.org/D72222?

[nickdesaulniers]

clang/lib/Sema/SemaDeclAttr.cpp

4923

Ah, https://reviews.llvm.org/D72221 is checking the __attribute__ syntax, https://reviews.llvm.org/D72222 is checking the command line -f syntax.

[nickdesaulniers]

clang/lib/CodeGen/CodeGenFunction.cpp

826

In that case, it would not make sense to have start without a size, and thus should be checked for in the verification.

[aaron.ballman]

clang/include/clang/Basic/Attr.td

686

Should this be inheriting from TargetSpecificAttr as well given that the attribute is only supported on some architectures?

clang/lib/Sema/SemaDeclAttr.cpp

4921

If you make the attribute target-specific, this code can be removed.

4928

Should not be a need to check this -- it will always have at least one argument because the common attribute handling code will diagnose otherwise given the subject list.

clang/test/CodeGen/patchable-function-entry.c

21

Can you also add a test verifying that this attribute works with redeclarations as well? Something like:

[[gnu::patchable_function_entry(12, 4)]] void func(void);

void func(void) { // Definition should have the noop sled
}

[MaskRay]

Address review comments

clang/include/clang/Basic/Attr.td

686

Thanks for the suggestion. Didn't know TargetSpecificAttr.

clang/include/clang/Basic/AttrDocs.td

3995

Thanks. (I am a non-native English speaker and any advice will be highly appreciated.)

(So, I believe GCC's documentation misses _the_)

clang/lib/CodeGen/CodeGenFunction.cpp

826

Agreed. Updated D72215 (semantics of the IR function attribute "patchable-function-entry") as well.

clang/lib/Sema/SemaDeclAttr.cpp

4923

Yes. D72221 is for the Clang function attribute while D72222 is for -fpatchable-function-entry=.

clang/test/CodeGen/patchable-function-entry.c

21

__attribute__((patchable_function_entry(2,0))) void f20decl();
__attribute__((noinline)) void f20decl() {}

Checked this case. I'll delete __attribute__((noinline)) since it does not seem to be clear what it intends to do.

[merge_guards_bot]

Unit tests: fail. 61271 tests passed, 1 failed and 736 were skipped.

failed: Clang.CodeGen/patchable-function-entry.c

clang-tidy: fail. Please fix clang-tidy findings.

clang-format: fail. Please format your changes with clang-format by running git-clang-format HEAD^ or applying this patch.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

[nickdesaulniers]

Thanks for all of the work that went into this. Looks like review comments have all been addressed.

[aaron.ballman]

LGTM!

clang/include/clang/Basic/Attr.td

def XRayLogArgs : InheritableAttr {
   // representation, so ParamIdxArgument is inappropriate.  It is never used
   // at the AST level of representation, so it never needs to be adjusted not
   // to include any C++ implicit this parameter.  Thus, we just store it and
   // use it as an unsigned that never needs adjustment.
   let Args = [UnsignedArgument<"ArgumentCount">];
   let Documentation = [XRayDocs];
 }
 
+def PatchableFunctionEntry
+    : InheritableAttr,
+      TargetSpecificAttr<TargetArch<["aarch64", "x86", "x86_64"]>> {
+  let Spellings = [GCC<"patchable_function_entry">];
+  let Subjects = SubjectList<[Function, ObjCMethod]>;
+  let Args = [UnsignedArgument<"Count">, DefaultIntArgument<"Offset", 0>];
+  let Documentation = [PatchableFunctionEntryDocs];
+}
+
 def TLSModel : InheritableAttr {
   let Spellings = [GCC<"tls_model">];
   let Subjects = SubjectList<[TLSVar], ErrorDiag>;
   let Args = [StringArgument<"Model">];
   let Documentation = [TLSModelDocs];
 }
 
 def AnalyzerNoReturn : InheritableAttr {

clang/include/clang/Basic/AttrDocs.td

 Conversely, ``__attribute__((xray_never_instrument))`` or ``[[clang::xray_never_instrument]]`` will inhibit the insertion of these instrumentation points.
 
 If a function has neither of these attributes, they become subject to the XRay heuristics used to determine whether a function should be instrumented or otherwise.
 
 ``__attribute__((xray_log_args(N)))`` or ``[[clang::xray_log_args(N)]]`` is used to preserve N function arguments for the logging function.  Currently, only N==1 is supported.
   }];
 }
 
+def PatchableFunctionEntryDocs : Documentation {
+  let Category = DocCatFunction;
+  let Content = [{
+``__attribute__((patchable_function_entry(N,M)))`` is used to generate M NOPs
+before the function entry and N-M NOPs after the function entry. This attribute
+takes precedence over the command line option ``-fpatchable-function-entry=N,M``.
+``M`` defaults to 0 if omitted.
+
+Currently, only M=0 is supported.
+}];
+}
+
 def TransparentUnionDocs : Documentation {
   let Category = DocCatDecl;
   let Content = [{
 This attribute can be applied to a union to change the behaviour of calls to
 functions that have an argument with a transparent union type. The compiler
 behaviour is changed in the following manner:
 
 - A value whose type is any member of the transparent union can be passed as an

clang/lib/CodeGen/CodeGenFunction.cpp

#undef SANITIZER
   // are not aware of how to move the extra UBSan instructions across the split
   // coroutine boundaries.
   if (D && SanOpts.has(SanitizerKind::Null))
     if (const auto *FD = dyn_cast<FunctionDecl>(D))
       if (FD->getBody() &&
           FD->getBody()->getStmtClass() == Stmt::CoroutineBodyStmtClass)
         SanOpts.Mask &= ~SanitizerKind::Null;
 
-  // Apply xray attributes to the function (as a string, for now)
   if (D) {
+    // Apply xray attributes to the function (as a string, for now)
     if (const auto *XRayAttr = D->getAttr<XRayInstrumentAttr>()) {
       if (CGM.getCodeGenOpts().XRayInstrumentationBundle.has(
               XRayInstrKind::Function)) {
         if (XRayAttr->alwaysXRayInstrument() && ShouldXRayInstrumentFunction())
           Fn->addFnAttr("function-instrument", "xray-always");
         if (XRayAttr->neverXRayInstrument())
           Fn->addFnAttr("function-instrument", "xray-never");
         if (const auto *LogArgs = D->getAttr<XRayLogArgsAttr>())
           if (ShouldXRayInstrumentFunction())
             Fn->addFnAttr("xray-log-args",
                           llvm::utostr(LogArgs->getArgumentCount()));
       }
     } else {
       if (ShouldXRayInstrumentFunction() && !CGM.imbueXRayAttrs(Fn, Loc))
         Fn->addFnAttr(
             "xray-instruction-threshold",
             llvm::itostr(CGM.getCodeGenOpts().XRayInstructionThreshold));
     }
+
+    if (const auto *Attr = D->getAttr<PatchableFunctionEntryAttr>()) {
+      // Attr->getStart is currently ignored.
+      Fn->addFnAttr("patchable-function-entry",
+                    std::to_string(Attr->getCount()));
+    }
   }
 
   // Add no-jump-tables value.
   Fn->addFnAttr("no-jump-tables",
                 llvm::toStringRef(CGM.getCodeGenOpts().NoUseJumpTables));
 
   // Add no-inline-line-tables value.
   if (CGM.getCodeGenOpts().NoInlineLineTables)

clang/lib/Sema/SemaDeclAttr.cpp

if (!checkFunctionOrMethodParameterIndex(S, D, AL, 1, AL.getArgAsExpr(0),
                                            true /* CanIndexImplicitThis */))
     return;
 
   // ArgCount isn't a parameter index [0;n), it's a count [1;n]
   D->addAttr(::new (S.Context)
                  XRayLogArgsAttr(S.Context, AL, ArgCount.getSourceIndex()));
 }
 
+static void handlePatchableFunctionEntryAttr(Sema &S, Decl *D,
+                                             const ParsedAttr &AL) {
+  uint32_t Count = 0, Offset = 0;
+  if (!checkUInt32Argument(S, AL, AL.getArgAsExpr(0), Count, 0, true))
+    return;
+  if (AL.getNumArgs() == 2) {
+    Expr *Arg = AL.getArgAsExpr(1);
+    if (!checkUInt32Argument(S, AL, Arg, Offset, 1, true))
+      return;
+    if (Offset) {
+      S.Diag(getAttrLoc(AL), diag::err_attribute_argument_out_of_range)
+          << &AL << 0 << 0 << Arg->getBeginLoc();
+      return;
+    }
+  }
+  D->addAttr(::new (S.Context)
+                 PatchableFunctionEntryAttr(S.Context, AL, Count, Offset));
+}
+
 static bool ArmMveAliasValid(unsigned BuiltinID, StringRef AliasName) {
   if (AliasName.startswith("__arm_"))
     AliasName = AliasName.substr(6);
   switch (BuiltinID) {
 #include "clang/Basic/arm_mve_builtin_aliases.inc"
   default:
     return false;
   }
static void ProcessDeclAttribute(Sema &S, Scope *scope, Decl *D,
   // XRay attributes.
   case ParsedAttr::AT_XRayInstrument:
     handleSimpleAttribute<XRayInstrumentAttr>(S, D, AL);
     break;
   case ParsedAttr::AT_XRayLogArgs:
     handleXRayLogArgsAttr(S, D, AL);
     break;
 
+  case ParsedAttr::AT_PatchableFunctionEntry:
+    handlePatchableFunctionEntryAttr(S, D, AL);
+    break;
+
   // Move semantics attribute.
   case ParsedAttr::AT_Reinitializes:
     handleSimpleAttribute<ReinitializesAttr>(S, D, AL);
     break;
 
   case ParsedAttr::AT_AlwaysDestroy:
   case ParsedAttr::AT_NoDestroy:
     handleDestroyAttr(S, D, AL);

clang/test/CodeGen/patchable-function-entry.c

+// RUN: %clang_cc1 -triple aarch64 -emit-llvm %s -o - | FileCheck %s
+
+// CHECK: define void @f0() #0
+__attribute__((patchable_function_entry(0))) void f0() {}
+
+// CHECK: define void @f00() #0
+__attribute__((patchable_function_entry(0, 0))) void f00() {}
+
+// CHECK: define void @f2() #1
+__attribute__((patchable_function_entry(2))) void f2() {}
+
+// CHECK: define void @f20() #1
+__attribute__((patchable_function_entry(2, 0))) void f20() {}
+
+// CHECK: define void @f20decl() #1
+__attribute__((patchable_function_entry(2, 0))) void f20decl();
+void f20decl() {}
+
+/// M in patchable_function_entry(N,M) is currently ignored.
+// CHECK: attributes #0 = { {{.*}} "patchable-function-entry"="0"
+// CHECK: attributes #1 = { {{.*}} "patchable-function-entry"="2"

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

 // CHECK-NEXT: ObjCSubclassingRestricted (SubjectMatchRule_objc_interface)
 // CHECK-NEXT: OpenCLIntelReqdSubGroupSize (SubjectMatchRule_function)
 // CHECK-NEXT: OpenCLNoSVM (SubjectMatchRule_variable)
 // CHECK-NEXT: OptimizeNone (SubjectMatchRule_function, SubjectMatchRule_objc_method)
 // CHECK-NEXT: Overloadable (SubjectMatchRule_function)
 // CHECK-NEXT: Owner (SubjectMatchRule_record_not_is_union)
 // CHECK-NEXT: ParamTypestate (SubjectMatchRule_variable_is_parameter)
 // CHECK-NEXT: PassObjectSize (SubjectMatchRule_variable_is_parameter)
+// CHECK-NEXT: PatchableFunctionEntry (SubjectMatchRule_function, SubjectMatchRule_objc_method)
 // CHECK-NEXT: Pointer (SubjectMatchRule_record_not_is_union)
 // CHECK-NEXT: ReleaseHandle (SubjectMatchRule_variable_is_parameter)
 // CHECK-NEXT: RenderScriptKernel (SubjectMatchRule_function)
 // CHECK-NEXT: ReqdWorkGroupSize (SubjectMatchRule_function)
 // CHECK-NEXT: Restrict (SubjectMatchRule_function)
 // CHECK-NEXT: ReturnTypestate (SubjectMatchRule_function, SubjectMatchRule_variable_is_parameter)
 // CHECK-NEXT: ReturnsNonNull (SubjectMatchRule_objc_method, SubjectMatchRule_function)
 // CHECK-NEXT: ReturnsTwice (SubjectMatchRule_function)

clang/test/Sema/patchable-function-entry-attr.c

+// RUN: %clang_cc1 -triple aarch64 -fsyntax-only -verify %s
+
+// expected-error@+1 {{'patchable_function_entry' attribute takes at least 1 argument}}
+__attribute__((patchable_function_entry)) void f();
+
+// expected-error@+1 {{'patchable_function_entry' attribute takes no more than 2 arguments}}
+__attribute__((patchable_function_entry(0, 0, 0))) void f();
+
+// expected-error@+1 {{'patchable_function_entry' attribute requires a non-negative integral compile time constant expression}}
+__attribute__((patchable_function_entry(-1))) void f();
+
+int i;
+// expected-error@+1 {{'patchable_function_entry' attribute requires parameter 0 to be an integer constant}}
+__attribute__((patchable_function_entry(i))) void f();
+
+// expected-error@+1 {{'patchable_function_entry' attribute requires integer constant between 0 and 0 inclusive}}
+__attribute__((patchable_function_entry(1, 1))) void f();

clang/test/Sema/patchable-function-entry-attr.cpp

+// RUN: %clang_cc1 -triple aarch64 -fsyntax-only -verify=silence %s
+// RUN: %clang_cc1 -triple i386 -fsyntax-only -verify=silence %s
+// RUN: %clang_cc1 -triple x86_64 -fsyntax-only -verify=silence %s
+// RUN: %clang_cc1 -triple ppc64le -fsyntax-only -verify %s
+
+// silence-no-diagnostics
+
+// expected-warning@+1 {{unknown attribute 'patchable_function_entry' ignored}}
+[[gnu::patchable_function_entry(0)]] void f();