This is an archive of the discontinued LLVM Phabricator instance.

[BOLT] Add .relr.dyn section support
ClosedPublic

Authored by yota9 on Mar 14 2023, 1:10 PM.

Details

Summary

Vladislav Khmelevsky,
Advanced Software Technology Lab, Huawei

Diff Detail

Event Timeline

yota9 created this revision.Mar 14 2023, 1:10 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 14 2023, 1:10 PM
yota9 requested review of this revision.Mar 14 2023, 1:10 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 14 2023, 1:10 PM

We have a build error in premerge checks:

/var/lib/buildkite-agent/builds/llvm-project/bolt/lib/Core/Relocation.cpp:644:36: error: function definition is not allowed here
uint64_t Relocation::getRelative() {

yota9 updated this revision to Diff 505384.Mar 15 2023, 12:25 AM

Fix merge

Code looks good. A few comments.

bolt/lib/Rewrite/RewriteInstance.cpp
2393

for -> in

2421

assert that DynamicRelrSize is a multiple of DynamicRelrEntrySize?

2423–2439

This has untested code because our testcase currently is very simple and doesn't have entries with bitmaps. Can we come up with a slightly more complicated testcase, just for relr, that exercises this decoding mechanism? (I'm not saying the code is wrong, it's just nice to have a testcase in case we ever change this and need to verify that it still works as intended).

5462

Is it possible to have some relocations in input RELA and some others in RELR? e.g. does the RELR extension allow for this?

5691

I guess you also rely on DynamicRelrEntrySize being non-zero for this patch to work, so maybe check here too, in addition to DynamicRelrAddress and DynamicRelrSize.

yota9 marked 5 inline comments as done.Mar 16 2023, 8:18 AM
yota9 added inline comments.
bolt/lib/Rewrite/RewriteInstance.cpp
2421

Would add check on dynamic reading

2423–2439

Extended the testcase a bit

5462

I was thinking about this too, but for now I don't see a reason a linker to make such a strange thing. Theoretically it is possible, but in practice - I don't think so. Even if I don't think we can currently handle such situations since we're not preserving input section information for each relocation. The same problem might appear when patching RELA and JMPREL relocations, currently we're saving only which type goes where, but theoretically there might be a problem e.g. different linkers might place some TLS relocations in any of these sections. When and if we would support rewriting binary and we would be able to remove old relocation section and create new we would be able to handle such cases, but for now I think the logic would stay as-is..

5691

Would add error check

yota9 updated this revision to Diff 505819.Mar 16 2023, 8:22 AM
yota9 marked 4 inline comments as done.

Address comments

rafauler accepted this revision.Mar 16 2023, 2:52 PM

LGTM with nits:

bolt/lib/Rewrite/RewriteInstance.cpp
5695

expected
presented -> present

5696

\n

5699

Expected -> expected

5700

\n

This revision is now accepted and ready to land.Mar 16 2023, 2:52 PM
This revision was automatically updated to reflect the committed changes.