Page MenuHomePhabricator

[LLD][ELF][ARM] Fix support for SBREL type relocations
ClosedPublic

Authored by tamas.petz on Feb 14 2020, 4:29 AM.

Details

Summary

With this patch lld recognizes ARM SBREL relocations.
R_ARM*_MOVW_BREL relocations are not tested because they are not used.

Diff Detail

Event Timeline

tamas.petz created this revision.Feb 14 2020, 4:29 AM
psmith added a subscriber: psmith.Feb 14 2020, 8:20 AM

Thanks for the patch, I'll aim to take a look early next week, hopefully Monday.

The code changes look good to me. I'm unfortunately going to have be a pain and ask that you split the additional tests not related to the new relocation support into a separate change, something like "[LLD][ELF][ARM] add missing test cases for ... " that I'd be happy to approve. This helps keep tracking down failures via bisection easier.

I've added a couple more frequent reviewers to see if they have any additional comments.

For the benefit of other reviewers, reference to ELF for the ARM Architecture for definitions of the relocations https://static.docs.arm.com/ihi0044/g/aaelf32.pdf

lld/test/ELF/arm-mov-relocs.s
30

Another convention is where the address isn't significant to the line and follows on from the previous instruction is to only put the address on the first instruction. I think it is also the trend to remove the early indentation. For example:

// CHECK: 12000: movw  r0, #0
// CHECK:              movw  r1, #4
// CHECK:              movw  r2, #12
// CHECK:              movw  r3, #65532
// CHECK:              movw  r4, #0

The code changes look good to me. I'm unfortunately going to have be a pain and ask that you split the additional tests not related to the new relocation support into a separate change, something like "[LLD][ELF][ARM] add missing test cases for ... " that I'd be happy to approve. This helps keep tracking down failures via bisection easier.

I've added a couple more frequent reviewers to see if they have any additional comments.

For the benefit of other reviewers, reference to ELF for the ARM Architecture for definitions of the relocations https://static.docs.arm.com/ihi0044/g/aaelf32.pdf

Peter has mentioned most points I wanted to say. If we can think of a good category name, we can place new tests in arm-reloc-$category.s.

Use CHECK-LABEL: instead of CHECK: for section .R_* (see ppc32-reloc-addr.s). They can improve FileCheck diagnostics when something changes.

Don't just write // 0x20000 - . = 56832. It is unclear what 0x20000 refers to. In riscv-tls-*, you can see I write ## rv32: &.got[0] - . = 0x2214 - . = 4096*1+112

lld/test/ELF/arm-mov-relocs.s
21

Is the overalignment (8) significant?

tamas.petz added inline comments.Feb 18 2020, 12:28 AM
lld/test/ELF/arm-mov-relocs.s
21

Technically it is not.
It is only for readability, the different sections have their start addresses increasing by 0x100.
When I tried to follow the existing pattern where all addresses are written I found this to be useful.
It was easier to add new tests as it will happen in an upcoming patch.

tamas.petz retitled this revision from [LLD][ARM] Fix support for SBREL type relocations to [LLD][ELF][ARM] Fix support for SBREL type relocations.
tamas.petz edited the summary of this revision. (Show Details)
peter.smith accepted this revision.Feb 18 2020, 7:03 AM

Thanks for the update. Apart from the extra / needed in some of the comment strings this looks good to me. Happy for you to add those prior to commit.

lld/test/ELF/arm-mov-relocs.s
9–15

The comment doesn't involve lit commands so these should start with /// for example:

///  * R_ARM_MOVW_ABS_NC and R_ARM_MOVT_ABS
///  * R_ARM_MOVW_PREL_NC and R_ARM_MOVT_PREL
///  * R_ARM_MOVW_BREL_NC and R_ARM_MOVT_BREL
///
///  * R_ARM_THM_MOVW_BREL_NC and R_ARM_THM_MOVT_BREL
49

This type of explanatory comment would need the extra /
/// :upper16:label3 + 4 = :upper16:0x30000 = 3

This revision is now accepted and ready to land.Feb 18 2020, 7:03 AM

Thank you, Peter.
Yes, add the missing /-es and commit this change on my behalf, please.
I will add the missing test cases shortly after.

This revision was automatically updated to reflect the committed changes.