[PATCH] D44393: [WebAssembly] Add missing implementation for --initial/max-memory args

[Nicholas Wilson via Phabricator via llvm-commits]

ncw added inline comments.


================
Comment at: lld/trunk/wasm/Writer.cpp:611
+  if (Config->InitialMemory != 0) {
+    if (Config->InitialMemory != alignTo(Config->InitialMemory, WasmPageSize))
+      error("initial memory must be " + Twine(WasmPageSize) + "-byte aligned");
----------------
ruiu wrote:
> This can be `Config->InitMemory % WasmPageSize != 0`
OK, it's just copied from the block above however where the same idiom is used. Should compile down to the same thing.


================
Comment at: lld/trunk/wasm/Writer.cpp:612
+    if (Config->InitialMemory != alignTo(Config->InitialMemory, WasmPageSize))
+      error("initial memory must be " + Twine(WasmPageSize) + "-byte aligned");
+    if (MemoryPtr > Config->InitialMemory)
----------------
ruiu wrote:
> Something like
> 
>   error: -initial-memory=<value>: value is not aligned to 4096 bytes
> 
> is more common style of an error message.
OK, I'll tidy that in a future commit. Thanks.


================
Comment at: lld/trunk/wasm/Writer.cpp:615
+      error("initial memory too small, " + Twine(MemoryPtr) + " bytes needed");
+    else
+      MemoryPtr = Config->InitialMemory;
----------------
ruiu wrote:
> This `else` looks odd because if there is an error in the first `if`, the control reaches here, but if there is an error in the second `if`, it doesn't. Maybe we should just set it unconditionally. We won't use a value if there's an error because we'll exit before we use a dummy value.
You say we'll exit before we use a dummy value - but I don't think exit is no-return?

In particular, if exit returns then we'll carry on to the MaxMemory block below, which uses MemoryPtr, so we have to leave this block with a "safe" value for MemoryPtr.

That was my logic anyway.


Repository:
  rL LLVM

https://reviews.llvm.org/D44393