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

Sam Clegg via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Wed Feb 21 10:17:34 PST 2018


sbc100 added inline comments.


================
Comment at: test/wasm/many-functions.ll:14
 define i32 @func() {
 entry:
   %call = tail call i32 @func()
----------------
ruiu wrote:
> I know this test tests "many functions", but it seems a bit too many. Do you really need this many functions?
This is designed to test the case there there are more then 127 functions, which means the number of functions no longer can serialized as a single byte LEB.  Since wasm uses LEBs extensively there can be a serialization bugs that only show up as we go from 127 to 128 elements.    if you can think of a better way to tickle this case I'm open to changing this test. 

(Also, I just noticed the comment above above describes this)


================
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
----------------
ruiu wrote:
> after relocations are applied
But relocations are applies after the memcpy, no?


================
Comment at: wasm/InputGlobal.h:19-20
+
+using llvm::wasm::WasmGlobal;
+using llvm::wasm::WasmInitExpr;
+
----------------
ruiu wrote:
> I think `using` in a header isn't a good idea.
We have other uses like this of `using` specific types.   If its OK with you I'll cleanup in a separate change and we can discuss there.


================
Comment at: wasm/InputGlobal.h:42
+
+  WasmGlobal Global;
+
----------------
ruiu wrote:
> How large is WasmGlobal? I wonder which is better, keeping it through a reference or copying it here.
Around 16 bytes or so.   And there is currently only one of them :)  There may be more soon, but currently we are only talking about __stack_pointer.   If we find users are generating a lot of these we could reconsider this.    In that case we could need to handle the __stack_pointer differently since we currently really on the mutability of this field to set the SP value.

For now I think this is fine.


================
Comment at: wasm/InputGlobal.h:45
+protected:
+  llvm::Optional<uint32_t> OutputIndex;
+};
----------------
ruiu wrote:
> It is better to keep this kind of things trivially destructible.
Why?  I think its better to match the pattern used in InputChunks.h (initially anyway).  

(Also, Optional is only non-trivial on gcc due to a compiler bug)


Repository:
  rLLD LLVM Linker

https://reviews.llvm.org/D43264





More information about the llvm-commits mailing list