[PATCH] D61186: [LLD][ELF] /DISCARD/ output sections should not be orphans

[Andrew Ng via Phabricator via llvm-commits]

andrewng marked 2 inline comments as done.
andrewng added a comment.

In D61186#1480437 <https://reviews.llvm.org/D61186#1480437>, @grimar wrote:

> From what I see in the output for this case LLD creates an `/DISCARD/` output section. 
>  This does not look correct to me, because `/DISCARD/` is kind of key word, but not a section name I believe.
>  Also, bfd does not do that.
>
> So I think issue first of all is that we are trying to create output section with this name, it should never be in the output.


Yes, this is a difference compared to bfd and gold, but our policy of keeping empty output sections already differs from bfd and gold, so you could argue that this is not an exception, although definitely undesirable. The '/DISCARD/' output section is only output in this unusual case where it has a PHDR assigned to it, as it is this property that makes it "not discardable".

This patch is only trying to address the problems caused by the '/DISCARD/' being treated as an orphan. I think that fixing the behaviour of '/DISCARD/' to be more like bfd/gold is a more complex change and should be addressed by a separate patch.



================
Comment at: ELF/LinkerScript.cpp:485
         Sec->SectionCommands.clear();
+        Sec->SectionIndex = 0; // Not an orphan.
         continue;
----------------
grimar wrote:
> For other reviewers, this is for the following place, right?
> https://github.com/llvm-mirror/lld/blob/master/ELF/Writer.cpp#L1385
Yes, that's right.


================
Comment at: test/ELF/linkerscript/discard-phdr.s:44
+.section .baz,"aw"
+ .byte 0
----------------
grimar wrote:
> Does not seem you need all of this sections for the test? Please reduce the number.
These are actually required to provoke the issue. It is a somewhat unusual situation (based on a real scenario), so whether this test should be "kept" is debatable.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D61186/new/

https://reviews.llvm.org/D61186