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

[Sam Clegg via Phabricator via llvm-commits]

sbc100 added inline comments.


================
Comment at: wasm/InputChunks.h:183
+// form the final GLOBALS section.
+class InputGlobal : public InputChunk {
+public:
----------------
ruiu wrote:
> 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?
Yes, are you very astute.  This was my feedback on @ncw 's original change too.  However after playing around with it locally a bunch I decided it might be OK (for now anyway).

I did put a TODO below about this.

One idea which would work OK is to remove this from the "InputChunk" class hierarchy.   However, if I do this then this new class is out on its own, maybe in a new file.  Not a big deal I guess.

If we come up with higher level concept such as a more general "ElementFromIntoFile" :)  perhaps we could share a bare class?  I'm happy to experiment more with that once this lands.  Or if you feel strongly, before it lands.


================
Comment at: wasm/InputFiles.cpp:248
       }
-    }
-    case WasmSymbol::SymbolType::FUNCTION_IMPORT:
-      S = createUndefined(WasmSym, Symbol::Kind::UndefinedFunctionKind,
-                          getFunctionSig(WasmSym));
+      S = createUndefined(WasmSym);
       break;
----------------
ruiu wrote:
> 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));
Makes sense.   I was trying to minimize the delta.  Perhaps I split that off into a pre-(or post) change.


================
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;
----------------
ruiu wrote:
> It seems more straightforward if you define getDataAddress and getDataSize.
Remove this function completely.


================
Comment at: wasm/Symbols.h:152-154
+  void setChunk(InputChunk* C) {
+    Chunk = C;
+  }
----------------
ruiu wrote:
> 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?
This exists purely for the synthetic function which we create in Writer::createCtorFunction().  In this case we want to create the symbol at startup, and then create its body later on.   I also agree this could be factored more elegantly.   I'll add a TODO comment here.


================
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);
+  }
----------------
ruiu wrote:
> Why does this have two constructors?
One is for file-defined globals and one is for synthetic defined globals.  Comment added.


Repository:
  rLLD LLVM Linker

https://reviews.llvm.org/D43264