[PATCH] D159543: [BOLT] Fix .relr section addend patching

[Maksim Panchenko via Phabricator via llvm-commits]

maksfb added inline comments.


================
Comment at: bolt/lib/Core/BinaryEmitter.cpp:322-328
+  if (Function.size() == 0 && Function.hasIslandsInfo()) {
+    // If function has only constant island emit alignment before emitting
+    // symbols, so the emitted nops (if any) won't be part of the constant
+    // islands data and we would address data correctly.
+    const uint16_t Alignment = Function.getConstantIslandAlignment();
+    Streamer.emitCodeAlignment(Align(Alignment), &*BC.STI);
+  }
----------------
yota9 wrote:
> maksfb wrote:
> > The code in `AlignerPass` tries to address the same issue: https://github.com/llvm/llvm-project/blob/b7961f2cb97556bfc50f7828d5f869d96ab9352e/bolt/lib/Passes/Aligner.cpp#L166-L171
> > 
> > That code doesn't work because having 64-byte alignment with max 8-byte alignment will often do nothing.
> > 
> > 
> > What if the function is not empty, would we have a similar issue with alignment issued before the symbol?
> If the function is non-empty we would emit the function first, then align the CI emitting NOPs to the end of the current function and then emit CI symbols, so I don't expect any problems here since we're not addressing CI by function symbols/offset. On the other hand this condition is used for pure data in code, and currently emitting its symbols first and then aligning the data results in the problems, that is why we need this condition to be here
I understand the need for the fix, but I don't follow why we have the new code in `BinaryEmitter` fixing the broken functionality in `AlignerPass`. Could you also point to the code that works for non-empty functions and why we can't make it work for empty functions?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D159543