Bug 161710 – Add support for WASM Memory.

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Keith Miller]

...

[Keith Miller]

Created attachment 289843 [details]
Patch

[Filip Pizlo]

Comment on attachment 289843 [details]
Patch

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

> Source/JavaScriptCore/testWASM.cpp:385
> +        Vector<uint8_t> vector = {
> +            0x00, 0x61, 0x73, 0x6d, 0x0c, 0x00, 0x00, 0x00, 0x01, 0x87, 0x80, 0x80, 0x80, 0x00, 0x01, 0x40,
> +            0x03, 0x01, 0x01, 0x01, 0x00, 0x03, 0x82, 0x80, 0x80, 0x80, 0x00, 0x01, 0x00, 0x05, 0x83, 0x80,
> +            0x80, 0x80, 0x00, 0x01, 0x01, 0x01, 0x07, 0x8f, 0x80, 0x80, 0x80, 0x00, 0x01, 0x0b, 0x77, 0x72,
> +            0x69, 0x74, 0x65, 0x5f, 0x61, 0x72, 0x72, 0x61, 0x79, 0x00, 0x00, 0x0a, 0xaf, 0x80, 0x80, 0x80,
> +            0x00, 0x01, 0xa9, 0x80, 0x80, 0x80, 0x00, 0x01, 0x01, 0x01, 0x10, 0x00, 0x15, 0x03, 0x01, 0x00,
> +            0x02, 0x00, 0x14, 0x03, 0x14, 0x02, 0x56, 0x07, 0x01, 0x14, 0x01, 0x14, 0x03, 0x40, 0x14, 0x00,
> +            0x2e, 0x00, 0x00, 0x10, 0x01, 0x14, 0x03, 0x40, 0x15, 0x03, 0x06, 0x00, 0x0f, 0x0f, 0x09, 0x0f
> +        };
> +

Oh man, we should figure out a better way of writing these test cases!

> Source/JavaScriptCore/wasm/WASMB3IRGenerator.cpp:344
> +    // We need to make sure that we add the memoryOffset to the pointer with a 32bit add
> +    // so if it overflows we stay within our mapped memory.
> +    pointer = m_currentBlock->appendNew<Value>(m_proc, Add, Origin(), pointer, memoryOffset);
> +
> +    PatchpointValue* patchpoint = m_currentBlock->appendNew<PatchpointValue>(m_proc, typeForLoad(op), Origin());
> +    patchpoint->appendSomeRegister(pointer);
> +    patchpoint->numGPScratchRegisters++;
> +    patchpoint->setGenerator([=] (CCallHelpers& jit, const StackmapGenerationParams& params) {
> +        GPRReg scratch = params.gpScratch(0);
> +        GPRReg result = params[0].gpr();
> +        GPRReg ptr = params[1].gpr();
> +
> +        jit.load32(bitwise_cast<char*>(&m_memory) + ModuleMemoryInformation::offsetOfSize(), scratch);
> +        CCallHelpers::Jump invalid = jit.branch32(CCallHelpers::AboveOrEqual, ptr, scratch);
> +
> +        jit.move(CCallHelpers::ImmPtr(m_memory.memory()), scratch);
> +        loadForOp(jit, op, MacroAssembler::BaseIndex(scratch, ptr, MacroAssembler::Scale::TimesOne), result);
> +        CCallHelpers::Jump done = jit.jump();
> +
> +        invalid.link(&jit);
> +        // Right now we crash; In the future we should do the trapping thing.
> +        jit.breakpoint();
> +        done.link(&jit);
> +    });
> +
> +    result = patchpoint;

Whoa!!!  Why not just use B3 32-bit load followed by B3 32->64 sign extend or zero extend?

If this results in bad code, you could easily add B3 instruction selection rules for this.

[Keith Miller]

Created attachment 289954 [details]
Patch

[WebKit Commit Bot]

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


