Bug 166448 – WebAssembly: Implement grow_memory and current_memory

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Saam Barati]

...

[Saam Barati]

Created attachment 297698 [details]
WIP

[JF Bastien]

Comment on attachment 297698 [details]
WIP

View in context: https://bugs.webkit.org/attachment.cgi?id=297698&action=review

> Source/JavaScriptCore/wasm/WasmMemory.cpp:45
> +    m_mappedCapacity = maximum ? maximum.bytes() : PageCount::max().bytes();

If initial is 0 you should allocate some value anyways (below it falls back to m_size).

Can you add an ASSERT that the size is smaller or equal to static_cast<uint64_t>(std::numeric_limits<uint32_t>::max())+1? That's super unintuitive...

> Source/JavaScriptCore/wasm/WasmMemory.cpp:65
> +bool Memory::grow(PageCount newSize)

I'd return Expected<void*, String> here.

> Source/JavaScriptCore/wasm/WasmMemory.cpp:67
> +    RELEASE_ASSERT(m_memory);

This means initial allocation failed, right? That could have been OK if the memory size was 0, no? It's only bad when we try to grow and it fails.

> Source/JavaScriptCore/wasm/WasmMemory.cpp:74
> +        bool success = !mprotect(m_memory, static_cast<size_t>(desiredSize), PROT_READ | PROT_WRITE);

You don't need to re-protect the initial pages.

> Source/JavaScriptCore/wasm/WasmMemory.cpp:81
> +    // Otherwise, lets try to make some new memory.

*let's

> Source/JavaScriptCore/wasm/WasmMemory.cpp:82
> +    void* newMemory = mmap(nullptr, desiredSize, PROT_NONE, MAP_PRIVATE | MAP_ANON, -1, 0);

mremap first? I don't know if you should make the trailing pages RW first though (and then ask it to give you more at the end, potentially moving everything).

> Source/JavaScriptCore/wasm/WasmMemory.cpp:85
> +    bool success = !mprotect(newMemory, static_cast<size_t>(desiredSize), PROT_READ | PROT_WRITE);

Same, I would only protect the new pages, not the ones that were already RW.

> Source/JavaScriptCore/wasm/WasmMemory.cpp:90
> +    munmap(m_memory, m_mappedCapacity);

Yeah mremap should handle this already, and won't use up to 2x physical pages since it can just play with the pagetable.

[Saam Barati]

Comment on attachment 297698 [details]
WIP

View in context: https://bugs.webkit.org/attachment.cgi?id=297698&action=review

>> Source/JavaScriptCore/wasm/WasmMemory.cpp:67
>> +    RELEASE_ASSERT(m_memory);
> 
> This means initial allocation failed, right? That could have been OK if the memory size was 0, no? It's only bad when we try to grow and it fails.

I don't think we ever create a Memory which fails at allocation time. We should figure out if we should allow that.

>> Source/JavaScriptCore/wasm/WasmMemory.cpp:74
>> +        bool success = !mprotect(m_memory, static_cast<size_t>(desiredSize), PROT_READ | PROT_WRITE);
> 
> You don't need to re-protect the initial pages.

Fixed.

>> Source/JavaScriptCore/wasm/WasmMemory.cpp:81
>> +    // Otherwise, lets try to make some new memory.
> 
> *let's

Fixed.

>> Source/JavaScriptCore/wasm/WasmMemory.cpp:82
>> +    void* newMemory = mmap(nullptr, desiredSize, PROT_NONE, MAP_PRIVATE | MAP_ANON, -1, 0);
> 
> mremap first? I don't know if you should make the trailing pages RW first though (and then ask it to give you more at the end, potentially moving everything).

mremap isn't a thing on darwin.

>> Source/JavaScriptCore/wasm/WasmMemory.cpp:85
>> +    bool success = !mprotect(newMemory, static_cast<size_t>(desiredSize), PROT_READ | PROT_WRITE);
> 
> Same, I would only protect the new pages, not the ones that were already RW.

