[PATCH] D43264: [WebAssembly] Add explicit symbol table

[Rui Ueyama via Phabricator via llvm-commits]

ruiu added inline comments.


================
Comment at: wasm/InputChunks.h:183
+// form the final GLOBALS section.
+class InputGlobal : public InputChunk {
+public:
----------------
Chunk is a concept to represent some contiguous bytes in an input file or a synthesized one. Essentially, a chunk is a section or something similar.

It doesn't seems to me that this class falls in that category, though I may be wrong due to lack of knowledge on wasm. Is this an appropriate abstraction?


================
Comment at: wasm/InputFiles.cpp:248
       }
-    }
-    case WasmSymbol::SymbolType::FUNCTION_IMPORT:
-      S = createUndefined(WasmSym, Symbol::Kind::UndefinedFunctionKind,
-                          getFunctionSig(WasmSym));
+      S = createUndefined(WasmSym);
       break;
----------------
I'd just call `Symbols.push_back(createUndefined(WasmSym))` and remove `S`.

Or, I'd move this switch-cases to a new function returning `Symbol *` and call like this:

  Symbols.push_back(createSymbol(WasmSym));


================
Comment at: wasm/InputFiles.h:111
   ArrayRef<Symbol *> getSymbols() const { return Symbols; }
-
-  FunctionSymbol *getFunctionSymbol(uint32_t Index) const {
-    return cast<FunctionSymbol>(FunctionSymbols[Index]);
-  }
-
-  GlobalSymbol *getGlobalSymbol(uint32_t Index) const {
-    return cast<GlobalSymbol>(GlobalSymbols[Index]);
-  }
+  Symbol *getSymbol(uint32_t Symbol) const { return Symbols[Symbol]; }
+  FunctionSymbol *getFunctionSymbol(uint32_t Index) const;
----------------
Symbol -> Index


================
Comment at: wasm/InputFiles.h:132-133
   InputSegment *getSegment(const WasmSymbol &WasmSym) const;
-  const WasmSignature *getFunctionSig(const WasmSymbol &Sym) const;
-  uint32_t getGlobalValue(const WasmSymbol &Sym) const;
+  std::pair<uint32_t, uint32_t>
+  getDataAddressAndSize(const WasmSymbol &WasmSym) const;
   InputFunction *getFunction(const WasmSymbol &Sym) const;
----------------
It seems more straightforward if you define getDataAddress and getDataSize.


================
Comment at: wasm/MarkLive.cpp:73-75
+      if (Reloc.Type == R_WEBASSEMBLY_TYPE_INDEX_LEB)
+        continue;
+      Enqueue(Chunk.File->getSymbol(Reloc.Index));
----------------
Since we skip only one line, I'd flip the condition so that we can wreite

  for (...)
    if (Reloc.Type != ...)
      Enqueue(...);


================
Comment at: wasm/SymbolTable.cpp:134-137
+  error(toString(NewType) + " type mismatch: " + Existing.getName() +
+        "\n>>> defined as " + OldSigStr + " in " +
+        toString(Existing.getFile()) + "\n>>> defined as " + NewSigStr +
+        " in " + F.getName());
----------------
If you wrap this up as a lambda

  auto Error = [&](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());
  };

then you can eliminate OldSigStr and NewSigStr variables from this function.


================
Comment at: wasm/Symbols.cpp:140
+  const InputSegment *Segment = dyn_cast<InputSegment>(Chunk);
+  return Segment->OutputSegmentOffset + (VirtualAddress - Segment->startVA());
+}
----------------
Extraneous ()


================
Comment at: wasm/Symbols.cpp:201
+  llvm_unreachable("Invalid symbol type");
+  return "";
+}
----------------
I believe llvm_unreachable is defined as _Noreturn, so you don't need this `return ""` after llvm_noreturn.


================
Comment at: wasm/Symbols.cpp:217
+    llvm_unreachable("kind with no type");
+    return static_cast<WasmSymbolType>(-1);
+  }
----------------
Ditto


================
Comment at: wasm/Symbols.h:152-154
+  void setChunk(InputChunk* C) {
+    Chunk = C;
+  }
----------------
A function is created for a chunk, so it is a bit odd that there's a situation that you have to change a chunk for an existing function symbol. What is this for?


================
Comment at: wasm/Symbols.h:238-244
+  DefinedGlobal(StringRef Name, uint32_t Flags, InputFile *File, InputChunk *C)
+      : GlobalSymbol(Name, DefinedGlobalKind, Flags, File, C) {}
+
+  DefinedGlobal(StringRef Name, uint32_t Flags, const WasmGlobalType *Type)
+      : GlobalSymbol(Name, DefinedGlobalKind, Flags, nullptr, nullptr) {
+    setGlobalType(Type);
+  }
----------------
Why does this have two constructors?


================
Comment at: wasm/Writer.cpp:194-198
+    } else if (auto *GlobalSym = dyn_cast<GlobalSymbol>(Sym)) {
+      Import.Kind = WASM_EXTERNAL_GLOBAL;
+      Import.Global = GlobalSym->getGlobalType();
+    } else {
+      llvm_unreachable("invalid import type");
----------------
This is where you want to use cast instead of dyn_cast.

  } else {
     auto *GlobalSym = cast<GlobalSymbol>(Sym);
     ....
  }


================
Comment at: wasm/Writer.cpp:412-416
+      uint32_t Kind = Sym->isFunction()
+                          ? llvm::wasm::WASM_SYMBOL_TYPE_FUNCTION
+                          : Sym->isGlobal()
+                                ? llvm::wasm::WASM_SYMBOL_TYPE_GLOBAL
+                                : llvm::wasm::WASM_SYMBOL_TYPE_DATA;
----------------
This nested ?: operators look a bit fancy to me. I'd perhaps make this a tiny helper function that takes a Sym and returns a Kind.


================
Comment at: wasm/WriterUtils.cpp:221-225
+  SmallString<128> S;
+  if (Sig.Mutable)
+    S += "mutable ";
+  S += toString(static_cast<ValType>(Sig.Type));
+  return S.str();
----------------
This is perhaps a personal taste, but isn't this better?

  std::string S = toString(static_cast<ValType>(Sig.Type));
  if (Sig.Mutable)
    return "mutable " + S;
  return S;


Repository:
  rLLD LLVM Linker

https://reviews.llvm.org/D43264