[PATCH] D37520: [ELF] - Fix removing of unused synthetic sections.

[Rui Ueyama via Phabricator via llvm-commits]

ruiu added inline comments.


================
Comment at: ELF/Writer.cpp:1205-1209
+    bool IsEmpty = llvm::all_of(OS->Commands, [](BaseCommand *B) {
+      auto *ISD = dyn_cast<InputSectionDescription>(B);
+      return ISD && ISD->Sections.empty();
+    });
+    if (IsEmpty)
----------------
grimar wrote:
> ruiu wrote:
> > grimar wrote:
> > > ruiu wrote:
> > > > I think I do not understand this code. You don't need to use all_of, right? The only way in which OS becomes empty is (1) it contains only one InputSectionDescription and (2) that InputSectionDescription is empty, no?
> > > No. If OS has 2 or more ISDs which are empty, it is also empty.
> > > For example in script from testcase we have ".got  : { *(.got) *(.got) }",
> > > which means we have OS `.got` and 2 ISDs, one has input section and one is empty from start.
> > > When we remove unused input section from first ISD it becomes empty, and we want to remove whole OS.
> > > For that we have to check if all ISDs are empty.
> > So you are saying that .got output section is not created for `.got : { *(.got) }`  if .got synthetic input section is empty, that is what I expected.
> > 
> > But what about some output section .foo where it is created as `.foo : { *(.bar) }`, if no input file have .bar section? Do we still create .foo?
> > 
> > If yes, that behavior is inconsistent.
> > 
> > If no, there must be a place we remove empty output sections. Doing the same thing here doesn't make sense.
> > But what about some output section .foo where it is created as .foo : { *(.bar) }, if no input file have .bar section? Do we still create .foo?
> > 
> 
> No.
> 
> > If no, there must be a place we remove empty output sections. Doing the same thing here doesn't make sense.
> 
> That place is `adjustSectionsBeforeSorting()`. There we mark output sections as Live even if they
> have no output sections but still have other commands.
> For `.foo : { *(.bar) }`, `.foo` will remain dead because `Live` flag is never set (it is normally set when we add any input section).
> 
> Alternative to removing of OS sections here is to set `Live` bit to false under some condition, which is I belive "if all ISDs are empty" and that is what this patch anyways already checks for removing the section.
> FWIW I already did that in one of early diff of this patch (https://reviews.llvm.org/D37520?id=114561) and refused from that diff because it seems better to do as separate patch, as it touches other parts of code and unrelated to this patch because original code removes OS as well.
I don't think I fully understand your justification. Are you saying that there is a reason that you have to have two pieces of code that basically does the same thing?


https://reviews.llvm.org/D37520