[BOLT][PAC] Warn about synchronous unwind tables

[bgergely0]

BOLT currently ignores functions with synchronous PAuth DWARF info.
When more than 10% of functions get ignored for inconsistencies, we
should emit a warning to only use asynchronous unwind tables.

See also: #165215

[bgergely0]

Warning

This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
Learn more

• [BOLT] Fix thread-safety of PointerAuthCFIAnalyzer #165365
• [BOLT][PAC] Warn about synchronous unwind tables #165227 👈 (View in Graphite)
• [BOLT][NFC] Rename Pointer Auth DWARF rewriter passes #164622
• [BOLT] Improve InsertNegateRAStatePass::inferUnknownStates #163381
• [BOLT] Simplify RAState helpers (NFCI) #162820
• main

This stack of pull requests is managed by Graphite. Learn more about stacking.

[llvmbot]

@llvm/pr-subscribers-bolt

Author: Gergely Bálint (bgergely0)

Changes

BOLT currently ignores functions with synchronous PAuth DWARF info.
When more than 10% of functions get ignored for inconsistencies, we
should emit a warning to only use asynchronous unwind tables.

See also: #165215

---

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

2 Files Affected:

• (modified) bolt/lib/Passes/PointerAuthCFIAnalyzer.cpp (+7-1)
• (added) bolt/test/runtime/AArch64/pacret-synchronous-unwind.cpp (+32)

diff --git a/bolt/lib/Passes/PointerAuthCFIAnalyzer.cpp b/bolt/lib/Passes/PointerAuthCFIAnalyzer.cpp
index 2fc5a2fda086a..6bcb5a6bd1801 100644
--- a/bolt/lib/Passes/PointerAuthCFIAnalyzer.cpp
+++ b/bolt/lib/Passes/PointerAuthCFIAnalyzer.cpp
@@ -130,11 +130,17 @@ Error PointerAuthCFIAnalyzer::runOnFunctions(BinaryContext &BC) {
   ParallelUtilities::runOnEachFunction(
       BC, ParallelUtilities::SchedulingPolicy::SP_INST_LINEAR, WorkFun,
       SkipPredicate, "PointerAuthCFIAnalyzer");
+
+  float IgnoredPercent = (100.0 * FunctionsIgnored) / Total;
   BC.outs() << "BOLT-INFO: PointerAuthCFIAnalyzer ran on " << Total
             << " functions. Ignored " << FunctionsIgnored << " functions "
-            << format("(%.2lf%%)", (100.0 * FunctionsIgnored) / Total)
+            << format("(%.2lf%%)", IgnoredPercent)
             << " because of CFI inconsistencies\n";
 
+  if (IgnoredPercent >= 10.0)
+    BC.outs() << "BOLT-WARNING: PointerAuthCFIAnalyzer only supports "
+                 "asynchronous unwind tables.\n";
+
   return Error::success();
 }
 
diff --git a/bolt/test/runtime/AArch64/pacret-synchronous-unwind.cpp b/bolt/test/runtime/AArch64/pacret-synchronous-unwind.cpp
new file mode 100644
index 0000000000000..e90882833323d
--- /dev/null
+++ b/bolt/test/runtime/AArch64/pacret-synchronous-unwind.cpp
@@ -0,0 +1,32 @@
+// Test to demonstrate that functions compiled with synchronous unwind tables
+// are ignored by the PointerAuthCFIAnalyzer.
+// Exception handling is needed to have _any_ unwind tables, otherwise the
+// PointerAuthCFIAnalyzer does not run on these functions, so it does not ignore
+// any function.
+//
+// REQUIRES: system-linux,bolt-runtime
+//
+// RUN: %clangxx --target=aarch64-unknown-linux-gnu \
+// RUN: -mbranch-protection=pac-ret \
+// RUN: -fno-asynchronous-unwind-tables \
+// RUN: %s -o %t.exe -Wl,-q
+// RUN: llvm-bolt %t.exe -o %t.bolt | FileCheck %s --check-prefix=CHECK
+//
+// CHECK: PointerAuthCFIAnalyzer ran on 3 functions. Ignored
+// CHECK-NOT: 0 functions (0.00%) because of CFI inconsistencies
+// CHECK-SAME: 1 functions (33.33%) because of CFI inconsistencies
+// CHECK-NEXT: PointerAuthCFIAnalyzer only supports asynchronous unwind tables
+
+#include <cstdio>
+#include <stdexcept>
+
+void foo() { throw std::runtime_error("Exception from foo()."); }
+
+int main() {
+  try {
+    foo();
+  } catch (const std::exception &e) {
+    printf("Exception caught: %s\n", e.what());
+  }
+  return 0;
+}

[bgergely0]

Looks like the code formatting bot is broken, but the patch is formatted locally.

[bgergely0]

As an input binary with sync unwind tables generates many warnings, it made sense to add the verbosity checking in this PR as well.

[github-actions]

✅ With the latest revision this PR passed the C/C++ code formatter.

[paschalis-mpeis]

Can you elaborate how the >=10% threshold for the warning was chosen (instead of emiting it in all cases)? Maybe some of these details might also fit in the design doc too , where you talk about inconsistencies.

[bgergely0]

As the related issue states:

Example: when applying BOLT to llama.cpp, this is seen in the logs: Ignored 1633 functions (99.03%) because of CFI inconsistencies. The huge % of ignored functions makes BOLT useless in such cases.

I think the double-digit territory is a good boundary to start emitting this warning. After all, we don't check/don't know if this is the reason for the errors, but the higher the percentage the more likely that this is the issue. Low-percentage issues can be from older compiler versions emitting CFIs incorrectly (as I've seen while testing the work).

So the 10% is a "magic constant" and is up for discussion, I think it is a reasonable value to start emitting warnings.

[paschalis-mpeis]

Sounds good. So BOLT doesnt know whether inconsistencies are due to sync/async unwind tables, but instead makes an assumption to help the user? Could you summarise this with a comment in code around the point you emit the warning?

Since you are on this, should it handle cases like the below? (we've started seeing these recently)

BOLT-INFO: MarkRAStates ran on 0 functions. Ignored 0 functions (nan%) because of CFI inconsistencies