Bug 163404

Created attachment 291545 [details] patch Basic WebAssembly global object, its constructor / function properties (all useless for now), test, JSC option (defaults to false).

Comment on attachment 291545 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=291545&action=review Looks good but you have a couple issues. I think you need to add the getCallData/getConstructData to your Constructors. > Source/JavaScriptCore/runtime/CommonIdentifiers.h:39 > + macro(CompileError) \ IIRC you don't need to have the names in both and BuiltinNames.h. If so, I think we should try to reduce the number of common identifiers we have as they get included everywhere. > Source/JavaScriptCore/wasm/WebAssemblyObject.cpp:154 > + m_ ## NAME ## Structure.set(vm, this, NAME ## Structure); \ > + } \ > + \ > + private: \ > + WebAssembly ## NAME ## Constructor(VM& vm, Structure* structure) \ > + : InternalFunction(vm, structure) \ > + { \ > + } \ > + \ > + WriteBarrier<Structure> m_ ## NAME ## Structure; \ I don't think this will work because your structure will get GCed. You need to add a visitChildren method to ensure that the GC knows about these structures. In the future, I think you should just put the structure on the global object.

Created attachment 291688 [details] patch Address review comments: - Implement getCallData/getConstructData/visitChildren, and add related tests (call function properties and assert that they throw, invoke constructor properties and assert that they also throw). - Don't use common identifiers, only use builtin names. Also, merge with the change which just landed in https://bugs.webkit.org/show_bug.cgi?id=163267 I think this is now good to go!

