Add function attribute "patchable-function-prefix" to support -fpatchable-function-entry=N,M where M>0

[MaskRay]

Summary

Similar to the function attribute prefix (prefix data),
"patchable-function-prefix" inserts data (M NOPs) before the function
entry label.

-fpatchable-function-entry=2,1 (1 NOP before entry, 1 NOP after entry)
will look like:

  .type	foo,@function
.Ltmp0:               # @foo
  nop
foo:
.Lfunc_begin0:
  # optional `bti c` (AArch64 Branch Target Identification) or
  # `endbr64` (Intel Indirect Branch Tracking)
  nop

  .section  __patchable_function_entries,"awo",@progbits,get,unique,0
  .p2align  3
  .quad .Ltmp0

-fpatchable-function-entry=N,0 + -mbranch-protection=bti/-fcf-protection=branch has two reasonable
placements (https://gcc.gnu.org/ml/gcc-patches/2020-01/msg01185.html):

(a)         (b)

func:       func:
.Ltmp0:     bti c
  bti c     .Ltmp0:
  nop       nop

(a) needs no additional code. If the consensus is to go for (b), we will
need more code in AArch64BranchTargets.cpp / X86IndirectBranchTracking.cpp .

[MaskRay]

-fpatchable-function-entry=N,M (where M>0) + -mbranch-protection=bti is quite clear (https://gcc.gnu.org/ml/gcc-patches/2020-01/msg01185.html).

For -fpatchable-function-entry=N,0 + -mbranch-protection=bti/-fcf-protection=branch, if you have preference for placement (a) or (b), please make a comment:) and/or reply at https://gcc.gnu.org/ml/gcc-patches/2020-01/msg01185.html
If you are not subscribed to the gcc-patches mailing list (like me), you will have to download the raw email file Other format: [Raw text] and reply...

(b) will require separate patches.

[merge_guards_bot]

Unit tests: pass. 62041 tests passed, 0 failed and 783 were skipped.

clang-tidy: unknown.

clang-format: pass.

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

[peter.smith]

In D73070#1830367, @MaskRay wrote:

-fpatchable-function-entry=N,M (where M>0) + -mbranch-protection=bti is quite clear (https://gcc.gnu.org/ml/gcc-patches/2020-01/msg01185.html).

For -fpatchable-function-entry=N,0 + -mbranch-protection=bti/-fcf-protection=branch, if you have preference for placement (a) or (b), please make a comment:) and/or reply at https://gcc.gnu.org/ml/gcc-patches/2020-01/msg01185.html
If you are not subscribed to the gcc-patches mailing list (like me), you will have to download the raw email file Other format: [Raw text] and reply...

(b) will require separate patches.

FWIW it looks like GCC have gone with (b) https://gcc.gnu.org/ml/gcc-patches/2020-01/msg01348.html , FWIW my sentiments tend towards the argument given in https://gcc.gnu.org/ml/gcc-patches/2020-01/msg01322.html for option b.

[nickdesaulniers]

Thanks for the patch!

llvm/lib/IR/Verifier.cpp

1860

Why do we make a copy of the StringRef? (Sorry I didn't notice in code review of the below test case previously). I assume this has something to do with avoiding "consuming" an attribute?

llvm/lib/Target/AArch64/AArch64AsmPrinter.cpp

1301

I think it would be better to use TargetInstrInfo::getNoop() via MF->getSubtarget().getInstrInfo()->getNoop(Noop);. Then you could make this a non-virtual function call in the base AsmPrinter class. In fact, emitting nops is useful and could be a method on the base AsmPrinter, and that could be reused between AsmPrinter::EmitFunctionHeader() and AsmPrinter::EmitFunctionBody().

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

56

Ditto about .Lfunc_end2 placement (see below comment).

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

83

Can you please include where .Lfunc_end5 should be in a CHECK, so that reviewers can verify that .size is set correctly?

84

What is this NOFSECT checking? That there's no entry for f1? Why?

92

TIL: https://llvm.org/docs/LangRef.html#prefix-data

95

Please add a CHECK for prefix: after the nop CHECK.

[MaskRay]

Address review comments.
Fix a bug when "patchable-function-entry" is 0 while "patchable-function-prefix" > 0

llvm/lib/IR/Verifier.cpp

1860

My fault. StringRef::getAsInteger does not consume the prefix in case of a parse error.

Updated.

llvm/lib/Target/AArch64/AArch64AsmPrinter.cpp

1301

TIL getNoop. I was looking for such an operation bug did not find one. This is much more elegant! Added AsmPrinter::emitNops.

Unfortunately we cannot retire LowerPATCHABLE_FUNCTION_ENTER.

AsmPrinter (assembly printer/object file emitter) does the following:

1. Emit data before the function entry
2. Emit the function entry label and the label for __patchable_function_entries
3. Emit the function body (which may have optional bti c or endbr64)

Due to bti c (AArch64) and endbr64 (x86), we cannot make the handling of the function attribute "patchable-function-entry" generic.
(-fpatchable-function-entry=, -mbranch-protection= (AArch64 BTI) and -fcf-protection= (Intel Indirect Branch Tracking) are implemented as passes which can prepend instructions to the function body.)

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

84

Added a comment above.

+;; Without -function-sections, f2 is in the same text section as f1.
+;; They share the __patchable_function_entries section.
+;; With -function-sections, f1 and f2 are in different text sections.
+;; Use separate __patchable_function_entries.

[MaskRay]

Improve tests a bit

[merge_guards_bot]

Unit tests: fail. 62080 tests passed, 1 failed and 784 were skipped.

failed: Clang.Driver/cc-print-options.c

clang-tidy: unknown.

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-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

[merge_guards_bot]

Unit tests: fail. 62080 tests passed, 1 failed and 784 were skipped.

failed: Clang.Driver/cc-print-options.c

clang-tidy: unknown.

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-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

[nickdesaulniers]

llvm/include/llvm/CodeGen/AsmPrinter.h

456

unsigned N

Can we mark this method const?

llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp

2806

guard or assert that N > 0

llvm/lib/Target/AArch64/AArch64AsmPrinter.cpp

1301

Looks like we can still modify AsmPrinter::EmitFunctionBody() and ARMAsmPrinter::EmitSled() to now make use of AsmPrinter::emitNops() in this change.

[MaskRay]

emitNops(int N) -> emitNops(unsigned N);

llvm/include/llvm/CodeGen/AsmPrinter.h

456

We can't. void AsmPrinter::EmitToStreamer(MCStreamer &S, const MCInst &Inst) { is not const.

[merge_guards_bot]

Unit tests: pass. 62081 tests passed, 0 failed and 784 were skipped.

clang-tidy: unknown.

clang-format: pass.

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

[nickdesaulniers]

I don't mind landing this without aarch64 bti interplay matching GCC precisely; I think it can be done in a follow up. I just request we don't try to land this in the clang-10 release unless that is also resolved. Otherwise clang-11.

llvm/lib/Target/AArch64/AArch64AsmPrinter.cpp

1301

bump

[MaskRay]

Use emitNops() to replace two NOP emitting sites.

[MaskRay]

llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp

2806

Should N==0 be allowed? It does no harm.

[merge_guards_bot]

Unit tests: pass. 62111 tests passed, 0 failed and 808 were skipped.

clang-tidy: unknown.

clang-format: pass.

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

[kristof.beyls]

In D73070#1834196, @nickdesaulniers wrote:

I don't mind landing this without aarch64 bti interplay matching GCC precisely; I think it can be done in a follow up. I just request we don't try to land this in the clang-10 release unless that is also resolved. Otherwise clang-11.

I think these "patchable function entry" sequences are effectively binary interface, i.e. ABI, as other binary processing tools that use these patchable function entries will make assumptions about the structure of these sequences.
Therefore, I think it's important that clang and gcc do produce the same sequences, otherwise we're causing de-facto ABI incompatibility.

[MaskRay]

In D73070#1834291, @kristof.beyls wrote:

In D73070#1834196, @nickdesaulniers wrote:

I don't mind landing this without aarch64 bti interplay matching GCC precisely; I think it can be done in a follow up. I just request we don't try to land this in the clang-10 release unless that is also resolved. Otherwise clang-11.

I think these "patchable function entry" sequences are effectively binary interface, i.e. ABI, as other binary processing tools that use these patchable function entries will make assumptions about the structure of these sequences.
Therefore, I think it's important that clang and gcc do produce the same sequences, otherwise we're causing de-facto ABI incompatibility.

Not considering AArch64 BTI, we don't have a compatibility issue. I shall also mention that nobody on their side considers endbr64 yet.

For AArch64 BTI users, due to https://gcc.gnu.org/bugzilla/show_bug.cgi?id=92424 , the Linux kernel may adopt a better configure-time test than a simple if $(cc-option,-fpatchable-function-entry=2). It should detect behavior differences across different versions of GCC and Clang. External users should be aware of these differences. I think we have some implementation leeway here.

Eventually the two compilers should be compatible, but I don't want to say that we should give up and blindly follow their decision;-) I wish people can evaluate the pros and cons not only from implementation complexity and status quo. I also think we should give x86 folks a chance to chime in. GCC 10 is at a very late stage, so they don't want to change the .Lpatch placement.

-fpatchable-function-entry=2,0 is included in the release/10.x branch. I don't want to push the N,M where M>0 changes to release/10.x.

[peter.smith]

-fpatchable-function-entry=2,0 is included in the release/10.x branch. I don't want to push the N,M where M>0 changes to release/10.x.

I've left a comment in D72222 but thought it would be worth drawing attention to it here as D72222 is quite old and easily missed, CI has found errors in 3 linux files for an allmodconfig build on a clang 10. The details are in the comment in D72222, which isn't at fault itself but it is the first that enables -fpatchable-functions to be selected by the kernel make system.

[nickdesaulniers]

In D73070#1836446, @peter.smith wrote:

-fpatchable-function-entry=2,0 is included in the release/10.x branch. I don't want to push the N,M where M>0 changes to release/10.x.

I've left a comment in D72222 but thought it would be worth drawing attention to it here as D72222 is quite old and easily missed, CI has found errors in 3 linux files for an allmodconfig build on a clang 10. The details are in the comment in D72222, which isn't at fault itself but it is the first that enables -fpatchable-functions to be selected by the kernel make system.

linux-next: https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/?h=next-20200123 (ie, kernel staging tree) test for support in arch/arm64/Kconfig via seeing if the compiler recognizes -fpatchable-function-entry=2 (if so, tries to use it).

Failing asserts should be fixed first before this series, then this series should be rebased on top.

In D73070#1834520, @MaskRay wrote:

-fpatchable-function-entry=2,0 is included in the release/10.x branch. I don't want to push the N,M where M>0 changes to release/10.x.

Did it land before the branch, or was it cherry-picked over by @hans ? I worry that that might make feature detection in the Linux kernel for this feature difficult. We (kernel, @mrutland) may need to at least keep in mind that there may be separate checks for cc-option for -fpatchable-function-entry=M vs -fpatchable-function-entry=M,N.

In D73070#1834291, @kristof.beyls wrote:

In D73070#1834196, @nickdesaulniers wrote:

I don't mind landing this without aarch64 bti interplay matching GCC precisely; I think it can be done in a follow up. I just request we don't try to land this in the clang-10 release unless that is also resolved. Otherwise clang-11.

I think these "patchable function entry" sequences are effectively binary interface, i.e. ABI, as other binary processing tools that use these patchable function entries will make assumptions about the structure of these sequences.
Therefore, I think it's important that clang and gcc do produce the same sequences, otherwise we're causing de-facto ABI incompatibility.

I agree 100%; if there's no rush to get this into clang-10, then I'd rather take the series as is and iterate on fixes. Or maybe a final patch on top in this series as a final fixup to patchable function entry and BTI interwork would be satisfactory before landing the whole set? Either way, I consider this patch reviewed, so I'm signing off on it, but I would appreciate it @MaskRay if you did *NOT* land the series without @kristof.beyls ' explicit sign off and response to this suggestion (and with it rebased on fixes for the failed assertions @peter.smith cites above). I don't feel strongly about it though, so I won't be offended if anyone strongly disagrees.

[MaskRay]

In D73070#1836598, @nickdesaulniers wrote:

In D73070#1836446, @peter.smith wrote:

-fpatchable-function-entry=2,0 is included in the release/10.x branch. I don't want to push the N,M where M>0 changes to release/10.x.

I've left a comment in D72222 but thought it would be worth drawing attention to it here as D72222 is quite old and easily missed, CI has found errors in 3 linux files for an allmodconfig build on a clang 10. The details are in the comment in D72222, which isn't at fault itself but it is the first that enables -fpatchable-functions to be selected by the kernel make system.

linux-next: https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/?h=next-20200123 (ie, kernel staging tree) test for support in arch/arm64/Kconfig via seeing if the compiler recognizes -fpatchable-function-entry=2 (if so, tries to use it).

Failing asserts should be fixed first before this series, then this series should be rebased on top.

In D73070#1834520, @MaskRay wrote:

-fpatchable-function-entry=2,0 is included in the release/10.x branch. I don't want to push the N,M where M>0 changes to release/10.x.

Did it land before the branch, or was it cherry-picked over by @hans ? I worry that that might make feature detection in the Linux kernel for this feature difficult. We (kernel, @mrutland) may need to at least keep in mind that there may be separate checks for cc-option for -fpatchable-function-entry=M vs -fpatchable-function-entry=M,N.

In D73070#1834291, @kristof.beyls wrote:

In D73070#1834196, @nickdesaulniers wrote:

I don't mind landing this without aarch64 bti interplay matching GCC precisely; I think it can be done in a follow up. I just request we don't try to land this in the clang-10 release unless that is also resolved. Otherwise clang-11.

I think these "patchable function entry" sequences are effectively binary interface, i.e. ABI, as other binary processing tools that use these patchable function entries will make assumptions about the structure of these sequences.
Therefore, I think it's important that clang and gcc do produce the same sequences, otherwise we're causing de-facto ABI incompatibility.

I agree 100%; if there's no rush to get this into clang-10, then I'd rather take the series as is and iterate on fixes. Or maybe a final patch on top in this series as a final fixup to patchable function entry and BTI interwork would be satisfactory before landing the whole set? Either way, I consider this patch reviewed, so I'm signing off on it, but I would appreciate it @MaskRay if you did *NOT* land the series without @kristof.beyls ' explicit sign off and response to this suggestion (and with it rebased on fixes for the failed assertions @peter.smith cites above). I don't feel strongly about it though, so I won't be offended if anyone strongly disagrees.

The driver change -fpatchable-function-entry=N,0 is in release/10.x . D73070 (this), D73071 and D73072 are not.

I wish we don't make -fpatchable-function-entry= in clang 10.0.0, then there will no no problem at all.

(For this patch series, I don't see how they can make the situation worse. BTI c's interaction with -mbranch-protection=bti is an existing problem in GCC (https://gcc.gnu.org/bugzilla/show_bug.cgi?id=92424)
https://gcc.gnu.org/ml/gcc-patches/2020-01/msg01334.html ) Current release/10.x will do: nop; nop; bti c , which is considered incorrect. I fixed this in rG9a24488cb67a90f889529987275c5e411ce01dda (not in release/10.x)

[hans]

In D73070#1836755, @MaskRay wrote:

In D73070#1836598, @nickdesaulniers wrote:

In D73070#1836446, @peter.smith wrote:

-fpatchable-function-entry=2,0 is included in the release/10.x branch. I don't want to push the N,M where M>0 changes to release/10.x.

I've left a comment in D72222 but thought it would be worth drawing attention to it here as D72222 is quite old and easily missed, CI has found errors in 3 linux files for an allmodconfig build on a clang 10. The details are in the comment in D72222, which isn't at fault itself but it is the first that enables -fpatchable-functions to be selected by the kernel make system.

linux-next: https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/?h=next-20200123 (ie, kernel staging tree) test for support in arch/arm64/Kconfig via seeing if the compiler recognizes -fpatchable-function-entry=2 (if so, tries to use it).

Failing asserts should be fixed first before this series, then this series should be rebased on top.

In D73070#1834520, @MaskRay wrote:

-fpatchable-function-entry=2,0 is included in the release/10.x branch. I don't want to push the N,M where M>0 changes to release/10.x.

Did it land before the branch, or was it cherry-picked over by @hans ? I worry that that might make feature detection in the Linux kernel for this feature difficult. We (kernel, @mrutland) may need to at least keep in mind that there may be separate checks for cc-option for -fpatchable-function-entry=M vs -fpatchable-function-entry=M,N.

In D73070#1834291, @kristof.beyls wrote:

In D73070#1834196, @nickdesaulniers wrote:

I don't mind landing this without aarch64 bti interplay matching GCC precisely; I think it can be done in a follow up. I just request we don't try to land this in the clang-10 release unless that is also resolved. Otherwise clang-11.

I think these "patchable function entry" sequences are effectively binary interface, i.e. ABI, as other binary processing tools that use these patchable function entries will make assumptions about the structure of these sequences.
Therefore, I think it's important that clang and gcc do produce the same sequences, otherwise we're causing de-facto ABI incompatibility.

I agree 100%; if there's no rush to get this into clang-10, then I'd rather take the series as is and iterate on fixes. Or maybe a final patch on top in this series as a final fixup to patchable function entry and BTI interwork would be satisfactory before landing the whole set? Either way, I consider this patch reviewed, so I'm signing off on it, but I would appreciate it @MaskRay if you did *NOT* land the series without @kristof.beyls ' explicit sign off and response to this suggestion (and with it rebased on fixes for the failed assertions @peter.smith cites above). I don't feel strongly about it though, so I won't be offended if anyone strongly disagrees.

The driver change -fpatchable-function-entry=N,0 is in release/10.x . D73070 (this), D73071 and D73072 are not.

I wish we don't make -fpatchable-function-entry= in clang 10.0.0, then there will no no problem at all.

(For this patch series, I don't see how they can make the situation worse. BTI c's interaction with -mbranch-protection=bti is an existing problem in GCC (https://gcc.gnu.org/bugzilla/show_bug.cgi?id=92424)
https://gcc.gnu.org/ml/gcc-patches/2020-01/msg01334.html ) Current release/10.x will do: nop; nop; bti c , which is considered incorrect. I fixed this in rG9a24488cb67a90f889529987275c5e411ce01dda (not in release/10.x)

I haven't followed this discussion, but it sounds like there are changes that should be pushed to 10.x. Can you please summarize which ones?

llvm/include/llvm/CodeGen/AsmPrinter.h

public:
 
   /// Optimization remark emitter.
   MachineOptimizationRemarkEmitter *ORE;
 
   MachineBlockFrequencyInfo *MBFI;
 
   ProfileSummaryInfo *PSI;
 
+  /// The symbol for the entry in __patchable_function_entires.
+  MCSymbol *CurrentPatchableFunctionEntrySym = nullptr;
+
   /// The symbol for the current function. This is recalculated at the beginning
   /// of each call to runOnMachineFunction().
   MCSymbol *CurrentFnSym = nullptr;
 
   /// The symbol for the current function descriptor on AIX. This is created
   /// at the beginning of each call to SetupMachineFunction().
   MCSymbol *CurrentFnDescSym = nullptr;
 
public:
   /// fall-through.
   virtual bool
   isBlockOnlyReachableByFallthrough(const MachineBasicBlock *MBB) const;
 
   /// Targets can override this to customize the output of IMPLICIT_DEF
   /// instructions in verbose mode.
   virtual void emitImplicitDef(const MachineInstr *MI) const;
 
+  /// Emit N NOP instructions.
+  void emitNops(unsigned N);
+
   //===------------------------------------------------------------------===//
   // Symbol Lowering Routines.
   //===------------------------------------------------------------------===//
 
   MCSymbol *createTempSymbol(const Twine &Name) const;
 
   /// Return the MCSymbol for a private symbol with global value name as its
   /// base, with the specified suffix.

llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp

if (MAI->hasSubsectionsViaSymbols()) {
 
       // Emit an .alt_entry directive for the actual function symbol.
       OutStreamer->EmitSymbolAttribute(CurrentFnSym, MCSA_AltEntry);
     } else {
       EmitGlobalConstant(F.getParent()->getDataLayout(), F.getPrefixData());
     }
   }
 
+  // Emit M NOPs for -fpatchable-function-entry=N,M where M>0. We arbitrarily
+  // place prefix data before NOPs.
+  unsigned PatchableFunctionPrefix = 0;
+  (void)F.getFnAttribute("patchable-function-prefix")
+      .getValueAsString()
+      .getAsInteger(10, PatchableFunctionPrefix);
+  if (PatchableFunctionPrefix) {
+    CurrentPatchableFunctionEntrySym =
+        OutContext.createLinkerPrivateTempSymbol();
+    OutStreamer->EmitLabel(CurrentPatchableFunctionEntrySym);
+    emitNops(PatchableFunctionPrefix);
+  } else {
+    CurrentPatchableFunctionEntrySym = CurrentFnBegin;
+  }
+
   // Emit the function descriptor. This is a virtual function to allow targets
   // to emit their specific function descriptor.
   if (MAI->needsFunctionDescriptors())
     EmitFunctionDescriptor();
 
   // Emit the CurrentFnSym. This is a virtual function to allow targets to do
   // their wild and crazy things as required.
   EmitFunctionEntryLabel();

 
 void AsmPrinter::printOffset(int64_t Offset, raw_ostream &OS) const {
   if (Offset > 0)
     OS << '+' << Offset;
   else if (Offset < 0)
     OS << Offset;
 }
 
+void AsmPrinter::emitNops(unsigned N) {
+  MCInst Nop;
+  MF->getSubtarget().getInstrInfo()->getNoop(Nop);
+  for (; N; --N)
+    EmitToStreamer(*OutStreamer, Nop);
+}
+
 //===----------------------------------------------------------------------===//
 // Symbol Lowering Routines.
 //===----------------------------------------------------------------------===//
 
 MCSymbol *AsmPrinter::createTempSymbol(const Twine &Name) const {
   return OutContext.createTempSymbol(Name, true);
 }
 
void AsmPrinter::recordSled(MCSymbol *Sled, const MachineInstr &MI,
   if (Kind == SledKind::FUNCTION_ENTER && LogArgs)
     Kind = SledKind::LOG_ARGS_ENTER;
   Sleds.emplace_back(XRayFunctionEntry{Sled, CurrentFnSym, Kind,
                                        AlwaysInstrument, &F, Version});
 }
 
 void AsmPrinter::emitPatchableFunctionEntries() {
   const Function &F = MF->getFunction();
-  unsigned PatchableFunctionEntry = 0;
+  unsigned PatchableFunctionPrefix = 0, PatchableFunctionEntry = 0;
+  (void)F.getFnAttribute("patchable-function-prefix")
+      .getValueAsString()
+      .getAsInteger(10, PatchableFunctionPrefix);
   (void)F.getFnAttribute("patchable-function-entry")
       .getValueAsString()
       .getAsInteger(10, PatchableFunctionEntry);
-  if (!PatchableFunctionEntry)
+  if (!PatchableFunctionPrefix && !PatchableFunctionEntry)
     return;
   const unsigned PointerSize = getPointerSize();
   if (TM.getTargetTriple().isOSBinFormatELF()) {
     auto Flags = ELF::SHF_WRITE | ELF::SHF_ALLOC;
 
     // As of binutils 2.33, GNU as does not support section flag "o" or linkage
     // field "unique". Use SHF_LINK_ORDER if we are using the integrated
     // assembler.
if (MAI->useIntegratedAssembler()) {
       OutStreamer->SwitchSection(OutContext.getELFSection(
           "__patchable_function_entries", ELF::SHT_PROGBITS, Flags, 0,
           GroupName, UniqueID, cast<MCSymbolELF>(CurrentFnSym)));
     } else {
       OutStreamer->SwitchSection(OutContext.getELFSection(
           "__patchable_function_entries", ELF::SHT_PROGBITS, Flags));
     }
     EmitAlignment(Align(PointerSize));
-    OutStreamer->EmitSymbolValue(CurrentFnBegin, PointerSize);
+    OutStreamer->EmitSymbolValue(CurrentPatchableFunctionEntrySym, PointerSize);
   }
 }
 
 uint16_t AsmPrinter::getDwarfVersion() const {
   return OutStreamer->getContext().getDwarfVersion();
 }
 
 void AsmPrinter::setDwarfVersion(uint16_t Version) {
   OutStreamer->getContext().setDwarfVersion(Version);
 }

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-prefix")) {
+    StringRef S = Attrs
+                      .getAttribute(AttributeList::FunctionIndex,
+                                    "patchable-function-prefix")
+                      .getValueAsString();
+    unsigned N;
+    if (S.getAsInteger(10, N))
+      CheckFailed(
+          "\"patchable-function-prefix\" takes an unsigned integer: " + S, V);
+  }
   if (Attrs.hasFnAttribute("patchable-function-entry")) {
-    StringRef S0 = Attrs
-                       .getAttribute(AttributeList::FunctionIndex,
-                                     "patchable-function-entry")
-                       .getValueAsString();
-    StringRef S = S0;
+    StringRef S = Attrs
     unsigned N;
     if (S.getAsInteger(10, N))
       CheckFailed(
-          "\"patchable-function-entry\" takes an unsigned integer: " + S0, V);
+          "\"patchable-function-entry\" takes an unsigned integer: " + S, 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;

llvm/lib/Target/AArch64/AArch64AsmPrinter.cpp

 {
   const Function &F = MF->getFunction();
   if (F.hasFnAttribute("patchable-function-entry")) {
     unsigned Num;
     if (F.getFnAttribute("patchable-function-entry")
             .getValueAsString()
             .getAsInteger(10, Num))
       return;
-    for (; Num; --Num)
-      EmitToStreamer(*OutStreamer, MCInstBuilder(AArch64::HINT).addImm(0));
+    emitNops(Num);
     return;
   }
 
   EmitSled(MI, SledKind::FUNCTION_ENTER);
 }
 
 void AArch64AsmPrinter::LowerPATCHABLE_FUNCTION_EXIT(const MachineInstr &MI)
 {
void AArch64AsmPrinter::EmitInstruction(const MachineInstr *MI) {
   // Finally, do the automated lowerings for everything else.
   MCInst TmpInst;
   MCInstLowering.Lower(MI, TmpInst);
   EmitToStreamer(*OutStreamer, TmpInst);
 }
 
 // Force static initialization.
 extern "C" LLVM_EXTERNAL_VISIBILITY void LLVMInitializeAArch64AsmPrinter() {
   RegisterAsmPrinter<AArch64AsmPrinter> X(getTheAArch64leTarget());
   RegisterAsmPrinter<AArch64AsmPrinter> Y(getTheAArch64beTarget());
   RegisterAsmPrinter<AArch64AsmPrinter> Z(getTheARM64Target());
   RegisterAsmPrinter<AArch64AsmPrinter> W(getTheARM64_32Target());
   RegisterAsmPrinter<AArch64AsmPrinter> V(getTheAArch64_32Target());
 }

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

 ; RUN: llc -mtriple=aarch64 %s -o - | FileCheck --check-prefixes=CHECK %s
 
-define i32 @f0() "patchable-function-entry"="0" "branch-target-enforcement" {
+define void @f0() "patchable-function-entry"="0" "branch-target-enforcement" {
 ; CHECK-LABEL: f0:
-; CHECK-NEXT:  .Lfunc_begin0:
-; CHECK:       %bb.0:
+; CHECK-NEXT: .Lfunc_begin0:
+; CHECK:      // %bb.0:
 ; CHECK-NEXT:  hint #34
-; CHECK-NEXT:  mov w0, wzr
-; CHECK-NOT:   .section __patchable_function_entries
-  ret i32 0
+; CHECK-NEXT:  ret
+; CHECK-NOT:  .section __patchable_function_entries
+  ret void
 }
 
-define i32 @f1() "patchable-function-entry"="1" "branch-target-enforcement" {
+;; -fpatchable-function-entry=1 -mbranch-protection=bti
+define void @f1() "patchable-function-entry"="1" "branch-target-enforcement" {
 ; CHECK-LABEL: f1:
-; CHECK-NEXT:  .Lfunc_begin1:
+; CHECK-NEXT: .Lfunc_begin1:
 ; CHECK:       hint #34
 ; CHECK-NEXT:  nop
-; CHECK-NEXT:  mov w0, wzr
-; CHECK:       .section __patchable_function_entries,"awo",@progbits,f1,unique,0
-; CHECK-NEXT:  .p2align 3
-; CHECK-NEXT:  .xword .Lfunc_begin1
-  ret i32 0
+; CHECK-NEXT:  ret
+; CHECK:      .section __patchable_function_entries,"awo",@progbits,f1,unique,0
+; CHECK-NEXT: .p2align 3
+; CHECK-NEXT: .xword .Lfunc_begin1
+  ret void
+}
+
+;; -fpatchable-function-entry=2,1 -mbranch-protection=bti
+define void @f2_1() "patchable-function-entry"="1" "patchable-function-prefix"="1" "branch-target-enforcement" {
+; CHECK-LABEL: .type f2_1,@function
+; CHECK-NEXT: .Ltmp0:
+; CHECK-NEXT:  nop
+; CHECK-NEXT: f2_1:
+; CHECK-NEXT: .Lfunc_begin2:
+; CHECK:      // %bb.0:
+; CHECK-NEXT:  hint #34
+; CHECK-NEXT:  nop
+; CHECK-NEXT:  ret
+; CHECK:      .Lfunc_end2:
+; CHECK-NEXT: .size f2_1, .Lfunc_end2-f2_1
+; CHECK:      .section __patchable_function_entries,"awo",@progbits,f1,unique,0
+; CHECK-NEXT: .p2align 3
+; CHECK-NEXT: .xword .Ltmp0
+  ret void
 }

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

 ; CHECK:       nop
 ; CHECK-NEXT:  mov w0, wzr
 ; CHECK:       .section __patchable_function_entries,"awo",@progbits,f1,unique,0
 ; CHECK-NEXT:  .p2align 3
 ; CHECK-NEXT:  .xword .Lfunc_begin1
   ret i32 0
 }
 
+;; Without -function-sections, f2 is in the same text section as f1.
+;; They share the __patchable_function_entries section.
+;; With -function-sections, f1 and f2 are in different text sections.
+;; Use separate __patchable_function_entries.
 define void @f2() "patchable-function-entry"="2" {
 ; CHECK-LABEL: f2:
 ; CHECK-NEXT: .Lfunc_begin2:
 ; CHECK-COUNT-2: nop
 ; CHECK-NEXT:  ret
 ; NOFSECT:     .section __patchable_function_entries,"awo",@progbits,f1,unique,0
 ; FSECT:       .section __patchable_function_entries,"awo",@progbits,f2,unique,1
 ; CHECK-NEXT:  .p2align 3

 ; CHECK-NEXT:  sub sp, sp, #16
 ; NOFSECT      .section __patchable_function_entries,"aGwo",@progbits,f5,comdat,f5,unique,2
 ; FSECT:       .section __patchable_function_entries,"aGwo",@progbits,f5,comdat,f5,unique,3
 ; CHECK:       .p2align 3
 ; CHECK-NEXT:  .xword .Lfunc_begin4
   %frame = alloca i8, i32 16
   ret void
 }
+
+;; -fpatchable-function-entry=3,2
+;; "patchable-function-prefix" emits data before the function entry label.
+define void @f3_2() "patchable-function-entry"="1" "patchable-function-prefix"="2" {
+; CHECK-LABEL: .type f3_2,@function
+; CHECK-NEXT: .Ltmp1: // @f3_2
+; CHECK-NEXT:  nop
+; CHECK-NEXT:  nop
+; CHECK-NEXT: f3_2:
+; CHECK:      // %bb.0:
+; CHECK-NEXT:  nop
+; CHECK-NEXT:  ret
+;; .size does not include the prefix.
+; CHECK:      .Lfunc_end5:
+; CHECK-NEXT: .size f3_2, .Lfunc_end5-f3_2
+; NOFSECT     .section __patchable_function_entries,"awo",@progbits,f1,unique,0
+; FSECT:      .section __patchable_function_entries,"awo",@progbits,f3_2,unique,4
+; CHECK:      .p2align 3
+; CHECK-NEXT: .xword .Ltmp1
+  ret void
+}
+
+;; When prefix data is used, arbitrarily place NOPs after prefix data.
+define void @prefix() "patchable-function-entry"="0" "patchable-function-prefix"="1" prefix i32 1 {
+; CHECK-LABEL: .type prefix,@function
+; CHECK-NEXT: .word 1 // @prefix
+; CHECK:      .Ltmp2:
+; CHECK:       nop
+; CHECK-NEXT: prefix:
+;; Emit a __patchable_function_entries entry even if "patchable-function-entry" is 0.
+; NOFSECT     .section __patchable_function_entries,"awo",@progbits,prefix,unique,0
+; FSECT:      .section __patchable_function_entries,"awo",@progbits,prefix,unique,5
+; CHECK:      .p2align 3
+; CHECK-NEXT: .xword .Ltmp2
+  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" takes an unsigned integer:
 ; CHECK: "patchable-function-entry" takes an unsigned integer: a
 ; CHECK: "patchable-function-entry" takes an unsigned integer: -1
 ; CHECK: "patchable-function-entry" takes an unsigned integer: 3,
 
 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" { ret void }
 define void @f3comma() "patchable-function-entry"="3," { ret void }
+
+; CHECK: "patchable-function-prefix" takes an unsigned integer:
+; CHECK: "patchable-function-prefix" takes an unsigned integer: a
+; CHECK: "patchable-function-prefix" takes an unsigned integer: -1
+; CHECK: "patchable-function-prefix" takes an unsigned integer: 3,
+
+define void @g() "patchable-function-prefix" { ret void }
+define void @ga() "patchable-function-prefix"="a" { ret void }
+define void @g_1() "patchable-function-prefix"="-1" { ret void }
+define void @g3comma() "patchable-function-prefix"="3," { ret void }