[NFC][BOLT] Refactor ForcePatch option

[paschalis-mpeis]

Move force-patch flag to CommandLineOpts and add details on PatchEntries.

[llvmbot]

@llvm/pr-subscribers-bolt

Author: Paschalis Mpeis (paschalis-mpeis)

Changes

Move force-patch flag to CommandLineOpts and add details on PatchEntries.

---

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

4 Files Affected:

• (modified) bolt/include/bolt/Utils/CommandLineOpts.h (+1)
• (modified) bolt/lib/Passes/PatchEntries.cpp (+11-10)
• (modified) bolt/lib/Rewrite/MachORewriteInstance.cpp (+3-3)
• (modified) bolt/lib/Utils/CommandLineOpts.cpp (+6)

diff --git a/bolt/include/bolt/Utils/CommandLineOpts.h b/bolt/include/bolt/Utils/CommandLineOpts.h
index 111eb650c3746..ee110eb8707f7 100644
--- a/bolt/include/bolt/Utils/CommandLineOpts.h
+++ b/bolt/include/bolt/Utils/CommandLineOpts.h
@@ -37,6 +37,7 @@ extern llvm::cl::opt<unsigned> BucketsPerLine;
 extern llvm::cl::opt<bool> DiffOnly;
 extern llvm::cl::opt<bool> EnableBAT;
 extern llvm::cl::opt<bool> EqualizeBBCounts;
+extern llvm::cl::opt<bool> ForcePatch;
 extern llvm::cl::opt<bool> RemoveSymtab;
 extern llvm::cl::opt<unsigned> ExecutionCountThreshold;
 extern llvm::cl::opt<unsigned> HeatmapBlock;
diff --git a/bolt/lib/Passes/PatchEntries.cpp b/bolt/lib/Passes/PatchEntries.cpp
index 981d1b70af907..afe489b4667c3 100644
--- a/bolt/lib/Passes/PatchEntries.cpp
+++ b/bolt/lib/Passes/PatchEntries.cpp
@@ -6,27 +6,28 @@
 //
 //===----------------------------------------------------------------------===//
 //
-// This file implements the PatchEntries class that is used for patching
-// the original function entry points.
+// This file implements the PatchEntries class that is used for patching the
+// original function entry points. This ensures that only the new/optimized code
+// executes and that the old code is never used. This is necessary due to
+// current BOLT limitations of not being able to duplicate all function's
+// associated metadata (e.g., .eh_frame, exception ranges, debug info,
+// jump-tables).
+//
+// NOTE: A successful run of 'scanExternalRefs' can relax this requirement as
+// it also ensures that old code is never executed.
 //
 //===----------------------------------------------------------------------===//
 
 #include "bolt/Passes/PatchEntries.h"
+#include "bolt/Utils/CommandLineOpts.h"
 #include "bolt/Utils/NameResolver.h"
 #include "llvm/ADT/STLExtras.h"
 #include "llvm/Support/CommandLine.h"
 
 namespace opts {
-
 extern llvm::cl::OptionCategory BoltCategory;
-
 extern llvm::cl::opt<unsigned> Verbosity;
-
-llvm::cl::opt<bool>
-    ForcePatch("force-patch",
-               llvm::cl::desc("force patching of original entry points"),
-               llvm::cl::Hidden, llvm::cl::cat(BoltCategory));
-}
+} // namespace opts
 
 namespace llvm {
 namespace bolt {
diff --git a/bolt/lib/Rewrite/MachORewriteInstance.cpp b/bolt/lib/Rewrite/MachORewriteInstance.cpp
index 2f05b03290ba3..335b7b42ddde5 100644
--- a/bolt/lib/Rewrite/MachORewriteInstance.cpp
+++ b/bolt/lib/Rewrite/MachORewriteInstance.cpp
@@ -20,6 +20,7 @@
 #include "bolt/Rewrite/JITLinkLinker.h"
 #include "bolt/Rewrite/RewriteInstance.h"
 #include "bolt/RuntimeLibs/InstrumentationRuntimeLibrary.h"
+#include "bolt/Utils/CommandLineOpts.h"
 #include "bolt/Utils/Utils.h"
 #include "llvm/MC/MCObjectStreamer.h"
 #include "llvm/Support/Errc.h"
@@ -32,9 +33,8 @@ namespace opts {
 
 using namespace llvm;
 extern cl::opt<unsigned> AlignText;
-//FIXME! Upstream change
-//extern cl::opt<bool> CheckOverlappingElements;
-extern cl::opt<bool> ForcePatch;
+// FIXME! Upstream change
+// extern cl::opt<bool> CheckOverlappingElements;
 extern cl::opt<bool> Instrument;
 extern cl::opt<bool> InstrumentCalls;
 extern cl::opt<bolt::JumpTableSupportLevel> JumpTables;
diff --git a/bolt/lib/Utils/CommandLineOpts.cpp b/bolt/lib/Utils/CommandLineOpts.cpp
index 17f090aa61ee9..ad714371436e0 100644
--- a/bolt/lib/Utils/CommandLineOpts.cpp
+++ b/bolt/lib/Utils/CommandLineOpts.cpp
@@ -80,6 +80,12 @@ cl::opt<bool> EqualizeBBCounts(
              "in non-LBR and shrink wrapping)"),
     cl::ZeroOrMore, cl::init(false), cl::Hidden, cl::cat(BoltOptCategory));
 
+llvm::cl::opt<bool> ForcePatch(
+    "force-patch",
+    llvm::cl::desc("force patching of original entry points to ensure "
+                   "execution follows only the new/optimized code."),
+    llvm::cl::Hidden, llvm::cl::cat(BoltCategory));
+
 cl::opt<bool> RemoveSymtab("remove-symtab", cl::desc("Remove .symtab section"),
                            cl::cat(BoltCategory));
 

[aaupov]

What's the context? Is that a part of a larger change?

[paschalis-mpeis]

Hey Amir,

Yes. I will stack #116964 on top of this. I'll be using the flag in core (BinaryContext) and in core unit-tests, and I figured it's simpler if I move this to Utils.

[paschalis-mpeis]

Any feedback on this PR?

It is for #116964. It makes unit tests depend on BOLTUtils instead of BOLTPasses (see example). No strong opinion on my end.

cc: @maksfb

[yota9]

LGTM Thanks!

[maksfb]

Looks good! Maybe retitle to "Refactor ForcePatch option"?

[paschalis-mpeis]

Great! Thank you both for your reviews.