Page MenuHomePhabricator

[ARM] RELA relocations for 32bit ARM ignore the addend.
AcceptedPublic

Authored by wolfgangp on Jun 30 2021, 11:02 AM.

Details

Summary

Fixes PR 50473.

The test case uses llvm-dwarfdump to verify correct resolution.

Diff Detail

Event Timeline

wolfgangp created this revision.Jun 30 2021, 11:02 AM
wolfgangp requested review of this revision.Jun 30 2021, 11:02 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 30 2021, 11:02 AM
MaskRay added inline comments.Jun 30 2021, 3:10 PM
llvm/lib/Object/RelocationResolver.cpp
314

The code is written in the assumption that targets preferring RELA always have Addend while targets preferring REL always have LocData.

Are you trying to add RELA support to ARM? I think it is unpopular but maybe you have specific needs which are not specified.

No objection, but a comment is needed.

llvm/test/DebugInfo/ARM/dwarfdump-rela.yaml
34

rela

MaskRay added inline comments.Jun 30 2021, 3:12 PM
llvm/lib/Object/RelocationResolver.cpp
314

I think adding LocData and Addend is wrong. When RELA is used, LocData (implicit addend) should be ignored.

I've not seen SHT_RELA used for debug data before and I think all of the other LLVM tools assume SHT_REL. Are you planning to add SHT_RELA support to the other LLVM tools?

llvm/lib/Object/RelocationResolver.cpp
314

Agreed. The ELF spec says in http://www.sco.com/developers/gabi/latest/ch4.reloc.html
"As specified previously, only Elf32_Rela and Elf64_Rela entries contain an explicit addend. Entries of type Elf32_Rel and Elf64_Rel store an implicit addend in the location to be modified. Depending on the processor architecture, one form or the other might be necessary or more convenient. Consequently, an implementation for a particular machine may use one form exclusively or either form depending on context."

"The typical application of an ELF relocation is to determine the referenced symbol value, extract the addend (either from the field to be relocated or from the addend field contained in the relocation record, as appropriate for the type of relocation record), apply the expression implied by the relocation type to the symbol and addend, extract the desired part of the expression result, and place it in the field to be relocated."

I've not seen SHT_RELA used for debug data before and I think all of the other LLVM tools assume SHT_REL. Are you planning to add SHT_RELA support to the other LLVM tools?

We have at least one internal test case generated by a proprietary ARM compiler that is using RELA relocations, which is handled just fine by dwarfdump (David Anderson's dwarfdump, that is), so there is some incentive for us to handle it. Also, the code I'm planning to change is used by all llvm tools, so all tools should benefit. From what I'm seeing there is SHT_RELA awareness in the other tools as well.

llvm/lib/Object/RelocationResolver.cpp
314

Ah, thanks for pointing that out. It is curious, though, because some RISCV relocations clearly seem to use both addend and location data (see here).
Accordingly, when I zero out LocData for RELA relocations, some RISCV test cases start failing. I may have to restrict zeroing out LocData to ARM platforms, which I don't particularly like.

MaskRay added inline comments.Jul 1 2021, 9:52 AM
llvm/lib/Object/RelocationResolver.cpp
314

The special RISC-V relocations are R_RISCV_SUB*. They do label differences and the explicit addend is always zero. It can be implemented by "consecutive relocations" but processing the relocation consecutively can also be correct. If anything, it should not be used as examples for other targets because it imposes many restrictions that other targets may not have.

wolfgangp updated this revision to Diff 356041.Jul 1 2021, 4:21 PM
wolfgangp marked 3 inline comments as done.

Ignore LocData for RELA relocations except for RISCV, which uses both LocData and Addend.

Updated the test case by adding non-zero data to the location to verify it is ignored.

Ping. Any comments on this more RISCV-specific approach?

llvm/test/DebugInfo/ARM/dwarfdump-rela.yaml
34

It it's OK with you, I fancy leaving the name as 'rel', just to make sure we don't key off the name of the section instead of the section type.

MaskRay added inline comments.
llvm/lib/Object/RelocationResolver.cpp
318

Add an assert

llvm/test/DebugInfo/ARM/dwarfdump-rela.yaml
25

Delete AddressAlign: 0x1

34

Delete EntSize: 0x0

34

I think it is not the test's purpose to test this, so I'd avoid it.

I am fine with this patch, but want to hear from @peter.smith.

I added @Higuoxing who may comment on the yaml2obj test.

With MaskRay's proposed addition of an assert I am happy with it.

wolfgangp marked 4 inline comments as done.

Addressed the review comments:

  • added an assert that verifies that either the addend or LocData is 0
  • renamed the relocation section into .rela.xxx
  • deleted some unnecessary section properties
MaskRay accepted this revision.Jul 14 2021, 11:40 AM
This revision is now accepted and ready to land.Jul 14 2021, 11:40 AM