Skip to content

[BOLT] Run PatchEntries pass before LongJmp #137236

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
May 1, 2025
Merged

Conversation

maksfb
Copy link
Contributor

@maksfb maksfb commented Apr 24, 2025

With --force-patch option, every original function entry point is overwritten with a trampoline to a new version of the function to prevent the execution of the original code.

If the function size is too small for the trampoline code, we are forced to bail out on rewriting the function. That presented a problem on AArch64 due to LongJmp pass that assumed the presence of the new copy of the function. If the new copy was not emitted it could have lead to a relocation overflow.

Run PatchEntries pass before LongJmp and make the latter aware of the functions that are not going to be emitted. Make --force-patch option behavior on AArch64 consistent with other architectures.

With --force-patch option, every original function entry point is
overwritten with a trampoline to a new version of the function to
prevent the execution of the original code.

If the function size is too small for the trampoline code, we are forced
to bail out on rewriting the function. That presented a problem on
AArch64 due to LongJmp pass that assumed the presence of the new copy of
the function. If the new copy was not emitted it could have lead to a
relocation overflow.

Run PatchEntries pass before LongJmp and make the latter aware of the
functions that are not going to be emitted. Make --force-patch option
behavior on AArch64 consistent with other architectures.
@llvmbot
Copy link
Member

llvmbot commented Apr 24, 2025

@llvm/pr-subscribers-bolt

Author: Maksim Panchenko (maksfb)

Changes

With --force-patch option, every original function entry point is overwritten with a trampoline to a new version of the function to prevent the execution of the original code.

If the function size is too small for the trampoline code, we are forced to bail out on rewriting the function. That presented a problem on AArch64 due to LongJmp pass that assumed the presence of the new copy of the function. If the new copy was not emitted it could have lead to a relocation overflow.

Run PatchEntries pass before LongJmp and make the latter aware of the functions that are not going to be emitted. Make --force-patch option behavior on AArch64 consistent with other architectures.


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

2 Files Affected:

  • (modified) bolt/lib/Passes/PatchEntries.cpp (+1-12)
  • (modified) bolt/lib/Rewrite/BinaryPassManager.cpp (+4-4)
diff --git a/bolt/lib/Passes/PatchEntries.cpp b/bolt/lib/Passes/PatchEntries.cpp
index 8a2f0a39a56cc..4877e7dd8fdf3 100644
--- a/bolt/lib/Passes/PatchEntries.cpp
+++ b/bolt/lib/Passes/PatchEntries.cpp
@@ -98,21 +98,10 @@ Error PatchEntries::runOnFunctions(BinaryContext &BC) {
     });
 
     if (!Success) {
-      // We can't change output layout for AArch64 due to LongJmp pass
-      if (BC.isAArch64()) {
-        if (opts::ForcePatch) {
-          BC.errs() << "BOLT-ERROR: unable to patch entries in " << Function
-                    << "\n";
-          return createFatalBOLTError("");
-        }
-
-        continue;
-      }
-
       // If the original function entries cannot be patched, then we cannot
       // safely emit new function body.
       BC.errs() << "BOLT-WARNING: failed to patch entries in " << Function
-                << ". The function will not be optimized.\n";
+                << ". The function will not be optimized\n";
       Function.setIgnored();
       continue;
     }
