Page MenuHomePhabricator

[LLD][ELF][ARM] Redesign of .ARM.exidx handling to use a SyntheticSection
ClosedPublic

Authored by peter.smith on Mar 11 2019, 9:15 AM.

Details

Summary

In D58047 Synthesise missing .ARM.exidx sections. Rui made the comment

We used to handle .ARM.exidx sections as regular sections with a sentinel section that is synthesized and added to the end. We later added code to merge contiguous .ARM.exidx sections, and now this patch is adding sentinels at various points. At this point, maybe it is easier to handle .ARM.exidx as a single synthetic section just like MergeInputSection? That synthetic section removes all input section whose name is .ARM.exidx from the input section list and add itself to the input section. That design gives you more flexibility than the current design, as the intermediate data representation doesn't have to be an InputSection.

This is an implementation of the idea to handle .ARM.exidx processing inside a single SyntheticSection. It incorporates the changes in D58047 and reuses the test modifications. There is one additional test change to arm-data-prel.s to account for the single SyntheticSection. I've put what I think the advantages and disadvantages of the approach are below. At this stage I've tested it on the LLD test suite, I'm going to do some more testing on real world programs to make sure I haven't made any mistakes, but I think the approach can be made to work.

Assumptions:

  • We can't get rid of SHF_LINK_ORDER as this is used by the sanitizers
  • There is at most one .ARM.exidx OutputSection per link unit. This assumption may be broken with pcc's executable split into shared libraries proposal, but I think that this can be expanded to cover that case).

Requirements:

  • .ARM.exidx InputSections form a single contiguous table with each entry a pair (Offset to function, Unwind instructions for function).
  • The table must be ordered in the OutputSection according to the ascending order of function address.
  • The address range of a table entry is [Address of function, Address of next function in table)
  • The linker should synthesise table entries for functions that don't have them in order to terminate their address range.
  • The linker should synthesise a terminating sentinel entry as some unwinders require it.
  • The exidx_start and exidx_end symbols must be defined if referenced at the start and end of the table.
  • A PT_ARM_EXIDX program header is created to describe the location of the contiguous range of .arm.exidx sections
  • All .ARM.exidx InputSections including any synthetic sections can be DISCARDED

Design:
Instead of working backwards from the .ARM.exidx InputSections to the executable sections, work forwards from the executable sections in the output to find the .ARM.exidx sections in their dependentSections. This permits us to synthesize the EXIDX_CANTUNWIND range terminators and the sentinel in the table without having to create a new type of SyntheticSection to represent the linker created sections.

Changes:

  • Writer.cpp: Instead of creating an ARMExidxSentinelSection we create an ARMExidxSyntheticSection that marks all the .ARM.exidx InputSections as not live.
  • Writer.cpp: At SHF_LINK_ORDER processing time we call the finalizeContent() member function of ARMExidxSyntheticSection to perform the ordering and table compression.
  • SyntheticSections.cpp: The writeTo() member function of the ARMExidxSyntheticSection needs to derive the table contents from the Executable InputSections.

Advantages:

  • The vast majority of the ARM specific code is in ARMExidxSyntheticSection.
  • We don't need to create multiple range terminating sentinel sections.

Disadvantages:

  • To avoid duplicating a lot of the relocation code writeTo() takes advantage of the .ARM.exidx table structure. Modifying it may need more ARM specific knowledge.

I've kept D58047 open as it may be preferable to keep the old design rather than go down this path.

Diff Detail

Repository
rL LLVM

Event Timeline

peter.smith created this revision.Mar 11 2019, 9:15 AM
pcc added a comment.Mar 11 2019, 11:18 AM

Thanks Peter. I think I prefer this approach.

