[ELF] Move IsNeeded logic from SymbolTable::addShared to MarkLive, and check IsUsedInRegularObj

[MaskRay]

Summary

In glibc, libc.so is a linker script with an as-needed dependency on ld-linux-x86-64.so.2

GROUP ( /lib/x86_64-linux-gnu/libc.so.6 /usr/lib/x86_64-linux-gnu/libc_nonshared.a  AS_NEEDED ( /lib/x86_64-linux-gnu/ld-linux-x86-64.so.2 ) )

ld-linux-x86-64.so.2 (as-needed) defines some symbols which resolve undefined references in libc.so.6, it will therefore be added as a DT_NEEDED entry, which isn't necessary.

The test case as-needed-not-in-regular.s emulates the libc.so scenario, where ld.bfd and gold don't add DT_NEEDED for a.so

The relevant code in gold/resolve.cc:

// If we have a non-WEAK reference from a regular object to a
// dynamic object, mark the dynamic object as needed.
if (to->is_from_dynobj() && to->in_reg() && !to->is_undef_binding_weak())
  to->object()->set_is_needed();

in_reg() appears to do something similar to IsUsedInRegularObj.

This patch makes lld do the similar thing, but moves the check from
addShared to a later stage MarkLive where all symbols are scanned.

[ruiu]

ELF/SymbolTable.cpp

508–509

This part of the logic seems a bit unnecessarily complicated (though I believe I wrote this.) Let's simplify this first.

[MaskRay]

ELF/SymbolTable.cpp

508–509

seems a bit unnecessarily complicated

It is. It took me some time to understand if !Config->GcSections is correct here and later I think it is. I'll also do some experiments to see if the logic can be simplified.

[ruiu]

I sent you a patch: https://reviews.llvm.org/D55903

[MaskRay]

rebase

[ruiu]

ELF/SymbolTable.cpp

510–511

Thinking about this code, I'm no longer sure if this code makes sense. If IsNeeded is set correctly for --gc-sections, we can just remove this if and let the later pass to set an appropriate bit to IsNeeded, maybe?

[MaskRay]

ELF/SymbolTable.cpp

510–511

This if handles the --no-gc-sections case. No later passes set the IsNeeded bit.
For the --gc-sections case, a later pass (MarkLive.cpp:resolveReloc) sets the IsNeeded bit.

[grimar]

test/ELF/as-needed-not-in-regular.s

3

A minor suggestion. I am trying to add a short description to each test
case I am writing. I.e. about what the purpose, what we do and what we expect.
It's sometimes really useful because commit log is not always helpful.
Could you add such comment?

[MaskRay]

test/ELF/as-needed-not-in-regular.s

3

I've already added one " The symbol a is not referenced by a regular object." but I can extend it. Thanks for the reminder.

[MaskRay]

Elaborate a comment

[ruiu]

ELF/SymbolTable.cpp

510–511

I still want to make this simpler... At least I don't think this is correct per se, as you need to address the case in which a shared symbol is added first and then an undefined symbol is added later. In that case you need to set IsNeeded too.

I wonder if a DSO is needed if and only if there is a shared symbol for the DSO whose isWeak() and IsUsedRegularObj are both true. If that's the case, we can remove the code from here, and add another loop after the symbol resolution to visit all symbols to set IsNeeded bit for DSOs.

[MaskRay]

Move logic to MarkLive

[ruiu]

Looks great! LGTM

[MaskRay]

Add another test as-needed-in-regular.s

[ruiu]

Please commit.

[farahmagee]

Add canBeVersioned(mapquest directions)

ELF/MarkLive.cpp

