[PATCH] D40710: [LLD][ELF] Revert r318924 Skip over empty sections when checking for contiguous relro

[Peter Smith via Phabricator via llvm-commits]

peter.smith created this revision.
Herald added a subscriber: emaste.

PR35478 https://bugs.llvm.org/show_bug.cgi?id=35478 points out a flaw in the implementation of r318924 from https://reviews.llvm.org/D40364. The implementation depends on the Size field being set or the SyntheticSection::empty() being accurate. These functions are not reliable as some linker script commands that have yet to be processed may affect the results, causing some non-zero size sections to be reported as zero size.

I think the first step is to revert r318924 and come up with a better solution for the underlying problem rather than trying to layer more heuristics onto the zero sized output section.

Chances are I'll be out of office by the time anyone sees this so feel free to commit the revert if you agree with me.

Fixes PR35478

Current thoughts on the underlying problem:
Revisiting the motivation for adding the zero size check in the first place; it was to prevent 0 sized SyntheticSections that a user does not have full control over from needlessly breaking the PT_GNU_RELRO, rather than trying to accommodate arbitrarily complex linker scripts. Looking at the code, it looks like removeUnusedSyntheticSections() should remove zero sized synthetic sections. It does, but it doesn't set the Parent to nullptr, this has the side effect that Sec == InX::BssRelRo->getParent() will make the parent OutputSection of InX::BssRelRo RelRo even if there is no InX::BssRelRo.

I tried a quick experiment with setting the Parent to nullptr and this flushed out a few interesting test failures, it feels like playing Jenga with every change:

- In the isRelroSection() we have to consider the case where there is no .plt and .plt.got but there is a ifunc plt with accompanying (ifunc .got or .plt.got)
- The PPC64  has PltHeaderSize == 0. Unfortunately HeaderSize == 0 is used to choose between the ifunc plt or normal plt. We seem to get away with this at the moment, but tests start to fail when Parent is set to nullptr for the .got.plt.
- The InX::BssRelRo and InX::Bss never get their sizes set and they are always removed by removeUnusedSyntheticSections(), their purpose seems to be as some kind of proxy for add .bss or .bss.relro InputSections into their parent OutputSections, they therefore don't behave like other SyntheticSections anyway.

My thinking is that some work is needed to make sure that the Sec == SyntheticSection->getParent() does a bit more checking before returning true, particularly for InX::BssRelRo as that has special behaviour. I'll hope to post something for review as soon as possible.


https://reviews.llvm.org/D40710

Files:
  ELF/Writer.cpp
  test/ELF/relro-non-contiguous-zerosize.s

-------------- next part --------------
A non-text attachment was scrubbed...
Name: D40710.125093.patch
Type: text/x-patch
Size: 3528 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20171201/c7377bd6/attachment.bin>