[LLD][ELF] make default for get{ARM,AArch64}UndefinedRelativeWeakVA unreachable
ClosedPublic

Authored by peter.smith on Tue, Jun 13, 2:17 AM.

Details

Summary

The get{ARM,AArch64}UndefinedRelativeWeakVA() functions should only be called for PC-relative relocations. Complete the supported pc-relative relocations in the switch statement and make the default case unreachable.

The R_ARM_TARGET relocation can be evaluated as R_ARM_REL32 but it is only used in the context of exception tables, and is never output with respect to a weak reference so it does not appear in the switch statement.

Diff Detail

Repository
rL LLVM
peter.smith created this revision.Tue, Jun 13, 2:17 AM
ruiu added inline comments.Tue, Jun 13, 12:36 PM
ELF/InputSection.cpp
405–406 ↗(On Diff #102304)

What does this comment explain?

437–438 ↗(On Diff #102304)

Ditto

444 ↗(On Diff #102304)

to the place ...?

I'll update the comments and move the llvm_unreachable.

ELF/InputSection.cpp
405–406 ↗(On Diff #102304)

In the ARM ABI there is a general rule for handling pc-relative relocations. There is a further special case for the subset of pc-relative instructions that are also branch relocations. I wanted to explain why the branch relocations resolved to the address of the next instruction, but the other relocations did not.

I'll update the comments to make this more explicit.

444 ↗(On Diff #102304)

I'm using the long form for P in the PC-relative relocation calculation (S + A) - P. The place is commonly used in ELF processor supplements [*], but I'll make it clearer.

For example in the AMD64 ABI:

P Represents the place (section offset or address) of the storage unit being relocated (computed using r_offset).

Updated diff with comment changes. Let me know if you want any changes, otherwise I'll commit tomorrow?

ruiu added inline comments.Wed, Jun 14, 10:37 AM
ELF/InputSection.cpp
405–406 ↗(On Diff #102304)

Thank you for adding the explanation.

So, does this comment explain return P + 2 + A? I'd move this after case instead of writing it before case.

425 ↗(On Diff #102502)

Move this after the last case too.

Updated diff to move comments and make it clearer that the +2 is for a 16-bit thumb instruction and +4 for ARM.

This revision was automatically updated to reflect the committed changes.

I've committed as r305673 as the last changes were only comment related. I'm happy to alter further if needed.