[WebAssembly] Separate addUndefined into addUndefined{Function,Data,Global}.

[ruiu]

Summary

Previously, one function adds all types of undefined symbols. That
doesn't fit to the wasm's undefined symbol semantics well because
different types of undefined symbols are very different in wasm.
As a result, separate control flows merge in this addUndefined function
and then separate again for each type. That wasn't easy to read.

This patch separates the function into three functions. Now it is pretty
clear what we are doing for each undefined symbol type.

[ruiu]

• fix typo

[sbc100]

lld/wasm/SymbolTable.cpp

111 ↗

(On Diff #135702)

We don't need this if. The signature checking on functions is optional to allow certain C programs to link. Shouldn't be needed to wasm globals (which are not used in C).

[ruiu]

lld/wasm/SymbolTable.cpp

111 ↗

(On Diff #135702)

Done.

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, WASM_SYMBOL_TYPE_FUNCTION, 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)) {
     Parser.PrintHelp(outs(), ArgsArr[0], "LLVM Linker", false);
     return;
if (!Config->Relocatable) {
     WasmSym::StackPointer = Symtab->addSyntheticGlobal(
         "__stack_pointer", WASM_SYMBOL_VISIBILITY_HIDDEN, StackPointer);
     WasmSym::HeapBase = Symtab->addSyntheticDataSymbol("__heap_base");
     WasmSym::DsoHandle = Symtab->addSyntheticDataSymbol(
         "__dso_handle", WASM_SYMBOL_VISIBILITY_HIDDEN);
     WasmSym::DataEnd = Symtab->addSyntheticDataSymbol("__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.

wasm/InputFiles.cpp

for (const SymbolRef &Sym : WasmObj->symbols()) {
     // in which case this simply insertes the existing symbol into the correct
     // slot in the Symbols array.
     if (!IsDefined)
       Symbols.push_back(createUndefined(WasmSym));
   }
 }
 
 Symbol *ObjFile::createUndefined(const WasmSymbol &Sym) {
-  return Symtab->addUndefined(
-      Sym.Info.Name, static_cast<WasmSymbolType>(Sym.Info.Kind), Sym.Info.Flags,
-      this, Sym.FunctionType, Sym.GlobalType);
+  StringRef Name = Sym.Info.Name;
+  uint32_t Flags = Sym.Info.Flags;
+
+  switch (Sym.Info.Kind) {
+  case WASM_SYMBOL_TYPE_FUNCTION:
+    return Symtab->addUndefinedFunction(Name, Flags, this, Sym.FunctionType);
+  case WASM_SYMBOL_TYPE_DATA:
+    return Symtab->addUndefinedData(Name, Flags, this);
+  case WASM_SYMBOL_TYPE_GLOBAL:
+    return Symtab->addUndefinedGlobal(Name, Flags, this, Sym.GlobalType);
+  }
+  llvm_unreachable("unkown symbol kind");
 }
 
 Symbol *ObjFile::createDefinedFunction(const WasmSymbol &Sym,
                                        InputFunction *Function) {
   if (Sym.isBindingLocal())
     return make<DefinedFunction>(Sym.Info.Name, Sym.Info.Flags, this, Function);
   return Symtab->addDefinedFunction(Sym.Info.Name, Sym.Info.Flags, this,
                                     Function);

wasm/SymbolTable.h

public:
   std::vector<ObjFile *> ObjectFiles;
 
   void reportRemainingUndefines();
 
   ArrayRef<Symbol *> getSymbols() const { return SymVector; }
   Symbol *find(StringRef Name);
   ObjFile *findComdat(StringRef Name) const;
 
-  Symbol *addDefinedFunction(StringRef Name, uint32_t Flags, InputFile *F,
+  Symbol *addDefinedFunction(StringRef Name, uint32_t Flags, InputFile *File,
                              InputFunction *Function = nullptr);
-  Symbol *addDefinedData(StringRef Name, uint32_t Flags, InputFile *F,
+  Symbol *addDefinedData(StringRef Name, uint32_t Flags, InputFile *File,
                          InputSegment *Segment = nullptr, uint32_t Address = 0,
                          uint32_t Size = 0);
-  Symbol *addDefinedGlobal(StringRef Name, uint32_t Flags, InputFile *F,
+  Symbol *addDefinedGlobal(StringRef Name, uint32_t Flags, InputFile *File,
                            InputGlobal *G);
-  Symbol *addUndefinedFunction(StringRef Name, const WasmSignature *Type);
-  Symbol *addUndefined(StringRef Name, WasmSymbolType Type, uint32_t Flags,
-                       InputFile *F, const WasmSignature *Signature = nullptr,
-                       const WasmGlobalType *GlobalType = nullptr);
+
+  Symbol *addUndefinedFunction(StringRef Name, uint32_t Flags, InputFile *File,
+                               const WasmSignature *Signature);
+  Symbol *addUndefinedData(StringRef Name, uint32_t Flags, InputFile *File);
+  Symbol *addUndefinedGlobal(StringRef Name, uint32_t Flags, InputFile *File,
+                             const WasmGlobalType *Type);
+
   void addLazy(ArchiveFile *F, const Archive::Symbol *Sym);
   bool addComdat(StringRef Name, ObjFile *);
 
   DefinedData *addSyntheticDataSymbol(StringRef Name, uint32_t Flags = 0);
   DefinedGlobal *addSyntheticGlobal(StringRef Name, uint32_t Flags,
                                     InputGlobal *Global);
   DefinedFunction *addSyntheticFunction(StringRef Name,
                                         const WasmSignature *Type,

wasm/SymbolTable.cpp

std::pair<Symbol *, bool> SymbolTable::insert(StringRef Name) {
   Symbol *&Sym = SymMap[CachedHashStringRef(Name)];
   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,
-                             WasmSymbolType NewType,
-                             const WasmSignature *NewFunctionSig,
-                             const WasmGlobalType *NewGlobalType) {
-  if (Existing.isLazy())
-    return;
-
-  WasmSymbolType ExistingType = Existing.getWasmType();
-
-  // First check the symbol types match (i.e. either both are function
-  // symbols or both are data symbols).
-  if (NewType != ExistingType) {
-    error("symbol type mismatch: " + Existing.getName() + "\n>>> defined as " +
-          toString(ExistingType) + " in " + toString(Existing.getFile()) +
-          "\n>>> defined as " + toString(NewType) + " in " + F.getName());
-    return;
-  }
+static void reportTypeError(const Symbol *Existing, const InputFile *File,
+                            StringRef Type) {
+  error("symbol type mismatch: " + toString(*Existing) + "\n>>> defined as " +
+        toString(Existing->getWasmType()) + " in " +
+        toString(Existing->getFile()) + "\n>>> defined as " + Type + " in " +
+        toString(File));
 
-  // For function/global symbols, optionally check the type matches too.
-  if (NewType == WASM_SYMBOL_TYPE_DATA || !Config->CheckSignatures)
+static void checkFunctionType(const Symbol *Existing, const InputFile *File,
+                              const WasmSignature *NewSig) {
+  if (!isa<FunctionSymbol>(Existing)) {
+    reportTypeError(Existing, File, "Function");
     return;
+  }
 
-  DEBUG(dbgs() << "checkSymbolTypes: " << Existing.getName() << "\n");
-
-  auto ReportError = [&](const Twine &Old, const Twine &New) {
-    error(toString(NewType) + " type mismatch: " + Existing.getName() +
-          "\n>>> defined as " + Old + " in " + toString(Existing.getFile()) +
-          "\n>>> defined as " + New + " in " + F.getName());
-  };
-
-  if (NewType == WASM_SYMBOL_TYPE_FUNCTION) {
-    // 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).
-    auto &Sym = cast<FunctionSymbol>(Existing);
-    const WasmSignature *OldSig = Sym.getFunctionType();
-    if (!OldSig)
-      return;
+  if (!Config->CheckSignatures)
 
-    assert(NewFunctionSig);
-    if (*NewFunctionSig == *OldSig)
-      return;
+  const WasmSignature *OldSig =
+      cast<FunctionSymbol>(Existing)->getFunctionType();
+  if (OldSig && *NewSig != *OldSig) {
+    error("Function type mismatch: " + Existing->getName() +
+          "\n>>> defined as " + toString(*OldSig) + " in " +
+          toString(Existing->getFile()) + "\n>>> defined as " +
+          toString(*NewSig) + " in " + toString(File));
+  }
+}
 
-    ReportError(toString(*OldSig), toString(*NewFunctionSig));
-  } else {
-    auto &Sym = cast<GlobalSymbol>(Existing);
-
-    assert(NewGlobalType != nullptr);
-    const WasmGlobalType *OldType = Sym.getGlobalType();
-    if (*NewGlobalType == *OldType)
-      return;
+// 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 checkGlobalType(const Symbol *Existing, const InputFile *File,
+                            const WasmGlobalType *NewType) {
+  if (!isa<GlobalSymbol>(Existing)) {
+    reportTypeError(Existing, File, "Global");
+  }
 
-    ReportError(toString(*OldType), toString(*NewGlobalType));
-  }
+  const WasmGlobalType *OldType = cast<GlobalSymbol>(Existing)->getGlobalType();
+  if (*NewType != *OldType) {
+    error("Global type mismatch: " + Existing->getName() + "\n>>> defined as " +
+          toString(*OldType) + " in " + toString(Existing->getFile()) +
+          "\n>>> defined as " + toString(*NewType) + " in " + toString(File));
+  }
+
+static void checkDataType(const Symbol *Existing, const InputFile *File) {
+  if (!isa<DataSymbol>(Existing))
+    reportTypeError(Existing, File, "Data");
 }
 
 DefinedFunction *SymbolTable::addSyntheticFunction(StringRef Name,
                                                    const WasmSignature *Type,
                                                    uint32_t Flags) {
   DEBUG(dbgs() << "addSyntheticFunction: " << Name << "\n");
   Symbol *S;
   bool WasInserted;
DefinedGlobal *SymbolTable::addSyntheticGlobal(StringRef Name, uint32_t Flags,
   DEBUG(dbgs() << "addSyntheticGlobal: " << Name << " -> " << Global << "\n");
   Symbol *S;
   bool WasInserted;
   std::tie(S, WasInserted) = insert(Name);
   assert(WasInserted);
   return replaceSymbol<DefinedGlobal>(S, Name, Flags, nullptr, Global);
 }
 
-static bool shouldReplace(const Symbol &Existing, InputFile *NewFile,
-                          WasmSymbolType NewType, uint32_t NewFlags,
-                          const WasmSignature *NewFuncType = nullptr,
-                          const WasmGlobalType *NewGlobalType = nullptr) {
-
-  // 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, NewType, NewFuncType, NewGlobalType);
-
+static bool shouldReplace(const Symbol *Existing, InputFile *NewFile,
+                          uint32_t NewFlags) {
   // If existing symbol is undefined, replace it.
-  if (!Existing.isDefined()) {
+  if (!Existing->isDefined()) {
     DEBUG(dbgs() << "resolving existing undefined symbol: "
-                 << Existing.getName() << "\n");
+                 << Existing->getName() << "\n");
     return true;
   }
 
   // Now we have two defined symbols. If the new one is weak, we can ignore it.
   if ((NewFlags & WASM_SYMBOL_BINDING_MASK) == WASM_SYMBOL_BINDING_WEAK) {
     DEBUG(dbgs() << "existing symbol takes precedence\n");
     return false;
   }
 
   // If the existing symbol is weak, we should replace it.
-  if (Existing.isWeak()) {
+  if (Existing->isWeak()) {
     DEBUG(dbgs() << "replacing existing weak symbol\n");
     return true;
   }
 
   // Neither symbol is week. They conflict.
-  error("duplicate symbol: " + toString(Existing) + "\n>>> defined in " +
-        toString(Existing.getFile()) + "\n>>> defined in " + toString(NewFile));
+  error("duplicate symbol: " + toString(*Existing) + "\n>>> defined in " +
+        toString(Existing->getFile()) + "\n>>> defined in " +
+        toString(NewFile));
   return true;
 }
 
 Symbol *SymbolTable::addDefinedFunction(StringRef Name, uint32_t Flags,
-                                        InputFile *F, InputFunction *Function) {
+                                        InputFile *File,
+                                        InputFunction *Function) {
   DEBUG(dbgs() << "addDefinedFunction: " << Name << "\n");
   Symbol *S;
   bool WasInserted;
   std::tie(S, WasInserted) = insert(Name);
-  if (WasInserted || shouldReplace(*S, F, WASM_SYMBOL_TYPE_FUNCTION, Flags,
-                                   &Function->Signature))
-    replaceSymbol<DefinedFunction>(S, Name, Flags, F, Function);
+
+  if (WasInserted || S->isLazy()) {
+    replaceSymbol<DefinedFunction>(S, Name, Flags, File, Function);
+    return S;
+  }
+
+  checkFunctionType(S, File, &Function->Signature);
+
+  if (shouldReplace(S, File, Flags))
+    replaceSymbol<DefinedFunction>(S, Name, Flags, File, Function);
   return S;
 }
 
 Symbol *SymbolTable::addDefinedData(StringRef Name, uint32_t Flags,
-                                    InputFile *F, InputSegment *Segment,
+                                    InputFile *File, InputSegment *Segment,
                                     uint32_t Address, uint32_t Size) {
   DEBUG(dbgs() << "addDefinedData:" << Name << " addr:" << Address << "\n");
   Symbol *S;
   bool WasInserted;
   std::tie(S, WasInserted) = insert(Name);
-  if (WasInserted || shouldReplace(*S, F, WASM_SYMBOL_TYPE_DATA, Flags))
-    replaceSymbol<DefinedData>(S, Name, Flags, F, Segment, Address, Size);
+
+  if (WasInserted || S->isLazy()) {
+    replaceSymbol<DefinedData>(S, Name, Flags, File, Segment, Address, Size);
+    return S;
+  }
+
+  checkDataType(S, File);
+
+  if (shouldReplace(S, File, Flags))
+    replaceSymbol<DefinedData>(S, Name, Flags, File, Segment, Address, Size);
   return S;
 }
 
 Symbol *SymbolTable::addDefinedGlobal(StringRef Name, uint32_t Flags,
-                                      InputFile *F, InputGlobal *Global) {
+                                      InputFile *File, InputGlobal *Global) {
   DEBUG(dbgs() << "addDefinedGlobal:" << Name << "\n");
   Symbol *S;
   bool WasInserted;
   std::tie(S, WasInserted) = insert(Name);
-  if (WasInserted || shouldReplace(*S, F, WASM_SYMBOL_TYPE_GLOBAL, Flags,
-                                   nullptr, &Global->getType()))
-    replaceSymbol<DefinedGlobal>(S, Name, Flags, F, Global);
+
+  if (WasInserted || S->isLazy()) {
+    replaceSymbol<DefinedGlobal>(S, Name, Flags, File, Global);
     return S;
   }
 
-Symbol *SymbolTable::addUndefined(StringRef Name, WasmSymbolType Type,
-                                  uint32_t Flags, InputFile *F,
-                                  const WasmSignature *FunctionType,
-                                  const WasmGlobalType *GlobalType) {
-  DEBUG(dbgs() << "addUndefined type=" << Type << ": " << Name << "\n");
+  checkGlobalType(S, File, &Global->getType());
+
+  if (shouldReplace(S, File, Flags))
+    replaceSymbol<DefinedGlobal>(S, Name, Flags, File, Global);
+  return S;
+}
+
+Symbol *SymbolTable::addUndefinedFunction(StringRef Name, uint32_t Flags,
+                                          InputFile *File,
+                                          const WasmSignature *Sig) {
+  DEBUG(dbgs() << "addUndefinedFunction: " << Name << "\n");
 
   Symbol *S;
   bool WasInserted;
   std::tie(S, WasInserted) = insert(Name);
 
-  if (WasInserted) {
-    switch (Type) {
-    case WASM_SYMBOL_TYPE_FUNCTION:
-      replaceSymbol<UndefinedFunction>(S, Name, Flags, F, FunctionType);
-      break;
-    case WASM_SYMBOL_TYPE_GLOBAL:
-      replaceSymbol<UndefinedGlobal>(S, Name, Flags, F, GlobalType);
-      break;
-    case WASM_SYMBOL_TYPE_DATA:
-      replaceSymbol<UndefinedData>(S, Name, Flags, F);
-      break;
-    }
-    return S;
-  }
+  if (WasInserted)
+    replaceSymbol<UndefinedFunction>(S, Name, Flags, File, Sig);
+  else if (auto *Lazy = dyn_cast<LazySymbol>(S))
+    cast<ArchiveFile>(Lazy->getFile())->addMember(&Lazy->getArchiveSymbol());
+  else if (S->isDefined())
+    checkFunctionType(S, File, Sig);
 
-  if (auto *Lazy = dyn_cast<LazySymbol>(S)) {
-    DEBUG(dbgs() << "resolved by existing lazy\n");
+Symbol *SymbolTable::addUndefinedData(StringRef Name, uint32_t Flags,
+                                      InputFile *File) {
+  DEBUG(dbgs() << "addUndefinedData: " << Name << "\n");
+
+  Symbol *S;
+  bool WasInserted;
+  std::tie(S, WasInserted) = insert(Name);
+
+  if (WasInserted)
+    replaceSymbol<UndefinedData>(S, Name, Flags, File);
+  else if (auto *Lazy = dyn_cast<LazySymbol>(S))
     cast<ArchiveFile>(Lazy->getFile())->addMember(&Lazy->getArchiveSymbol());
-    return S;
-  }
+  else if (S->isDefined())
+    checkDataType(S, File);
 
-  if (S->isDefined()) {
-    DEBUG(dbgs() << "resolved by existing\n");
-    checkSymbolTypes(*S, *F, Type, FunctionType, GlobalType);
-  }
+Symbol *SymbolTable::addUndefinedGlobal(StringRef Name, uint32_t Flags,
+                                        InputFile *File,
+                                        const WasmGlobalType *Type) {
+  DEBUG(dbgs() << "addUndefinedGlobal: " << Name << "\n");
+
+  Symbol *S;
+  bool WasInserted;
+  std::tie(S, WasInserted) = insert(Name);
 
+  if (WasInserted)
+    replaceSymbol<UndefinedGlobal>(S, Name, Flags, File, Type);
+  else if (auto *Lazy = dyn_cast<LazySymbol>(S))
+    cast<ArchiveFile>(Lazy->getFile())->addMember(&Lazy->getArchiveSymbol());
+  else if (S->isDefined())
+    checkGlobalType(S, File, Type);
   return S;
 }
 
-void SymbolTable::addLazy(ArchiveFile *F, const Archive::Symbol *Sym) {
+void SymbolTable::addLazy(ArchiveFile *File, const Archive::Symbol *Sym) {
   DEBUG(dbgs() << "addLazy: " << Sym->getName() << "\n");
   StringRef Name = Sym->getName();
 
   Symbol *S;
   bool WasInserted;
   std::tie(S, WasInserted) = insert(Name);
 
   if (WasInserted) {
-    replaceSymbol<LazySymbol>(S, Name, F, *Sym);
+    replaceSymbol<LazySymbol>(S, Name, File, *Sym);
     return;
   }
 
   // If there is an existing undefined symbol, load a new one from the archive.
   if (S->isUndefined()) {
     DEBUG(dbgs() << "replacing existing undefined\n");
-    F->addMember(Sym);
+    File->addMember(Sym);
   }
 }
 
 bool SymbolTable::addComdat(StringRef Name, ObjFile *F) {
   DEBUG(dbgs() << "addComdat: " << Name << "\n");
   ObjFile *&File = ComdatMap[CachedHashStringRef(Name)];
   if (File) {
     DEBUG(dbgs() << "COMDAT already defined\n");