This is all new memory here, that said, I should just pass PROT_READ|PROT_WRITE to mmap.

[Saam Barati]

Created attachment 297706 [details]
WIP

[JF Bastien]

> >> Source/JavaScriptCore/wasm/WasmMemory.cpp:82
> >> +    void* newMemory = mmap(nullptr, desiredSize, PROT_NONE, MAP_PRIVATE | MAP_ANON, -1, 0);
> > 
> > mremap first? I don't know if you should make the trailing pages RW first though (and then ask it to give you more at the end, potentially moving everything).
> 
> mremap isn't a thing on darwin.

Boo! We should first try mmap with MAP_FIXED then, to try to extend our VA without requiring 2x the already-allocated physical pages. If that fails then we'll need to mmap+memcpy. I'm wary of this failing though, so we should have a FIXME on it (i.e. I think we should mmap without backing physical memory, and then memcpy a few pages at a time, munmap, and so on, so that we never really have too many physical pages).

Maybe MAP_FIXED can also be a FIXME for now, but I think we'll want it soon.

[JF Bastien]

Comment on attachment 297706 [details]
WIP

View in context: https://bugs.webkit.org/attachment.cgi?id=297706&action=review

I think we want to detect errno when mmap returns MAP_FAILED:
 - For MAP_FIXED we want to handle ENOMEM with the fallback.
 - Linux can errno with EAGAIN, it looks like Darwin can't? Let's ignore then.

We currently don't base out allocation on getpagesize, we probably should. Add a FIXME?

