Page MenuHomePhabricator

[LLD][ELF][ARM] Fix crash when discarding all of the InputSections that have .ARM.exidx sections
ClosedPublic

Authored by peter.smith on Sep 20 2019, 10:59 AM.

Details

Summary

While experimenting with some test cases for D67761 earlier today I found that I could crash the linker when I discarded all the InputSections, but not the .ARM.exidx sections that had a dependency on them. This was ostensibly down to the OutputSection code using a non-existing InputSection to obtain a OutputSection for the OutputSection link order dependency. Attempts to clumsily fix this resulted in other crashes as the executableSections and exidxSections weren't in synch.

The fix makes sure that we only add to the exidxSections when the section it depends on would be added to the executableSections. This allows us to remove the empty flag and then implement a more sophisticated needed() so that we discard the .ARM.exidx section if it wouldn't be valid to write it.

This is independent of the fix for D67761.

Diff Detail

Repository
rL LLVM

Event Timeline

peter.smith created this revision.Sep 20 2019, 10:59 AM
MaskRay added inline comments.Sep 21 2019, 7:18 AM
lld/ELF/SyntheticSections.cpp
3228 ↗(On Diff #221068)
if (isec->type == SHT_ARM_EXIDX) {
  exidxSections.push_back(isec);
  return true;
}

probably also works. finalizeContents() has logic to filter dead InputSections, so I'm thinking whether we should simplify the logic here.

lld/test/ELF/arm-exidx-discard-all.s
1 ↗(On Diff #221068)

How about moving this to linkerscript/? There are 2 arm-exidx* tests.

peter.smith marked 2 inline comments as done.Sep 23 2019, 3:38 PM
peter.smith added inline comments.
lld/ELF/SyntheticSections.cpp
3228 ↗(On Diff #221068)

I tried that initially but got test failures (crashes) for the case where there was a zero sized .text section with a non-zero size .ARM.exidx section (garbage collection off). The .ARM.exidx section is added, but the executable section isn't, isNeeded() returns true. This leaves the executableSections and exidxSections out of synch.

It is possible to account for this case by making isNeeded() more complex by making it search for both executable and exidx sections, but I thought it better to keep the executableSections and exidxSections synchronised at the start.

I can make the change if you prefer.

lld/test/ELF/arm-exidx-discard-all.s
1 ↗(On Diff #221068)

Sure I'll update the patch.

Changes from last version, moved exidx discard tests to linkerscripts subdir.

MaskRay accepted this revision.Sep 23 2019, 7:12 PM
MaskRay added inline comments.
lld/ELF/SyntheticSections.cpp
3228 ↗(On Diff #221068)

OK. I think having the check here is fine.

I thought it better to keep the executableSections and exidxSections synchronised at the start.

Keeping them synchronized is a nice property to have.

This revision is now accepted and ready to land.Sep 23 2019, 7:12 PM
This revision was automatically updated to reflect the committed changes.