[BOLT][AArch64] Allow binary-analysis and heatmap tool to run with pac-ret binaries

[bgergely0]

Quoting bolt/docs/BinaryAnalysis.md:

BOLT cannot currently handle functions with cfi_negate_ra_state correctly, i.e. any binaries built with -mbranch-protection=pac-ret. The scanner is meant to be used on specifically such binaries, so this is a major limitation! Work is going on in PR #120064 to fix this.

For pac-ret analysis to work, we don't actually need the whole work in #120064 to be merged.

Some context

The BOLT codebase holds different tools: BOLT itself is used to optimize the layout of a binary, while other tools only need to generate the same internal representation to aid in that process.

The .cfi_negate_ra_state Call Frame Instruction needs to be read, tracked, and eventually emitted by tools that produce a binary: this CFI signals to the unwinder if it needs to strip the PAC bits from pointers or not.

This means we can ignore these CFIs in llvm-bolt-binary-analysis and the heatmaptool.

Solution

In places where previously BOLT would run to an llvm_unreachable("Unsupported CFI opcode") for OpNegateRAState, we first check from options which tool is running, and only crash with unreachable when needed.

[github-actions]

Thank you for submitting a Pull Request (PR) to the LLVM Project!

This PR will be automatically labeled and the relevant teams will be notified.

If you wish to, you can add reviewers by using the "Reviewers" section on this page.

If this is not working for you, it is probably because you do not have write permissions for the repository. In which case you can instead tag reviewers by name in a comment by using @ followed by their GitHub username.

If you have received no comments on your PR for a week, you can request a review by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate is once a week. Please remember that you are asking for valuable time from other developers.

If you have further questions, they may be answered by the LLVM GitHub User Guide.

You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums.

[bgergely0]

PTAL @kbeyls, @paschalis-mpeis

[llvmbot]

@llvm/pr-subscribers-bolt

Author: Gergely Bálint (bgergely0)

Changes

Quoting bolt/docs/BinaryAnalysis.md:
> BOLT cannot currently handle functions with cfi_negate_ra_state correctly, i.e. any binaries built with -mbranch-protection=pac-ret. The scanner is meant to be used on specifically such binaries, so this is a major limitation! Work is going on in PR #120064 to fix this.

For pac-ret analysis to work, we don't actually need the whole work in #120064 to be merged.

Some context

The BOLT codebase holds different tools: BOLT itself is used to optimize the layout of a binary, while other tools only need to generate the same internal representation to aid in that process, for example perf2bolt only processes profile information, and does not output an optimized binary.

The llvm-bolt-binary-analysis tool is also such a tool.

This is important, because the .cfi_negate_ra_state Call Frame Instruction needs to be read, tracked, and eventually emitted by tools that produce a binary: this CFI signals to the unwinder if it needs to strip the PAC bits from pointers or not.

This means, that for all other tools, we don't actually have to read it from the binary!

Solution

The solution is fairly simple: we need to pass the ToolName to CFIReaderWriter::fillCFIInfoFor, and branch based on this ToolName.

if (getToolName() == "llvm-bolt") {
  BC.errs() << "BOLT-ERROR: pointer authentication is not supported "
               "yet. Please compile "
               "your target binary using '-mbranch-protection=none'.\n";
  exit(1);
}

This is a two-bird-one-stone solution: it also adds a user-friendly error message about the lack of PAC support. Previously, users got a fairly arcane error about a unsupported CFI opcode. The new error is actionable, so hopefully it would lead to less confusion while support for PAC is added.

---

Full diff: https://github.com/llvm/llvm-project/pull/136664.diff

5 Files Affected:

• (modified) bolt/docs/BinaryAnalysis.md (-6)
• (modified) bolt/include/bolt/Core/Exceptions.h (+5-1)
• (modified) bolt/lib/Core/Exceptions.cpp (+12-3)
• (modified) bolt/lib/Rewrite/RewriteInstance.cpp (+2-1)
• (modified) bolt/test/AArch64/dw_cfa_gnu_window_save.test (+3-3)

