[ORC][JITLink] Retain Weak flags in JITDylib interfaces, propagate to LinkGraph.

Previously we stripped Weak flags from JITDylib symbol table entries once they
were resolved (there was no particularly good reason for this). Now we want to
retain them and query them when setting the Linkage on external symbols in
LinkGraphs during symbol resolution (this was the motivation for 75404e9).
Making weak linkage of external definitions discoverable in the LinkGraph will
in turn allow future plugins to implement correct handling for them (by
recording locations that depend on exported weak definitions and pointing all
of these at one chosen definition at runtime).

Author: lhames

llvm/include/llvm/ExecutionEngine/JITLink/JITLink.h

@@ -413,19 +413,6 @@ class Symbol {
     setCallable(IsCallable);
   }
 
-  static Symbol &constructCommon(BumpPtrAllocator &Allocator, Block &Base,
-                                 StringRef Name, orc::ExecutorAddrDiff Size,
-                                 Scope S, bool IsLive) {
-    assert(!Name.empty() && "Common symbol name cannot be empty");
-    assert(Base.isDefined() &&
-           "Cannot create common symbol from undefined block");
-    assert(static_cast<Block &>(Base).getSize() == Size &&
-           "Common symbol size should match underlying block size");
-    auto *Sym = Allocator.Allocate<Symbol>();
-    new (Sym) Symbol(Base, 0, Name, Size, Linkage::Weak, S, IsLive, false);
-    return *Sym;
-  }
-
   static Symbol &constructExternal(BumpPtrAllocator &Allocator,
                                    Addressable &Base, StringRef Name,
                                    orc::ExecutorAddrDiff Size, Linkage L,
@@ -1127,22 +1114,6 @@ class LinkGraph {
     return Sym;
   }
 
-  /// Convenience method for adding a weak zero-fill symbol.
-  Symbol &addCommonSymbol(StringRef Name, Scope S, Section &Section,
-                          orc::ExecutorAddr Address, orc::ExecutorAddrDiff Size,
-                          uint64_t Alignment, bool IsLive) {
-    assert(llvm::count_if(defined_symbols(),
-                          [&](const Symbol *Sym) {
-                            return Sym->getName() == Name;
-                          }) == 0 &&
-           "Duplicate defined symbol");
-    auto &Sym = Symbol::constructCommon(
-        Allocator, createBlock(Section, Size, Address, Alignment, 0), Name,
-        Size, S, IsLive);
-    Section.addSymbol(Sym);
-    return Sym;
-  }
-
   /// Add an anonymous symbol.
   Symbol &addAnonymousSymbol(Block &Content, orc::ExecutorAddrDiff Offset,
                              orc::ExecutorAddrDiff Size, bool IsCallable,

llvm/lib/ExecutionEngine/JITLink/COFFLinkGraphBuilder.cpp

@@ -454,9 +454,11 @@ Expected<Symbol *> COFFLinkGraphBuilder::createDefinedSymbol(
     object::COFFSymbolRef Symbol, const object::coff_section *Section) {
   if (Symbol.isCommon()) {
     // FIXME: correct alignment
-    return &G->addCommonSymbol(SymbolName, Scope::Default, getCommonSection(),
-                               orc::ExecutorAddr(), Symbol.getValue(),
-                               Symbol.getValue(), false);
+    return &G->addDefinedSymbol(
+        G->createZeroFillBlock(getCommonSection(), Symbol.getValue(),
+                               orc::ExecutorAddr(), Symbol.getValue(), 0),
+        0, SymbolName, Symbol.getValue(), Linkage::Strong, Scope::Default,
+        false, false);
   }
   if (Symbol.isAbsolute())
     return &G->addAbsoluteSymbol(SymbolName,

llvm/lib/ExecutionEngine/JITLink/ELFLinkGraphBuilder.h

@@ -402,9 +402,10 @@ template <typename ELFT> Error ELFLinkGraphBuilder<ELFT>::graphifySymbols() {
 
     // Handle common symbols specially.
     if (Sym.isCommon()) {
-      Symbol &GSym = G->addCommonSymbol(*Name, Scope::Default,
-                                        getCommonSection(), orc::ExecutorAddr(),
-                                        Sym.st_size, Sym.getValue(), false);
+      Symbol &GSym = G->addDefinedSymbol(
+          G->createZeroFillBlock(getCommonSection(), Sym.st_size,
+                                 orc::ExecutorAddr(), Sym.getValue(), 0),
+          0, *Name, Sym.st_size, Linkage::Strong, Scope::Default, false, false);
       setGraphSymbol(SymIndex, GSym);
       continue;
     }

llvm/lib/ExecutionEngine/JITLink/JITLinkGeneric.cpp

@@ -217,19 +217,29 @@ void JITLinkerBase::applyLookupResult(AsyncLookupResult Result) {
     assert(!Sym->getAddress() && "Symbol already resolved");
     assert(!Sym->isDefined() && "Symbol being resolved is already defined");
     auto ResultI = Result.find(Sym->getName());
-    if (ResultI != Result.end())
+    if (ResultI != Result.end()) {
       Sym->getAddressable().setAddress(
           orc::ExecutorAddr(ResultI->second.getAddress()));
-    else
+      Sym->setLinkage(ResultI->second.getFlags().isWeak() ? Linkage::Weak
+                                                          : Linkage::Strong);
+    } else
       assert(Sym->isWeaklyReferenced() &&
              "Failed to resolve non-weak reference");
   }
 
   LLVM_DEBUG({
     dbgs() << "Externals after applying lookup result:\n";
-    for (auto *Sym : G->external_symbols())
+    for (auto *Sym : G->external_symbols()) {
       dbgs() << "  " << Sym->getName() << ": "
-             << formatv("{0:x16}", Sym->getAddress().getValue()) << "\n";
+             << formatv("{0:x16}", Sym->getAddress().getValue());
+      switch (Sym->getLinkage()) {
+      case Linkage::Strong:
+        break;
+      case Linkage::Weak:
+        dbgs() << " (weak)";
+      }
+      dbgs() << "\n";
+    }
   });
 }
 

llvm/lib/ExecutionEngine/JITLink/MachOLinkGraphBuilder.cpp

@@ -350,11 +350,13 @@ Error MachOLinkGraphBuilder::graphifyRegularSymbols() {
         if (!NSym.Name)
           return make_error<JITLinkError>("Anonymous common symbol at index " +
                                           Twine(KV.first));
-        NSym.GraphSymbol = &G->addCommonSymbol(
-            *NSym.Name, NSym.S, getCommonSection(), orc::ExecutorAddr(),
-            orc::ExecutorAddrDiff(NSym.Value),
-            1ull << MachO::GET_COMM_ALIGN(NSym.Desc),
-            NSym.Desc & MachO::N_NO_DEAD_STRIP);
+        NSym.GraphSymbol = &G->addDefinedSymbol(
+            G->createZeroFillBlock(getCommonSection(),
+                                   orc::ExecutorAddrDiff(NSym.Value),
+                                   orc::ExecutorAddr(),
+                                   1ull << MachO::GET_COMM_ALIGN(NSym.Desc), 0),
+            0, *NSym.Name, orc::ExecutorAddrDiff(NSym.Value), Linkage::Strong,
+            NSym.S, false, NSym.Desc & MachO::N_NO_DEAD_STRIP);
       } else {
         if (!NSym.Name)
           return make_error<JITLinkError>("Anonymous external symbol at "

llvm/lib/ExecutionEngine/Orc/Core.cpp

@@ -970,10 +970,9 @@ Error JITDylib::resolve(MaterializationResponsibility &MR,
             SymbolsInErrorState.insert(KV.first);
           else {
             auto Flags = KV.second.getFlags();
-            Flags &= ~(JITSymbolFlags::Weak | JITSymbolFlags::Common);
+            Flags &= ~JITSymbolFlags::Common;
             assert(Flags ==
-                       (SymI->second.getFlags() &
-                        ~(JITSymbolFlags::Weak | JITSymbolFlags::Common)) &&
+                       (SymI->second.getFlags() & ~JITSymbolFlags::Common) &&
                    "Resolved flags should match the declared flags");
 
             Worklist.push_back(
@@ -2909,13 +2908,13 @@ Error ExecutionSession::OL_notifyResolved(MaterializationResponsibility &MR,
   });
 #ifndef NDEBUG
   for (auto &KV : Symbols) {
-    auto WeakFlags = JITSymbolFlags::Weak | JITSymbolFlags::Common;
     auto I = MR.SymbolFlags.find(KV.first);
     assert(I != MR.SymbolFlags.end() &&
            "Resolving symbol outside this responsibility set");
     assert(!I->second.hasMaterializationSideEffectsOnly() &&
            "Can't resolve materialization-side-effects-only symbol");
-    assert((KV.second.getFlags() & ~WeakFlags) == (I->second & ~WeakFlags) &&
+    assert((KV.second.getFlags() & ~JITSymbolFlags::Common) ==
+               (I->second & ~JITSymbolFlags::Common) &&
            "Resolving symbol with incorrect flags");
   }
 #endif

llvm/lib/ExecutionEngine/Orc/ObjectLinkingLayer.cpp

@@ -218,6 +218,8 @@ class ObjectLinkingLayerJITLinkContext final : public JITLinkContext {
           Flags |= JITSymbolFlags::Callable;
         if (Sym->getScope() == Scope::Default)
           Flags |= JITSymbolFlags::Exported;
+        if (Sym->getLinkage() == Linkage::Weak)
+          Flags |= JITSymbolFlags::Weak;
 
         InternedResult[InternedName] =
             JITEvaluatedSymbol(Sym->getAddress().getValue(), Flags);

llvm/lib/ExecutionEngine/Orc/RTDyldObjectLinkingLayer.cpp

@@ -270,17 +270,21 @@ Error RTDyldObjectLinkingLayer::onObjLoad(
 
     auto InternedName = getExecutionSession().intern(KV.first);
     auto Flags = KV.second.getFlags();
-
-    // Override object flags and claim responsibility for symbols if
-    // requested.
-    if (OverrideObjectFlags || AutoClaimObjectSymbols) {
-      auto I = R.getSymbols().find(InternedName);
-
-      if (OverrideObjectFlags && I != R.getSymbols().end())
+    auto I = R.getSymbols().find(InternedName);
+    if (I != R.getSymbols().end()) {
+      // Override object flags and claim responsibility for symbols if
+      // requested.
+      if (OverrideObjectFlags)
         Flags = I->second;
-      else if (AutoClaimObjectSymbols && I == R.getSymbols().end())
-        ExtraSymbolsToClaim[InternedName] = Flags;
-    }
+      else {
+        // RuntimeDyld/MCJIT's weak tracking isn't compatible with ORC's. Even
+        // if we're not overriding flags in general we should set the weak flag
+        // according to the MaterializationResponsibility object symbol table.
+        if (I->second.isWeak())
+          Flags |= JITSymbolFlags::Weak;
+      }
+    } else if (AutoClaimObjectSymbols)
+      ExtraSymbolsToClaim[InternedName] = Flags;
 
     Symbols[InternedName] = JITEvaluatedSymbol(KV.second.getAddress(), Flags);
   }

llvm/test/ExecutionEngine/JITLink/X86/COFF_common_symbol.s

@@ -6,7 +6,7 @@
 #
 # CHECK: Creating graph symbols...
 # CHECK:      7: Creating defined graph symbol for COFF symbol "var" in (common) (index: 0)
-# CHECK-NEXT:   0x0 (block + 0x00000000): size: 0x00000004, linkage: weak, scope: default, dead  -   var
+# CHECK-NEXT:   0x0 (block + 0x00000000): size: 0x00000004, linkage: strong, scope: default, dead  -   var
 
 	.text