diff --git a/bolt/lib/Rewrite/BinaryPassManager.cpp b/bolt/lib/Rewrite/BinaryPassManager.cpp
index dd48653931eb9..996d2e972599d 100644
--- a/bolt/lib/Rewrite/BinaryPassManager.cpp
+++ b/bolt/lib/Rewrite/BinaryPassManager.cpp
@@ -497,6 +497,10 @@ Error BinaryFunctionPassManager::runAllPasses(BinaryContext &BC) {
   // memory profiling data.
   Manager.registerPass(std::make_unique<ReorderData>());
 
+  // Patch original function entries
+  if (BC.HasRelocations)
+    Manager.registerPass(std::make_unique<PatchEntries>());
+
   if (BC.isAArch64()) {
     Manager.registerPass(
         std::make_unique<ADRRelaxationPass>(PrintAdrRelaxation));
@@ -524,10 +528,6 @@ Error BinaryFunctionPassManager::runAllPasses(BinaryContext &BC) {
   // Assign each function an output section.
   Manager.registerPass(std::make_unique<AssignSections>());
 
-  // Patch original function entries
-  if (BC.HasRelocations)
-    Manager.registerPass(std::make_unique<PatchEntries>());
-
   // This pass turns tail calls into jumps which makes them invisible to
   // function reordering. It's unsafe to use any CFG or instruction analysis
   // after this point.

@maksfb maksfb merged commit 7d6fda4 into llvm:main May 1, 2025
12 checks passed
IanWood1 pushed a commit to IanWood1/llvm-project that referenced this pull request May 6, 2025
With --force-patch option, every original function entry point is
overwritten with a trampoline to a new version of the function to
prevent the execution of the original code.

If the function size is too small for the trampoline code, we are forced
to bail out on rewriting the function. That presented a problem on
AArch64 due to LongJmp pass that assumed the presence of the new copy of
the function. If the new copy was not emitted it could have lead to a
relocation overflow.

Run PatchEntries pass before LongJmp and make the latter aware of the
functions that are not going to be emitted. Make --force-patch option
behavior on AArch64 consistent with other architectures.
IanWood1 pushed a commit to IanWood1/llvm-project that referenced this pull request May 6, 2025
With --force-patch option, every original function entry point is
overwritten with a trampoline to a new version of the function to
prevent the execution of the original code.

If the function size is too small for the trampoline code, we are forced
to bail out on rewriting the function. That presented a problem on
AArch64 due to LongJmp pass that assumed the presence of the new copy of
the function. If the new copy was not emitted it could have lead to a
relocation overflow.

Run PatchEntries pass before LongJmp and make the latter aware of the
functions that are not going to be emitted. Make --force-patch option
behavior on AArch64 consistent with other architectures.
IanWood1 pushed a commit to IanWood1/llvm-project that referenced this pull request May 6, 2025
With --force-patch option, every original function entry point is
overwritten with a trampoline to a new version of the function to
prevent the execution of the original code.

If the function size is too small for the trampoline code, we are forced
to bail out on rewriting the function. That presented a problem on
AArch64 due to LongJmp pass that assumed the presence of the new copy of
the function. If the new copy was not emitted it could have lead to a
relocation overflow.

Run PatchEntries pass before LongJmp and make the latter aware of the
functions that are not going to be emitted. Make --force-patch option
behavior on AArch64 consistent with other architectures.
GeorgeARM pushed a commit to GeorgeARM/llvm-project that referenced this pull request May 7, 2025
With --force-patch option, every original function entry point is
overwritten with a trampoline to a new version of the function to
prevent the execution of the original code.

If the function size is too small for the trampoline code, we are forced
to bail out on rewriting the function. That presented a problem on
AArch64 due to LongJmp pass that assumed the presence of the new copy of
the function. If the new copy was not emitted it could have lead to a
relocation overflow.

Run PatchEntries pass before LongJmp and make the latter aware of the
functions that are not going to be emitted. Make --force-patch option
behavior on AArch64 consistent with other architectures.
Ankur-0429 pushed a commit to Ankur-0429/llvm-project that referenced this pull request May 9, 2025
With --force-patch option, every original function entry point is
overwritten with a trampoline to a new version of the function to
prevent the execution of the original code.

If the function size is too small for the trampoline code, we are forced
to bail out on rewriting the function. That presented a problem on
AArch64 due to LongJmp pass that assumed the presence of the new copy of
the function. If the new copy was not emitted it could have lead to a
relocation overflow.

Run PatchEntries pass before LongJmp and make the latter aware of the
functions that are not going to be emitted. Make --force-patch option
behavior on AArch64 consistent with other architectures.
@maksfb maksfb deleted the gh-patch-entries branch May 15, 2025 04:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants