This is an archive of the discontinued LLVM Phabricator instance.

[BOLT] Fix plt relocations symbol match
ClosedPublic

Authored by yota9 on Mar 18 2022, 1:57 PM.

Details

Summary

The BFD linker adds the symbol versioning string to the symbol name in symtab.
Skip the versioning part in order to find the registered PLT function.

Vladislav Khmelevsky,
Advanced Software Technology Lab, Huawei

Diff Detail

Event Timeline

yota9 created this revision.Mar 18 2022, 1:57 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 18 2022, 1:57 PM
Herald added a subscriber: ayermolo. · View Herald Transcript
yota9 requested review of this revision.Mar 18 2022, 1:57 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 18 2022, 1:57 PM
Amir added a comment.Mar 21 2022, 6:50 AM

Can you please follow the example of compiler-rt to check that bfd is available on the test system and only enable the new test if it is?
Check here: compiler-rt/cmake/config-ix.cmake: GNU_LD_EXECUTABLE
And here: compiler-rt/test/lit.common.cfg.py: config.available_features.add('binutils_lto')

bolt/lib/Rewrite/RewriteInstance.cpp
1890

Can you please add an example of a full symbol name with versioning?

bolt/test/runtime/AArch64/plt.c
0

Can you please keep lld here and add invocation with bfd below as an extra test?

yota9 marked 2 inline comments as done.Mar 22 2022, 7:15 AM
yota9 added inline comments.
bolt/lib/Rewrite/RewriteInstance.cpp
1890

Sure

yota9 updated this revision to Diff 417291.Mar 22 2022, 7:16 AM
yota9 marked an inline comment as done.

Add gnu_ld check + separate test

maksfb added inline comments.Mar 22 2022, 11:19 AM
bolt/test/runtime/plt_gnu_ld.test
10 ↗(On Diff #417291)

Hi @yota9, could you please make the checks for properly updated PLT explicit instead of relying on executing the binary?

maksfb added inline comments.Mar 22 2022, 12:07 PM
bolt/lib/Rewrite/RewriteInstance.cpp
1886–1889

Could you please factor out the code above? Why is it AArch64-specific?

yota9 added inline comments.Mar 22 2022, 12:23 PM
bolt/lib/Rewrite/RewriteInstance.cpp
1886–1889

Sorry you want me to remove IsAArch64 check? I didn't check it on X86. AFAIU there is no need for x86 to have this code since this is used for aarch64 because there are instruction pairs like adrp + add, which give us the final symbol value in pair, but the X86 may use extracted value for single instruction

bolt/test/runtime/plt_gnu_ld.test
10 ↗(On Diff #417291)

I will try t

10 ↗(On Diff #417291)

Sorry, something went wrong here :) I will try to do it soon :)

yota9 updated this revision to Diff 418207.Mar 25 2022, 6:29 AM

Update tests, move PLT symbol search code to separate function

Summary: s/linkeradds/linker adds/ and s/symbtab/symtab/

bolt/include/bolt/Core/BinaryContext.h
779

nit: could you use PLT (all caps) in function names to be consistent with the rest of the code in BOLT?

yota9 marked an inline comment as done.Mar 27 2022, 12:17 PM
yota9 added inline comments.
bolt/include/bolt/Core/BinaryContext.h
779

Yes, sure, I always forget about PLT.. :D

yota9 updated this revision to Diff 418464.Mar 27 2022, 12:24 PM
yota9 marked an inline comment as done.

Address comments

yota9 edited the summary of this revision. (Show Details)Mar 27 2022, 12:24 PM

@yota9, regarding the tests - will we be able to detect the change in behavior on non-aarch64 (x86) platform? I.e. will any of the tests fail without the fix?

@maksfb I see your point. From the one hand we have a fix. From the other hand the tests are common for x86 and aarch64, i.e. it doesn't check the fix, it checks the overall possibility to work with PLT properly. Ideally we should have aarch64 machine available for testing, we've got this task in the backlog and Elvina should start investigation on aarch64 buildbot soon. As for now I would like these runtime tests to exist, but I will add one more non-runtime aarch64-specific test (that is using BOLT asm dump) so we will be able to check it on non-arm platform

Great. Adding non-runtime test sounds good to me.

yota9 updated this revision to Diff 419254.Mar 30 2022, 1:29 PM

Add non-runtime tests

@yota9, sorry, AArch64/plt_gnu_ld.test wouldn't build on x86. Could you use YAML and yaml2obj for the test?

yota9 updated this revision to Diff 419557.Mar 31 2022, 1:21 PM

Add BFD linked yaml test file

yota9 marked an inline comment as done.Mar 31 2022, 1:23 PM

@yota9, sorry, AArch64/plt_gnu_ld.test wouldn't build on x86. Could you use YAML and yaml2obj for the test?

Done :)

Could you please rename test files to use "-" instead of "_"?

bolt/test/AArch64/plt_gnu_ld.test
7 ↗(On Diff #419557)

I tried that fix locally and got "no PT_LOAD pheader seen" error.

bolt/test/AArch64/plt_lld.test
4 ↗(On Diff #419557)

plt.c will not cross-compile since it includes header files.

yota9 marked 2 inline comments as done.Apr 1 2022, 11:55 AM
yota9 added inline comments.
bolt/test/AArch64/plt_gnu_ld.test
7 ↗(On Diff #419557)

The old file was remained on the pass, thanks for the remark.

yota9 updated this revision to Diff 419824.Apr 1 2022, 11:55 AM
yota9 marked an inline comment as done.

@maksfb Thanks done

maksfb added a comment.Apr 1 2022, 8:13 PM

AArch64/plt-lld.test is still broken.

yota9 added a comment.Apr 2 2022, 4:50 AM

@maksfb I don't see the reason, everything works for me. Do you use this patch with your aarch64 tests changes? The nostdlib flag should be passed from your patch, I don't see any other problem to run the test on x86.

yota9 updated this revision to Diff 419975.Apr 2 2022, 6:40 AM

@maksfb You were right, after rebase on your patch I've got the problems too.
I was sure the undefined symbols with unresolved-symbols=ignore-all option will create PLT entry by using symbol name, but it does not.
Anyway we will just create stub library now for the testing purposes and the problem will gone.

yota9 updated this revision to Diff 419994.Apr 2 2022, 9:39 AM

nit header protection

maksfb accepted this revision.Apr 4 2022, 11:38 PM
This revision is now accepted and ready to land.Apr 4 2022, 11:38 PM
This revision was automatically updated to reflect the committed changes.