> JSTests/wasm/js-api/memory-grow.js:14
> +        for (let i = 0; i < pageSize; i++) {

Can you first assert that the content is all zero? Also, after grow, that all new memory is also zero?

> Source/JavaScriptCore/wasm/WasmMemory.cpp:77
> +            return false;

IIUC this should never happen, right? We've mmap'd the pages, we own them, their protection should succeed? This seems like something we should CRASH() on, or maybe just neuter the memory (and all instances which point to it). What if it half-fails to protect things? This going wrong seems like we're in trouble, we shouldn't try to behave.

> Source/JavaScriptCore/wasm/WasmMemory.cpp:88
> +    munmap(m_memory, m_mappedCapacity);

Here is where I'd add a FIXME to copy a few pages at a time.

Also, check that munmap returned 0, and CRASH() if it didn't.

> Source/JavaScriptCore/wasm/js/JSWebAssemblyMemory.cpp:62
> +        m_buffer->transferTo(dummyContents);

This holds on to the backing memory, right? So if there's an array buffer or a view that refers to it, it'll stay live? Do we test for this (wasm.mem, get buffer, keep buffer but drop wasm.mem, force GC, check that buffer still valid)?

Can be a FIXME as well.

> Source/JavaScriptCore/wasm/js/JSWebAssemblyMemory.cpp:102
> +        // FIXME: what's the correct behavior for delta == 0?

Bug #

> Source/JavaScriptCore/wasm/js/WebAssemblyMemoryPrototype.cpp:89
> +    return JSValue::encode(jsNumber(result.pageCount()));

Isn't that the new size? We want the old one.

> Source/JavaScriptCore/wasm/js/WebAssemblyMemoryPrototype.cpp:108
> +    JSC_NATIVE_FUNCTION_WITHOUT_TRANSITION("grow", webAssemblyMemoryProtoFuncGrow, DontEnum, 1);

This should be in the .lut.h above instead of here (both should). You add it to the comment, and magic happens.

[JF Bastien]

I'm touching things around MemoryInformation, and I think this patch now makes it incorrect:
    explicit operator bool() const { return !!m_initial; }

This means that code which checks "do I have a memory?" are currently OK: either the program started with one, or they don't have one and never will. Once you implement your patch the check will now be erroneous!

I'd like the bool pattern to keep working but for it to now say "can this have a memory now or any point in the future through grow".

I'm changing the wasm B3 IR generator, and when I save / restore thing off of VM I just want to know whether to save / restore memory+size. Saving occurs from VM, restoring occurs from Instance->memory (onto VM, as well as in registers).

[Saam Barati]

(In reply to comment #7)
> I'm touching things around MemoryInformation, and I think this patch now
> makes it incorrect:
>     explicit operator bool() const { return !!m_initial; }
> 
> This means that code which checks "do I have a memory?" are currently OK:
> either the program started with one, or they don't have one and never will.
> Once you implement your patch the check will now be erroneous!
> 
> I'd like the bool pattern to keep working but for it to now say "can this
> have a memory now or any point in the future through grow".
> 
> I'm changing the wasm B3 IR generator, and when I save / restore thing off
> of VM I just want to know whether to save / restore memory+size. Saving
> occurs from VM, restoring occurs from Instance->memory (onto VM, as well as
> in registers).

The check is correct as is now.
!!m_initial is a check against if the PageCount is valid, not if the PageCount is zero.

[Saam Barati]

Created attachment 297726 [details]
WIP

Still need to address some of JF's comments, but I think grow_memory is now implemented.

[Saam Barati]

Created attachment 297743 [details]
WIP

This patch might be the bulk of the work to implement grow_memory and current_memory.
That said, I'm currently running into issues with wabt improperly generating binaries that reflect the wast file:
https://github.com/WebAssembly/wabt/issues/256

[Saam Barati]

The remaining spec-test failures for me on a debug build are:
wasm.yaml/wasm/spec-tests/call.wast.js.default-wasm: ERROR: Unexpected exit code: 139
wasm.yaml/wasm/spec-tests/call_indirect.wast.js.default-wasm: ERROR: Unexpected exit code: 139
wasm.yaml/wasm/spec-tests/custom_section.wast.js.default-wasm: ERROR: Unexpected exit code: 3
wasm.yaml/wasm/spec-tests/exports.wast.js.default-wasm: ERROR: Unexpected exit code: 3
wasm.yaml/wasm/spec-tests/fac.wast.js.default-wasm: ERROR: Unexpected exit code: 139
wasm.yaml/wasm/spec-tests/float_exprs.wast.js.default-wasm: ERROR: Unexpected exit code: 3
wasm.yaml/wasm/spec-tests/func_ptrs.wast.js.default-wasm: ERROR: Unexpected exit code: 3
wasm.yaml/wasm/spec-tests/globals.wast.js.default-wasm: ERROR: Unexpected exit code: 3
wasm.yaml/wasm/spec-tests/i32.wast.js.default-wasm: ERROR: Unexpected exit code: 136
wasm.yaml/wasm/spec-tests/imports.wast.js.default-wasm: ERROR: Unexpected exit code: 3
wasm.yaml/wasm/spec-tests/int_exprs.wast.js.default-wasm: ERROR: Unexpected exit code: 3
wasm.yaml/wasm/spec-tests/linking.wast.js.default-wasm: ERROR: Unexpected exit code: 139
wasm.yaml/wasm/spec-tests/memory.wast.js.default-wasm: ERROR: Unexpected exit code: 3
wasm.yaml/wasm/spec-tests/i64.wast.js.default-wasm: ERROR: Unexpected exit code: 136
wasm.yaml/wasm/spec-tests/names.wast.js.default-wasm: ERROR: Unexpected exit code: 3
wasm.yaml/wasm/spec-tests/resizing.wast.js.default-wasm: ERROR: Unexpected exit code: 139
wasm.yaml/wasm/spec-tests/traps.wast.js.default-wasm: ERROR: Unexpected exit code: 136
wasm.yaml/wasm/spec-tests/skip-stack-guard-page.wast.js.default-wasm: ERROR: Unexpected exit code: 139

[Saam Barati]

Created attachment 297744 [details]
WIP

Writing some tests.

[Saam Barati]

*** Bug 166475 has been marked as a duplicate of this bug. ***

[Radar WebKit Bug Importer]

<rdar://problem/29803676>

[Saam Barati]

Created attachment 297748 [details]
WIP

parseVarUint1 fix.

[Saam Barati]

Created attachment 297753 [details]
WIP

Might be done. Still writing tests.

[JF Bastien]

Comment on attachment 297753 [details]
WIP

View in context: https://bugs.webkit.org/attachment.cgi?id=297753&action=review

> JSTests/wasm/function-tests/grow-memory.js:17
> +    assert.truthy(threw);

assert.throws(() => new WebAssembly.Module(bin), WebAssembly.LinkError, msg);
It now even supports checking the message without context.

> Source/JavaScriptCore/wasm/WasmB3IRGenerator.cpp:351
> +            m_currentBlock->appendNew<ConstPtrValue>(m_proc, Origin(), &m_vm.topJSWebAssemblyInstance));

I wrote a similar thing in my current patch.

> Source/JavaScriptCore/wasm/WasmB3IRGenerator.cpp:355
> +            memory, JSWebAssemblyMemory::offsetOfMemory());

