This is an archive of the discontinued LLVM Phabricator instance.

[LLD][ARM] Garbage collection support for .ARM.exidx sections
ClosedPublic

Authored by peter.smith on Oct 4 2016, 6:08 AM.

Details

Summary

The .ARM.exidx sections have a reverse dependency on the InputSections that they have a link order dependency to. In effect they should be live only if the InputSection they refer to via the sh_link field is live.

I've used a single pointer to InputSection rather than a vector as there is an InputSection can only have at most one InputSection (.ARM.exidx) with a SHF_LINK_ORDER dependency on it.

This has been split off from https://reviews.llvm.org/D25127 which contains the basic .ARM.exidx recognition and sorting. It implements Rui's suggestion of adding the reverse dependency to InputSections.

The lld changes don't depend on D25127 but the test case may do as it checks the contents of the .ARM.exidx section.

Diff Detail

Event Timeline

peter.smith updated this revision to Diff 73467.Oct 4 2016, 6:08 AM
peter.smith retitled this revision from to [LLD][ARM] Garbage collection support for .ARM.exidx sections.
peter.smith updated this object.
peter.smith added reviewers: ruiu, rafael.
peter.smith added a subscriber: llvm-commits.
ruiu added inline comments.Oct 4 2016, 1:29 PM
ELF/InputFiles.cpp
259

Can you make this a separate function, say, initializeARMLinkOrderSection, and call it from ObjectFile::parse?

Please do this only when Config->GcSections is true.

ELF/InputSection.h
232

Variable names should start with uppercase letters.

Is it guaranteed that a section has at most one corresponding .ARM.exidx section?

test/ELF/arm-exidx-gc.s
13

I don't know much about ARM assembly, but do you actually need this amount of assembly code to test your feature?

peter.smith updated this revision to Diff 73605.Oct 5 2016, 2:28 AM

Thanks for the comments. I've made the suggested review changes.

Yes it is guaranteed that there is a 1:1 mapping between a .ARM.exidx section and an executable section. It is stated in the exceptions spec[*] and if it weren't true then SHF_LINK_ORDER couldn't work reliably.

I've cut the assembly down a bit in the test. The difficulty often isn't the ARM assembly, it is provoking the assembler to generate the necessary exception tables. For example .cantunwind and .handlerdata need .fnstart and .fnend to be accepted.

An object producer must generate:

  • One fragment of index table for each code section.
  • One exception-handling table entry corresponding to each function that may need to be unwound.

Each fragment of index table (read-only data) must be generated in its own ELF section. It must contain an index entry for each non-leaf function in the associated code section, in the same order as that of the functions in the code section. The index table section name must be .ARM.exidx optionally followed by further characters. The section type must be SHT_ARM_EXIDX (see [AAELF]). It must have the SHF_LINK_ORDER flag set in the sh_flags field of its section header and be linked to its associated code section via the sh_link field of its section header.

ruiu accepted this revision.Oct 5 2016, 12:47 PM
ruiu edited edge metadata.

LGTM

ELF/InputFiles.cpp
148

clang-format-diff please.

287

Reverse the condition and continue earlier.

291

It is not expected to fail, so dyn_cast -> cast.

This revision is now accepted and ready to land.Oct 5 2016, 12:47 PM
rafael edited edge metadata.Oct 6 2016, 12:02 PM

Why do you need to store the dependency?

It seem that you should be able to just run mark sections live normally and then do one last pass over the sections. Any SHF_LINK_ORDER pointer to a live section makes the current one live too.

The initial patch did start that way. The SHF_LINK_ORDER sections that become live may make other sections that have further SHF_LINK_ORDER dependencies live so it ends up needing to converge. The move to adding the reverse dependency came from an earlier review comment.

I'll try and see what impact the extra field in InputSection has in terms of performance. I've not got a setup for benchmarking right now but I'll take a look at setting one up.

Another possibility is to store the reverse dependency in a map so that only the ARM Target pays the performance cost.

ruiu added a comment.Oct 7 2016, 1:10 PM

We need to store reverse edges somewhere. It doesn't have to be InputSection, but I think this is a good place to add them.

peter.smith closed this revision.Oct 10 2016, 3:22 AM

Committed revision 283734 with Rui's final suggestions above.