This is an archive of the discontinued LLVM Phabricator instance.

[BOLT] Fix .relr section addend patching
ClosedPublic

Authored by yota9 on Sep 22 2023, 8:50 AM.

Details

Summary

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.

Diff Detail

Event Timeline

yota9 created this revision.Sep 22 2023, 8:50 AM
yota9 requested review of this revision.Sep 22 2023, 8:50 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 22 2023, 8:50 AM
yota9 edited the summary of this revision. (Show Details)Sep 22 2023, 10:00 AM
maksfb added inline comments.Sep 25 2023, 5:07 PM
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?

yota9 added inline comments.Sep 25 2023, 8:58 PM
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

maksfb added inline comments.Sep 25 2023, 9:07 PM
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?

yota9 added inline comments.Sep 25 2023, 10:07 PM
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

yota9 updated this revision to Diff 557349.Sep 25 2023, 10:12 PM

Update aligner pass

yota9 edited the summary of this revision. (Show Details)Sep 25 2023, 10:13 PM

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!

yota9 updated this revision to Diff 557386.Sep 26 2023, 11:26 PM

Separate changes

yota9 edited the summary of this revision. (Show Details)Sep 26 2023, 11:27 PM
yota9 updated this revision to Diff 557388.Sep 26 2023, 11:30 PM

Separate changes

maksfb accepted this revision.Sep 27 2023, 2:22 PM

LGTM. Thanks!

This revision is now accepted and ready to land.Sep 27 2023, 2:22 PM
This revision was automatically updated to reflect the committed changes.