ERROR: Source/JavaScriptCore/wasm/WASMOps.h:110:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/wasm/WASMOps.h:114:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 2 in 12 files


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

[Keith Miller]

Comment on attachment 289954 [details]
Patch

Via offline discussion with pizlo I think we are going to change how this works.

[Keith Miller]

Created attachment 290637 [details]
WIP

[Keith Miller]

Created attachment 290687 [details]
Patch

[Csaba Osztrogonác]

Comment on attachment 290687 [details]
Patch

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

> Source/JavaScriptCore/CMakeLists.txt:861
> +    wasm/WASMMMemory.cpp

typo: WASMMMemory.cpp -> WASMMemory.cpp

[Keith Miller]

Comment on attachment 290687 [details]
Patch

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

>> Source/JavaScriptCore/CMakeLists.txt:861
>> +    wasm/WASMMMemory.cpp
> 
> typo: WASMMMemory.cpp -> WASMMemory.cpp

Whoops, fixed. Thanks.

[Keith Miller]

Created attachment 290714 [details]
Patch

[Mark Lam]

Comment on attachment 290714 [details]
Patch

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

> Source/JavaScriptCore/wasm/WASMMemory.cpp:42
> +        // Try again with a different number.
> +        result = mmap(nullptr, maxPageNum, PROT_NONE, MAP_PRIVATE | MAP_ANON, 0, 0);

Drive by comment: How is this different from the try above?  The comment said to try with a different number, but it appears we're using the same numbers here.

[Keith Miller]

Comment on attachment 290714 [details]
Patch

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

>> Source/JavaScriptCore/wasm/WASMMemory.cpp:42
>> +        result = mmap(nullptr, maxPageNum, PROT_NONE, MAP_PRIVATE | MAP_ANON, 0, 0);
> 
> Drive by comment: How is this different from the try above?  The comment said to try with a different number, but it appears we're using the same numbers here.

Whoops, that should be maxSize.

[Keith Miller]

Created attachment 290716 [details]
Patch

[JF Bastien]

Comment on attachment 290716 [details]
Patch

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

Haven't gone through everything, LMK when ready to review for real.

> Source/JavaScriptCore/wasm/WASMB3IRGenerator.cpp:310
> +        break;

This can be auto-generated from the JSON file's "return".

> Source/JavaScriptCore/wasm/WASMB3IRGenerator.cpp:390
> +        return 8;

Ditto.

> JSTests/stress/wasm/generate-wasmops-header.js:42
> +    ...opcodeMacroizer(op => (op.category === "store")),

(discussed on IRC) it's probably better to look at "return" to distinguish load / store instead of changing the category, because atomics like cmpxchg will make this weird.

[Keith Miller]

Comment on attachment 290716 [details]
Patch

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

>> Source/JavaScriptCore/wasm/WASMB3IRGenerator.cpp:310
>> +        break;
> 
> This can be auto-generated from the JSON file's "return".

I'm not sure I see how since the return field only states the WASM type of the return not the number of bytes being read from memory.

[Keith Miller]

Comment on attachment 290716 [details]
Patch

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

>> JSTests/stress/wasm/generate-wasmops-header.js:42
>> +    ...opcodeMacroizer(op => (op.category === "store")),
> 
> (discussed on IRC) it's probably better to look at "return" to distinguish load / store instead of changing the category, because atomics like cmpxchg will make this weird.

Fixed.

[JF Bastien]

Comment on attachment 290716 [details]
Patch

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

>>> Source/JavaScriptCore/wasm/WASMB3IRGenerator.cpp:310
>>> +        break;
>> 
>> This can be auto-generated from the JSON file's "return".
> 
> I'm not sure I see how since the return field only states the WASM type of the return not the number of bytes being read from memory.

Oh right. We could add it to the file then? Later patch, I'm happy to do that :-)
Seems like we'll want that info for tests, and for other ops like atomics.

[Keith Miller]

Created attachment 290723 [details]
Patch

[JF Bastien]

Comment on attachment 290723 [details]
Patch

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

