[ELF] Demote !isUsedInRegularObj lazy symbol

[MaskRay]

Summary

I think D79300 has fixed the D51892 (__i686.get_pc_thunk.bx) issue, so
we can bring back rL330869.
D79300 says would error undefined symbol instead of the more relevant discarded section
but it doesn't reproduce now.

This avoids a quirk in isUndefWeak().

[peter.smith]

WIll take a look in more detail next week. IIUC so far, we have a quirk in isUndefWeak because we don't, currently, demote lazy symbols that weren't resolved, due to a problem that we now think is resolved. At a glance it sounds reasonable to me, just need to understand what the original problem was and how D79300 fixes it.

[peter.smith]

LGTM. Following this through I can't think of a test case that we can write as if all is well it should error out before we get to the problem case.

lld/ELF/SyntheticSections.cpp

3165

IIUC we could write assert(!s.sym->isLazy()); here. Not sure whether it would be worth doing it in the long term as it will look somewhat arbitrary. Maybe worth a test run with it in just to see if it hits anything unexpected.

[MaskRay]

add an assert

[MaskRay]

Thanks for the review!

lld/ELF/Driver.cpp

 // 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.
 static void demoteSharedSymbols() {
   llvm::TimeTraceScope timeScope("Demote shared symbols");
   for (Symbol *sym : symtab->symbols()) {
     auto *s = dyn_cast<SharedSymbol>(sym);
-    if (!s || s->getFile().isNeeded)
+    if (!((s && !s->getFile().isNeeded) ||
+          (sym->isLazy() && sym->isUsedInRegularObj)))
       continue;
 
-    bool used = s->used;
-    s->replace(Undefined{nullptr, s->getName(), STB_WEAK, s->stOther, s->type});
-    s->used = used;
+    bool used = sym->used;
+    sym->replace(
+        Undefined{nullptr, sym->getName(), STB_WEAK, sym->stOther, sym->type});
+    sym->used = used;
+    sym->versionId = VER_NDX_GLOBAL;
   }
 }
 
 // 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))
     if (d->section)

lld/ELF/Symbols.h

public:
   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. This only works once
   // all input files have been added.
-  bool isUndefWeak() const {
-    // See comment on lazy symbols for details.
-    return isWeak() && (isUndefined() || isLazy());
-  }
+  bool isUndefWeak() const { return isWeak() && isUndefined(); }
 
   StringRef getName() const {
     if (nameSize == (uint32_t)-1)
       nameSize = strlen(nameData);
     return {nameData, nameSize};
   }
 
   void setName(StringRef s) {

lld/ELF/SyntheticSections.cpp

 
 size_t VersionTableSection::getSize() const {
   return (getPartition().dynSymTab->getSymbols().size() + 1) * 2;
 }
 
 void VersionTableSection::writeTo(uint8_t *buf) {
   buf += 2;
   for (const SymbolTableEntry &s : getPartition().dynSymTab->getSymbols()) {
-    // Use the original versionId for an unfetched lazy symbol (undefined weak),
-    // which must be VER_NDX_GLOBAL (an undefined versioned symbol is an error).
-    write16(buf, s.sym->isLazy() ? static_cast<uint16_t>(VER_NDX_GLOBAL)
-                                 : s.sym->versionId);
+    // For an unfetched lazy symbol (undefined weak), it must have been
+    // converted to Undefined and have VER_NDX_GLOBAL version here.
+    assert(!s.sym->isLazy());
+    write16(buf, s.sym->versionId);
     buf += 2;
   }
 }
 
 bool VersionTableSection::isNeeded() const {
   return isLive() &&
          (getPartition().verDef || getPartition().verNeed->isNeeded());
 }