ELF/SyntheticSections.cpp
3188 ↗(On Diff #190095)

Should the handling of .ARM.exidx input sections look more like .eh_frame sections, perhaps? You could:

  • remove .ARM.exidx sections from the InputSections list in combineEhFrameSections (and maybe rename the function) and add them to a separate list
  • arrange for forEachRelSec to visit those sections

Now you should be able to write the sections in the usual way with writeTo.

ELF/Writer.cpp
1438 ↗(On Diff #190095)

Can/should the ARMExidxSyntheticSection be a field in InStruct? Then it can be finalized with the other synthetic sections.

ruiu added a comment.Mar 11 2019, 2:40 PM

Thanks Peter, I think I like this approach than the other one.

ELF/SyntheticSections.cpp
3058 ↗(On Diff #190095)

Since you are using this variable only in ARMExidxSyntheticSection::writeTo, you can move this to that function.

3066 ↗(On Diff #190095)

nit: I'd add {} to if if its else if has {}.

3095 ↗(On Diff #190095)

Could you add a brief function comment?

peter.smith marked 6 inline comments as done.Mar 12 2019, 8:12 AM

Thanks for the comments, I'll upload another diff with the changes.

ELF/SyntheticSections.cpp
3188 ↗(On Diff #190095)

Yes I think that is a better approach. I've updated the diff to do that.

peter.smith marked an inline comment as done.

Updated the diff with the requested changes. I've followed pcc's idea to make the section behave more like the EHFrame section. This gets rid of some of the nastier relocation handling code in writeTo().

Just a few nits about coding style from me.

ELF/SyntheticSections.cpp
3093 ↗(On Diff #190262)

I think LLD style would be to use {, }:

else {
  Size += 8;
}
3098 ↗(On Diff #190262)

Will it be better to do Size = 8; from start?

3136 ↗(On Diff #190262)

No need to use curly bracers.

3217 ↗(On Diff #190262)

Comments in LLD usually ends with a full stop.

peter.smith marked 4 inline comments as done.

Updated to make setSizeAndOffsets() use of Size less confusing, fixed some curly brace nits.

ruiu added a comment.Mar 12 2019, 1:29 PM

Overall looking good.

ELF/SyntheticSections.cpp
3111 ↗(On Diff #190270)

This does not capture anything, so I'd define it as a regular function instead of a lambda.

3121 ↗(On Diff #190270)

Do you need to cast to ulittle32_t?

3137 ↗(On Diff #190270)

I don't think you need a cast to ulittle32_t.

3169 ↗(On Diff #190270)

Why don't you use back?

pcc added inline comments.Mar 12 2019, 1:45 PM
ELF/Writer.cpp
940 ↗(On Diff #190270)

This should probably either call Fn or we should change forEachRelSec to not take a function parameter.

peter.smith marked 6 inline comments as done.Mar 13 2019, 3:31 AM

Thanks for the comments I'll update the diff.

ELF/SyntheticSections.cpp
3098 ↗(On Diff #190262)

I'm using Size to set the OutSecOff so I need it to start from 0. I'll use a local Offset and derive Size from it to make that a bit clearer.

ELF/Writer.cpp
940 ↗(On Diff #190270)

I've changed it to call Fn for the moment, a simple followup NFC refactoring change could remove the parameter if we thought there was not likely to be another likely use of forEachRelSec.

peter.smith marked an inline comment as done.

Updated diff to address review comments.

Ping. I think I have addressed all the existing comments. Happy to make any further changes if needed.

ruiu accepted this revision.Mar 20 2019, 4:39 PM

LGTM

ELF/SyntheticSections.cpp
3121 ↗(On Diff #190270)

I tried this and indeed you need a cast to ulittle32_t.

This revision is now accepted and ready to land.Mar 20 2019, 4:39 PM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptMar 21 2019, 7:08 AM
peter.smith reopened this revision.Mar 22 2019, 9:18 AM

I've discovered 3 problems with this, the first 2 are trivial to fix, the last is not so I think I'll need to send for review when I've fixed rather than just recommit. The trivial problems are:

  • When all the .ARM.exidx sections from input files are merged into a synthetic one the getLinkOrderDep() function can't find any .ARM.exidx section and returns nullptr.
    • 1 line fix to just return the front() of the ExecutableSections vector, as that will always exist.
  • If the In.ARMexidx is discarded via \DISCARD\ then scanning its relocations adds a dependency to the personality routine (undefined symbol error).
    • 1 line fix to skip the relocation scanning if In.ARM.exidx is not live.

The difficult problem to fix is that --emit-relocs doesn't work with the SyntheticSection.

  • The .ARM.exidx InputSections have been removed so there is no way to get the OutputSection
  • If table merging is on then we end up with spurious relocations for the removed table. This is true of the existing implementation.

I'm investigating the best way of fixing this. The approach I'm currently favouring is following the example of EhInputSection. The alternative is some special cases for .ARM.exidx, I think that these could prove fragile so I'm tending towards the former even if it is more code.

This revision is now accepted and ready to land.Mar 22 2019, 9:18 AM

I've discovered 3 problems with this, the first 2 are trivial to fix, the last is not so I think I'll need to send for review when I've fixed rather than just recommit. The trivial problems are:

  • When all the .ARM.exidx sections from input files are merged into a synthetic one the getLinkOrderDep() function can't find any .ARM.exidx section and returns nullptr.
    • 1 line fix to just return the front() of the ExecutableSections vector, as that will always exist.
  • If the In.ARMexidx is discarded via \DISCARD\ then scanning its relocations adds a dependency to the personality routine (undefined symbol error).
    • 1 line fix to skip the relocation scanning if In.ARM.exidx is not live.

      The difficult problem to fix is that --emit-relocs doesn't work with the SyntheticSection.
  • The .ARM.exidx InputSections have been removed so there is no way to get the OutputSection
  • If table merging is on then we end up with spurious relocations for the removed table. This is true of the existing implementation.

    I'm investigating the best way of fixing this. The approach I'm currently favouring is following the example of EhInputSection. The alternative is some special cases for .ARM.exidx, I think that these could prove fragile so I'm tending towards the former even if it is more code.

Updated diff with proposed fixes for the three problems found. As mentioned above the first two changes are trivial, I think the last should be looked at in case of disagreement. There are 3 problems caused by --emit-relocs:

  • The relocation sections for the .ARM.exidx sections need to find the In.ARMExidx to find their OutputSection in an anlagous way to the EHInputSections need to find the In.EhFrame section.
  • The merging of duplicate entries will result in removed .ARM.exidx sections, with --emit-relocs we'll need to remove the associated .rel.ARM.exidx section
  • There are no .rel.ARM.exidx sections for linker created table entries.

I've fixed the first two parts with this diff. I don't think that there is an easy fix for --emit-relocs and linker created table entries. I'm not too worried about that as I don't think we do so for other synthetic sections like PLT entries. I'll add some comments to highlight the changes.

peter.smith marked 4 inline comments as done.Mar 25 2019, 10:30 AM

I've added comments to highlight the changes. I've also added test cases for them.

ELF/InputSection.cpp
198 ↗(On Diff #192132)

In conjunction with making In.ARMexidx the parent of the input .ARM.exidx sections; this fixes a segfault when --emit-relocs is used.

ELF/OutputSections.cpp
314 ↗(On Diff #192132)

This removes .rel.ARM.Exidx sections that are associated with merged .ARM.Exidx sections. A simpler but less effective fix would be to disable merging when --emit-relocs is used.

ELF/SyntheticSections.cpp
3180 ↗(On Diff #192132)

This fixes a segfault when all ExecutableSections with a .ARM.exidx input section are merged as duplicates.

ELF/Writer.cpp
925 ↗(On Diff #192132)

Added In.ARMExidx->Live to prevent undefined symbol errors caused by the personality routine being relocated against.

ruiu added a comment.Mar 25 2019, 11:24 AM

This may be a silly question, but is --emit-reloc for .ARM.exidx important? If we re-create an .ARM.exidx section, we probably have to re-create a relocation section for .ARM.exidx just for --emit-relocs, but I'm wondering if there's an actual user who wants that behavior. Perhaps this is a kind of question no one really knows, though...

This may be a silly question, but is --emit-reloc for .ARM.exidx important? If we re-create an .ARM.exidx section, we probably have to re-create a relocation section for .ARM.exidx just for --emit-relocs, but I'm wondering if there's an actual user who wants that behavior. Perhaps this is a kind of question no one really knows, though...

I think it is a good question. I've only come across two use cases for it, the first is as a form of dynamic linking for OS kernels that want to avoid GOT style PIE/PIC. Examples include x86 and mips linux kernel, and I've also seen it in Fuchsia, which may use Arm. However in the case of an OS kernel I'm not expecting any C++ exceptions. The second use case is binary analysis where some tool such as Facebook's Bolt post-processes the ELF file guided by the relocations, in this particular case I'm guessing that relocations in C++ exception tables aren't that interesting, it would certainly be possible to derive the relocations from the table output.

Given that the relocations for --emit-relocs had been broken when table merging is on for some time (retained relocations for dropped sections) I suspect that no-one cares about it right now. For now I'd be happy to remove all relocation sections from the Input .ARM.exidx sections when --emit-relocs were used. If someone came up with a critical use case then we could add them back. Does that sound like a way forward?

Updated the diff to just remove the .rel.ARM.exidx relocation sections when --emit-relocations is used. This is a few lines in the ARMExidxSyntheticSection constructor. This permits us to remove the additional fixup code in InputSections.cpp and OutputSections.cpp. The test remains to make sure we don't crash with --emit-relocs and to check that there are no .rel.ARM.exidx relocation sections.

ruiu accepted this revision.Mar 27 2019, 7:38 PM

LGTM

This revision was automatically updated to reflect the committed changes.