> Source/JavaScriptCore/wasm/WASMB3IRGenerator.cpp:404
> +        FALLTHROUGH;

Is i64 trunk + 32-bit value stored into 8-bit a b3-ism? Seems odd.

> Source/JavaScriptCore/wasm/WASMB3IRGenerator.cpp:657
> +    // Return the result, if neeeded.

typo

> Source/JavaScriptCore/wasm/WASMB3IRGenerator.cpp:680
> +    std::unique_ptr<FunctionCompilation> result = std::make_unique<FunctionCompilation>();

Just my style, but I prefer: auto result(std::make_unique<FunctionCompilation>());

> Source/JavaScriptCore/wasm/WASMFunctionParser.h:158
> +        // TODO: What do I do with this?

It's just a int which can be ignored. Could determine whether to split ops on ARM, doesn't seem useful on x86 (movdqa / movdqu isn't a thing anymore AFAIK). I assume that would just be taught to B3, passed from here.

I think you need to validate the alignment though (power of 2, no more than natural). Note that alignment is on base+imm, not just base.

This seems missing from the design repo... https://github.com/WebAssembly/spec/issues/217 and https://github.com/WebAssembly/spec/issues/302
I'll fix the design repo.

> Source/JavaScriptCore/wasm/WASMFunctionParser.h:176
> +        // TODO: What do I do with this?

Same.

> Source/JavaScriptCore/wasm/WASMMemory.cpp:39
> +    void* result = mmap(nullptr, maxPageNum, PROT_NONE, MAP_PRIVATE | MAP_ANON, 0, 0);

Shouldn't this be maxPageNum * pageSize?

> Source/JavaScriptCore/wasm/WASMMemory.cpp:48
> +        munmap(result, maxSize);

maxSize may not have been what you allocated above.

> Source/JavaScriptCore/wasm/WASMMemory.h:64
> +            munmap(m_memory, m_maxSize);

This is also wrong if you retried allocation.

[Keith Miller]

Created attachment 291830 [details]
Patch

[JF Bastien]

Comment on attachment 291830 [details]
Patch

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

> Source/JavaScriptCore/wasm/WASMB3IRGenerator.cpp:306
> +    RELEASE_ASSERT_NOT_REACHED();

This could be auto-generated from the JSON file.

> Source/JavaScriptCore/wasm/WASMB3IRGenerator.cpp:388
> +    RELEASE_ASSERT_NOT_REACHED();

Same.

> Source/JavaScriptCore/wasm/WASMB3IRGenerator.cpp:646
> +            for (unsigned i = 0; i < sizes.size(); ++i)

Can you use a range-based for loop?

> Source/JavaScriptCore/wasm/WASMCallingConvention.h:80
> +        unsigned stackArgumentCount = arguments.size() < m_registerArguments.size() ? 0 : arguments.size() - m_registerArguments.size();

Vector::size returns a size_t, seems like a better fit here?

[Geoffrey Garen]

Comment on attachment 291830 [details]
Patch

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

r=me with a bunch of things to fix

> Source/JavaScriptCore/ChangeLog:21
> +        size. Currently, B3 only barely works with pinned values because
> +        we cannot change the size of memory.

Is this still true? I think this has changed.

> Source/JavaScriptCore/wasm/WASMB3IRGenerator.cpp:218
> +    // FIXME: We should support more than one size register. see: https://bugs.webkit.org/show_bug.cgi?id=162952

I don't think you need this comment here -- we have a record in bugzilla. You should add in buzilla commentary about how this is a proposed optimization.

> Source/JavaScriptCore/wasm/WASMB3IRGenerator.cpp:231
> +        // FIXME: We should support more than one size register. see: https://bugs.webkit.org/show_bug.cgi?id=162952

Ditto.

> Source/JavaScriptCore/wasm/WASMB3IRGenerator.cpp:233
> +        m_proc.pinRegister(m_memorySizeGPR);

This should be a loop to pin all registers in the vector.

> Source/JavaScriptCore/wasm/WASMB3IRGenerator.cpp:238
> +        // FIXME: Change this when we add trapping. See: https://bugs.webkit.org/show_bug.cgi?id=163351

I think this fixme is too vague since it doesn't say what we want to change to. I would say something about turning traps into exceptions rather than crashing.

> Source/JavaScriptCore/wasm/WASMB3IRGenerator.cpp:280
> +    // FIXME: We should support more than one size register. see: https://bugs.webkit.org/show_bug.cgi?id=162952

Ditto.

>> Source/JavaScriptCore/wasm/WASMB3IRGenerator.cpp:306
>> +    default:
>> +        break;
>> +    }
>> +    RELEASE_ASSERT_NOT_REACHED();
> 
> This could be auto-generated from the JSON file.

