[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

509–510

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

509–510

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

512–514

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

512–514

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

512–514

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/SymbolTable.cpp

if (WasInserted) {
     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)
+    if (!S->isWeak() && S->IsUsedInRegularObj && !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;

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

+# REQUIRES: x86
+
+# RUN: echo '.globl a; .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: echo '.globl b; .type b, @function; b: jmp a@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 a (defined in a.so) is not referenced by a regular object,
+# don't add a DT_NEEDED entry for it.
+# CHECK-NOT: a.so
+
+.global _start
+_start:
+  jmp b@PLT