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

[George Rimar via Phabricator via llvm-commits]

grimar 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)
----------------
ruiu wrote:
> 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?
What we have now is code in `removeEmptyCommands()` which removes all output sections that are not live.

In current method we have live section which should be dead after removing synthetic, so either needs to be marked as dead so that `removeEmptyCommands()` can handle it or be removed.

For first case code here can probably be (after additional patch changing things outside this method, without that if will not work):

**Approach #1:**
```
   OS->Live = !llvm::all_of(OS->Commands, [](BaseCommand *B) {
      auto *ISD = dyn_cast<InputSectionDescription>(B);
      return ISD && ISD->Sections.empty();
    });
```

for second - code from this patch, which is:

**Approach #2:**
```
    bool IsEmpty = llvm::all_of(OS->Commands, [](BaseCommand *B) {
      auto *ISD = dyn_cast<InputSectionDescription>(B);
      return ISD && ISD->Sections.empty();
    });
    if (IsEmpty)
      llvm::erase_if(Script->Opt.Commands,
                     [&](BaseCommand *Cmd) { return Cmd == OS; });
```

Difference is in `llvm::erase_if` call, which also present in original code this patch fixes. Doing approach #1 is probably better.
But that is compoletely unrelative to what this patch fixes and should be (or should not be) done separatelly. That is my point.


https://reviews.llvm.org/D37520