template <class ELFT> static void doGcSections() {
   while (!Q.empty())
     forEachSuccessor<ELFT>(*Q.pop_back_val(), Enqueue);
 }
 
 // Before calling this function, Live bits are off for all
 // input sections. This function make some or all of them on
 // so that they are emitted to the output file.
 template <class ELFT> void elf::markLive() {
-  // If -gc-sections is missing, no sections are removed.
   if (!Config->GcSections) {
+    // If -gc-sections is missing, no sections are removed.
     for (InputSectionBase *Sec : InputSections)
       Sec->Live = true;
+
+    // If a DSO defines a symbol referenced in a regular object, it is needed.
+    for (Symbol *Sym : Symtab->getSymbols())
+      if (auto *S = dyn_cast<SharedSymbol>(Sym))
+        if (S->IsUsedInRegularObj && !S->isWeak())
+          S->getFile<ELFT>().IsNeeded = true;
     return;
   }
 
   // The -gc-sections option works only for SHF_ALLOC sections
   // (sections that are memory-mapped at runtime). So we can
   // unconditionally make non-SHF_ALLOC sections alive except
   // SHF_LINK_ORDER and SHT_REL/SHT_RELA sections.
   //

ELF/SymbolTable.cpp

if (WasInserted) {
     replaceSymbol<SharedSymbol>(S, File, Name, Sym.getBinding(), Sym.st_other,
                                 Sym.getType(), Sym.st_value, Sym.st_size,
                                 Alignment, VerdefIndex);
     return;
   }
 
   if ((S->isUndefined() || S->isLazy()) && S->Visibility == STV_DEFAULT) {
     uint8_t Binding = S->Binding;
-    bool WasUndefined = S->isUndefined();
     replaceSymbol<SharedSymbol>(S, File, Name, Sym.getBinding(), Sym.st_other,
                                 Sym.getType(), Sym.st_value, Sym.st_size,
                                 Alignment, VerdefIndex);
 
     S->Binding = Binding;
-    if (!S->isWeak() && !Config->GcSections && WasUndefined)
-      File.IsNeeded = true;
   }
 }
 
 Symbol *SymbolTable::addBitcode(StringRef Name, uint8_t Binding,
                                 uint8_t StOther, uint8_t Type,
                                 bool CanOmitFromDynSym, BitcodeFile &F) {
   Symbol *S;
   bool WasInserted;
   std::tie(S, WasInserted) =

test/ELF/as-needed-in-regular.s

+# REQUIRES: x86
+
+# RUN: echo '.globl a; .type a, @function; .type a, @function; a: ret' | \
+# RUN:   llvm-mc -filetype=obj -triple=x86_64-unknown-linux - -o %ta.o
+# RUN: ld.lld %ta.o --shared --soname=a.so -o %ta.so
+
+# RUN: llvm-mc -filetype=obj -triple=x86_64-unknown-linux %s -o %t.o
+# RUN: ld.lld %t.o --as-needed %ta.so -o %t
+# RUN: llvm-readelf -d %t | FileCheck %s
+# RUN: ld.lld %t.o --as-needed %ta.so --gc-sections -o %t
+# RUN: llvm-readelf -d %t | FileCheck %s
+
+# The order of %ta.so and %t.o does not matter.
+
+# RUN: ld.lld --as-needed %ta.so %t.o -o %t
+# RUN: llvm-readelf -d %t | FileCheck %s
+# RUN: ld.lld --as-needed %ta.so %t.o --gc-sections -o %t
+# RUN: llvm-readelf -d %t | FileCheck %s
+
+# CHECK: a.so
+
+.global _start
+_start:
+  jmp a@PLT

test/ELF/as-needed-not-in-regular.s

+# REQUIRES: x86
+
+# RUN: echo '.globl a1, a2; .type a1, @function; .type a2, @function; a1: a2: ret' | \
+# RUN:   llvm-mc -filetype=obj -triple=x86_64-unknown-linux - -o %ta.o
+# RUN: ld.lld %ta.o --shared --soname=a.so -o %ta.so
+
+# RUN: echo '.globl b; .type b, @function; b: jmp a1@PLT' | \
+# RUN:   llvm-mc -filetype=obj -triple=x86_64-unknown-linux - -o %tb.o
+# RUN: ld.lld %tb.o %ta.so --shared --soname=b.so -o %tb.so
+
+# RUN: llvm-mc -filetype=obj -triple=x86_64-unknown-linux %s -o %t.o
+# RUN: ld.lld %t.o %tb.so --as-needed %ta.so -o %t
+# RUN: llvm-readelf -d %t | FileCheck %s
+
+# RUN: ld.lld %t.o %tb.so --as-needed %ta.so --gc-sections -o %t
+# RUN: llvm-readelf -d %t | FileCheck %s
+
+# The symbol a1 (defined in a.so) is not referenced by a regular object,
+# the reference to a2 is weak, don't add a DT_NEEDED entry for a.so.
+# CHECK-NOT: a.so
+
+.global _start
+.weak a2
+_start:
+  jmp b@PLT
+  jmp a2