Does the JSON file indicate how many bytes per memory op? Maybe you're proposing adding that to the JSON file?

It's better to leave the default case out because that way the compiler will do static checking for case exhaustion. If you want to leave the assert in, just in case, that's cool with me.

> Source/JavaScriptCore/wasm/WASMB3IRGenerator.cpp:309
> +// FIXME: B3 should support Int64 small loads and 16-bit unsigned loads.

Same comment as below.

>> Source/JavaScriptCore/wasm/WASMB3IRGenerator.cpp:388
>> +    RELEASE_ASSERT_NOT_REACHED();
> 
> Same.

It's better to leave the default case out because that way the compiler will do static checking for case exhaustion. If you want to leave the assert in, just in case, that's cool with me.

> Source/JavaScriptCore/wasm/WASMB3IRGenerator.cpp:392
> +// FIXME: We should just support an Int64 store8.

I think this comment might be misleading because it suggests that something is wrong with this function and also that the change to Int64 store8 would be trivial. I would just remove this comment. If you added a bug URL, you could say "http://blah, we might be able to optimize this by adding an implicitly truncating store8 of an Int64."

> Source/JavaScriptCore/wasm/WASMB3IRGenerator.cpp:607
> +    // FIXME: Is this supposed to trap if less than expected number of arguments?

Let's pick a behavior we like and raise an issue in the spec. I'd suggest throwing an exception.

> Source/JavaScriptCore/wasm/WASMMemory.cpp:49
> +        if (!result)
> +            return;

mmap returns MAP_FAILED on error, no NULL! :P

