Bug 165936 – WebAssembly: Implement the WebAssembly.compile and WebAssembly.validate

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Saam Barati]

...

[Saam Barati]

Created attachment 297280 [details]
WIP

This has a different patch embedded in it ATM

[Saam Barati]

This will allow us to run the unity headless benchmark.

[Saam Barati]

Created attachment 297384 [details]
patch

[Saam Barati]

Comment on attachment 297384 [details]
patch

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

> JSTests/wasm/js-api/test_Module.js:22
> +async function testPromiseAPI() {

I'm going to add one more test for it passing.

[WebKit Commit Bot]

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


ERROR: Source/JavaScriptCore/wasm/js/WebAssemblyModuleConstructor.cpp:78:  One line control clauses should not use braces.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/wasm/js/WebAssemblyModuleConstructor.cpp:82:  One line control clauses should not use braces.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/wasm/js/WebAssemblyModuleConstructor.cpp:93:  One line control clauses should not use braces.  [whitespace/braces] [4]
Total errors found: 3 in 9 files


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

[Saam Barati]

Created attachment 297386 [details]
patch

[WebKit Commit Bot]

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


ERROR: Source/JavaScriptCore/wasm/js/WebAssemblyModuleConstructor.cpp:78:  One line control clauses should not use braces.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/wasm/js/WebAssemblyModuleConstructor.cpp:82:  One line control clauses should not use braces.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/wasm/js/WebAssemblyModuleConstructor.cpp:93:  One line control clauses should not use braces.  [whitespace/braces] [4]
Total errors found: 3 in 9 files


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

[Saam Barati]

Created attachment 297387 [details]
patch

[Saam Barati]

Created attachment 297388 [details]
patch

[Saam Barati]

(In reply to comment #2)
> This will allow us to run the unity headless benchmark.

Actually we need to make a particular RELEASE_ASSERT_NOT_REACHED a jit.breakpoint() for now to make it run.

[Saam Barati]

(In reply to comment #10)
> (In reply to comment #2)
> > This will allow us to run the unity headless benchmark.
> 
> Actually we need to make a particular RELEASE_ASSERT_NOT_REACHED a
> jit.breakpoint() for now to make it run.

Inside importStubGenerator.

[JF Bastien]

Comment on attachment 297388 [details]
patch

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

> JSTests/wasm/js-api/test_Module.js:36
> +            await WebAssembly.compile(builder.WebAssembly().get());

I'd put this test in its own file (WebAssembly-compile.js or something) since this file is for .Module specifically.

> Source/JavaScriptCore/wasm/JSWebAssembly.cpp:51
> +    JSPromiseDeferred* promise =  JSPromiseDeferred::create(exec, exec->lexicalGlobalObject());

Weird spacing.

> Source/JavaScriptCore/wasm/JSWebAssembly.cpp:54
> +    JSValue module = WebAssemblyModuleConstructor::createModule(exec, exec->lexicalGlobalObject()->WebAssemblyModuleStructure());

This is synchronous though, right? We'll need a bug to make it async?

> Source/JavaScriptCore/wasm/JSWebAssembly.cpp:78
> +    }

Test for this is missing.

Also, would it be worth sharing some of this with constructJSWebAssemblyModule?

> Source/JavaScriptCore/wasm/JSWebAssembly.cpp:89
> +    return JSValue::encode(jsBoolean(plan.isValidModule()));

Validation can throw on OOM. Right now we don't distinguish it (you just get an error message). Can you add a FIXME for https://bugs.webkit.org/show_bug.cgi?id=165997 ?

> Source/JavaScriptCore/wasm/JSWebAssembly.cpp:111
> +    JSC_NATIVE_FUNCTION_WITHOUT_TRANSITION("compile", webAssemblyCompileFunc, DontEnum, 1);

These should use the .lut.h file.

[Saam Barati]

Comment on attachment 297388 [details]
patch

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

>> Source/JavaScriptCore/wasm/JSWebAssembly.cpp:51
>> +    JSPromiseDeferred* promise =  JSPromiseDeferred::create(exec, exec->lexicalGlobalObject());
> 
> Weird spacing.

Thanks, will fix.

>> Source/JavaScriptCore/wasm/JSWebAssembly.cpp:54
>> +    JSValue module = WebAssemblyModuleConstructor::createModule(exec, exec->lexicalGlobalObject()->WebAssemblyModuleStructure());
> 
> This is synchronous though, right? We'll need a bug to make it async?

Yeah this is synchronous still. I'll file a bug to make it truly asynchronous.

>> Source/JavaScriptCore/wasm/JSWebAssembly.cpp:78
>> +    }
> 
> Test for this is missing.
> 
> Also, would it be worth sharing some of this with constructJSWebAssemblyModule?

I'll add a test and yes.

>> Source/JavaScriptCore/wasm/JSWebAssembly.cpp:89
>> +    return JSValue::encode(jsBoolean(plan.isValidModule()));
> 
> Validation can throw on OOM. Right now we don't distinguish it (you just get an error message). Can you add a FIXME for https://bugs.webkit.org/show_bug.cgi?id=165997 ?

What are the expected semantics there? If Validation throws an OOM, how do we distinguish that from a valid module or not? Seems like it should be invalid IMO since we know that you'll OOM

>> Source/JavaScriptCore/wasm/JSWebAssembly.cpp:111
>> +    JSC_NATIVE_FUNCTION_WITHOUT_TRANSITION("compile", webAssemblyCompileFunc, DontEnum, 1);
> 
> These should use the .lut.h file.

ok

[Saam Barati]

Comment on attachment 297388 [details]
patch

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

>>> Source/JavaScriptCore/wasm/JSWebAssembly.cpp:111
>>> +    JSC_NATIVE_FUNCTION_WITHOUT_TRANSITION("compile", webAssemblyCompileFunc, DontEnum, 1);
>> 
>> These should use the .lut.h file.
> 
> ok

There is no static property table here. I'll defer this work to later.

[JF Bastien]

> >> Source/JavaScriptCore/wasm/JSWebAssembly.cpp:89
> >> +    return JSValue::encode(jsBoolean(plan.isValidModule()));
> > 
> > Validation can throw on OOM. Right now we don't distinguish it (you just get an error message). Can you add a FIXME for https://bugs.webkit.org/show_bug.cgi?id=165997 ?
> 
> What are the expected semantics there? If Validation throws an OOM, how do
> we distinguish that from a valid module or not? Seems like it should be
> invalid IMO since we know that you'll OOM

I think it should be an OOM exception as other OOMs. I added this to the list of things to clarify about OOM:
  https://github.com/WebAssembly/design/issues/903#issuecomment-267848357

[Saam Barati]

Created attachment 297446 [details]
patch

Addressed JF's comments.

[Mark Lam]

Comment on attachment 297446 [details]
patch

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

r=me with issues resolved.

> Source/JavaScriptCore/wasm/WasmPlan.cpp:81
> +        ASSERT(functionLength <= m_sourceLength);

I think you should make an even stronger assertion here that (functionStart + functionLength <= m_source + m_sourceLength).

> Source/JavaScriptCore/wasm/WasmPlan.cpp:84
> +        auto validateResult = validateFunction(functionStart, functionLength, signature, m_functionIndexSpace, *m_moduleInformation);

nit: "validateResult" sounds like an action whereas we would expect a noun.  How about "validationResult" instead?

> Source/JavaScriptCore/wasm/WasmPlan.cpp:101
> +    if (!isValidModule()) // This also collects a bunch of information needed to do the actual compilation.

I'm wondering if we can name the function in a way to communicate what this comment is trying to say.  Would it be sufficient to name the function "parseAndValidateModule()" instead?

> Source/JavaScriptCore/wasm/js/JSWebAssemblyHelpers.h:54
> +    // If the given bytes argument is not a BufferSource, a TypeError exception is thrown.

I know that this comment came from pre-existing code, but the reference to "bytes" here doesn't patch the arguments of this function.  How about changing it to "value"?

> Source/JavaScriptCore/wasm/js/WebAssemblyModuleConstructor.cpp:58
> +    auto* structure = InternalFunction::createSubclassStructure(exec, exec->newTarget(), exec->lexicalGlobalObject()->WebAssemblyModuleStructure());

There was an exception check after the old placement of createSubclassStructure() (below).  Should that exception check be moved here as well?

> Source/JavaScriptCore/wasm/js/WebAssemblyModuleConstructor.cpp:86
>      RETURN_IF_EXCEPTION(scope, { });

Is this exception check still needed if we no longer do the createSubclassStructure() here?

[JF Bastien]

> > Source/JavaScriptCore/wasm/WasmPlan.cpp:81
> > +        ASSERT(functionLength <= m_sourceLength);
> 
> I think you should make an even stronger assertion here that (functionStart
> + functionLength <= m_source + m_sourceLength).

I'd make sure it can't overflow though.


> > Source/JavaScriptCore/wasm/js/JSWebAssemblyHelpers.h:54
> > +    // If the given bytes argument is not a BufferSource, a TypeError exception is thrown.
> 
> I know that this comment came from pre-existing code, but the reference to
> "bytes" here doesn't patch the arguments of this function.  How about
> changing it to "value"?

It comes from here: https://github.com/WebAssembly/design/blob/master/JS.md#function-properties-of-the-webassembly-object
I'd rather keep the wording as-is from the spec, so I'd change JS.md first.

[Oliver Hunt]

Comment on attachment 297446 [details]
patch

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

>> Source/JavaScriptCore/wasm/WasmPlan.cpp:81
>> +        ASSERT(functionLength <= m_sourceLength);
> 
> I think you should make an even stronger assertion here that (functionStart + functionLength <= m_source + m_sourceLength).

Is function length being provided by the binary or actual parsing? If the binary is providing it then functionStart + functionLength could overflow.

[JF Bastien]

(In reply to comment #19)
> Comment on attachment 297446 [details]
> patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=297446&action=review
> 
> >> Source/JavaScriptCore/wasm/WasmPlan.cpp:81
> >> +        ASSERT(functionLength <= m_sourceLength);
> > 
> > I think you should make an even stronger assertion here that (functionStart + functionLength <= m_source + m_sourceLength).
> 
> Is function length being provided by the binary or actual parsing? If the
> binary is providing it then functionStart + functionLength could overflow.

Agreed on overflow. It's provided by the binary here:
https://github.com/WebAssembly/design/blob/master/BinaryEncoding.md#function-bodies

We setup start / end in the parser here:
https://github.com/WebKit/webkit/blob/54a8f928520be7461e1a809203c7828ad817e2f8/Source/JavaScriptCore/wasm/WasmModuleParser.cpp#L206

We then fill out start / end here:
https://github.com/WebKit/webkit/blob/54a8f928520be7461e1a809203c7828ad817e2f8/Source/JavaScriptCore/wasm/WasmModuleParser.cpp#L488

It would be great to get another set of eyes on the entire WasmModuleParser. I think it's pretty much done except for:
https://bugs.webkit.org/show_bug.cgi?id=165833

[Saam Barati]

Comment on attachment 297446 [details]
patch

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

>>>> Source/JavaScriptCore/wasm/WasmPlan.cpp:81
>>>> +        ASSERT(functionLength <= m_sourceLength);
>>> 
>>> I think you should make an even stronger assertion here that (functionStart + functionLength <= m_source + m_sourceLength).
>> 
>> Is function length being provided by the binary or actual parsing? If the binary is providing it then functionStart + functionLength could overflow.
> 
> Agreed on overflow. It's provided by the binary here:
> https://github.com/WebAssembly/design/blob/master/BinaryEncoding.md#function-bodies
> 
> We setup start / end in the parser here:
> https://github.com/WebKit/webkit/blob/54a8f928520be7461e1a809203c7828ad817e2f8/Source/JavaScriptCore/wasm/WasmModuleParser.cpp#L206
> 
> We then fill out start / end here:
> https://github.com/WebKit/webkit/blob/54a8f928520be7461e1a809203c7828ad817e2f8/Source/JavaScriptCore/wasm/WasmModuleParser.cpp#L488
> 
> It would be great to get another set of eyes on the entire WasmModuleParser. I think it's pretty much done except for:
> https://bugs.webkit.org/show_bug.cgi?id=165833

I'll used Checked<ChrashOnOverflow> for now and we can later come up with a nicer way to error out here.

>> Source/JavaScriptCore/wasm/WasmPlan.cpp:84
>> +        auto validateResult = validateFunction(functionStart, functionLength, signature, m_functionIndexSpace, *m_moduleInformation);
> 
> nit: "validateResult" sounds like an action whereas we would expect a noun.  How about "validationResult" instead?

I agree, I was just moving the old code around, but I like your name more than the previous "validateResult"

>> Source/JavaScriptCore/wasm/WasmPlan.cpp:101
>> +    if (!isValidModule()) // This also collects a bunch of information needed to do the actual compilation.
> 
> I'm wondering if we can name the function in a way to communicate what this comment is trying to say.  Would it be sufficient to name the function "parseAndValidateModule()" instead?

SGTM

>> Source/JavaScriptCore/wasm/js/WebAssemblyModuleConstructor.cpp:58
>> +    auto* structure = InternalFunction::createSubclassStructure(exec, exec->newTarget(), exec->lexicalGlobalObject()->WebAssemblyModuleStructure());
> 
> There was an exception check after the old placement of createSubclassStructure() (below).  Should that exception check be moved here as well?

Yeah this needs an exception check. Good catch.

>> Source/JavaScriptCore/wasm/js/WebAssemblyModuleConstructor.cpp:86
>>      RETURN_IF_EXCEPTION(scope, { });
> 
> Is this exception check still needed if we no longer do the createSubclassStructure() here?

Yup, this isn't needed anymore. Good catch. I moved the old code around in a sloppy way here.

[Saam Barati]

Comment on attachment 297446 [details]
patch

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

>>>>> Source/JavaScriptCore/wasm/WasmPlan.cpp:81
>>>>> +        ASSERT(functionLength <= m_sourceLength);
>>>> 
>>>> I think you should make an even stronger assertion here that (functionStart + functionLength <= m_source + m_sourceLength).
>>> 
>>> Is function length being provided by the binary or actual parsing? If the binary is providing it then functionStart + functionLength could overflow.
>> 
>> Agreed on overflow. It's provided by the binary here:
>> https://github.com/WebAssembly/design/blob/master/BinaryEncoding.md#function-bodies
>> 
>> We setup start / end in the parser here:
>> https://github.com/WebKit/webkit/blob/54a8f928520be7461e1a809203c7828ad817e2f8/Source/JavaScriptCore/wasm/WasmModuleParser.cpp#L206
>> 
>> We then fill out start / end here:
>> https://github.com/WebKit/webkit/blob/54a8f928520be7461e1a809203c7828ad817e2f8/Source/JavaScriptCore/wasm/WasmModuleParser.cpp#L488
>> 
>> It would be great to get another set of eyes on the entire WasmModuleParser. I think it's pretty much done except for:
>> https://bugs.webkit.org/show_bug.cgi?id=165833
> 
> I'll used Checked<ChrashOnOverflow> for now and we can later come up with a nicer way to error out here.

Actually, why would we need to do this? Wouldn't it be an early validation error to have any function with a bounds that exceeded the buffer? If we don't already have a check that does this, we should in the WasmModuleParser, not here.

[JF Bastien]

(In reply to comment #22)
> Comment on attachment 297446 [details]
> patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=297446&action=review
> 
> >>>>> Source/JavaScriptCore/wasm/WasmPlan.cpp:81
> >>>>> +        ASSERT(functionLength <= m_sourceLength);
> >>>> 
> >>>> I think you should make an even stronger assertion here that (functionStart + functionLength <= m_source + m_sourceLength).
> >>> 
> >>> Is function length being provided by the binary or actual parsing? If the binary is providing it then functionStart + functionLength could overflow.
> >> 
> >> Agreed on overflow. It's provided by the binary here:
> >> https://github.com/WebAssembly/design/blob/master/BinaryEncoding.md#function-bodies
> >> 
> >> We setup start / end in the parser here:
> >> https://github.com/WebKit/webkit/blob/54a8f928520be7461e1a809203c7828ad817e2f8/Source/JavaScriptCore/wasm/WasmModuleParser.cpp#L206
> >> 
> >> We then fill out start / end here:
> >> https://github.com/WebKit/webkit/blob/54a8f928520be7461e1a809203c7828ad817e2f8/Source/JavaScriptCore/wasm/WasmModuleParser.cpp#L488
> >> 
> >> It would be great to get another set of eyes on the entire WasmModuleParser. I think it's pretty much done except for:
> >> https://bugs.webkit.org/show_bug.cgi?id=165833
> > 
> > I'll used Checked<ChrashOnOverflow> for now and we can later come up with a nicer way to error out here.
> 
> Actually, why would we need to do this? Wouldn't it be an early validation
> error to have any function with a bounds that exceeded the buffer? If we
> don't already have a check that does this, we should in the
> WasmModuleParser, not here.

Indeed, if we get an overflow here we're in trouble already. That's why the assert should check assuming there could be overflow, otherwise it's totally useless: it means we're trying to make sure the parser didn't mess up, but we're messing up here too!

[Saam Barati]

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