(In reply to comment #3) > Created attachment 291688 [details] > patch > > Address review comments: > - Implement getCallData/getConstructData/visitChildren, and add related > tests (call function properties and assert that they throw, invoke > constructor properties and assert that they also throw). > - Don't use common identifiers, only use builtin names. > > Also, merge with the change which just landed in > https://bugs.webkit.org/show_bug.cgi?id=163267 > > I think this is now good to go! Your patch looks a little light :P. I think you forgot to include some changes or something.

(In reply to comment #4) > (In reply to comment #3) > > Created attachment 291688 [details] > > patch > > > > Address review comments: > > - Implement getCallData/getConstructData/visitChildren, and add related > > tests (call function properties and assert that they throw, invoke > > constructor properties and assert that they also throw). > > - Don't use common identifiers, only use builtin names. > > > > Also, merge with the change which just landed in > > https://bugs.webkit.org/show_bug.cgi?id=163267 > > > > I think this is now good to go! > > Your patch looks a little light :P. I think you forgot to include some > changes or something. No that's all of it.

Created attachment 291705 [details] patch MSVC bot was sad at zero-sized static array. make it happy by having at least one value in arrays (and initialize one of them which ins't default-initializable because the union is being silly). They're not read so their content doesn't matter (and they'll eventually be auto-generated when I implement them).

Created attachment 291720 [details] patch Fixing MSVC bot made GTK bot unhappy because it's being silly about initialization of POD static globals (they should default init, GTK bot thinks a warning is cool). Whack that mole.

Comment on attachment 291720 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=291720&action=review r=me with some comments. > Source/JavaScriptCore/wasm/WebAssemblyObject.cpp:53 > +// FIXME https://bugs.webkit.org/show_bug.cgi?id=161709 Implement each of these in their own header file, and autogenerate *.lut.h files for *PrototypeTable. nit: I think we usually put the url after the comment. > Source/JavaScriptCore/wasm/WebAssemblyObject.cpp:55 > + class WebAssembly ## NAME : public JSDestructibleObject { \ Normally we would call this JSWebAssembly ## NAME. The typical convention is JS ## NAME for instance objects and NAME ## Protototype / NAME ## Constructor for prototypes / constructors, respectively. > Source/JavaScriptCore/wasm/WebAssemblyObject.cpp:83 > + const ClassInfo WebAssembly ## NAME::s_info = { "Object", &Base::s_info, 0, CREATE_METHOD_TABLE(WebAssembly ## NAME) }; \ "Object" should be something to identify this object to a JSC user. Perhaps "WebAssembly ## NAME". That string is only used by the inspector to produce nicely formatted names for each object it sees. > Source/JavaScriptCore/wasm/WebAssemblyObject.cpp:85 > + class WebAssembly ## NAME ## Prototype : public WebAssembly ## NAME { \ Why does this subclass the instance object? Modern JS has prototypes act as normal objects with a few exceptions for Array.prototype, etc. > Source/JavaScriptCore/wasm/WebAssemblyObject.cpp:118 > + const ClassInfo WebAssembly ## NAME ## Prototype::s_info = { "Object", &WebAssembly ## NAME::s_info, &NAME ## PrototypeTable, CREATE_METHOD_TABLE(WebAssembly ## NAME ## Prototype) }; \ Ditto. > Source/JavaScriptCore/wasm/WebAssemblyObject.cpp:159 > + putDirectWithoutTransition(vm, vm.propertyNames->length, jsNumber(0), ReadOnly | DontEnum | DontDelete); \ Should this be functionLength and not 0?

Created attachment 291831 [details] patch (In reply to comment #8) > Comment on attachment 291720 [details] > patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=291720&action=review > > r=me with some comments. > > > Source/JavaScriptCore/wasm/WebAssemblyObject.cpp:53 > > +// FIXME https://bugs.webkit.org/show_bug.cgi?id=161709 Implement each of these in their own header file, and autogenerate *.lut.h files for *PrototypeTable. > > nit: I think we usually put the url after the comment. Done. > > Source/JavaScriptCore/wasm/WebAssemblyObject.cpp:55 > > + class WebAssembly ## NAME : public JSDestructibleObject { \ > > Normally we would call this JSWebAssembly ## NAME. > > The typical convention is JS ## NAME for instance objects and NAME ## > Protototype / NAME ## Constructor for prototypes / constructors, > respectively. I followed what IntlObject and IntlCollator do. Which is "right"? > > Source/JavaScriptCore/wasm/WebAssemblyObject.cpp:83 > > + const ClassInfo WebAssembly ## NAME::s_info = { "Object", &Base::s_info, 0, CREATE_METHOD_TABLE(WebAssembly ## NAME) }; \ > > "Object" should be something to identify this object to a JSC user. Perhaps > "WebAssembly ## NAME". That string is only used by the inspector to produce > nicely formatted names for each object it sees. Same here, IntlCollator (and all other Intl constructors) use "Object". Are they wrong? I can fix them as a separate patch. > > Source/JavaScriptCore/wasm/WebAssemblyObject.cpp:85 > > + class WebAssembly ## NAME ## Prototype : public WebAssembly ## NAME { \ > > Why does this subclass the instance object? Modern JS has prototypes act as > normal objects with a few exceptions for Array.prototype, etc. Same here: that's what IntlCollatorPrototype does. I don't know what's right, let me know which to go for and I can separately fix Intl* if they're also wrong. > > Source/JavaScriptCore/wasm/WebAssemblyObject.cpp:118 > > + const ClassInfo WebAssembly ## NAME ## Prototype::s_info = { "Object", &WebAssembly ## NAME::s_info, &NAME ## PrototypeTable, CREATE_METHOD_TABLE(WebAssembly ## NAME ## Prototype) }; \ > > Ditto. Ditto². > > Source/JavaScriptCore/wasm/WebAssemblyObject.cpp:159 > > + putDirectWithoutTransition(vm, vm.propertyNames->length, jsNumber(0), ReadOnly | DontEnum | DontDelete); \ > > Should this be functionLength and not 0? Yeah I think you're right.

Created attachment 291836 [details] patch We discussed this offline: Intl is wrong, so I updated WebAssembly as suggested.

Comment on attachment 291836 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=291836&action=review > Source/JavaScriptCore/wasm/WebAssemblyObject.cpp:55 > + class JSWebAssembly ## NAME : public JSDestructibleObject { \ Is the plan to add fields to these objects that will require destruction? If not, this can just inherit from JSNonFinalObject.

(In reply to comment #11) > Comment on attachment 291836 [details] > patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=291836&action=review > > > Source/JavaScriptCore/wasm/WebAssemblyObject.cpp:55 > > + class JSWebAssembly ## NAME : public JSDestructibleObject { \ > > Is the plan to add fields to these objects that will require destruction? If > not, this can just inherit from JSNonFinalObject. Im not 100% sure I understand the semantics, but I believe so: WebAssembly's Module / Instance / Memory / Table would require destructor action I think. The two error types won't, and should do as you suggest.

Comment on attachment 291836 [details] patch Clearing flags on attachment: 291836 Committed r207432: <http://trac.webkit.org/changeset/207432>

All reviewed patches have been landed. Closing bug.