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

[Huan Nguyen via Phabricator via llvm-commits]

nhuhuan added inline comments.


================
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();
----------------
rafauler wrote:
> 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
I wrote an equivalent comment: "Assumption: landing pad cannot target current fragment entry"


================
Comment at: bolt/lib/Core/Exceptions.cpp:226
+        BC.setSymbolToFunctionMap(LPLabel, Fragment);
+        Labels[LPOffset] = LPLabel;
+      }
----------------
rafauler wrote:
> Is this correct for the case where LPOffset is an offset outside this function (LPLabel is set by Fragment->addEntryPointAtOffset(FragmentOffset)) ?
You're correct. I explain this in the last paragraph in description.
Let me clarify a bit more here:

The link between callsite and landing pad fragment is Labels[LPOffset].
If LSDA is not updated, the LPOffset will be the same, while target function
may be moved. This link is to keep connection between two fragments.
I also see its benefit on merging split function task in future.


================
Comment at: bolt/lib/Core/Exceptions.cpp:543
 
-bool CFIReaderWriter::fillCFIInfoFor(BinaryFunction &Function) const {
+void CFIReaderWriter::extractLSDAAddress(BinaryFunction &Function) const {
   uint64_t Address = Function.getAddress();
----------------
rafauler wrote:
> fillLSDAAddressFor
Addressed in another diff: D130663


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130072