Page MenuHomePhabricator

[LLD][ELF] Make createThunks() iterate until no more thunks added
ClosedPublic

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

Details

Summary

In preparation for supporting range extension thunks we now continually call createThunks() until no more thunks are added. This requires us to record
the thunks we add on each pass and only merge the new ones into the OutputSection. We also need to check if a Relocation is targeting a thunk to prevent us from infinitely creating more thunks.

With the currently supported thunks we allocate all the thunks on the first pass, the second pass will not result in any more thunks.

Diff Detail

Repository
rL LLVM

Event Timeline

peter.smith created this revision.Jun 8 2017, 7:06 AM
ruiu added inline comments.Jun 8 2017, 11:02 AM
ELF/Relocations.cpp
1098 ↗(On Diff #101907)

Does this mean you are expecting that it always converges at most two iterations? I'm not sure if that's a correct assumption.

ELF/Relocations.h
129 ↗(On Diff #101907)

Please add a comment.

Thanks for the comments. When range extension thunks are introduced we can no longer guarantee that only 2 passes will be needed so the limit will need to be raised. With the current supported thunks they will all be generated in the first pass, the second pass will find no more thunks. I've added a comment to make that explicit, and have added a comment to Passes as suggested.

ruiu edited edge metadata.Jun 13 2017, 12:42 PM

If I expect that it always converges on the first iteration, I wouldn't use a for-loop, but just call createThunks twice and assert that the second return value is always false. You can convert it to a loop later.

Thank you for all the comments. I've updated the diff to:

  • Remove the skipping of relocations if we are already a ThunkSection, as it is not needed for the current set of thunks.
  • Remove AddressesChanged.
  • Remove the loop to be replaced with two calls.

There is a type of range extension Thunk that can be used when the target is only just out of range. The contents of the thunk is just a branch instruction, which itself has a limited range and could trigger another thunk. I'm not proposing to implement this type of thunk as it adds quite a bit of additional complexity.

My plan once I've got the existing reviews accepted (D34034, D34035 and D35037) is:

  • allocate addresses before creating range thunks.
  • pre-create ThunkSections at roughly branch range intervals rather than just at the end of the executable section.
  • Add range thunk support, these will be assigned to a ThunkSection in range of the caller
  • Add script and non-script test cases

These will most likely be split up into smaller patches than I've identified here.

The code still has
TS = make<ThunkSection>(TOS, IS->OutSecOff);
so now we create an extra thunk, no?

Yes you are right. My apologies for not catching that earlier. I've updated the diff to remove it.

This revision was automatically updated to reflect the committed changes.