[PATCH] D43112: [WebAssembly] Use Symbol class heirarchy. NFC.

[Rui Ueyama via Phabricator via llvm-commits]

ruiu added a comment.

Overall looking very nice. It closes the gap between wasm and other lld ports and make it easier to read.



================
Comment at: wasm/InputFiles.cpp:307
 
-Symbol *ObjFile::createDefined(const WasmSymbol &Sym, Symbol::Kind Kind,
-                               InputChunk *Chunk, uint32_t Address) {
-  Symbol *S;
-  if (Sym.isBindingLocal()) {
-    S = make<Symbol>(Sym.Name, true);
-    S->update(Kind, this, Sym.Flags, Chunk, Address);
-    return S;
-  }
-  return Symtab->addDefined(Sym.Name, Kind, Sym.Flags, this, Chunk, Address);
+Symbol *ObjFile::createDefinedFunction(const WasmSymbol &Sym,
+                                       InputChunk *Chunk) {
----------------
Ideally it should return `FunctionSymbol *`.


================
Comment at: wasm/InputFiles.h:111
+  FunctionSymbol *getFunctionSymbol(uint32_t Index) const {
+    return cast<FunctionSymbol>(FunctionSymbols[Index]);
   }
----------------
Can you make this type safe? I.e. can you change the type of FunctionSymbols so that you don't need to use cast?


================
Comment at: wasm/SymbolTable.cpp:164
 
-Symbol *SymbolTable::addDefined(StringRef Name, Symbol::Kind Kind,
-                                uint32_t Flags, InputFile *F, InputChunk *Chunk,
+Symbol *SymbolTable::addDefined(bool IsFunction, StringRef Name, uint32_t Flags,
+                                InputFile *F, InputChunk *Chunk,
----------------
It is not clear to me why we need addDefined function even though we have addDefinedFunction and addDefinedGlobal functions. What is this for?


================
Comment at: wasm/SymbolTable.cpp:226
 
 Symbol *SymbolTable::addUndefined(StringRef Name, Symbol::Kind Kind,
                                   uint32_t Flags, InputFile *F,
----------------
Ditto -- having addUndefinedFunction and addUndefined doesn't seem orthogonal because the former seems like a subset of the latter.


================
Comment at: wasm/Symbols.h:88
+
+class FunctionSymbol : public Symbol {
+public:
----------------
This class hierarchy is interesting

  Symbol
    FunctionSymbol
      DefinedFunction
      UndefinedFunction
    GlobalSymbol
      DefinedGlobal
      UndefinedGlobal
    Lazy

because in ELF and COFF, defined/undefined is a broader category than function/non-function. But I can see that your class hierarchy just represents how symbols are organized in wasm. (That's one of the reasons why I think that the current design of lld, in which all ports share only the design but not a concrete implementation. If we had to implement coff/elf/wasm/etc on all "unified" code base, that would have been extremely hard to do.)


================
Comment at: wasm/Symbols.h:126
+};
+
+class GlobalSymbol : public Symbol {
----------------
Can you move UndefinedFunction here so that related classes are adjacent in source code?


Repository:
  rLLD LLVM Linker

https://reviews.llvm.org/D43112