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

Mon Apr 29 02:57:58 PDT 2019

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

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

> I think it is 2 lines of code change actually :) I believe the much more correct way to fix it is this: https://reviews.llvm.org/D61251
>  With this approach there is no `/DISCARD/` in the output.
>  (note, I did not change the original test case there).

Unfortunately, this doesn't completely work, although it does "work" for this particular issue. The problem is if the "/DISCARD/" has a PHDR assignment that needs to propagate to other sections, which is why the "isDiscardable" function returns false for sections with explicit PHDR assignment. It's an unlikely but still possible scenario.

================
Comment at: test/ELF/linkerscript/discard-phdr.s:44
+.section .baz,"aw"
+ .byte 0
----------------
grimar wrote:
> andrewng wrote:
> > 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.
> No need to replicate the full real case. The test case should show the minimal reproducible scenario of the different output before/after the patch. That is how we do in LLD. 
Just to be clear, you are saying that there is no need to demonstrate the issue being addressed? So the only requirement for a test is to show a change in the output?

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

https://reviews.llvm.org/D61186