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

[Rui Ueyama via Phabricator via llvm-commits]

ruiu added inline comments.


================
Comment at: test/wasm/many-functions.ll:14
 define i32 @func() {
 entry:
   %call = tail call i32 @func()
----------------
I know this test tests "many functions", but it seems a bit too many. Do you really need this many functions?


================
Comment at: wasm/Driver.cpp:309
+    StackPointerGlobal.InitExpr.Opcode = WASM_OPCODE_I32_CONST;
+    StackPointer = llvm::make_unique<InputGlobal>(StackPointerGlobal);
+    StackPointer->Live = true;
----------------
Why don't you use make<>?


================
Comment at: wasm/InputChunks.h:13
+//
+// They are writen directly to the mmap'd output file afterwhich relcoations
+// are applied.  Because each Chunk is independed they can be written in
----------------
after relocations are applied


================
Comment at: wasm/InputFiles.cpp:214-215
+  for (const WasmGlobal &G : WasmObj->globals()) {
+    InputGlobal *Global = make<InputGlobal>(G);
+    Globals.emplace_back(Global);
+  }
----------------
You can eliminate Global local variable.


================
Comment at: wasm/InputFiles.h:124
   Symbol *createDefinedFunction(const WasmSymbol &Sym, InputFunction *Function);
-  Symbol *createUndefined(const WasmSymbol &Sym, Symbol::Kind Kind,
-                          const WasmSignature *Signature = nullptr);
+  Symbol *createDefinedGlobal(const WasmSymbol &Sym, InputGlobal *Chunk);
+  Symbol *createUndefined(const WasmSymbol &Sym);
----------------
InputGlobal *Global


================
Comment at: wasm/InputGlobal.h:1
+//===- InputChunks.h --------------------------------------------*- C++ -*-===//
+//
----------------
Wrong header name


================
Comment at: wasm/InputGlobal.h:19-20
+
+using llvm::wasm::WasmGlobal;
+using llvm::wasm::WasmInitExpr;
+
----------------
I think `using` in a header isn't a good idea.


================
Comment at: wasm/InputGlobal.h:25
+
+// Represents a single Wasm Global within an input file. These are combined to
+// form the final GLOBALS section.
----------------
Wasm Global Variable is perhaps better? Global functions are global, so the use of Global is wasm-speak.


================
Comment at: wasm/InputGlobal.h:42
+
+  WasmGlobal Global;
+
----------------
How large is WasmGlobal? I wonder which is better, keeping it through a reference or copying it here.


================
Comment at: wasm/InputGlobal.h:45
+protected:
+  llvm::Optional<uint32_t> OutputIndex;
+};
----------------
It is better to keep this kind of things trivially destructible.


================
Comment at: wasm/Symbols.h:212
+  GlobalSymbol(StringRef Name, Kind K, uint32_t Flags, InputFile *F,
+               const WasmGlobalType *GlobalType = nullptr)
+      : Symbol(Name, K, Flags, F), GlobalType(GlobalType) {}
----------------
Ditto


================
Comment at: wasm/Symbols.h:234-235
+public:
+  UndefinedGlobal(StringRef Name, uint32_t Flags, InputFile *File = nullptr,
+                  const WasmGlobalType *Type = nullptr)
+      : GlobalSymbol(Name, UndefinedGlobalKind, Flags, File, Type) {}
----------------
I prefer explicit code even if it's a bit verbose, so let's avoid default parameters in lld. I believe you want to pass (null, null) only when you are adding synthetic sections, so in that case, you can explicitly pass (null, null).


================
Comment at: wasm/Writer.cpp:131
+  std::vector<const DefinedData *> DefinedFakeGlobals;
+  std::vector<InputGlobal *> DefinedGlobals;
   std::vector<InputFunction *> DefinedFunctions;
----------------
`DefinedGlobal` is a symbol type, so this variable name is a it confusing.


================
Comment at: wasm/Writer.cpp:699-708
   for (ObjFile *File : Symtab->ObjectFiles) {
     for (Symbol *Sym : File->getSymbols()) {
-      if (!Sym->isDefined() || File != Sym->getFile())
-        continue;
-      if (!isa<FunctionSymbol>(Sym))
-        continue;
-      if (!Sym->getChunk()->Live)
-        continue;
+      if (File == Sym->getFile())
+        ExportSym(Sym);
+    }
+  }
+  for (Symbol *Sym : Symtab->getSymbols()) {
----------------
Let's be consistent -- omit {}.


Repository:
  rLLD LLVM Linker

https://reviews.llvm.org/D43264