Page MenuHomePhabricator

[LLD][ELF] Introduce range extension thunks for ARM
ClosedPublic

Authored by peter.smith on Jun 27 2017, 8:06 AM.

Details

Summary

This change adds initial support for range extension thunks. All thunks must be created within the first pass so some corner cases are not supported. A follow up patch will add support for multiple passes.

With this change the existing tests arm-branch-error.s and arm-thumb-branch-error.s now no longer fail with an out of range branch. These have been renamed and tests added for the range extension thunk.

This is patch 8/11 of range thunks. It is dependent on all previous patches D34690, D34689, D34688 and their transitive dependencies

Diff Detail

Repository
rL LLVM

Event Timeline

peter.smith created this revision.Jun 27 2017, 8:06 AM

Rebased, no other differences.

ruiu added inline comments.Jul 11 2017, 6:17 PM
ELF/Relocations.cpp
1020 ↗(On Diff #105402)

Does this mean that this would use the upper bound if a referrer's address is greater than a thunk? Is it guaranteed that a thunk for this symbol is created within a range this way?

peter.smith added inline comments.Jul 12 2017, 7:29 AM
ELF/Relocations.cpp
1020 ↗(On Diff #105402)

If I'm reading the expression correctly; if the address of the referrer is greater than the ThunkSection we can insert a Thunk into the ThunkSection if we are in range of its lower bound. If the address of the referrer is lower than that of the ThunkSection we need to be in range of its upper bound.

Given that we process referrers and ThunkSections in ascending order of address this should mean that on that pass we are in range.

referrer low (want to check that we are in range of upper bound of ThunkSection)
...
ThunkSection Base (TSBase lower bound)
    Thunks
ThunkSection Limit (TSLimit upper bound)
...
referrer high (want to check that we are in range of lower bound of ThunkSection)

Rebased diff on top of changes to D34689.

I'm planning to see how many test cases that I can take from D31665 and D31666 and will add them here as soon as I can.

Updated to add tests from D31665 and D31666.

Rebase to account for r308056 and r308057, no other changes.

Rebased to account for r309311 Merge OutputSectionCommand and OutputSection. No other changes.

Rebased to account for changes in D34689

smeenai added inline comments.Sep 7 2017, 11:04 PM
ELF/Arch/ARM.cpp
239 ↗(On Diff #114183)

You should use LLVM_FALLTHROUGH.

250 ↗(On Diff #114183)

Same here.

ELF/Relocations.cpp
1031 ↗(On Diff #114183)

This assumes that IS->OutSecOff is in range of Src, correct?

If I understand correctly, you can insert a ThunkSection at the start or the end of an InputSection, but not in the middle. Over here, should we instead check if the start and the end of IS are in range of Src, use whichever one is in range, and raise an error if neither of them is?

1043 ↗(On Diff #114183)

The changes in this function are just an NFC variable rename, and don't seem super related to this patch. You could upload them separately (or even just commit them directly).

1147 ↗(On Diff #114183)

Nits: The comma should be a semicolon, and there should be a period at the end.

1148–1149 ↗(On Diff #114183)

Did clang-format do this? The combined line is under 80 chars, so I'm not sure why it would be split.

ELF/Thunks.cpp
249 ↗(On Diff #114183)

Are the changes in this function related to this diff?

ELF/Writer.cpp
1392 ↗(On Diff #114183)

This comment should be updated.

peter.smith marked 7 inline comments as done.

I've updated the diff with the review changes and a couple of new tests.

ELF/Relocations.cpp
1031 ↗(On Diff #114183)

Agreed, I've made the change and I've added a couple of test cases for it.

1043 ↗(On Diff #114183)

I've extracted these into D37627

1148–1149 ↗(On Diff #114183)

I think I had run clang-format on the previous and next patch but not this one. I've now run it on this one.

ELF/Thunks.cpp
249 ↗(On Diff #114183)

Yes they are. Thunks were previously only supported for branch (B, B.w) instructions as these cannot be transformed into a BLX for interworking. The change here permits BL and BLX instructions to have Thunks.

ELF/Writer.cpp
1392 ↗(On Diff #114183)

I've removed it.

Rebase to account for changes In D34689 and D37627, no other changes.

Rebased on top of refactoring change D37743. No other changes.

pcc added a subscriber: pcc.Oct 10 2017, 4:47 PM
pcc added inline comments.
ELF/Arch/ARM.cpp
239 ↗(On Diff #114835)

This line needs a semicolon.

250 ↗(On Diff #114835)

Same.

Thank you for pointing out the missing semicolons. I have updated the diff and rebased in light of recent refactoring changes.

Rebased in light of RelType refactoring, no other changes.

phosek added a subscriber: phosek.Oct 16 2017, 4:36 PM
ruiu accepted this revision.Oct 25 2017, 8:08 PM

LGTM

ELF/Relocations.h
144 ↗(On Diff #118766)

Can you use RelType instead of uint32_t?

This revision is now accepted and ready to land.Oct 25 2017, 8:08 PM
This revision was automatically updated to reflect the committed changes.