Page MenuHomePhabricator

[LLD][ELF][ARM] Synthesise missing .ARM.exidx sections.
AbandonedPublic

Authored by peter.smith on Feb 11 2019, 4:18 AM.

Details

Summary

When ARM exceptions are used executable sections have an associated .ARM.exidx section containing unwind information for the executable section. The linker combines these .ARM.exidx sections into a single table ordered by the address of the sections they describe. When an exception is thrown the unwinder looks up the unwinding information by binary searching the table. The .ARM.exidx section contains an table of 8-byte entries of theform:

| PREL31 relocation to function start | Unwind instructions |

The range of addresses covered by the table entry is terminated by the next table entry. This allows consecutive table entries that are identical to be merged, leaving a single large range of PC values matching the same set of unwinding information. Unfortunately if an executable section does not have an associated .ARM.exidx section but is placed after an executable section with unwinding information it can "inherit" the unwind information of the previous table entry instead of terminating it. To fix this problem the linker generates an EXIDX_CANTUNWIND entry for all executable sections without .ARM.exidx sections. I've followed the ld.gold high level behaviour here:

  • Generate EXIDX_CANTUNWIND sections only when there is a .ARM.exidx OutputSection.
  • Only generate EXIDX_CANTUNWIND sections for executable, non-0 sized non-synthetic sections.
  • We want consecutive linker generated EXIDX_CANTUNWIND sections to be merged as if they were from InputFiles.

The implementation extends the sentinel section concept to an arbitrary InputSection. This permits the terminating SentinelSection to be merged into a previous EXIDX_CANTUNWIND entry, although I haven't done that in this patch to limit the number of changes.

Fixes pr40277 https://bugs.llvm.org/show_bug.cgi?id=40277 which has been blocking a project from moving to LLD on Arm.

Diff Detail

Event Timeline

peter.smith created this revision.Feb 11 2019, 4:18 AM

Few suggestions about the code/style are below.

ELF/SyntheticSections.h
937

Could you extend the comment? Without reading the patch description it
is probably not clear what is EXIDX_CANTUNWIND entry and why
normal and terminating entries are required.

974

Could it be inlined in the constructor?

RawData = { ... };

getSize() seems could either return a constant or RawData.size() then it seems.

ELF/Writer.cpp
1496
Dep->Type & SHT_ARM_EXIDX
1500

This would probably look simpler a bit if was inversed + early return was used:

if (!IS->Live || IS->kind() == InputSectionBase::Synthetic ||
            !(IS->Flags & SHF_EXECINSTR) || !IS->getSize())
return false;
...

Also, so you need IS->Live? I would expect input sections included in output sections to be live.

1507

You do not need to wrap for in curly bracers.

1511–1512
assert(Sentinel->IsSentinel);
peter.smith marked 6 inline comments as done.Feb 11 2019, 8:45 AM

Thanks for the comments. I will upload another diff in a few minutes.

ELF/SyntheticSections.h
937

Sure have done so. This was a valuable exercise as I remembered the precise reason we had to have a terminating sentinel (libunwind bug).

974

Not easily as RawData is a member of a base class. I could add another constructor for SyntheticSection that forwards Data to InputSection but I'm not sure it is worth it. Given that all sections will get the same value I've made it a static.

ELF/Writer.cpp
1500

I do as I'm iterating through InputSections and not the contents of the OutputSection. As it happens I found that adding the Sections at this point breaks LinkerScript Symbol assignment. I've moved the addition to the same place as the existing sentinel and I've made a modification to arm-exidx-sentinel-and-assignment.s so that it fails if I don't.

Uploaded new diff to address review comments, main changes:

  • Added expanded comment explaining why we need these sections.
  • Moved the addition of the synthetic sections to where we add the sentinel and added test that fails if we don't.
ruiu added inline comments.Feb 11 2019, 10:23 AM
ELF/SyntheticSections.cpp
3055

You are assigning a nullptr to a boolean member.