> Source/JavaScriptCore/wasm/WASMMemory.h:41
> +struct PinnedRegisterInfo {

This is a good place to explain that you're planning on experimenting with holding different offsets. That would explain why this data type is a vector and not just a single entry.

> Source/JavaScriptCore/wasm/WASMMemory.h:47
> +constexpr uint32_t maxPageNum = static_cast<uint32_t>((1ull << 32) / pageSize);

Let's call this maxPageCount. We try to avoid abbreviation in WebKit style.

> Source/JavaScriptCore/wasm/WASMMemory.h:74
> +    bool growMemory(uint32_t newSize)

Let's call this "grow". It already resides in a class name "memory".

> Source/JavaScriptCore/wasm/WASMMemory.h:90
> +    uint32_t m_maxBytes { 0 };

Let's call this m_capacity.

> Source/JavaScriptCore/wasm/WASMMemory.h:91
> +    uint64_t m_mappedBytes { 0 };

Let's call this m_mappedCapacity.

> Source/JavaScriptCore/wasm/WASMModuleParser.cpp:180
> +    max *= 64 * KB;
> +    size *= 64 * KB;

This should use pageSize and the "size" and "capacity" naming scheme.

[Keith Miller]

Created attachment 291886 [details]
Patch for landing

[Keith Miller]

Created attachment 291887 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 291887 [details]
Patch for landing

Rejecting attachment 291887 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-02', 'build', '--no-clean', '--no-update', '--build-style=release', '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit

Last 500 characters of output:
ta/EWS/WebKit/Source/JavaScriptCore/yarr/YarrCanonicalizeUCS2.cpp -o /Volumes/Data/EWS/WebKit/WebKitBuild/JavaScriptCore.build/Release/JavaScriptCore.build/Objects-normal/x86_64/YarrCanonicalizeUCS2.o

** BUILD FAILED **


The following build commands failed:
	CompileC /Volumes/Data/EWS/WebKit/WebKitBuild/JavaScriptCore.build/Release/JavaScriptCore.build/Objects-normal/x86_64/WASMB3IRGenerator.o wasm/WASMB3IRGenerator.cpp normal x86_64 c++ com.apple.compilers.llvm.clang.1_0.compiler
(1 failure)

Full output: http://webkit-queues.webkit.org/results/2307387

[WebKit Commit Bot]

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


ERROR: Source/JavaScriptCore/wasm/WASMB3IRGenerator.cpp:233:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/wasm/WASMB3IRGenerator.cpp:394:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
Total errors found: 2 in 20 files


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

[Keith Miller]

Created attachment 291892 [details]
Patch for landing

[Keith Miller]

Created attachment 291895 [details]
Patch for landing

[Keith Miller]

Comment on attachment 291830 [details]
Patch

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

>> Source/JavaScriptCore/wasm/WASMB3IRGenerator.cpp:646
>> +            for (unsigned i = 0; i < sizes.size(); ++i)
> 
> Can you use a range-based for loop?

Not really since I need the index to access sizeRegisters[i].

[WebKit Commit Bot]

Comment on attachment 291895 [details]
Patch for landing

Clearing flags on attachment: 291895

Committed r207453: <http://trac.webkit.org/changeset/207453>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Saam Barati]

Comment on attachment 291895 [details]
Patch for landing

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

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

I thought mmap should pass -1 for file descriptor argument for these use cases?

> Source/JavaScriptCore/wasm/WASMMemory.cpp:52
> +    if (mprotect(result, startingSize, PROT_READ | PROT_WRITE)) {

Nit: maybe it's worth asserting that m_mappedCapaxity < startingSize?

> Source/JavaScriptCore/wasm/WASMMemory.h:42
> +// entry. Specifically an extry where the sizeOffset == 0. If we have more than one size register,

Typo: extry => entry

> Source/JavaScriptCore/wasm/WASMMemory.h:49
> +constexpr uint32_t pageSize = 64 * KB;

Doesn't WTF expose this?

[Keith Miller]

Comment on attachment 291895 [details]
Patch for landing

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

Since this patch already landed I'm putting the changes in another bug https://bugs.webkit.org/show_bug.cgi?id=163601

>> Source/JavaScriptCore/wasm/WASMMemory.cpp:43
>> +    void* result = mmap(nullptr, m_mappedCapacity, PROT_NONE, MAP_PRIVATE | MAP_ANON, 0, 0);
> 
> I thought mmap should pass -1 for file descriptor argument for these use cases?

I thought it just ignored the file descriptor. But it looks like for MAP_ANON mmap uses the file descriptor for Mach flags so we should use -1. I'll also add a FIXME for getting a VM tag too: https://bugs.webkit.org/show_bug.cgi?id=163600.

>> Source/JavaScriptCore/wasm/WASMMemory.cpp:52
>> +    if (mprotect(result, startingSize, PROT_READ | PROT_WRITE)) {
> 
> Nit: maybe it's worth asserting that m_mappedCapaxity < startingSize?

Sure, I'll change that.

>> Source/JavaScriptCore/wasm/WASMMemory.h:42
>> +// entry. Specifically an extry where the sizeOffset == 0. If we have more than one size register,
> 
> Typo: extry => entry

Fixed.

>> Source/JavaScriptCore/wasm/WASMMemory.h:49
>> +constexpr uint32_t pageSize = 64 * KB;
> 
> Doesn't WTF expose this?

Nope. It exposes the system page size, which is different than the Wasm page size.