But deleted these. I materialize them from Instance instead, and only do so if there's a memory present.

> Source/JavaScriptCore/wasm/WasmB3IRGenerator.cpp:425
> +            size, m_currentBlock->appendNew<Const64Value>(m_proc, Origin(), PageCount::pageSize));

Right-shift by 16 would be better here (with a static assert on pageSize).

> Source/JavaScriptCore/wasm/WasmFunctionParser.h:485
> +        WASM_PARSER_FAIL_IF(!parseVarUInt1(reserved), "Can't parse reserved varUint1 for grow_memory");

Here and below, we haven't been capitalizing error messages (since they get appended to other text).

> Source/JavaScriptCore/wasm/WasmMemory.cpp:45
> +    failed = false;

I prefer starting with failed = true, and only setting it to false at the very end, on success.

> Source/JavaScriptCore/wasm/WasmMemory.cpp:57
> +    //void* result = mmap(nullptr, m_mappedCapacity, PROT_NONE, MAP_PRIVATE | MAP_ANON, -1, 0);

Debug stuff. As we discussed, maybe we need a runtime flag for this.

> Source/JavaScriptCore/wasm/WasmMemory.cpp:59
> +    if (/*result == MAP_FAILED*/ true) {

Ditto.

> Source/JavaScriptCore/wasm/WasmMemory.cpp:106
> +        munmap(m_memory, m_mappedCapacity);

RELEASE_ASSERT on munmap's return.

> Source/JavaScriptCore/wasm/WasmPageCount.h:56
> +        uint32_t numPages = bytes / pageSize;

uint64_t instead?

> Source/JavaScriptCore/wasm/WasmPageCount.h:74
> +    PageCount operator+(const PageCount& other)

Method can be const.

> Source/JavaScriptCore/wasm/js/JSWebAssemblyMemory.cpp:114
> +    // Neuter the old array.

Hmm, so when grow fails we don't neuter, but if it succeeds we neuter? That seems like an odd corner case that's not well-specified by the spec, right? Do we have a test for this?

> Source/JavaScriptCore/wasm/js/WebAssemblyMemoryPrototype.cpp:109
> +    JSC_NATIVE_FUNCTION_WITHOUT_TRANSITION("grow", webAssemblyMemoryProtoFuncGrow, DontEnum, 1);

.lut.h above instead of adding it here.

> Source/JavaScriptCore/wasm/js/WebAssemblyModuleRecord.cpp:265
> +                    // FIXME: what's the behabior here? Do we static_cast<uint32_t>(int32_t) or do we throw exception on < 0?

I think it's unsigned. It's possible to create a Memory which is > 2GiB so it needs to be initializable.

> Source/JavaScriptCore/wasm/js/WebAssemblyModuleRecord.cpp:268
> +                        offset = m_instance->loadI32Global(segment->offset.globalImportIndex());

segment->offset.globalImportIndex() is guaranteed to be valid here, right?

[Saam Barati]

Comment on attachment 297753 [details]
WIP

View in context: https://bugs.webkit.org/attachment.cgi?id=297753&action=review

>> JSTests/wasm/function-tests/grow-memory.js:17
>> +    assert.truthy(threw);
> 
> assert.throws(() => new WebAssembly.Module(bin), WebAssembly.LinkError, msg);
> It now even supports checking the message without context.

Ok I'll try this. However, it should be CompileError, not LinkError

>> Source/JavaScriptCore/wasm/WasmB3IRGenerator.cpp:351
>> +            m_currentBlock->appendNew<ConstPtrValue>(m_proc, Origin(), &m_vm.topJSWebAssemblyInstance));
> 
> I wrote a similar thing in my current patch.

