Summary: | WebAssembly: Implement the WebAssembly.compile and WebAssembly.validate | ||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Saam Barati <saam> | ||||||||||||||
Component: | JavaScriptCore | Assignee: | Saam Barati <saam> | ||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||
Severity: | Normal | CC: | benjamin, commit-queue, fpizlo, ggaren, gskachkov, jfbastien, keith_miller, mark.lam, msaboff, oliver, ticaiolima, ysuzuki | ||||||||||||||
Priority: | P2 | ||||||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||
OS: | Unspecified | ||||||||||||||||
Attachments: |
|
Description
Saam Barati
2016-12-15 17:18:16 PST
Created attachment 297280 [details]
WIP
This has a different patch embedded in it ATM
This will allow us to run the unity headless benchmark. Created attachment 297384 [details]
patch
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. 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.
Created attachment 297386 [details]
patch
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.
Created attachment 297387 [details]
patch
Created attachment 297388 [details]
patch
(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. (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. 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. 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 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. > >> 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 Created attachment 297446 [details]
patch
Addressed JF's comments.
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? > > 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. 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. (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 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. 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. (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! landed in: https://trac.webkit.org/changeset/209979 |