Remove SymbolTable::addUndefined and add SymbolTable::addUndefined{Function,Global}

[ruiu]

Summary

This patch should make the flow of control simpler, and this should
also make it clear that which information is actually used by the
symbol table. For example, previously, WasmSignature wasn't used
for globals.

[ruiu]

• use toString() instead of getName().

[ruiu]

• removed unused parameter from shouldReplace

[sbc100]

Sorry I didn't see this earlier. Looks good but will need rebase since the symtab change langed.

[sbc100]

Still planning on landing this?

[ruiu]

I think this is obsolete as most of the code has already been submitted.

lld/wasm/Driver.cpp

static StringRef getEntry(opt::InputArgList &Args, StringRef Default) {
   auto *Arg = Args.getLastArg(OPT_entry, OPT_no_entry);
   if (!Arg)
     return Default;
   if (Arg->getOption().getID() == OPT_no_entry)
     return "";
   return Arg->getValue();
 }
 
-static Symbol* addUndefinedFunction(StringRef Name, const WasmSignature *Type) {
-  return Symtab->addUndefined(Name, Symbol::UndefinedFunctionKind, 0, nullptr,
-                              Type);
-}
-
 void LinkerDriver::link(ArrayRef<const char *> ArgsArr) {
   WasmOptTable Parser;
   opt::InputArgList Args = Parser.parse(ArgsArr.slice(1));
 
   // Handle --help
   if (Args.hasArg(OPT_help)) {
     printHelp(ArgsArr[0]);
     return;
if (!Config->Relocatable) {
     WasmSym::CallCtors = Symtab->addSyntheticFunction(
         "__wasm_call_ctors", &NullSignature, WASM_SYMBOL_VISIBILITY_HIDDEN);
     WasmSym::StackPointer = Symtab->addSyntheticGlobal("__stack_pointer");
     WasmSym::HeapBase = Symtab->addSyntheticGlobal("__heap_base");
     WasmSym::DsoHandle = Symtab->addSyntheticGlobal("__dso_handle");
     WasmSym::DataEnd = Symtab->addSyntheticGlobal("__data_end");
 
     if (!Config->Entry.empty())
-      EntrySym = addUndefinedFunction(Config->Entry, &NullSignature);
+      EntrySym = Symtab->addUndefinedFunction(Config->Entry, 0, nullptr,
+                                              &NullSignature);
 
     // Handle the `--undefined <sym>` options.
     for (auto* Arg : Args.filtered(OPT_undefined))
-      addUndefinedFunction(Arg->getValue(), nullptr);
+      Symtab->addUndefinedFunction(Arg->getValue(), 0, nullptr, nullptr);
   }
 
   createFiles(Args);
   if (errorCount())
     return;
 
   // Add all files to the symbol table. This will add almost all
   // symbols that we need to the symbol table.

lld/wasm/InputFiles.cpp

case WasmSymbol::SymbolType::FUNCTION_EXPORT: {
       if (!isExcludedByComdat(Function)) {
         S = createDefinedFunction(WasmSym, Function);
         break;
       }
       Function->Live = false;
       LLVM_FALLTHROUGH; // Exclude function, and add the symbol as undefined
     }
     case WasmSymbol::SymbolType::FUNCTION_IMPORT:
-      S = createUndefined(WasmSym, Symbol::Kind::UndefinedFunctionKind,
+      S = Symtab->addUndefinedFunction(WasmSym.Name, WasmSym.Flags, this,
                                        getFunctionSig(WasmSym));
       break;
     case WasmSymbol::SymbolType::GLOBAL_EXPORT: {
       InputSegment *Segment = getSegment(WasmSym);
       if (!isExcludedByComdat(Segment)) {
         S = createDefinedGlobal(WasmSym, Segment, getGlobalValue(WasmSym));
         break;
       }
       Segment->Live = false;
       LLVM_FALLTHROUGH; // Exclude global, and add the symbol as undefined
     }
     case WasmSymbol::SymbolType::GLOBAL_IMPORT:
-      S = createUndefined(WasmSym, Symbol::Kind::UndefinedGlobalKind);
+      S = Symtab->addUndefinedGlobal(WasmSym.Name, WasmSym.Flags, this);
       break;
     }
 
     Symbols.push_back(S);
     if (WasmSym.isTypeFunction()) {
       FunctionSymbols[WasmSym.ElementIndex] = S;
       if (WasmSym.HasAltIndex)
         FunctionSymbols[WasmSym.AltIndex] = S;
     } else {
       GlobalSymbols[WasmSym.ElementIndex] = S;
       if (WasmSym.HasAltIndex)
         GlobalSymbols[WasmSym.AltIndex] = S;
     }
   }
 
   DEBUG(for (size_t I = 0; I < FunctionSymbols.size(); ++I)
             assert(FunctionSymbols[I] != nullptr);
         for (size_t I = 0; I < GlobalSymbols.size(); ++I)
             assert(GlobalSymbols[I] != nullptr););
 
   DEBUG(dbgs() << "Functions   : " << FunctionSymbols.size() << "\n");
   DEBUG(dbgs() << "Globals     : " << GlobalSymbols.size() << "\n");
 }
 