Cool. Whoever lands first should just rebase with the other's implementation. I like your idea of removing one indirection by making JSWebAssemblyMemory have a Wasm::Memory field instead of a unique_ptr<Wasm::Memory> field

>> Source/JavaScriptCore/wasm/WasmB3IRGenerator.cpp:355
>> +            memory, JSWebAssemblyMemory::offsetOfMemory());
> 
> But deleted these. I materialize them from Instance instead, and only do so if there's a memory present.

Yeah that makes sense.

>> Source/JavaScriptCore/wasm/WasmB3IRGenerator.cpp:425
>> +            size, m_currentBlock->appendNew<Const64Value>(m_proc, Origin(), PageCount::pageSize));
> 
> Right-shift by 16 would be better here (with a static assert on pageSize).

I suspect B3 would do this for us but I can go with your suggestion too.

>> Source/JavaScriptCore/wasm/WasmFunctionParser.h:485
>> +        WASM_PARSER_FAIL_IF(!parseVarUInt1(reserved), "Can't parse reserved varUint1 for grow_memory");
> 
> Here and below, we haven't been capitalizing error messages (since they get appended to other text).

Not sure what you mean. Should I not provide a message here?

>> Source/JavaScriptCore/wasm/WasmMemory.cpp:45
>> +    failed = false;
> 
> I prefer starting with failed = true, and only setting it to false at the very end, on success.

SGTM

>> Source/JavaScriptCore/wasm/WasmMemory.cpp:57
>> +    //void* result = mmap(nullptr, m_mappedCapacity, PROT_NONE, MAP_PRIVATE | MAP_ANON, -1, 0);
> 
> Debug stuff. As we discussed, maybe we need a runtime flag for this.

Yeah I'll add an option for this. Maybe "wasmSimulateLimited
Emory". We should also run all of our tests once with and once without this flag.

>> Source/JavaScriptCore/wasm/WasmMemory.cpp:106
>> +        munmap(m_memory, m_mappedCapacity);
> 
> RELEASE_ASSERT on munmap's return.

SGTM

>> Source/JavaScriptCore/wasm/WasmPageCount.h:56
>> +        uint32_t numPages = bytes / pageSize;
> 
> uint64_t instead?

This should always fit in uint32_t

>> Source/JavaScriptCore/wasm/WasmPageCount.h:74
>> +    PageCount operator+(const PageCount& other)
> 
> Method can be const.

Ok

>> Source/JavaScriptCore/wasm/js/WebAssemblyMemoryPrototype.cpp:109
>> +    JSC_NATIVE_FUNCTION_WITHOUT_TRANSITION("grow", webAssemblyMemoryProtoFuncGrow, DontEnum, 1);
> 
> .lut.h above instead of adding it here.

SGTM

[Saam Barati]

Comment on attachment 297753 [details]
WIP

View in context: https://bugs.webkit.org/attachment.cgi?id=297753&action=review

>> Source/JavaScriptCore/wasm/js/JSWebAssemblyMemory.cpp:114
>> +    // Neuter the old array.
> 
> Hmm, so when grow fails we don't neuter, but if it succeeds we neuter? That seems like an odd corner case that's not well-specified by the spec, right? Do we have a test for this?

It is well specified:
https://github.com/WebAssembly/design/blob/master/JS.md#webassemblymemoryprototypegrow

