[ELF] - Drop special flags for empty output sections.

[grimar]

Summary

This fixes PR36598.

LLD currently crashes when we have empty output section
with SHF_LINK_ORDER flag. This might happen if we place an empty
synthetic section in the linker script, but keep output section alive with the
use of additional symbol, for example.

The patch fixes the issue by dropping all special flags for empty sections.

[grimar]

• Simplified.

[grimar]

• Improved test case.

[• rafael]

George Rimar via Phabricator via llvm-commits
<[e-mail]> writes:

void LinkerScript::adjustSectionsBeforeSorting() {
@@ -839,32 +839,34 @@

// the previous sections. Only a few flags are needed to keep the impact low.
uint64_t Flags = SHF_ALLOC;

• for (BaseCommand *&Cmd : SectionCommands) {

+ for (BaseCommand *Cmd : SectionCommands) {

auto *Sec = dyn_cast<OutputSection>(Cmd);
if (!Sec)
  continue;
 
// A live output section means that some input section was added to it. It

• // might have been removed (gc, or empty synthetic section), but we at least

+ // might have been removed (if it was empty synthetic section), but we at least

// know the flags.
if (Sec->Live)

• Flags = Sec->Flags & (SHF_ALLOC | SHF_WRITE | SHF_EXECINSTR);
• else
• Sec->Flags = Flags; -
• if (isDiscardable(*Sec)) {
• Sec->Live = false;
• Cmd = nullptr;
• }

+ Flags = Sec->Flags;
+
+ // We do not want to keep any special flags for output section in case it is empty.
+ bool IsEmpty = getInputSections(Sec).empty();
+ if (IsEmpty)
+ Sec->Flags = Flags & (SHF_ALLOC | SHF_WRITE | SHF_EXECINSTR);
+
+ Sec->Live = !IsEmpty || !isDiscardable(*Sec);

I would keep this as

if (IsEmpty && isDiscardable(*Sec)) {
  Sec->Live = false;
  Cmd = nullptr;
}

}
 
// It is common practice to use very generic linker scripts. So for any
// given run some of the output sections in the script will be empty.
// We could create corresponding empty output sections, but that would
// clutter the output.
// We instead remove trivially empty sections. The bfd linker seems even
// more aggressive at removing them.

• llvm::erase_if(SectionCommands, [&](BaseCommand *Base) { return !Base; });

+ llvm::erase_if(SectionCommands, [&](BaseCommand *Base) {
+ return isa<OutputSection>(Base) && !cast<OutputSection>(Base)->Live;
+ });
}

And this as

llvm::erase_if(SectionCommands, [&](BaseCommand *Base) { return !Base; });

LGTM with that.

Cheers,
Rafael

[ruiu]

lld/trunk/ELF/LinkerScript.cpp

859–862 ↗

(On Diff #138134)

I believe this patch is correct, but this logic seems a bit weird to me. If an section is empty, we reset the section flag, but we also discard the section. If the section is discarded, why does its flag matter? Why do you have to reset it just before discarding it?

[• rafael]

Rui Ueyama via Phabricator <[e-mail]> writes:

ruiu added inline comments.

Comment at: lld/trunk/ELF/LinkerScript.cpp:859-862
+ if (IsEmpty && isDiscardable(*Sec)) {

  Sec->Live = false;
  Cmd = nullptr;
}

---

I believe this patch is correct, but this logic seems a bit weird to me. If an section is empty, we reset the section flag, but we also discard the section. If the section is discarded, why does its flag matter? Why do you have to reset it just before discarding it?

To handle the !isDiscardable case. The new code is:

if (IsEmpty)
  Sec->Flags = Flags & (SHF_ALLOC | SHF_WRITE | SHF_EXECINSTR);

if (IsEmpty && isDiscardable(*Sec)) {

So if the section is empty but must be kept for some other reason we
update the flags.

Cheers,
Rafael

[ruiu]

Ah, thanks! I misunderstood "&&" with "||".

ELF/LinkerScript.cpp

static bool isDiscardable(OutputSection &Sec) {
   // other expressions. We add script symbols as undefined, and want to ensure
   // all of them are defined in the output, hence have to keep them.
   if (Sec.ExpressionsUseSymbols)
     return false;
 
   for (BaseCommand *Base : Sec.SectionCommands)
     if (!isa<InputSectionDescription>(*Base))
       return false;
-  return getInputSections(&Sec).empty();
+  return true;
 }
 
 void LinkerScript::adjustSectionsBeforeSorting() {
   // If the output section contains only symbol assignments, create a
   // corresponding output section. The issue is what to do with linker script
   // like ".foo : { symbol = 42; }". One option would be to convert it to
   // "symbol = 42;". That is, move the symbol out of the empty section
   // description. That seems to be what bfd does for this simple case. The
void LinkerScript::adjustSectionsBeforeSorting() {
   //   * The program header creation.
   //   * The orphan section placement.
   //   * The address assignment.
   // The other option is to pick flags that minimize the impact the section
   // will have on the rest of the linker. That is why we copy the flags from
   // the previous sections. Only a few flags are needed to keep the impact low.
   uint64_t Flags = SHF_ALLOC;
 
-  for (BaseCommand *&Cmd : SectionCommands) {
+  for (BaseCommand *Cmd : SectionCommands) {
     auto *Sec = dyn_cast<OutputSection>(Cmd);
     if (!Sec)
       continue;
 
     // A live output section means that some input section was added to it. It
-    // might have been removed (gc, or empty synthetic section), but we at least
+    // might have been removed (if it was empty synthetic section), but we at least
     // know the flags.
     if (Sec->Live)
       Flags = Sec->Flags & (SHF_ALLOC | SHF_WRITE | SHF_EXECINSTR);
-    else
+
+    // We do not want to keep any special flags for output section in case it is empty.
+    bool IsEmpty = getInputSections(Sec).empty();
+    if (!Sec->Live || IsEmpty)
       Sec->Flags = Flags;
 
-    if (isDiscardable(*Sec)) {
-      Sec->Live = false;
-      Cmd = nullptr;
-    }
+    Sec->Live = !IsEmpty || !isDiscardable(*Sec);
   }
 
   // It is common practice to use very generic linker scripts. So for any
   // given run some of the output sections in the script will be empty.
   // We could create corresponding empty output sections, but that would
   // clutter the output.
   // We instead remove trivially empty sections. The bfd linker seems even
   // more aggressive at removing them.
-  llvm::erase_if(SectionCommands, [&](BaseCommand *Base) { return !Base; });
+  llvm::erase_if(SectionCommands, [&](BaseCommand *Base) {
+    return isa<OutputSection>(Base) && !cast<OutputSection>(Base)->Live;
+  });
 }
 
 void LinkerScript::adjustSectionsAfterSorting() {
   // Try and find an appropriate memory region to assign offsets in.
   for (BaseCommand *Base : SectionCommands) {
     if (auto *Sec = dyn_cast<OutputSection>(Base)) {
       if (!Sec->LMARegionName.empty()) {
         if (MemoryRegion *M = MemoryRegions.lookup(Sec->LMARegionName))

test/ELF/linkerscript/empty-link-order.test

+# REQUIRES: x86
+# RUN: llvm-mc -filetype=obj -triple=arm-arm-none-eabi -o %t.o < /dev/null
+
+SECTIONS {
+  .foo : {
+    bar = .;
+    *(.ARM.exidx*)
+  }
+}
+# RUN: ld.lld %t.o -o %t --script %s