Bug 164133

Created attachment 296655 [details] WIP

Created attachment 297048 [details] Patch

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".

Comment on attachment 297048 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=297048&action=review r=me with comments. > Source/JavaScriptCore/wasm/WasmFormat.h:113 > + Mutable = 1, > + Immutable = 0 Probably worth a comment saying this is to match the binary format. > Source/JavaScriptCore/wasm/WasmModuleParser.cpp:269 > + if (!parseGlobalType(global)) > + return false; Shouldn't this return false if the global is Mutable? I think you also need a test for this. > Source/JavaScriptCore/wasm/WasmModuleParser.cpp:448 > + switch (initializerOpcode) { Should this be a helper since its parsing init_expr (or whatever it's called) and it'll be used from elsewhere? > Source/JavaScriptCore/wasm/WasmValidate.cpp:197 > + m_errorMessage = ASCIILiteral("Attempt to use unknown global."); nit: I think this should be "Attempted" Also, this needs a test. > Source/JavaScriptCore/wasm/WasmValidate.cpp:204 > + m_errorMessage = ASCIILiteral("Attempt to use unknown global."); Ditto style and also needs a test. > Source/JavaScriptCore/wasm/WasmValidate.cpp:209 > + m_errorMessage = ASCIILiteral("Attempt to store to immutable global."); Ditto style and also needs a test. > Source/JavaScriptCore/wasm/WasmValidate.cpp:218 > + m_errorMessage = makeString("Attempt to set global with type: ", toString(globalType), " with a variable of type: ", toString(value)); You should add a test for this. > Source/JavaScriptCore/wasm/WasmValidate.cpp:445 > + Validate context(signature->returnType, info.globals, info.memory); Style: Maybe just pass in "info" instead since we're now using two fields on it? > Source/JavaScriptCore/wasm/js/JSWebAssemblyInstance.cpp:43 > + // FIXME: These objects çould be pretty big we should try to throw OOM here. Weird unicode here? > Source/JavaScriptCore/wasm/js/JSWebAssemblyInstance.cpp:45 > + instance->m_globals = reinterpret_cast<GlobalDataDescriptor*>(fastMalloc(info.globals.size() * sizeof(Register))); Style: I'd make this a MallocPtr. > Source/JavaScriptCore/wasm/js/JSWebAssemblyInstance.cpp:73 > + fastFree(m_globals); And then you can get rid of implementing the destructor. > Source/JavaScriptCore/wasm/js/JSWebAssemblyInstance.cpp:91 > + visitor.addOpaqueRoot(thisObject->m_globals); Why? This looks wrong. > Source/JavaScriptCore/wasm/js/JSWebAssemblyInstance.h:145 > + Please remove this newline. > Source/JavaScriptCore/wasm/js/WebAssemblyInstanceConstructor.cpp:200 > + if (!value.isNumber()) You should add a test for this. > Source/JavaScriptCore/wasm/js/WebAssemblyInstanceConstructor.cpp:215 > + switch (moduleInformation.globals[import.kindIndex].type) { > + case Wasm::I32: > + instance->setGlobal(numImportGlobals++, value.toInt32(exec)); > + break; > + case Wasm::F32: > + instance->setGlobal(numImportGlobals++, bitwise_cast<uint32_t>(value.toFloat(exec))); > + break; > + case Wasm::F64: > + instance->setGlobal(numImportGlobals++, bitwise_cast<uint64_t>(value.asNumber())); > + break; > + default: > + RELEASE_ASSERT_NOT_REACHED(); > + } You need to add an ASSERT(!throwScope.exception()) after this switch > Source/JavaScriptCore/wasm/js/WebAssemblyInstanceConstructor.cpp:265 > + instance->setGlobal(globalIndex, instance->loadI64Global(global.initialBitsOrImportNumber)); Why i64? Just to get the bits? > JSTests/wasm/Builder.js:526 > + throw new Error(`Start sections function index must either be a number or a string`); Why?

Comment on attachment 297048 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=297048&action=review You're missing one of the json files. >> Source/JavaScriptCore/wasm/WasmFormat.h:113 >> + Immutable = 0 > > Probably worth a comment saying this is to match the binary format. Or... wait for it... AUTOGEN!!! 🎉 > Source/JavaScriptCore/wasm/js/JSWebAssemblyInstance.h:102 > + m_globals[i].i64 = bits; You should use memcpy to avoid aliasing problems (memcpy makes all union members active). > JSTests/wasm/Builder_WebAssemblyBinary.js:63 > + throw new Error(`mutability should be either "mutable" or "immutable", but got ${global.mutablity}`); This should be done in the Builder, and an invalid integer should be allowed in unchecked mode. At this point you should only have an integer.

Comment on attachment 297048 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=297048&action=review >>> Source/JavaScriptCore/wasm/WasmFormat.h:113 >>> + Immutable = 0 >> >> Probably worth a comment saying this is to match the binary format. > > Or... wait for it... AUTOGEN!!! 🎉 Added a FIXME pointing to a bug. >> Source/JavaScriptCore/wasm/WasmModuleParser.cpp:269 >> + return false; > > Shouldn't this return false if the global is Mutable? I think you also need a test for this. Yes. Test added. >> Source/JavaScriptCore/wasm/WasmValidate.cpp:197 >> + m_errorMessage = ASCIILiteral("Attempt to use unknown global."); > > nit: I think this should be "Attempted" > Also, this needs a test. We use some combination of "Attempting" and "Attempt" everywhere else. If we want to make the wording more consistent we should do so in a follow up patch. Added test. >> Source/JavaScriptCore/wasm/WasmValidate.cpp:204 >> + m_errorMessage = ASCIILiteral("Attempt to use unknown global."); > > Ditto style and also needs a test. ditto. >> Source/JavaScriptCore/wasm/WasmValidate.cpp:209 >> + m_errorMessage = ASCIILiteral("Attempt to store to immutable global."); > > Ditto style and also needs a test. ditto. >> Source/JavaScriptCore/wasm/WasmValidate.cpp:445 >> + Validate context(signature->returnType, info.globals, info.memory); > > Style: Maybe just pass in "info" instead since we're now using two fields on it? done. I also renamed info to module since that's a better name anyway. >> Source/JavaScriptCore/wasm/js/JSWebAssemblyInstance.cpp:45 >> + instance->m_globals = reinterpret_cast<GlobalDataDescriptor*>(fastMalloc(info.globals.size() * sizeof(Register))); > > Style: I'd make this a MallocPtr. Changed. >> Source/JavaScriptCore/wasm/js/JSWebAssemblyInstance.cpp:91 >> + visitor.addOpaqueRoot(thisObject->m_globals); > > Why? This looks wrong. This was supposed to be reportExtraMemoryVisited. >> Source/JavaScriptCore/wasm/js/WebAssemblyInstanceConstructor.cpp:265 >> + instance->setGlobal(globalIndex, instance->loadI64Global(global.initialBitsOrImportNumber)); > > Why i64? Just to get the bits? Yeah.

Committed r209830: <http://trac.webkit.org/changeset/209830>