>> Source/JavaScriptCore/wasm/js/WebAssemblyModuleRecord.cpp:265
>> +                    // FIXME: what's the behabior here? Do we static_cast<uint32_t>(int32_t) or do we throw exception on < 0?
> 
> I think it's unsigned. It's possible to create a Memory which is > 2GiB so it needs to be initializable.

Yeah I agree. We should have a test where we properly do casting here. I'll add one

>> Source/JavaScriptCore/wasm/js/WebAssemblyModuleRecord.cpp:268
>> +                        offset = m_instance->loadI32Global(segment->offset.globalImportIndex());
> 
> segment->offset.globalImportIndex() is guaranteed to be valid here, right?

Yes

[JF Bastien]

> >> Source/JavaScriptCore/wasm/WasmFunctionParser.h:485
> >> +        WASM_PARSER_FAIL_IF(!parseVarUInt1(reserved), "Can't parse reserved varUint1 for grow_memory");
> > 
> > Here and below, we haven't been capitalizing error messages (since they get appended to other text).
> 
> Not sure what you mean. Should I not provide a message here?

I just mean lowercase first letter.

[Saam Barati]

Comment on attachment 297744 [details]
WIP

View in context: https://bugs.webkit.org/attachment.cgi?id=297744&action=review

> Source/JavaScriptCore/wasm/js/JSWebAssemblyMemory.cpp:120
> +    if (m_buffer) {
> +        ArrayBufferContents dummyContents;
> +        m_buffer->transferTo(dummyContents);
> +        m_buffer = nullptr;
> +        m_bufferWrapper.clear();
> +    }

Actually, I'm not sure what the behavior here is w.r.t the Wasm grow_memory opcode. Should we always neuter, or neuter on success only, like the JS API?

[Saam Barati]

Created attachment 297781 [details]
patch

[Saam Barati]

Comment on attachment 297781 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=297781&action=review

> Source/JavaScriptCore/wasm/WasmB3IRGenerator.cpp:380
> +    const MemoryInformation& memory = m_info.memory;
> +    for (auto info : memory.pinnedRegisters().sizeRegisters) {
> +        Value* sizeValue = m_currentBlock->appendNew<Value>(m_proc, Sub, Origin(), size, m_currentBlock->appendNew<Const64Value>(m_proc, Origin(), info.sizeOffset));
> +        arguments.append(sizeValue);
> +    }
> +
> +    B3::PatchpointValue* patchpoint = m_currentBlock->appendNew<B3::PatchpointValue>(m_proc, B3::Void, Origin());
> +    patchpoint->effects = Effects::none();
> +    patchpoint->effects.writesPinned = true;
> +
> +    patchpoint->append(ConstrainedValue(baseMemory, ValueRep::reg(memory.pinnedRegisters().baseMemoryPointer)));
> +    for (uint32_t i = 0; i < memory.pinnedRegisters().sizeRegisters.size(); ++i)
> +        patchpoint->append(ConstrainedValue(arguments[i], ValueRep::reg(memory.pinnedRegisters().sizeRegisters[i].sizeRegister)));
> +
> +    // FIXME: Is it valid to just make this constrained arguments to the patchpoint? Is there a less-hacky way to do this in B3?

Fil, Keith, is this valid to do?

[WebKit Commit Bot]

This patch modifies one of the wasm.json files. Please ensure that any changes in one have been mirrored to the other. You can find the wasm.json files at "Source/JavaScriptCore/wasm/wasm.json" and "JSTests/wasm/wasm.json".

[WebKit Commit Bot]

Attachment 297781 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/wasm/WasmMemory.cpp:48:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
ERROR: Source/JavaScriptCore/wasm/js/WebAssemblyMemoryPrototype.cpp:35:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/JavaScriptCore/wasm/WasmPageCount.h:55:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
ERROR: Source/JavaScriptCore/wasm/WasmFunctionParser.h:486:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
ERROR: Source/JavaScriptCore/wasm/WasmFunctionParser.h:503:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Total errors found: 5 in 30 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Filip Pizlo]