-Symbol *ObjFile::createUndefined(const WasmSymbol &Sym, Symbol::Kind Kind,
-                                 const WasmSignature *Signature) {
-  return Symtab->addUndefined(Sym.Name, Kind, Sym.Flags, this, Signature);
-}
-
 Symbol *ObjFile::createDefinedFunction(const WasmSymbol &Sym,
                                        InputFunction *Function) {
   if (Sym.isBindingLocal())
     return make<DefinedFunction>(Sym.Name, Sym.Flags, this, Function);
   return Symtab->addDefinedFunction(Sym.Name, Sym.Flags, this, Function);
 }
 
 Symbol *ObjFile::createDefinedGlobal(const WasmSymbol &Sym,

lld/wasm/SymbolTable.h

public:
   Symbol *find(StringRef Name);
   ObjFile *findComdat(StringRef Name) const;
 
   Symbol *addDefinedFunction(StringRef Name, uint32_t Flags, InputFile *F,
                              InputFunction *Function = nullptr);
   Symbol *addDefinedGlobal(StringRef Name, uint32_t Flags, InputFile *F,
                            InputSegment *Segment = nullptr,
                            uint32_t Address = 0);
-  Symbol *addUndefined(StringRef Name, Symbol::Kind Kind, uint32_t Flags,
-                       InputFile *F, const WasmSignature *Signature = nullptr);
-  Symbol *addUndefinedFunction(StringRef Name, const WasmSignature *Type);
+  Symbol *addUndefinedFunction(StringRef Name, uint32_t Flags, InputFile *F,
+                               const WasmSignature *Type);
+  Symbol *addUndefinedGlobal(StringRef Name, uint32_t Flags, InputFile *F);
   void addLazy(ArchiveFile *F, const Archive::Symbol *Sym);
   bool addComdat(StringRef Name, ObjFile *);
 
   DefinedGlobal *addSyntheticGlobal(StringRef Name, uint32_t Flags = 0);
   DefinedFunction *addSyntheticFunction(StringRef Name,
                                         const WasmSignature *Type,
                                         uint32_t Flags = 0);
 private:

lld/wasm/SymbolTable.cpp

if (Sym)
     return {Sym, false};
   Sym = reinterpret_cast<Symbol *>(make<SymbolUnion>());
   SymVector.emplace_back(Sym);
   return {Sym, true};
 }
 
 // Check the type of new symbol matches that of the symbol is replacing.
 // For functions this can also involve verifying that the signatures match.
