This is an archive of the discontinued LLVM Phabricator instance.

[ARM][LLD] .ARM.exidx support for executables/shared libraries
ClosedPublic

Authored by peter.smith on Sep 30 2016, 2:03 PM.

Details

Summary

This is the first part of a series of patches derived from https://reviews.llvm.org/D24728

Apologies for the delay in posting this, I wanted to wait for Phab to come back up.

In this patch support for .ARM.exidx sections has been added for Executables and Shared Libraries. The majority of the code is identical to the changes discussed in D24788 with the following differences:

  • Garbage collection is fully implemented and tested, .ARM.exidx is live only when the section it depends on is live.
  • The SHF_LINK_ORDER flag on the .ARM.exidx OutputSection is cleared for Executables and Shared libraries as it only has meaning in a relocatable object.
  • I've moved the code to find the link order dependency to InputSection from OutputSection as MarkLive needs it, and in a future patch for relocatable links the OutputSection::finalize() will also need it.

Follow up patches to come shortly:

  • Implement R_ARM_TARGET2 relocation.
  • Allow R_ARM_PREL31 and R_ARM_NONE relocations to be used in shared objects.
  • Maintain the full section name when doing a relocatable link, for example .text.foo should not be truncated and merged with other .text sections.
  • Add support for .ARM.exidx for relocatable links.

Diff Detail

Event Timeline

peter.smith retitled this revision from to [ARM][LLD] .ARM.exidx support for executables/shared libraries.
peter.smith updated this object.
peter.smith added reviewers: ruiu, rafael, grimar.
peter.smith added a subscriber: llvm-commits.

Updated diff to fix spurious extra newline, apologies for not catching first time.

ruiu edited edge metadata.Sep 30 2016, 3:30 PM

Generally looking good, a few minor comments.

ELF/InputSection.cpp
131

typename ELFT::Shdr -> ELF_Shdr

ELF/MarkLive.cpp
257–272 ↗(On Diff #73138)

Don't you have to repeat this process until it converges? I wonder if forEachSuccessor() makes more sections live, which in turn makes more .ARM.exidx sections live.

ELF/OutputSections.cpp
882–883

This is equivalent to this.

if (!Config->Relocatable)
  this->Header.sh_flags &= ~SHF_LINK_ORDER;
ELF/Writer.cpp
1410

Exidx -> exidx

1449

Remove the blank line.

1482

Remove {}

1483

OS -> *OS

peter.smith updated this revision to Diff 73282.Oct 3 2016, 7:51 AM
peter.smith edited edge metadata.

Thanks for the comments. I've applied all suggested changes.

ruiu added inline comments.Oct 3 2016, 5:13 PM
ELF/MarkLive.cpp
259 ↗(On Diff #73282)

I wonder if there's a better way to do this. How about adding

TinyPtrVector<InputSection<ELFT> *> Successors;

to InputSection<ELFT> class, adding reverse edges from .ARM.exidx sections to other sections to that vector as a preprocess, then handle that vector elements as successors in forEachSucessor? Then it is guranteed to converge by calling forEachSucessor<ELFT>(*Q.pop_back_val(), Enqueue) just once.

peter.smith updated this revision to Diff 73437.Oct 4 2016, 2:40 AM

Split out garbage collection changes from this patch on Rafael's request which didn't get copied into the review:
"Can you split this further?

It would be nice to have a patch taking care of only which sections
are gced. That is, it just makes sure that we keep only the sections
that refer to a kept section.

Cheers,
Rafael"

The only differences to the previous version are the removal of changes to MarkLive.cpp and the removal of the corresponding test that depended on it.

I'll work on Rui's suggestion for garbage collection in a separate review. Will hopefully have something later today.

Hope I've interpreted everyone correctly.

ruiu added a comment.Oct 5 2016, 12:55 PM

Do you think you can reduce the testcase for this patch too?

ELF/Writer.cpp
1448

Sort -> sort

peter.smith updated this revision to Diff 73758.Oct 6 2016, 4:14 AM

Thanks for the comments. I've made the case change to the function name and removed unnecessary assembly language directives from the tests.

I'll wait for this one before committing the GC change in D25234

ruiu accepted this revision.Oct 7 2016, 1:07 PM
ruiu edited edge metadata.

LGTM

This revision is now accepted and ready to land.Oct 7 2016, 1:07 PM
peter.smith closed this revision.Oct 10 2016, 2:54 AM

Committed revision 283730

Happy to make further changes if needed. The patch above applies with only line number offset differences and already has the garbage collection changes split out.

ELF/MarkLive.cpp
257–272 ↗(On Diff #73138)

Sadly yes for one annoying case. The personality routines such as __gxx_personality_v0 that do the unwinding are only referenced from .ARM.extab sections and in at least the gcc libraries these routines have .ARM.exidx. I've updated the code and test, which to my shame illustrated that the .ARM.exidx entries for my personality routines were being removed.

In the general case .ARM.exidx only has relocations to one executable section (the one it has a link order dependency on). It may refer to a .ARM.extab section which contains unwinding instructions that won't fit into 4 bytes. The .ARM.extab section may refer to personality routines which are the functions that interpret the data, but no other executable sections.