Do not keep shared symbols to garbage-collected eliminated DSOs.

[ruiu]

Summary

If all references to a DSO happen to be weak, --gc-sections eliminates
the DSO from DT_NEEDED. If that happens, we also need to eliminate
shared symbols created from the DSO. Otehrwise, they become dangling
references that point to non-exsitent DSO.

Fixes https://bugs.llvm.org/show_bug.cgi?id=36991

[pcc]

Does this also fix PR28335?

[ruiu]

I"m not sure if I understand https://bugs.llvm.org/show_bug.cgi?id=28335 correctly, but it sounds like it fixes that bug as well. The intention of this change is, when a DSO is eliminated because of the garbage collection, all traces of the DSO should be removed as if the DSO weren't present in the command line from the beginning.

[andrewrk]

I confirm that this fixes https://bugs.llvm.org/show_bug.cgi?id=36991

[pcc]

From a quick test it looks like this fixes PR28335 but only if --gc-sections is passed. Maybe we should be doing this unconditionally?

[ruiu]

• replace dangling shared symbols even if --gc-sections is not specified

[pcc]

lld/ELF/Driver.cpp

1065 ↗

(On Diff #142083)

nit: non-existent

lld/test/ELF/as-needed-weak.s

11 ↗

(On Diff #142083)

Won't this pass without the change?

Probably a better way to test this would be to check that foo is relocated properly.

[ruiu]

• fix a test and confirmed that the test fails without this patch

[pcc]

LGTM

[• espindola]

lld/ELF/Driver.cpp

1069 ↗

(On Diff #142100)

Is this doing the right thing if the symbol in the shared library is weak? Is it necessary? Doesn't IsNeeded already include all the required information?

[ruiu]

lld/ELF/Driver.cpp

1069 ↗

(On Diff #142100)

I believe this does the right thing for weak symbols in shared libraries, because if it is in use, IsNeeded will never be false. Otherwise, the symbol should be handled as if it weren't present from the beginning, so replacing it with an (unresolved) weak undefined seems like the right thing to do.

We could technically ignore shared symbols with a dead file everywhere, but I think purging such symbols all at once before processing further is less error-prone.

[• espindola]

LGTM with a small change.

lld/ELF/Driver.cpp

1069 ↗

(On Diff #142100)

So the isWeak is really redundant. Please simplify this if to

if (!S->getFile<ELFT>().IsNeeded)

[rnk]

This is causing LLD to drop __cxa_finalize from the symbol tables of some Chromium binaries, and that causes them to crash on shutdown. See the test failures here:
https://ci.chromium.org/buildbot/chromium.clang/ToTLinux/2152

I'm going to revert this for now. I found that re-linking just libipc_mojom_shared.so and libmojo_mojom_bindings_shared.so with LLD after this change causes ipc_tests to crash reliably on shutdown.

[• espindola]

In D45536#1069314, @rnk wrote:

This is causing LLD to drop __cxa_finalize from the symbol tables of some Chromium binaries, and that causes them to crash on shutdown. See the test failures here:
https://ci.chromium.org/buildbot/chromium.clang/ToTLinux/2152

I'm going to revert this for now. I found that re-linking just libipc_mojom_shared.so and libmojo_mojom_bindings_shared.so with LLD after this change causes ipc_tests to crash reliably on shutdown.

Could you please put a --reproduce package of those libraries in a bug report? Thanks!

[rnk]

In D45536#1069392, @espindola wrote:

Could you please put a --reproduce package of those libraries in a bug report? Thanks!

I made a reproducer here: https://drive.google.com/file/d/1LuMUQQK1F1AT5xJnGonBdeODqilqX5Fp/view?usp=sharing

This change caused __cxa_finalize to be removed from the dynamic symbol table, which ultimately lead to a crash in __do_global_dtors_aux during shutdown.

[pcc]

lld/trunk/ELF/Driver.cpp

1070 ↗

(On Diff #142270)

Does it work to replace these with absolute symbols with value 0?

[• espindola]

I am finally trying to reduce what went wrong with this patch.

lld/ELF/MarkLive.cpp

for (InputSectionBase *Sec : InputSections) {
     bool IsRel = (Sec->Type == SHT_REL || Sec->Type == SHT_RELA);
     if (!IsAlloc && !IsRel)
       Sec->Live = true;
   }
 
   // Follow the graph to mark all live sections.
   doGcSections<ELFT>();
 
+  // If all references to a DSO happen to be weak, the DSO is removed from
+  // DT_NEEDED, which creates dangling shared symbols to non-existent DSO.
+  // We'll replace such symbols with undefined ones to fix it.
+  for (Symbol *Sym : Symtab->getSymbols())
+    if (auto *S = dyn_cast<SharedSymbol>(Sym))
+      if (S->isWeak() && !S->getFile<ELFT>().IsNeeded)
+        replaceSymbol<Undefined>(S, nullptr, S->getName(), STB_WEAK, S->StOther,
+                                 S->Type);
+
   // Report garbage-collected sections.
   if (Config->PrintGcSections)
     for (InputSectionBase *Sec : InputSections)
       if (!Sec->Live)
         message("removing unused section " + toString(Sec));
 }
 
 template void elf::markLive<ELF32LE>();
 template void elf::markLive<ELF32BE>();
 template void elf::markLive<ELF64LE>();
 template void elf::markLive<ELF64BE>();

lld/test/ELF/gc-sections-shared.s

 # REQUIRES: x86
 # RUN: llvm-mc -filetype=obj -triple=x86_64-unknown-linux %p/Inputs/shared.s -o %t2.o
 # RUN: llvm-mc -filetype=obj -triple=x86_64-unknown-linux %p/Inputs/gc-sections-shared.s -o %t3.o
 # RUN: llvm-mc -filetype=obj -triple=x86_64-unknown-linux %p/Inputs/gc-sections-shared2.s -o %t4.o
 # RUN: ld.lld -shared %t2.o -o %t2.so
 # RUN: ld.lld -shared %t3.o -o %t3.so
 # RUN: ld.lld -shared %t4.o -o %t4.so
 # RUN: llvm-mc -filetype=obj -triple=x86_64-unknown-linux %s -o %t.o
 # RUN: ld.lld --gc-sections --export-dynamic-symbol foo -o %t %t.o --as-needed %t2.so %t3.so %t4.so
 # RUN: llvm-readobj --dynamic-table --dyn-symbols %t | FileCheck %s
 
-# This test the property that we have a needed line for every undefined.
-# It would also be OK to keep bar2 and the need for %t2.so
-# At the same time, weak symbols should not cause adding DT_NEEDED;
-# this case is checked with symbol qux and %t4.so.
-
 # CHECK:      DynamicSymbols [
 # CHECK-NEXT:   Symbol {
 # CHECK-NEXT:     Name:
 # CHECK-NEXT:     Value:
 # CHECK-NEXT:     Size:
 # CHECK-NEXT:     Binding: Local
 # CHECK-NEXT:     Type:
 # CHECK-NEXT:     Other:

 # CHECK-NEXT:     Name: foo
 # CHECK-NEXT:     Value:
 # CHECK-NEXT:     Size:
 # CHECK-NEXT:     Binding: Global
 # CHECK-NEXT:     Type:
 # CHECK-NEXT:     Other:
 # CHECK-NEXT:     Section: .text
 # CHECK-NEXT:   }
-# CHECK-NEXT:   Symbol {
-# CHECK-NEXT:     Name: qux
-# CHECK-NEXT:     Value:
-# CHECK-NEXT:     Size:
-# CHECK-NEXT:     Binding: Weak
-# CHECK-NEXT:     Type:
-# CHECK-NEXT:     Other:
-# CHECK-NEXT:     Section: Undefined
-# CHECK-NEXT:   }
 # CHECK-NEXT: ]
 
 # CHECK-NOT: NEEDED
 # CHECK:     NEEDED Shared library: [{{.*}}3.so]
 # CHECK-NOT: NEEDED
 
 # Test with %t.o at the end too.
 # RUN: ld.lld --gc-sections --export-dynamic-symbol foo -o %t --as-needed %t2.so %t3.so %t4.so %t.o

 # CHECK2-NEXT:     Value:
 # CHECK2-NEXT:     Size:
 # CHECK2-NEXT:     Binding: Global
 # CHECK2-NEXT:     Type:
 # CHECK2-NEXT:     Other:
 # CHECK2-NEXT:     Section: Undefined
 # CHECK2-NEXT:   }
 # CHECK2-NEXT:   Symbol {
-# CHECK2-NEXT:     Name: qux
-# CHECK2-NEXT:     Value:
-# CHECK2-NEXT:     Size:
-# CHECK2-NEXT:     Binding: Weak
-# CHECK2-NEXT:     Type:
-# CHECK2-NEXT:     Other:
-# CHECK2-NEXT:     Section: Undefined
-# CHECK2-NEXT:   }
-# CHECK2-NEXT:   Symbol {
 # CHECK2-NEXT:     Name: foo
 # CHECK2-NEXT:     Value:
 # CHECK2-NEXT:     Size:
 # CHECK2-NEXT:     Binding: Global
 # CHECK2-NEXT:     Type:
 # CHECK2-NEXT:     Other:
 # CHECK2-NEXT:     Section: .text
 # CHECK2-NEXT:   }