-static void checkSymbolTypes(const Symbol &Existing, const InputFile &F,
-                             bool NewIsFunction, const WasmSignature *NewSig) {
+static void checkFunctionTypes(const Symbol &Existing, const InputFile &F,
+                               const WasmSignature *NewSig) {
   if (Existing.isLazy())
     return;
 
   // First check the symbol types match (i.e. either both are function
   // symbols or both are data symbols).
-  if (isa<FunctionSymbol>(Existing) != NewIsFunction) {
-    error("symbol type mismatch: " + Existing.getName() + "\n>>> defined as " +
-          (isa<FunctionSymbol>(Existing) ? "Function" : "Global") + " in " +
-          toString(Existing.getFile()) + "\n>>> defined as " +
-          (NewIsFunction ? "Function" : "Global") + " in " + F.getName());
+  if (!isa<FunctionSymbol>(Existing)) {
+    error("symbol type mismatch: " + toString(Existing) +
+          "\n>>> defined as Global in " + toString(Existing.getFile()) +
+          "\n>>> defined as Function in " + toString(&F));
     return;
   }
 
   // For function symbols, optionally check the function signature matches too.
-  auto *ExistingFunc = dyn_cast<FunctionSymbol>(&Existing);
-  if (!ExistingFunc || !Config->CheckSignatures)
+  if (!Config->CheckSignatures)
     return;
-
-  const WasmSignature *OldSig = ExistingFunc->getFunctionType();
+  auto *Func = cast<FunctionSymbol>(&Existing);
+  const WasmSignature *OldSig = Func->getFunctionType();
 
   // Skip the signature check if the existing function has no signature (e.g.
   // if it is an undefined symbol generated by --undefined command line flag).
-  if (OldSig == nullptr)
-    return;
-
-  DEBUG(dbgs() << "checkSymbolTypes: " << ExistingFunc->getName() << "\n");
-  assert(NewSig);
-
-  if (*NewSig == *OldSig)
-    return;
-
-  error("function signature mismatch: " + ExistingFunc->getName() +
+  if (OldSig && *NewSig != *OldSig) {
+    error("function signature mismatch: " + Func->getName() +
           "\n>>> defined as " + toString(*OldSig) + " in " +
-        toString(ExistingFunc->getFile()) + "\n>>> defined as " +
-        toString(*NewSig) + " in " + F.getName());
+          toString(Func->getFile()) + "\n>>> defined as " + toString(*NewSig) +
+          " in " + toString(&F));
+  }
 }
 
-static void checkSymbolTypes(const Symbol &Existing, const InputFile &F,
-                             bool IsFunction, const InputChunk *Chunk) {
-  const WasmSignature *Sig = nullptr;
-  if (auto *F = dyn_cast_or_null<InputFunction>(Chunk))
-    Sig = &F->Signature;
-  return checkSymbolTypes(Existing, F, IsFunction, Sig);
+static void checkGlobalTypes(const Symbol &Existing, const InputFile &F) {
+  if (!Existing.isLazy() && isa<FunctionSymbol>(Existing))
+    error("symbol type mismatch: " + toString(Existing) +
+          "\n>>> defined as Function in" + toString(Existing.getFile()) +
+          "\n>>> defined as Global in " + toString(&F));
 }
 
 DefinedFunction *SymbolTable::addSyntheticFunction(StringRef Name,
                                                    const WasmSignature *Type,
                                                    uint32_t Flags) {
   DEBUG(dbgs() << "addSyntheticFunction: " << Name << "\n");
   Symbol *S;
   bool WasInserted;
   std::tie(S, WasInserted) = insert(Name);
   assert(WasInserted);
   return replaceSymbol<DefinedFunction>(S, Name, Flags, Type);
 }
 
 DefinedGlobal *SymbolTable::addSyntheticGlobal(StringRef Name, uint32_t Flags) {
   DEBUG(dbgs() << "addSyntheticGlobal: " << Name << "\n");
   Symbol *S;
   bool WasInserted;
   std::tie(S, WasInserted) = insert(Name);
   assert(WasInserted);
   return replaceSymbol<DefinedGlobal>(S, Name, Flags);
 }
 
 static bool shouldReplace(const Symbol &Existing, InputFile *NewFile,
-                          uint32_t NewFlags, InputChunk *NewChunk,
-                          bool NewIsFunction) {
+                          uint32_t NewFlags, InputChunk *NewChunk) {
   // If existing symbol is lazy, replace it without checking types since
   // lazy symbols don't have any type information.
   if (Existing.isLazy()) {
     DEBUG(dbgs() << "replacing existing lazy symbol: " << Existing.getName()
                  << "\n");
     return true;
   }
 
   // Now we have two wasm symbols, and all wasm symbols that have the same
   // symbol name must have the same type, even if they are undefined. This
   // is different from ELF because symbol types are not that significant
   // in ELF, and undefined symbols in ELF don't have type in the first place.
-  checkSymbolTypes(Existing, *NewFile, NewIsFunction, NewChunk);
+  if (auto *C = dyn_cast_or_null<InputFunction>(NewChunk))
+    checkFunctionTypes(Existing, *NewFile, &C->Signature);
+  else
+    checkGlobalTypes(Existing, *NewFile);
 
   // If existing symbol is undefined, replace it.
   if (!Existing.isDefined()) {
     DEBUG(dbgs() << "resolving existing undefined symbol: "
                  << Existing.getName() << "\n");
     return true;
   }
 

 }
 
 Symbol *SymbolTable::addDefinedFunction(StringRef Name, uint32_t Flags,
                                         InputFile *F, InputFunction *Function) {
   DEBUG(dbgs() << "addDefinedFunction: " << Name << "\n");
   Symbol *S;
   bool WasInserted;
   std::tie(S, WasInserted) = insert(Name);
-  if (WasInserted || shouldReplace(*S, F, Flags, Function, true))
+  if (WasInserted || shouldReplace(*S, F, Flags, Function))
     replaceSymbol<DefinedFunction>(S, Name, Flags, F, Function);
   return S;
 }
 
 Symbol *SymbolTable::addDefinedGlobal(StringRef Name, uint32_t Flags,
                                       InputFile *F, InputSegment *Segment,
                                       uint32_t Address) {
   DEBUG(dbgs() << "addDefinedGlobal:" << Name << " addr:" << Address << "\n");
   Symbol *S;
   bool WasInserted;
   std::tie(S, WasInserted) = insert(Name);
-  if (WasInserted || shouldReplace(*S, F, Flags, Segment, false))
+  if (WasInserted || shouldReplace(*S, F, Flags, Segment))
     replaceSymbol<DefinedGlobal>(S, Name, Flags, F, Segment, Address);
   return S;
 }
 
-Symbol *SymbolTable::addUndefined(StringRef Name, Symbol::Kind Kind,
-                                  uint32_t Flags, InputFile *F,
+Symbol *SymbolTable::addUndefinedFunction(StringRef Name, uint32_t Flags,
+                                          InputFile *F,
                                           const WasmSignature *Type) {
-  DEBUG(dbgs() << "addUndefined: " << Name << "\n");
+  DEBUG(dbgs() << "addUndefinedFunction: " << Name << "\n");
 
   Symbol *S;
   bool WasInserted;
   std::tie(S, WasInserted) = insert(Name);
 
-  bool IsFunction = Kind == Symbol::UndefinedFunctionKind;
   if (WasInserted) {
-    if (IsFunction)
-      replaceSymbol<UndefinedFunction>(S, Name, Flags, F, Type);
-    else
-      replaceSymbol<UndefinedGlobal>(S, Name, Flags, F);
+    return S;
+  }
+
+  if (auto *Lazy = dyn_cast<LazySymbol>(S)) {
+    DEBUG(dbgs() << "resolved by existing lazy\n");
+    cast<ArchiveFile>(Lazy->getFile())->addMember(&Lazy->getArchiveSymbol());
+    return S;
+  }
+
+  if (S->isDefined()) {
+    DEBUG(dbgs() << "resolved by existing\n");
+    checkFunctionTypes(*S, *F, Type);
+  }
+  return S;
+}
+
+Symbol *SymbolTable::addUndefinedGlobal(StringRef Name, uint32_t Flags,
+                                        InputFile *F) {
+  DEBUG(dbgs() << "addUndefinedGlobal: " << Name << "\n");
+
+  Symbol *S;
+  bool WasInserted;
+  std::tie(S, WasInserted) = insert(Name);
+
+  if (WasInserted) {
     return S;
   }
 
   if (auto *Lazy = dyn_cast<LazySymbol>(S)) {
     DEBUG(dbgs() << "resolved by existing lazy\n");
     cast<ArchiveFile>(Lazy->getFile())->addMember(&Lazy->getArchiveSymbol());
     return S;
   }
 
   if (S->isDefined()) {
     DEBUG(dbgs() << "resolved by existing\n");
-    checkSymbolTypes(*S, *F, IsFunction, Type);
+    checkGlobalTypes(*S, *F);
   }
   return S;
 }
 
 void SymbolTable::addLazy(ArchiveFile *F, const Archive::Symbol *Sym) {
   DEBUG(dbgs() << "addLazy: " << Sym->getName() << "\n");
   StringRef Name = Sym->getName();