Revert "Also demote lazy symbols."

[chh]

Summary

This reverts commit https://reviews.llvm.org/rL330869
for a regression to link Android dex2oatds.

[ruiu]

Can you add a test that failed without this revert?

[chh]

Rui, could you help me creating a test?
All I have now is Android's link command for dex2oatds.
I don't know how to reduce that to an lld unit test.

[ruiu]

I guess if you just pass a single file that contains undefined symbol __i686.get_pc_thunk.bx, you can reproduce the issue, no?

[chh]

Add a unit test case.

[ruiu]

We generally don't check in a binary file unless there's no way to generate a file from text. Looks like we already have a test that creates a .gnu.linkonce section, see test/ELF/comdat-linkonce.s. Could you create a test based on it?

test/ELF/i386-linkonce.s

3 ↗

(On Diff #164952)

Please write a test that is expected to succeed to make it more reliable.

[chh]

Make it a positive unit test. Create library to test from .s file.

[ruiu]

LGTM

Thank you for doing this!

test/ELF/i386-linkonce.s

9 ↗

(On Diff #164989)

nit: calll -> call for consistency with other tests.

ELF/Driver.cpp

if (auto *LO = dyn_cast<LazyObject>(Sym))
     MB = LO->File->MB;
   else
     MB = cast<LazyArchive>(Sym)->getMemberBuffer();
 
   if (isBitcode(MB))
     Symtab->fetchLazy<ELFT>(Sym);
 }
 
-template <class ELFT> static bool shouldDemote(Symbol &Sym) {
-  // If all references to a DSO happen to be weak, the DSO is not added to
-  // DT_NEEDED. If that happens, we need to eliminate shared symbols created
-  // from the DSO. Otherwise, they become dangling references that point to a
-  // non-existent DSO.
-  if (auto *S = dyn_cast<SharedSymbol>(&Sym))
-    return !S->getFile<ELFT>().IsNeeded;
-
-  // We are done processing archives, so lazy symbols that were used but not
-  // found can be converted to undefined. We could also just delete the other
-  // lazy symbols, but that seems to be more work than it is worth.
-  return Sym.isLazy() && Sym.IsUsedInRegularObj;
-}
-
-// Some files, such as .so or files between -{start,end}-lib may be removed
-// after their symbols are added to the symbol table. If that happens, we
-// need to remove symbols that refer files that no longer exist, so that
-// they won't appear in the symbol table of the output file.
-//
-// We remove symbols by demoting them to undefined symbol.
-template <class ELFT> static void demoteSymbols() {
+// If all references to a DSO happen to be weak, the DSO is not added
+// to DT_NEEDED. If that happens, we need to eliminate shared symbols
+// created from the DSO. Otherwise, they become dangling references
+// that point to a non-existent DSO.
+template <class ELFT> static void demoteSharedSymbols() {
   for (Symbol *Sym : Symtab->getSymbols()) {
-    if (shouldDemote<ELFT>(*Sym)) {
-      bool Used = Sym->Used;
-      replaceSymbol<Undefined>(Sym, nullptr, Sym->getName(), Sym->Binding,
-                               Sym->StOther, Sym->Type);
-      Sym->Used = Used;
+    if (auto *S = dyn_cast<SharedSymbol>(Sym)) {
+      if (!S->getFile<ELFT>().IsNeeded) {
+        bool Used = S->Used;
+        replaceSymbol<Undefined>(S, nullptr, S->getName(), STB_WEAK, S->StOther,
+                                 S->Type);
+        S->Used = Used;
+      }
     }
   }
 }
 
 // The section referred to by S is considered address-significant. Set the
 // KeepUnique flag on the section if appropriate.
 static void markAddrsig(Symbol *S) {
   if (auto *D = dyn_cast_or_null<Defined>(S))
template <class ELFT> void LinkerDriver::link(opt::InputArgList &Args) {
   if (!Config->Relocatable)
     InputSections.push_back(createCommentSection());
 
   // Do size optimizations: garbage collection, merging of SHF_MERGE sections
   // and identical code folding.
   decompressSections();
   splitSections<ELFT>();
   markLive<ELFT>();
-  demoteSymbols<ELFT>();
+  demoteSharedSymbols<ELFT>();
   mergeSections();
   if (Config->ICF != ICFLevel::None) {
     findKeepUniqueSections<ELFT>(Args);
     doIcf<ELFT>();
   }
 
   // Read the callgraph now that we know what was gced or icfed
   if (auto *Arg = Args.getLastArg(OPT_call_graph_ordering_file))
     if (Optional<MemoryBufferRef> Buffer = readFile(Arg->getValue()))
       readCallGraph(*Buffer);
 
   // Write the result to the file.
   writeResult<ELFT>();
 }

ELF/Symbols.h

public:
   bool isDefined() const { return SymbolKind == DefinedKind; }
   bool isShared() const { return SymbolKind == SharedKind; }
   bool isLocal() const { return Binding == llvm::ELF::STB_LOCAL; }
 
   bool isLazy() const {
     return SymbolKind == LazyArchiveKind || SymbolKind == LazyObjectKind;
   }
 
-  // True if this is an undefined weak symbol.
-  bool isUndefWeak() const { return isWeak() && isUndefined(); }
+  // True if this is an undefined weak symbol. This only works once
+  // all input files have been added.
+  bool isUndefWeak() const {
+    // See comment on lazy symbols for details.
+    return isWeak() && (isUndefined() || isLazy());
+  }
 
   StringRef getName() const {
     if (NameSize == (uint32_t)-1)
       NameSize = strlen(NameData);
     return {NameData, NameSize};
   }
 
   void setName(StringRef S) {

ELF/Symbols.cpp

case Symbol::DefinedKind: {
     }
     return VA;
   }
   case Symbol::SharedKind:
   case Symbol::UndefinedKind:
     return 0;
   case Symbol::LazyArchiveKind:
   case Symbol::LazyObjectKind:
-    llvm_unreachable("lazy symbol reached writer");
+    assert(Sym.IsUsedInRegularObj && "lazy symbol reached writer");
+    return 0;
   }
   llvm_unreachable("invalid symbol kind");
 }
 
 uint64_t Symbol::getVA(int64_t Addend) const {
   uint64_t OutVA = getSymVA(*this, Addend);
   return OutVA + Addend;
 }