Who sets IsSentinel to true? Looks like there's no code doing that.

peter.smith marked an inline comment as done.Feb 12 2019, 1:52 AM

I think what I have there is correct but it obviously could be clearer. I've added some alternatives inline.

ELF/SyntheticSections.cpp
3055

I don't think I am as the == will have higher precedence than the =. However I should find another way of expressing it. The simplest way is to put some parentheses around so that it is IsSentinel = (Link == nullptr);

What I need is a way of distinguishing between the Sentinel (so I can write the address of the end of the section). We do know that at at construction time as the Sentinel won't have a Section to link to at construction time as we don't know what it will be yet, in all other cases we do know. Alternatives:

  • 2 constructors, One with no argument (IsSentinel = True), One with an InputSection * argument (IsSentinel = False). This does duplicate quite a bit of code though.
  • Set IsSentinel = true when we assign the highest InputSection to be the Sentinel Section Link.

Any preferences?

ruiu added inline comments.Feb 13 2019, 1:48 PM
ELF/SyntheticSections.cpp
3055

Apologies, I misread == as =. I'm reading this patch again.

ruiu added a comment.Feb 13 2019, 3:10 PM

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.

ruiu added a comment.Feb 13 2019, 3:25 PM

If we define a synthetic ARM.exidx section which consumes all .ARM.exidx sections and replace them with itself, we can perhaps remove SHF_ORDER and other features that we add for .ARM.exidx sections. If there's no other use of the features, I'd like to remove them.

(We saw this pattern many times: some special section such as .ARM.exidx needs special handling by the linker, and that's implemented using a generic mechanism such as SHF_ORDER, but it later turned out that that generic mechanism does not cover all corner cases, so we ended up having a complex generic feature *and* a target-aware feature. In many cases, it should have been easier to implement as a special feature from the beginning.)

pcc added a subscriber: pcc.Feb 13 2019, 3:26 PM

SHF_LINK_ORDER is used by the sanitizers.

The LLVM associated symbol metadata uses SHF_LINK_ORDER (https://llvm.org/docs/LangRef.html#associated-metadata) and I think this is used by the at least some of the sanitizers. I think it would be difficult to get rid of completely.

I think it could be possible to implement .ARM.exidx as a single SyntheticSection as the unwinder requires there to be only one contiguous range. I'm not sure whether it would be considerably better than what we have now. I can think of a couple of ways to implement it:

  • Have the .ARM.exidx synthetic section be in effect a container for all .ARM.exidx InputSections, with a finalize() that does the sythesize entry, equivalent of SHF_LINK_ORDER and compression. This would in effect move some of the code out of Writer.cpp and into SyntheticSections.cpp but much of it would be the same. Instead of making a small number of Synthetic .ARM.exidx sections we'd create a single one, then write some code to filter all the InputSections into it.
  • Do something more radical like copying the contents of all .ARM.exidx sections along with an abstraction of the relocations (all the same type) into a giant buffer or table structure. We'd then sort the table, compress it and handle the relocations in writeTo() like we do with the PLT sections.

I can't immediately think that either one of those would be much better that what we have. If either of those or any other idea sounds plausible I can write a prototype to see what it looks like, I'd prefer to only do that if an approach sounds appealing though?

Thank you all for helping with this!

Friendly ping :)

(Looks like subscribing on its own no longer generates emails? If it does, apologies about the double ping)

At the moment I'm happy with the current implementation assuming that the .ARM.exidx sections from input objects are treated are modelled as InputSections. If I'm going to need to rewrite all of it as a single synthetic section I can do it, but that is quite a bit of work and there isn't a guarantee it will be any better. Would it be possible to take this patch for the benefit of the teams waiting on the PR, and I'll work on a replacement? If it turns out to be better we can switch to it?

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.

I've submitted D59216 to show what that would look like. I'd like to keep this open until there is an idea of which approach is preferable.

peter.smith abandoned this revision.Mar 21 2019, 7:05 AM

Abandoned in favour of D59216