[PATCH] D130072: [BOLT] Update labels for split landing pad

[Rafael Auler via Phabricator via llvm-commits]

rafauler added inline comments.


================
Comment at: bolt/lib/Core/BinaryFunction.cpp:1887
 
-      BinaryBasicBlock *LPBlock = getBasicBlockForLabel(EHInfo->first);
-      if (!BBLandingPads.count(LPBlock)) {
+      // For cross-fragment LPs, LPBlock = nullptr if LPFunc not yet buildCFG
+      // Solution: after buildCFG for all functions, rerun recomputeLandingPads
----------------
.. if LPFunc has no CFG yet


================
Comment at: bolt/lib/Core/BinaryFunction.cpp:1888
+      // For cross-fragment LPs, LPBlock = nullptr if LPFunc not yet buildCFG
+      // Solution: after buildCFG for all functions, rerun recomputeLandingPads
+      const MCSymbol *LPLabel = EHInfo->first;
----------------
Solution: rerun recomputeLandingPads after buildCFG for all functions


================
Comment at: bolt/lib/Core/Exceptions.cpp:203-205
+    // Special case, consider builtin_unreachable as part of this function
+    if (LPAddress == Address + getSize())
+      Fragment = this;
----------------
I would suggest moving this before MCSymbol *LPLabel = nullptr; so the code that calculates LPLabel is closer together


================
Comment at: bolt/lib/Core/Exceptions.cpp:209
+    // Note: landing pad can target other fragment entry -> split landing pad
+    if (LPAddress != Address) {
+      uint64_t FragmentOffset = LPAddress - Fragment->getAddress();
----------------
I was thinking in maybe adding a comment "Skip creating a landing pad if LPOffset is zero"  so it is clear that this is from the LSDA format


================
Comment at: bolt/lib/Core/Exceptions.cpp:217
+      } else {
+        // Treat split landing pad as the fragment's secondary fragment
+        auto Label = Fragment->Labels.find(FragmentOffset);
----------------
Treat split landing pad as the fragment's secondary entry point?

But maybe expand this comment, as it looks like this code section is handling _only_ that. Maybe:

  Create/fetch landing pad label. If necessary, treat split landing pad as...


================
Comment at: bolt/lib/Core/Exceptions.cpp:226
+        BC.setSymbolToFunctionMap(LPLabel, Fragment);
+        Labels[LPOffset] = LPLabel;
+      }
----------------
Is this correct for the case where LPOffset is an offset outside this function (LPLabel is set by Fragment->addEntryPointAtOffset(FragmentOffset)) ?


================
Comment at: bolt/lib/Rewrite/RewriteInstance.cpp:2925-2926
 
-    if (!Function.isSimple()) {
-      assert((!BC->HasRelocations || Function.getSize() == 0 ||
-              Function.hasIndirectTargetToSplitFragment()) &&
+    // If non-simple due to split jump table or split landing pad, process
+    // Every other non-simple cases, ignore
+    if (!Function.isSimple() && !Function.hasIndirectTargetToSplitFragment()) {
----------------
I would reverse for clarity: 
  Process non-simple cases due to split jump table or split landing pad
  Ignore all other non-simple cases


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D130072/new/

https://reviews.llvm.org/D130072