[WebAssembly] Split addDefined into two different methods. NFC.

[sbc100]

• make static

[ruiu]

wasm/SymbolTable.cpp

150 ↗

(On Diff #134998)

We can just pass File, Flags, Chunk and IsFunction to this class, then we don't need NewSymbol class, no?

[sbc100]

wasm/SymbolTable.cpp

150 ↗

(On Diff #134998)

(I assume you mean "..to this function).

Thats how it was before but I found it easier to read this way, since all the properties of the new symbol are under New this way, and i makes the call site a little more readable (to me). I can revert if you prefer.

Also, the symbol table change is going to add another field (or anther param) here for input global (that don't use chunks).

[ruiu]

wasm/SymbolTable.cpp

150 ↗

(On Diff #134998)

It feels a bit odd to me to define a struct to pack four arguments into one argument, so let's avoid doing that. If you prefer, you could name these parameters NewFile, NewFlags, NewChunk and NewIsFunction so that you could use them instead of New.File, New.Flags, New.Chunk and New.IsFunction.

[sbc100]

• feedback

[sbc100]

Done.

[ruiu]

LGTM

lld/trunk/wasm/InputFiles.cpp

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->addDefined(true, Sym.Name, Sym.Flags, this, Function);
+  return Symtab->addDefinedFunction(Sym.Name, Sym.Flags, this, Function);
 }
 
 Symbol *ObjFile::createDefinedGlobal(const WasmSymbol &Sym,
                                      InputSegment *Segment, uint32_t Address) {
   if (Sym.isBindingLocal())
     return make<DefinedGlobal>(Sym.Name, Sym.Flags, this, Segment, Address);
-  return Symtab->addDefined(false, Sym.Name, Sym.Flags, this, Segment, Address);
+  return Symtab->addDefinedGlobal(Sym.Name, Sym.Flags, this, Segment, Address);
 }
 
 void ArchiveFile::parse() {
   // Parse a MemoryBufferRef as an archive file.
   DEBUG(dbgs() << "Parsing library: " << toString(this) << "\n");
   File = CHECK(Archive::create(MB), toString(this));
 
   // Read the symbol table to construct Lazy symbols.

lld/trunk/wasm/SymbolTable.h

 // add*() functions, which are called by input files as they are parsed.
 // There is one add* function per symbol type.
 class SymbolTable {
 public:
   void addFile(InputFile *File);
 
   std::vector<ObjFile *> ObjectFiles;
 
-  void reportDuplicate(Symbol *Existing, InputFile *NewFile);
   void reportRemainingUndefines();
 
   ArrayRef<Symbol *> getSymbols() const { return SymVector; }
   Symbol *find(StringRef Name);
   ObjFile *findComdat(StringRef Name) const;
 
-  Symbol *addDefined(bool IsFunction, StringRef Name, uint32_t Flags,
-                     InputFile *F, InputChunk *Chunk = nullptr,
+  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);
   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,

lld/trunk/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};
 }
 
-void SymbolTable::reportDuplicate(Symbol *Existing, InputFile *NewFile) {
-  error("duplicate symbol: " + toString(*Existing) + "\n>>> defined in " +
-        toString(Existing->getFile()) + "\n>>> defined in " +
-        toString(NewFile));
-}
-
 // 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) {
   if (Existing.isLazy())
     return;
 
   // First check the symbol types match (i.e. either both are function
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);
 }
 
-Symbol *SymbolTable::addDefined(bool IsFunction, StringRef Name, uint32_t Flags,
-                                InputFile *F, InputChunk *Chunk,
-                                uint32_t Address) {
-  if (IsFunction)
-    DEBUG(dbgs() << "addDefined: func:" << Name << "\n");
-  else
-    DEBUG(dbgs() << "addDefined: global:" << Name << " addr:" << Address
-                 << "\n");
-  Symbol *S;
-  bool WasInserted;
+struct NewSymbol {
+  InputFile *File;
+  uint32_t Flags;
+  InputChunk *Chunk;
+  bool IsFunction;
+};
+
+static bool shouldReplace(const Symbol &Existing, const NewSymbol &New) {
   bool Replace = false;
   bool CheckTypes = false;
 
-  std::tie(S, WasInserted) = insert(Name);
-  if (WasInserted) {
-    Replace = true;
-  } else if (S->isLazy()) {
+  if (Existing.isLazy()) {
     // Existing symbol is lazy. Replace it without checking types since
     // lazy symbols don't have any type information.
-    DEBUG(dbgs() << "replacing existing lazy symbol: " << Name << "\n");
+    DEBUG(dbgs() << "replacing existing lazy symbol: " << Existing.getName()
+                 << "\n");
     Replace = true;
-  } else if (!S->isDefined()) {
+  } else if (!Existing.isDefined()) {
     // Existing symbol is undefined: replace it, while check types.
-    DEBUG(dbgs() << "resolving existing undefined symbol: " << Name << "\n");
+    DEBUG(dbgs() << "resolving existing undefined symbol: "
+                 << Existing.getName() << "\n");
     Replace = true;
     CheckTypes = true;
-  } else if ((Flags & WASM_SYMBOL_BINDING_MASK) == WASM_SYMBOL_BINDING_WEAK) {
+  } else if ((New.Flags & WASM_SYMBOL_BINDING_MASK) == WASM_SYMBOL_BINDING_WEAK) {
     // the new symbol is weak we can ignore it
     DEBUG(dbgs() << "existing symbol takes precedence\n");
     CheckTypes = true;
-  } else if (S->isWeak()) {
+  } else if (Existing.isWeak()) {
     // the existing symbol is, so we replace it
     DEBUG(dbgs() << "replacing existing weak symbol\n");
     Replace = true;
     CheckTypes = true;
   } else {
     // neither symbol is week. They conflict.
-    reportDuplicate(S, F);
+    error("duplicate symbol: " + toString(Existing) + "\n>>> defined in " +
+          toString(Existing.getFile()) + "\n>>> defined in " +
+          toString(New.File));
   }
 
-  if (Replace) {
-    if (CheckTypes)
-      checkSymbolTypes(*S, *F, IsFunction, Chunk);
-    if (IsFunction)
-      replaceSymbol<DefinedFunction>(S, Name, Flags, F,
-                                     cast<InputFunction>(Chunk));
-    else
-      replaceSymbol<DefinedGlobal>(S, Name, Flags, F, cast<InputSegment>(Chunk),
-                                   Address);
-  }
+    checkSymbolTypes(Existing, *New.File, New.IsFunction, New.Chunk);
+
+  return Replace;
+
+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);
+  NewSymbol New{F, Flags, Function, true};
+  if (WasInserted || shouldReplace(*S, New))
+    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);
+  NewSymbol New{F, Flags, Segment, false};
+  if (WasInserted || shouldReplace(*S, New))
+    replaceSymbol<DefinedGlobal>(S, Name, Flags, F, Segment, Address);
   return S;
 }
 
 Symbol *SymbolTable::addUndefined(StringRef Name, Symbol::Kind Kind,
                                   uint32_t Flags, InputFile *F,
                                   const WasmSignature *Type) {
   DEBUG(dbgs() << "addUndefined: " << Name << "\n");
   Symbol *S;