This is an archive of the discontinued LLVM Phabricator instance.

[WebAssembly] Add missing implementation for --initial/max-memory args
ClosedPublic

Authored by ncw on Mar 12 2018, 9:54 AM.

Diff Detail

Repository
rL LLVM

Event Timeline

ncw created this revision.Mar 12 2018, 9:54 AM

Code looks good. I have concerns about adding flags that don't have current users. Can you point to at least one before we add this?

ncw added a comment.EditedMar 13 2018, 6:36 AM

Code looks good. I have concerns about adding flags that don't have current users. Can you point to at least one before we add this?

Good question - it's not so much adding a flag, as fixing an existing broken flag, but you're right. I'm not aware of any current users, but I was planning to use it myself in my own projects, to help catching any memory leaks.

sbc100 accepted this revision.Mar 13 2018, 12:09 PM
This revision is now accepted and ready to land.Mar 13 2018, 12:09 PM
This revision was automatically updated to reflect the committed changes.
ruiu added inline comments.Mar 14 2018, 11:09 AM
lld/trunk/wasm/Writer.cpp
611

This can be Config->InitMemory % WasmPageSize != 0

612

Something like

error: -initial-memory=<value>: value is not aligned to 4096 bytes

is more common style of an error message.

615

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.

ncw added inline comments.Mar 14 2018, 11:51 AM
lld/trunk/wasm/Writer.cpp
611

OK, it's just copied from the block above however where the same idiom is used. Should compile down to the same thing.

612

OK, I'll tidy that in a future commit. Thanks.

615

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.

ruiu added inline comments.Mar 14 2018, 11:56 AM
lld/trunk/wasm/Writer.cpp
615

I mean error() counts the number of errors we've seen and before calling write(), we first check if the counter is zero, and if not, we return without calling that function (which will result in existing from the program.) So we do not really care too much about a wrong value in Config.

And with this code, a wrong value could still be set to MemoryPtr. If the first error test passes but the second test succeeds, this line is executed. That's inconsistent.