The new relocation offset in .relr section patching was calculated
wrong previously. Pass the new file offset to lambda instead of
re-calculating it in it. Test removes relocation from mytext section,
so in case of wrong offset calculation we won't emit right addend value
in expected place, i.e. on the new relocation offset.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
bolt/lib/Core/BinaryEmitter.cpp | ||
---|---|---|
322–328 ↗ | (On Diff #557247) | 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? |
bolt/lib/Core/BinaryEmitter.cpp | ||
---|---|---|
322–328 ↗ | (On Diff #557247) | 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 |
bolt/lib/Core/BinaryEmitter.cpp | ||
---|---|---|
322–328 ↗ | (On Diff #557247) | 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? |
bolt/lib/Core/BinaryEmitter.cpp | ||
---|---|---|
322–328 ↗ | (On Diff #557247) | Now I see your point and you're totally right, thanks! Let me update the patch. As for non-empty functions we're just emitting CI alignment at the beginning of emitConstantIslands |
The alignment change looks good to me (with a nit). The answer to my second question lies in the way we attach a label to a constant island. For. a pure CI, i.e. the one not attached to a function, it's the function label that defines the island. Even though the island will have another FunctionConstantIslandLabel, similar to regular CIs attached to a function, this label will not be used.
Can you commit this fix separately since it's not directly related to .relr patching?
bolt/lib/Passes/Aligner.cpp | ||
---|---|---|
165 ↗ | (On Diff #557349) |
Thanks @maksfb ! Yes, that is what I mean in my previous answer that first we emit the function and it's symbol and then CI and it's symbol. Also the case where we would search not by symbol directly, rather by getBinaryFunctionAtAddress, we would end up by finding CI aligned with NOP and the beginning. I would separate it, thanks!