[ELF] - Fix removing of unused synthetic sections.

[grimar]

Summary

Code that removes unused synthetic sections has a bug
that leaves unused section in output if its description is duplicated
in linkerscript. Testcase shows the issue, patch fixes it.

[ruiu]

ELF/Writer.cpp

-2–1

This loop needs a (brief) comment.

[grimar]

• Addressed review comments.

[ruiu]

Sorry, but I do not understand this function well. It seems too complicated compared to what it does (which doesn't sound too complicated). Can you refactor?

[grimar]

• Rebased and refactored.

[grimar]

• Rewrote comment.

[ruiu]

ELF/Writer.cpp

1204

// SS is an unused synthetic section. Remove it from output section commands.

1205–1208

This code still looks odd. Can SS belong to more than one OS->Commands?

1210

Please fix grammatical errors.

[grimar]

• Addressed review comments.
• Changed inplementation to use Live flag for removing output section.

ELF/Writer.cpp

1205–1208

No. That was just short way to write the code. Rewrote this place to longer, but more straightforward way,
do you prefer new way more ?

[grimar]

• Previous revision deviated too far from original intention of patch and can be separate refactoring, reverted it to use original approach.

[grimar]

Ping.

[ruiu]

ELF/Writer.cpp

-2–1

Do you actually have to scan over output sections? I mean, InputSection has getOutputSection function, and I wonder if you can just use to know the output section that an input section is contained.

1210

// If SS was the only section content, remove the entire output section.

[grimar]

• Addressed comments.

ELF/Writer.cpp

-2–1

Here we scan not over output sections, but over commands it contains. OS is an OutputSection*.

It would be possible to avoid scanning if we would have pointer to InputSectionDescription command in each section (and InputSectionDescription can probably have poiner to OutputSection then), but we do not have it currently and I doubt we really need it.

[ruiu]

ELF/Writer.cpp

-1–3

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?

[grimar]

ELF/Writer.cpp

-1–3

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.

[ruiu]

ELF/Writer.cpp

-1–3

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.

[grimar]

ELF/Writer.cpp

-1–3

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.

[ruiu]

ELF/Writer.cpp

-1–3

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?

[grimar]

ELF/Writer.cpp

-1–3

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.

[grimar]

• Rebased.

[grimar]

Ping.

ELF/Writer.cpp

 //===- Writer.cpp ---------------------------------------------------------===//
 //
 //                             The LLVM Linker
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
for (InputSectionBase *S : llvm::reverse(InputSections)) {
     if (!SS)
       return;
     OutputSection *OS = SS->getParent();
     if (!SS->empty() || !OS)
       continue;
     if ((SS == InX::Got || SS == InX::MipsGot) && ElfSym::GlobalOffsetTable)
       continue;
 
-    std::vector<BaseCommand *>::iterator Empty = OS->Commands.end();
-    for (auto I = OS->Commands.begin(), E = OS->Commands.end(); I != E; ++I) {
-      BaseCommand *B = *I;
-      if (auto *ISD = dyn_cast<InputSectionDescription>(B)) {
+    // Remove unused synthetic section.
+    for (BaseCommand *B : OS->Commands)
+      if (auto *ISD = dyn_cast<InputSectionDescription>(B))
         llvm::erase_if(ISD->Sections,
                        [=](InputSection *IS) { return IS == SS; });
-        if (ISD->Sections.empty())
-          Empty = I;
-      }
-    }
-    if (Empty != OS->Commands.end())
-      OS->Commands.erase(Empty);
 
-    // If there are no other sections in the output section, remove it from the
-    // output.
+    // Remove whole section description commands if them became empty.
+    llvm::erase_if(OS->Commands, [](BaseCommand *B) {
+      auto *ISD = dyn_cast<InputSectionDescription>(B);
+      return ISD && ISD->Sections.empty();
+    });
+
+    // Remove whole output section if it is empty finally.
     if (OS->Commands.empty())
       llvm::erase_if(Script->Opt.Commands,
                      [&](BaseCommand *Cmd) { return Cmd == OS; });
   }
 }
 
 // Returns true if a symbol can be replaced at load-time by a symbol
 // with the same name defined in other ELF executable or DSO.

test/ELF/linkerscript/unused-synthetic.s

 # REQUIRES: x86
 # RUN: llvm-mc -filetype=obj -triple=x86_64-unknown-linux %s -o %t.o
 # RUN: echo "SECTIONS { \
-# RUN:    .got  : { *(.got) } \
+# RUN:    .got  : { *(.got) *(.got) } \
 # RUN:    .plt  : { *(.plt) } \
 # RUN:    .text : { *(.text) } \
 # RUN:  }" > %t.script
 # RUN: ld.lld -shared -o %t.so --script %t.script %t.o
 
 # RUN: llvm-objdump -section-headers %t.so | FileCheck %s
 # CHECK-NOT:  .got
 # CHECK-NOT:  .plt
 # CHECK:      .text
 # CHECK-NEXT: .dynsym
 
 .global _start
 _start:
   nop