[AArch64] Add function attribute "patchable-function-entry" to add NOPs at function entry

[MaskRay]

Summary

The Linux kernel uses -fpatchable-function-entry to implement DYNAMIC_FTRACE_WITH_REGS
for arm64 and parisc. GCC 8 implemented
-fpatchable-function-entry, which can be seen as a generalized form of
-mnop-mcount. The N,M form (function entry points before the Mth NOP) is
currently only used by parisc.

This patch adds N,0 support to AArch64 codegen. N is represented as the
function attribute "patchable-function-entry". We will use a different
function attribute for M, if we decide to implement it.

The patch reuses the existing patchable-function pass, and
TargetOpcode::PATCHABLE_FUNCTION_ENTER which is currently used by XRay.

When the integrated assembler is used, __patchable_function_entries will
be created for each text section with the SHF_LINK_ORDER flag to prevent
--gc-sections (https://gcc.gnu.org/bugzilla/show_bug.cgi?id=93197) and
COMDAT (https://gcc.gnu.org/bugzilla/show_bug.cgi?id=93195) issues.

"patchable-function-entry"'s interaction with Branch Target
Identification is still unclear (see
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=92424 for GCC discussions).

[merge_guards_bot]

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

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

clang-format: pass.

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

[ostannard]

How does this interact with the AArch64 "branch-target-enforcement" attribute? For the generated, unpatched code to be correct the function needs to start with a BTI or PAC instruction if it could be indirectly called. Presumably the code doing the patching would also need to be aware of this.

The GCC docs claim that a __patchable_function_entries section will be emitted containing the addresses of every run of NOPs inserted, do you plan on adding that too?

llvm/lib/IR/Verifier.cpp

1862

I think it would be better to use separate attributes for the size and start offset, to avoid needing to do this parsing.

[nickdesaulniers]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=92424 for context.

[MaskRay]

Address comments.

The section __patchable_function_entries needs more thoughts so it is not
implemented. I'll probably use SHF_LINK_ORDER when -ffunction-sections, and add
SHF_WRITE (to prevent text relocations; apparently GCC does not consider these
issues).

[merge_guards_bot]

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

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

clang-format: pass.

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

[MaskRay]

Add __patchable_function_entries section

GCC's implementation has 2 problems: (1) it does not set the SHF_WRITE flag https://gcc.gnu.org/ml/gcc/2020-01/msg00048.html (2) sh_addralign is not pointersize

[merge_guards_bot]

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

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

clang-format: pass.

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

[mrutland]

The N,M form (function entry points before the Mth NOP) is
currently only used by parisc.

As a heads-up, I have rough plans to use the N,M form for the arm64 Linux kernel in the near future. We may need this so that we can allocate trampolines or literal pools next too patch sites for live-patching and/or large test kernels.

This patch adds N,0 support to AArch64 codegen. N is represented as the
function attribute "patchable-function-entry". We will use a different
function attribute for M, if we decide to implement it.

Other than the interaction with BTI, is implementing the full N,M form difficult today?

I'll poke the GCC folk w.r.t. the BTI issues.

[peter.smith]

In D72215#1807110, @MaskRay wrote:

Address comments.

The section __patchable_function_entries needs more thoughts so it is not
implemented. I'll probably use SHF_LINK_ORDER when -ffunction-sections, and add
SHF_WRITE (to prevent text relocations; apparently GCC does not consider these
issues).

It looks like we'll need to co-design this with GCC, binutils and the kernel to get something that works across the toolchains, it looks like this has already started https://www.mail-archive.com/[e-mail]/msg90391.html. I think SHF_LINK_ORDER would work in LLD, but I'm not sure it would have the desired behaviour under garbage collection in ld.bfd. In any case I think that this could be implemented in a follow up patch.

I think N,M support could be potentially added in a later patch as well, given Mark's comment. I think that if the N, M support isn't initially implemented it will be worth giving a "not-supported error message".

[MaskRay]

@reames Is D19046 the LLVM function attribute "patchable-function" and the target opcode PATCHABLE_OP used by any projects? Sanjoy said you may know.

[MaskRay]

In D72215#1807839, @peter.smith wrote:

In D72215#1807110, @MaskRay wrote:

Address comments.

The section __patchable_function_entries needs more thoughts so it is not
implemented. I'll probably use SHF_LINK_ORDER when -ffunction-sections, and add
SHF_WRITE (to prevent text relocations; apparently GCC does not consider these
issues).

It looks like we'll need to co-design this with GCC, binutils and the kernel to get something that works across the toolchains, it looks like this has already started https://www.mail-archive.com/[e-mail]/msg90391.html. I think SHF_LINK_ORDER would work in LLD, but I'm not sure it would have the desired behaviour under garbage collection in ld.bfd. In any case I think that this could be implemented in a follow up patch.

I only got two issues with GCC's __patchable_function_entries now: (1) it does not have the SHF_WRITE flag (2) sh_addralign=1 (instead of pointer-size). This patch is otherwise identical to my observable effects of GCC.

I think N,M support could be potentially added in a later patch as well, given Mark's comment. I think that if the N, M support isn't initially implemented it will be worth giving a "not-supported error message".

Yes. I think clangDriver is a better a place to error "unsupported". See D72222. I also mark -fxray-instrument incompatible there.

[MaskRay]

I think GCC's __patchable_function_entries has severe design flaws. https://gcc.gnu.org/ml/gcc/2020-01/msg00067.html

I have tried a different design in this patch, temporarily naming it __patchable_function_entry.

[merge_guards_bot]

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

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

clang-format: pass.

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

[MaskRay]

Learn from .stack_sizes

[merge_guards_bot]

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

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

clang-format: pass.

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

[peter.smith]

In D72215#1809576, @MaskRay wrote:

I think GCC's __patchable_function_entries has severe design flaws. https://gcc.gnu.org/ml/gcc/2020-01/msg00067.html

I have tried a different design in this patch, temporarily naming it __patchable_function_entry.

FWIW I think that this is definitely an improvement over the original. This should permit the patchable_function_entries to be read-only even when compiled position independent and should deal with dangling entries caused by GC and comdat entries. We may need an option to emit in the old format for compatibility, but I guess we'll need to see how the GCC community reacts.

llvm/test/CodeGen/AArch64/patchable-function-entry.ll

21

It might be worth a run of llc with -ffunction-sections where we'd expect to see f1, unique, 0.

[MaskRay]

Add -function-sections test

[merge_guards_bot]

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

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

clang-format: pass.

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

[MaskRay]

@peter.smith @ostannard PTAL.

Support -no-integrated-as

Unfortunately as of binutils 2.33, GNU as does not support the "o" flag (SHF_LINK_ORDER).

Switch from PC-relative relocation to symbolic relocation

R_MIPS_PC64 does not exist, so I don't dislike -fpatchable-function-entry= that much now.
If GCC/Linux devs want to add -fpatchable-function-entry=2,0,pcrel , we can follow, and it should be straightforward to implement that.

---

I believe Linux's -fpatchable-function-entry=2 Makefile is not clang friendly.
It does not add this option automatically.

But anyhow, applying D72222 and all its depedencies does not break defconfig or allyesconfig.
So this patch series should be safe to merge.

[merge_guards_bot]

Unit tests: pass. 61743 tests passed, 0 failed and 780 were skipped.

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

clang-format: pass.

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

[peter.smith]

LGTM thanks for the update.

[MaskRay]

Update description

[merge_guards_bot]

Unit tests: pass. 61758 tests passed, 0 failed and 780 were skipped.

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

clang-format: pass.

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

[reames]

In D72215#1809133, @MaskRay wrote:

@reames Is D19046 the LLVM function attribute "patchable-function" and the target opcode PATCHABLE_OP used by any projects? Sanjoy said you may know.

Sorry, for the slow response.

Yes, the existing attribute is in place. The difference between the two based on a quick read is that patchable-function expects a destructive patch (i.e. replacing an existing instruction) whereas the new code expects a non-destructive (i.e. replace nops) patch. There could be some more communing in the implementation and naming, but I have no problem with supporting both.

And yes, "patchable-function" is in use. We use it for target redirection when replacing a piece of compiled code with another.

llvm/lib/CodeGen/PatchableFunction.cpp

static bool doesNotGeneratecode(const MachineInstr &MI) {
   case TargetOpcode::GC_LABEL:
   case TargetOpcode::DBG_VALUE:
   case TargetOpcode::DBG_LABEL:
     return true;
   }
 }
 
 bool PatchableFunction::runOnMachineFunction(MachineFunction &MF) {
+  if (MF.getFunction().hasFnAttribute("patchable-function-entry")) {
+    MachineBasicBlock &FirstMBB = *MF.begin();
+    MachineInstr &FirstMI = *FirstMBB.begin();
+    const TargetInstrInfo *TII = MF.getSubtarget().getInstrInfo();
+    BuildMI(FirstMBB, FirstMI, FirstMI.getDebugLoc(),
+            TII->get(TargetOpcode::PATCHABLE_FUNCTION_ENTER));
+    return true;
+  }
+
   if (!MF.getFunction().hasFnAttribute("patchable-function"))
     return false;
 
 #ifndef NDEBUG
   Attribute PatchAttr = MF.getFunction().getFnAttribute("patchable-function");
   StringRef PatchType = PatchAttr.getValueAsString();
   assert(PatchType == "prologue-short-redirect" && "Only possibility today!");
 #endif

llvm/lib/IR/Verifier.cpp

void Verifier::verifyFunctionAttrs(FunctionType *FT, AttributeList Attrs,
   }
 
   if (Attrs.hasFnAttribute("frame-pointer")) {
     StringRef FP = Attrs.getAttribute(AttributeList::FunctionIndex,
                                       "frame-pointer").getValueAsString();
     if (FP != "all" && FP != "non-leaf" && FP != "none")
       CheckFailed("invalid value for 'frame-pointer' attribute: " + FP, V);
   }
+
+  if (Attrs.hasFnAttribute("patchable-function-entry")) {
+    StringRef S0 = Attrs
+                       .getAttribute(AttributeList::FunctionIndex,
+                                     "patchable-function-entry")
+                       .getValueAsString();
+    StringRef S = S0;
+    unsigned N;
+    if (S.consumeInteger(10, N) || !S.consume_front(",") ||
+        S.consumeInteger(10, N) || N)
+      CheckFailed("\"patchable-function-entry\" accepts N,0: " + S0, V);
+  }
 }
 
 void Verifier::verifyFunctionMetadata(
     ArrayRef<std::pair<unsigned, MDNode *>> MDs) {
   for (const auto &Pair : MDs) {
     if (Pair.first == LLVMContext::MD_prof) {
       MDNode *MD = Pair.second;
       Assert(MD->getNumOperands() >= 2,

llvm/lib/Target/AArch64/AArch64AsmPrinter.cpp

void AArch64AsmPrinter::EmitStartOfAsmFile(Module &M) {
   OutStreamer->EmitIntValue(4, 4);     // data size
   OutStreamer->EmitIntValue(Flags, 4); // data
   OutStreamer->EmitIntValue(0, 4);     // pad
 
   OutStreamer->endSection(Nt);
   OutStreamer->SwitchSection(Cur);
 }
 
 void AArch64AsmPrinter::LowerPATCHABLE_FUNCTION_ENTER(const MachineInstr &MI)
 {
+  const Function &F = MF->getFunction();
+  if (F.hasFnAttribute("patchable-function-entry")) {
+    StringRef Entry =
+        F.getFnAttribute("patchable-function-entry").getValueAsString();
+    unsigned Num;
+    if (Entry.consumeInteger(10, Num))
+      return;
+    for (; Num; --Num)
+      EmitToStreamer(*OutStreamer, MCInstBuilder(AArch64::HINT).addImm(0));
+    return;
+  }
+
   EmitSled(MI, SledKind::FUNCTION_ENTER);
 }
 
 void AArch64AsmPrinter::LowerPATCHABLE_FUNCTION_EXIT(const MachineInstr &MI)
 {
   EmitSled(MI, SledKind::FUNCTION_EXIT);
 }
 

llvm/test/CodeGen/AArch64/patchable-function-entry.ll

+; RUN: llc -mtriple=aarch64 %s -o - | FileCheck %s
+
+define i32 @f0() "patchable-function-entry"="0,0" {
+; CHECK-LABEL: f0:
+; CHECK-NOT:   nop
+; CHECK:       mov w0, wzr
+  ret i32 0
+}
+
+define i32 @f1() "patchable-function-entry"="1,0" {
+; CHECK-LABEL: f1:
+; CHECK:       nop
+; CHECK-NEXT:  mov w0, wzr
+  ret i32 0
+}
+
+define void @f3() "patchable-function-entry"="3,0" {
+; CHECK-LABEL:   f3:
+; CHECK-COUNT-3: nop
+; CHECK-NEXT:    sub sp, sp, #16
+  %frame = alloca i8, i32 16
+  ret void
+}

llvm/test/Verifier/invalid-patchable-function-entry.ll

+; RUN: not llvm-as %s -o /dev/null 2>&1 | FileCheck %s
+
+; CHECK: "patchable-function-entry" accepts N,0:
+; CHECK: "patchable-function-entry" accepts N,0: a
+; CHECK: "patchable-function-entry" accepts N,0: -1,0
+; CHECK: "patchable-function-entry" accepts N,0: 3
+; CHECK: "patchable-function-entry" accepts N,0: 3,1
+
+define void @f() "patchable-function-entry" { ret void }
+define void @fa() "patchable-function-entry"="a" { ret void }
+define void @f_1() "patchable-function-entry"="-1,0" { ret void }
+define void @f3() "patchable-function-entry"="3" { ret void }
+define void @f31() "patchable-function-entry"="3,1" { ret void }