This is an archive of the discontinued LLVM Phabricator instance.

[LLD][ELF] Introduce target specific inBranchRange() function
ClosedPublic

Authored by peter.smith on Jun 27 2017, 7:58 AM.

Details

Summary

In preparation for range extension thunks introduce a function that will check whether a branch identified by a relocation type at a source address can reach a destination.

For targets where range extension thunks are not supported the function will return true as it is not expected that branches are out of range. An implementation has been provided for ARM.

This is patch 7/11 of range thunks although as it is not hooked up it can be applied independently.

Diff Detail

Repository
rL LLVM

Event Timeline

peter.smith created this revision.Jun 27 2017, 7:58 AM
ruiu added inline comments.Jul 11 2017, 7:02 PM
ELF/Arch/ARM.cpp
227 ↗(On Diff #104165)

In this function, you computed the exact distance between two instructions, but does this much preciseness actually needed? A few bytes difference seems negligible when you want to know whether two instructions are in +-16MiB, for example.

My preference is to be precise as I think it makes reasoning about the behavior of the linker easier. From experience with ARM's proprietary linker, which got progressively more precise the main benefits were:

  • Being precise means that the same range measurement function can be used in multiple places and is consistent with the relocation range check.
  • It makes it simpler to write test cases at the extremes of range, and also to explain to users why the linker has generated a range-thunk when the branch should have been in range. This is not a problem for Linux like systems, but is important in embedded systems with non-contiguous memory.

-It makes it simpler to track down bugs, as this feature is most likely to trigger with large complicated test cases that users are often reluctant to share. Having the range match the relocation overflow check is useful in ruling out some problems.

In summary I think it is worth going to the extra effort here although the benefits are more long-term.

ruiu accepted this revision.Jul 14 2017, 1:55 PM

LGTM

ELF/Arch/ARM.cpp
230 ↗(On Diff #104165)

nit: add blank line

265–267 ↗(On Diff #104165)

maybe return abs(Src - Dst) <= Range;?

This revision is now accepted and ready to land.Jul 14 2017, 1:55 PM
This revision was automatically updated to reflect the committed changes.

Thanks for the review, I've committed r308188. The addition of abs(unsigned - unsigned) gave me some compile warnings from clang so I decided to keep the original.