Page MenuHomePhabricator

[BOLT] Search section based on relocation symbol
ClosedPublic

Authored by yavtuk on Feb 1 2023, 8:28 AM.

Details

Summary

We need to search referenced section based on relocations symbol section
to properly match end section symbols. For example on some binaries we
can observe that init_array_end/fini_array_end might be "placed" in to
the gap and since no section could be found for address the relocation
would be skipped resulting in wrong ADRP imm after emitting new text
resulting in binary sigsegv.

Credits for the test to Vladislav Khmelevskii aka yota9.

Diff Detail

Event Timeline

yota9 created this revision.Feb 1 2023, 8:28 AM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 1 2023, 8:28 AM
yota9 requested review of this revision.Feb 1 2023, 8:28 AM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 1 2023, 8:28 AM
yota9 added a comment.Feb 2 2023, 1:00 PM

Gentle reminder

Thanks for the fix! Overall looks good, just a question on the test itself

bolt/test/AArch64/array_end.c
31

Could you move REQUIRES, RUN, CHECK line to line 5, at the top of this file, for consistency with other checks?

32–33

Is it possible to prune the script to only the relevant lines that trigger the problematic behavior?

yota9 marked 2 inline comments as done.Feb 6 2023, 12:14 PM
yota9 added inline comments.
bolt/test/AArch64/array_end.c
32–33

Sure, I was thinking to check init_array_end symboll too, but there is a problem that bolt creates symbols using address only without respecting section belonging. E.g init_array_end symbol address == __fini_array_start , so on disassemble we might get wrong symbol names. It is not currently a big issue, but in case we would move (in this exmaple) fini_array section separately from init_array - it might be the problem. Just you to know, but it is out of scope of this patch.

yota9 updated this revision to Diff 495248.Feb 6 2023, 12:23 PM
yota9 marked an inline comment as done.

Address comments

yavtuk commandeered this revision.Feb 6 2023, 12:32 PM
yavtuk added a reviewer: yota9.
yavtuk edited the summary of this revision. (Show Details)Feb 6 2023, 12:36 PM
rafauler added inline comments.Feb 6 2023, 2:52 PM
bolt/test/AArch64/Inputs/array_end.ld_script
2–5 ↗(On Diff #495248)

Probably worth asking on discourse if there are any legal implications of checking in a GNU linkerscript into LLVM's codebase, since GNU works with a different license

yota9 updated this revision to Diff 495287.Feb 6 2023, 3:18 PM

@rafauler I've found out that we can make small linker scripts for lld, it uses it as a hint and adds missing sections by it self.

rafauler accepted this revision.Feb 6 2023, 6:15 PM

LGTM

This revision is now accepted and ready to land.Feb 6 2023, 6:15 PM