diff --git a/bolt/docs/BinaryAnalysis.md b/bolt/docs/BinaryAnalysis.md
index 9f0f018980517..b13410cd96355 100644
--- a/bolt/docs/BinaryAnalysis.md
+++ b/bolt/docs/BinaryAnalysis.md
@@ -180,12 +180,6 @@ The following are current known cases of false negatives:
    [prototype branch](
    https://github.com/llvm/llvm-project/compare/main...kbeyls:llvm-project:bolt-gadget-scanner-prototype).
 
-BOLT cannot currently handle functions with `cfi_negate_ra_state` correctly,
-i.e. any binaries built with `-mbranch-protection=pac-ret`. The scanner is meant
-to be used on specifically such binaries, so this is a major limitation! Work is
-going on in PR [#120064](https://github.com/llvm/llvm-project/pull/120064) to
-fix this.
-
 ## How to add your own binary analysis
 
 _TODO: this section needs to be written. Ideally, we should have a simple
diff --git a/bolt/include/bolt/Core/Exceptions.h b/bolt/include/bolt/Core/Exceptions.h
index f10cf776f0943..54e7fc50078da 100644
--- a/bolt/include/bolt/Core/Exceptions.h
+++ b/bolt/include/bolt/Core/Exceptions.h
@@ -37,7 +37,8 @@ class BinaryFunction;
 /// BinaryFunction, as well as rewriting CFI sections.
 class CFIReaderWriter {
 public:
-  explicit CFIReaderWriter(BinaryContext &BC, const DWARFDebugFrame &EHFrame);
+  explicit CFIReaderWriter(BinaryContext &BC, const DWARFDebugFrame &EHFrame,
+                           StringRef ToolName);
 
   bool fillCFIInfoFor(BinaryFunction &Function) const;
 
@@ -56,9 +57,12 @@ class CFIReaderWriter {
 
   const FDEsMap &getFDEs() const { return FDEs; }
 
+  StringRef getToolName() const { return ToolName; }
+
 private:
   BinaryContext &BC;
   FDEsMap FDEs;
+  StringRef ToolName;
 };
 
 /// Parse an existing .eh_frame and invoke the callback for each
diff --git a/bolt/lib/Core/Exceptions.cpp b/bolt/lib/Core/Exceptions.cpp
index 0b2e63b8ca6a7..cbb29a05bee20 100644
--- a/bolt/lib/Core/Exceptions.cpp
+++ b/bolt/lib/Core/Exceptions.cpp
@@ -463,8 +463,10 @@ void BinaryFunction::updateEHRanges() {
 const uint8_t DWARF_CFI_PRIMARY_OPCODE_MASK = 0xc0;
 
 CFIReaderWriter::CFIReaderWriter(BinaryContext &BC,
-                                 const DWARFDebugFrame &EHFrame)
+                                 const DWARFDebugFrame &EHFrame,
+                                 StringRef ToolName)
     : BC(BC) {
+  this->ToolName = ToolName;
   // Prepare FDEs for fast lookup
   for (const dwarf::FrameEntry &Entry : EHFrame.entries()) {
     const auto *CurFDE = dyn_cast<dwarf::FDE>(&Entry);
@@ -632,8 +634,15 @@ bool CFIReaderWriter::fillCFIInfoFor(BinaryFunction &Function) const {
       // DW_CFA_GNU_window_save and DW_CFA_GNU_NegateRAState just use the same
       // id but mean different things. The latter is used in AArch64.
       if (Function.getBinaryContext().isAArch64()) {
-        Function.addCFIInstruction(
-            Offset, MCCFIInstruction::createNegateRAState(nullptr));
+        // .cfi_negate_ra_state is only needed for tools producing binaries (so
+        // BOLT itself). Other BOLT-based tools (perf2bolt, merge-fdata,
+        // llvm-bolt-binary-analysis, etc.) can safely drop this CFI.
+        if (getToolName() == "llvm-bolt") {
+          BC.errs() << "BOLT-ERROR: pointer authentication is not supported "
+                       "yet. Please compile "
+                       "your target binary using '-mbranch-protection=none'.\n";
+          exit(1);
+        }
         break;
       }
       if (opts::Verbosity >= 1)
diff --git a/bolt/lib/Rewrite/RewriteInstance.cpp b/bolt/lib/Rewrite/RewriteInstance.cpp
index 69fb736d7bde0..6dc7cad4f538c 100644
--- a/bolt/lib/Rewrite/RewriteInstance.cpp
+++ b/bolt/lib/Rewrite/RewriteInstance.cpp
@@ -2048,7 +2048,8 @@ Error RewriteInstance::readSpecialSections() {
   Expected<const DWARFDebugFrame *> EHFrameOrError = BC->DwCtx->getEHFrame();
   if (!EHFrameOrError)
     report_error("expected valid eh_frame section", EHFrameOrError.takeError());
-  CFIRdWrt.reset(new CFIReaderWriter(*BC, *EHFrameOrError.get()));
+  StringRef ToolName = llvm::sys::path::stem(Argv[0]);
+  CFIRdWrt.reset(new CFIReaderWriter(*BC, *EHFrameOrError.get(), ToolName));
 
   processSectionMetadata();
 
diff --git a/bolt/test/AArch64/dw_cfa_gnu_window_save.test b/bolt/test/AArch64/dw_cfa_gnu_window_save.test
index 2e044b399720a..97b257ee04666 100644
--- a/bolt/test/AArch64/dw_cfa_gnu_window_save.test
+++ b/bolt/test/AArch64/dw_cfa_gnu_window_save.test
@@ -1,8 +1,8 @@
-# Check that llvm-bolt can handle DW_CFA_GNU_window_save on AArch64.
+# Check that llvm-bolt refuses binaries with DW_CFA_GNU_window_save on AArch64.
 
 RUN: yaml2obj %p/Inputs/dw_cfa_gnu_window_save.yaml &> %t.exe
-RUN: llvm-bolt %t.exe -o %t.bolt 2>&1 | FileCheck %s
+RUN not: llvm-bolt %t.exe -o %t.bolt 2>&1 | FileCheck %s
 
 CHECK-NOT: paciasp
 CHECK-NOT: autiasp
-CHECK-NOT: ERROR: unable to fill CFI.
+CHECK: BOLT-ERROR: pointer authentication is not supported yet.

[bgergely0]

also tagging @atrosinenko as I see you are very active on the pac-ret gadget scanner :)

[atrosinenko]

Thank you very much for unblocking gadget scanner - I think it is a good idea to first fix the "read only mode" and then discuss the general case which is much more complex.

I am rather cautious about matching executable name (though I'm not very familiar with BOLT's core). Maybe it is better to choose the mode of operation based on opts::BinaryAnalysisMode and other such options, see these lines for example. Then, instead of assigning this->ToolName, you could compute this property in constructor and assign it to a boolean field.

[bgergely0]

@atrosinenko Thanks for the review.

Maybe it is better to choose the mode of operation based on opts::BinaryAnalysisMode

Good point, probably better to only change the behaviour for the binary-analysis tool, because the current way it allows all tools besides BOLT. Doing perf2bolt when you cannot do bolt after isn't much help.

[paschalis-mpeis]

Hey Gergely,

Thanks for your work. I think it should be OK to restrict this to both llvm-bolt and perf2bolt. As said, the latter is not affected, but then llvm-bolt won't be able to run regardless.

[paschalis-mpeis]

Looks good, thanks!

Let's allow 1-2 days in case there are additional comments.

[atrosinenko]

@bgergely0 Thank you for the updates! I have no more comments except for one minor nit-picking (feel free to ignore).

I agree with @paschalis-mpeis, let's wait 1-2 days for others to comment.

[paschalis-mpeis]

Could also rename the title to something like:

[BOLT][AArch64] Abort on pac-ret binaries in llvm-bolt and perf2bolt

[kbeyls]

Thank you, this mostly looks good to me!
I just had one nit-pick comment.

I would also improve the title of this PR/commit message to be more descriptive, as @paschalis-mpeis recommended.

[paschalis-mpeis]

Noting that this new behavior is triggered on 39 tests on AArch64, causing all to fail.
It will have to be addressed before proceeding.

Total Discovered Tests: 547
  Skipped          :  13 (2.38%)
  Unsupported      :  81 (14.81%)
  Passed           : 413 (75.50%)
  Expectedly Failed:   1 (0.18%)
  Failed           :  39 (7.13%)

[bgergely0]

Looks like many unittest binaries have paciasp/autiasp + the .cfi_negate_ra_state in __do_global_dtors_aux.
These small unittest targets do not run into unwindCFIState and similar functions where unsupported CFI opcode would be triggered (so they did not trigger errors before).
Problem is, the __do_global_dtors_aux is coming from a precompiled object on the system where the user builts their BOLT input binary (as I currently understand it), so the user does not directly control the flags __do_global_dtors_aux is compiled with.

[bgergely0]

With the new commit I do the check "later", e.g. not in FillCFIInfo, but only when BOLT would crash anyways. With this, I can make sure no unittests that passed before my change will fail.

[paschalis-mpeis]

Hey Gergely,

Thanks for fixing the tests. Essentially now the error handling of OpNegateRAState is explicit. And your upcoming work will add support through a flag:

• #120064

[bgergely0]

Thanks for the approval @paschalis-mpeis!
I don't have write permissions the repo, can you merge this in a few days if others have no concerns?

[atrosinenko]

The patch looks good to me (though I'm not very familiar with BOLT's handling of CFI), with a trivial nit-picking. Thank you!

[paschalis-mpeis]

Thanks for the approval @paschalis-mpeis! I don't have write permissions the repo, can you merge this in a few days if others have no concerns?

Sure. I see you addressed the nits too, so I proceed now with merging. Thanks!

[github-actions]

@bgergely0 Congratulations on having your first Pull Request (PR) merged into the LLVM Project!

Your changes will be combined with recent changes from other authors, then tested by our build bots. If there is a problem with a build, you may receive a report in an email or a comment on this PR.

Please check whether problems have been caused by your change specifically, as the builds can include changes from many authors. It is not uncommon for your change to be included in a build that fails due to someone else's changes, or infrastructure issues.

How to do this, and the rest of the post-merge process, is covered in detail here.

If your change does cause a problem, it may be reverted, or you can revert it yourself. This is a normal part of LLVM development. You can fix your changes and open a new PR to merge them again.

If you don't get any reports, no action is required from you. Your changes are working as expected, well done!