(In reply to comment #23)
> Comment on attachment 297781 [details]
> patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=297781&action=review
> 
> > Source/JavaScriptCore/wasm/WasmB3IRGenerator.cpp:380
> > +    const MemoryInformation& memory = m_info.memory;
> > +    for (auto info : memory.pinnedRegisters().sizeRegisters) {
> > +        Value* sizeValue = m_currentBlock->appendNew<Value>(m_proc, Sub, Origin(), size, m_currentBlock->appendNew<Const64Value>(m_proc, Origin(), info.sizeOffset));
> > +        arguments.append(sizeValue);
> > +    }

Remove this. 

> > +
> > +    B3::PatchpointValue* patchpoiny = m_currentBlock->appendNew<B3::PatchpointValue>(m_proc, B3::Void, Origin());
> > +    patchpoint->effects = Effects::none();
> > +    patchpoint->effects.writesPinned = true;

Also indicate that you will clobber the pinned registers in the clobbered register set. 

> > +
> > +    patchpoint->append(ConstrainedValue(baseMemory, ValueRep::reg(memory.pinnedRegisters().baseMemoryPointer)));
> > +    for (uint32_t i = 0; i < memory.pinnedRegisters().sizeRegisters.size(); ++i)
> > +        patchpoint->append(ConstrainedValue(arguments[i], ValueRep::reg(memory.pinnedRegisters().sizeRegisters[i].sizeRegister)));

Remove this. 

> > +
> > +    // FIXME: Is it valid to just make this constrained arguments to the patchpoint? Is there a less-hacky way to do this in B3?
> 
> Fil, Keith, is this valid to do?

the patchpoint should have a generator that emits moves to those registers. Then you can make the changes I suggest above. 

I think what you're doing now is valid but not canonical. It's not canonical because it requires more code than what I suggest (I think).

[Saam Barati]

Created attachment 297796 [details]
patch

Addressed Fil's comments.

[WebKit Commit Bot]

Attachment 297796 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/wasm/WasmMemory.cpp:48:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
ERROR: Source/JavaScriptCore/wasm/js/WebAssemblyMemoryPrototype.cpp:35:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/JavaScriptCore/wasm/WasmPageCount.h:55:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
ERROR: Source/JavaScriptCore/wasm/WasmFunctionParser.h:486:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
ERROR: Source/JavaScriptCore/wasm/WasmFunctionParser.h:503:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Total errors found: 5 in 30 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Keith Miller]

Comment on attachment 297796 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=297796&action=review

r=me with some comments.Also, it might be worthwhile to add an unreachable grow_memory / current_memory instruction since we've had bugs decoding unreachable things in the past.

> Source/JavaScriptCore/ChangeLog:9
> +        This patch implements grow_memory, current_memory, and WebAssembly.prototype.grow.

Shouldn't this be WebAssembly.Memory.prototype.grow?

> Source/JavaScriptCore/wasm/WasmB3IRGenerator.cpp:414
> +    Value* size = m_currentBlock->appendNew<MemoryValue>(m_proc, Load, Int64, Origin(),

Nit: I would use pointerType() here instead of Int64.

[Saam Barati]

Comment on attachment 297796 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=297796&action=review

I also added a test for grow_memory/current_memory after Unreachable().

>> Source/JavaScriptCore/ChangeLog:9
>> +        This patch implements grow_memory, current_memory, and WebAssembly.prototype.grow.
> 
> Shouldn't this be WebAssembly.Memory.prototype.grow?

Yeah. And oops. I forgot to change this in my final changelog.

>> Source/JavaScriptCore/wasm/WasmB3IRGenerator.cpp:414
>> +    Value* size = m_currentBlock->appendNew<MemoryValue>(m_proc, Load, Int64, Origin(),
> 
> Nit: I would use pointerType() here instead of Int64.

I removed this field entirely from VM.

[Saam Barati]

landed in:
https://trac.webkit.org/changeset/210201

[Saam Barati]

Follow up build fix:
https://trac.webkit.org/changeset/210202