This is an archive of the discontinued LLVM Phabricator instance.

[BOLT] Support input binaries that use R_X86_GOTPC64
ClosedPublic

Authored by rafauler on Aug 25 2023, 4:30 PM.

Details

Reviewers
Amir
maksfb
Group Reviewers
Restricted Project
Commits
rG853e126ce32a: [BOLT] Support input binaries that use R_X86_GOTPC64
Summary

In large code model, the address of GOT is calculated by the
static linker via R_X86_GOTPC64 reloc applied against a MOVABSQ
instruction. In the final binary, it can be disassembled as a regular
immediate, but because such immediate is the result of PC-relative
pointer arithmetic, we need to parse this relocation and update this
calculation whenever we move code, otherwise we break the code trying
to read GOT.

A test case showing how GOT is accessed was provided.

Diff Detail

Event Timeline

rafauler created this revision.Aug 25 2023, 4:30 PM
Herald added a reviewer: maksfb. · View Herald Transcript
Herald added a project: Restricted Project. · View Herald Transcript
rafauler requested review of this revision.Aug 25 2023, 4:30 PM
Herald added a project: Restricted Project. · View Herald TranscriptAug 25 2023, 4:30 PM
maksfb added inline comments.Aug 29 2023, 12:05 PM
bolt/lib/Target/X86/X86MCSymbolizer.cpp
164

Can you encapsulate GOT MCSymbol lookup in a BinaryContext method?

rafauler added inline comments.Aug 29 2023, 2:11 PM
bolt/lib/Target/X86/X86MCSymbolizer.cpp
164

Absolutely

rafauler updated this revision to Diff 554535.Aug 29 2023, 5:35 PM

Reviewer suggestions

In this solution, we don't need to mark any symbols as globals and therefore I've removed the dependency on the parent diff "[BOLT] Local hidden should be global syms."

The consequence is that we need to special-case the recognition of the GOT symbol in two more places, in addition to the X86MCSymbolizer: analyzeRelocation() and in JITLinkLinker when resolving the address of symbols.

rafauler updated this revision to Diff 554536.Aug 29 2023, 5:47 PM

Triggering build tests again after removing parent revs

rafauler updated this revision to Diff 556509.Sep 11 2023, 5:25 PM

Fix stack alignment in testcases, which caused libc's puts() stack to
crash in ubuntu systems because libc was using movaps in malloc
initialization.

maksfb added inline comments.Oct 2 2023, 3:09 PM
bolt/include/bolt/Core/BinaryContext.h
876 ↗(On Diff #556509)

nit:

879 ↗(On Diff #556509)
maksfb added a comment.Oct 2 2023, 3:30 PM

Thanks for adding one more test case.

bolt/lib/Rewrite/JITLinkLinker.cpp
134 ↗(On Diff #556509)

When can GOT be a jump table?

bolt/lib/Target/X86/X86MCSymbolizer.cpp
167
176
183–184

Should we just always take this path instead of searching for the matching name?

bolt/test/runtime/X86/gotoff-large-code-model-2.s
10 ↗(On Diff #556509)
bolt/test/runtime/X86/gotoff-large-code-model.s
8

nit:

rafauler updated this revision to Diff 557543.Oct 2 2023, 4:43 PM

Suggestions

@maksfb thanks for the review

rafauler updated this revision to Diff 557544.Oct 2 2023, 5:55 PM

Forgot to update references to renamed functions

maksfb accepted this revision.Oct 2 2023, 6:59 PM

LGTM

bolt/lib/Rewrite/JITLinkLinker.cpp
147 ↗(On Diff #557544)
This revision is now accepted and ready to land.Oct 2 2023, 6:59 PM
This revision was automatically updated to reflect the committed changes.