This is an archive of the discontinued LLVM Phabricator instance.

[lldb][ObjectFileELF] Support AArch32 in ApplyRelocations
ClosedPublic

Authored by sgraenitz on Apr 5 2023, 10:11 AM.

Details

Summary

Allow the ObjectFileELF plugin to resolve R_ARM_ABS32 relocations from AArch32 object files. This fixes https://github.com/llvm/llvm-project/issues/61948

The existing architectures work with RELA-type relocation records that read addend from the relocation entry. REL-type relocations in AArch32 store addend in-place.
The new function doesn't re-use ELFRelocation::RelocAddend32(), because the interface doesn't match: in addition to the relocation entry we need the actual target section memory.

Diff Detail

Event Timeline

sgraenitz created this revision.Apr 5 2023, 10:11 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 5 2023, 10:11 AM
sgraenitz requested review of this revision.Apr 5 2023, 10:11 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 5 2023, 10:11 AM
Herald added a subscriber: MaskRay. · View Herald Transcript

Adding an abstraction for reading implicit addend or somehow integrate it into ELFRelocation::RelocAddend32() might be more confusing for the reader than keeping it in the function inline. What do you think?

lldb/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp
2660

IIUC we'd want to account for an endianness difference between debugger and target (in theory). However, non of the other cases seems to do it, so I didn't start with it either.

2712

In think debug info relocations have been R_ARM_ABS32 in all ARM/Thumb, pic/non-pic variations I tried. @peter.smith Does that hold in general? Otherwise, I'd like to report errors for the other cases and not let them run into the assert(false) below.

sgraenitz updated this revision to Diff 511151.Apr 5 2023, 10:31 AM

Add the missing 5th relocation to the test that exercises the out-of-range case

sgraenitz updated this revision to Diff 511155.Apr 5 2023, 10:57 AM

Second try for the out-of-range test

From the Arm side this looks good. Strictly speaking R_ARM_ABS32 doesn't require overflow checking, but in this context adding it makes sense. It is definitely the case that addends are signed in Arm, I guess there may be a 32-bit ABI that has unsigned REL addends, but I guess support can be added for that when needed.

lldb/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp
2640

IIUC the largest difference between this and ApplyELF64ABS32Relocation is the use of REL rather than RELA relocations. Perhaps worth naming it as ApplyELF32ABS32RelRelocation

sgraenitz updated this revision to Diff 513178.Apr 13 2023, 5:10 AM
sgraenitz marked an inline comment as done.

Rename new function to ApplyELF32ABS32RelRelocation()

sgraenitz updated this revision to Diff 513179.Apr 13 2023, 5:12 AM

Add error reporting for R_ARM_REL32 relocations

Addressed feedback and added handling for R_ARM_REL32. I didn't see a real-world case for it yet. Hope it's ok to have the error reported for the time being.

lldb/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp
2640

Thanks, very good point. Done.

Double-checked and found source locations on Raspberry Pi 3b with this patch applied https://github.com/llvm/llvm-project/issues/61948#issuecomment-1508305542
Is there more feedback on this? It would be nice to land it soon, i.e. before EuroLLVM =)

labath accepted this revision.Apr 28 2023, 4:47 AM
labath added inline comments.
lldb/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp
2660

We probably should.
What we also should do (and what other cases seem to get mostly right) is avoid dereferencing type-punned pointers (use memcpy to read).

2663
This revision is now accepted and ready to land.Apr 28 2023, 4:47 AM
sgraenitz updated this revision to Diff 518167.Apr 29 2023, 8:31 AM
sgraenitz marked an inline comment as done.

Address feedback and rebase

This revision was landed with ongoing or failed builds.Apr 29 2023, 8:35 AM
This revision was automatically updated to reflect the committed changes.