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

[Vladislav Khmelevsky via Phabricator via llvm-commits]

yota9 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);
+  }
----------------
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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D159543