[WebAssembly][Modules] Implement function import from wasm modules
Created attachment 338105 [details] Patch
Created attachment 338106 [details] Patch
Created attachment 338107 [details] Patch
Created attachment 338108 [details] Patch
Comment on attachment 338108 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=338108&action=review Wooh! Very cool :) r=me > Source/JavaScriptCore/parser/NodesAnalyzeModule.cpp:58 > + specifier->importedName() == analyzer.vm().propertyNames->timesIdentifier, It's nicer to have an enum class here, like: specifier->importedName() == analyzer.vm().propertyNames->timesIdentifier ? IMPORT_ENTIRE_NAMESPACE : IMPORT_SINGLE_IDENTIFIER > Source/JavaScriptCore/wasm/js/JSWebAssemblyInstance.cpp:203 > + false, Ditto here for enum class. > Source/JavaScriptCore/wasm/js/WebAssemblyModuleRecord.cpp:158 > + throwSyntaxError(exec, scope, makeString("Importing binding name 'default' cannot be resolved by star export entries.")); This message confused me. How can it happen? > Source/JavaScriptCore/wasm/js/WebAssemblyModuleRecord.cpp:184 > + value = jsUndefined(); Isn't it a TDZ in this case? IIUC that's where you have a circular import, and you're going to try using it in evaluate() ? That should fail wasm loading, just like TDZ does for JS. > Source/JavaScriptCore/wasm/js/WebAssemblyModuleRecord.cpp:230 > + } Drop extra braces here and below. > Tools/Scripts/run-jsc-stress-tests:1017 > +def runWebAssemblyDirect Maybe it's better to rename runWebAssembly above? The one you're adding is really the "run the simple thing", whereas above is "run with all these extra imports". This could be a follow-up cleanup.
Comment on attachment 338108 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=338108&action=review Thanks! >> Source/JavaScriptCore/parser/NodesAnalyzeModule.cpp:58 >> + specifier->importedName() == analyzer.vm().propertyNames->timesIdentifier, > > It's nicer to have an enum class here, like: > specifier->importedName() == analyzer.vm().propertyNames->timesIdentifier ? IMPORT_ENTIRE_NAMESPACE : IMPORT_SINGLE_IDENTIFIER Cool. Fixed. >> Source/JavaScriptCore/wasm/js/JSWebAssemblyInstance.cpp:203 >> + false, > > Ditto here for enum class. Fixed. >> Source/JavaScriptCore/wasm/js/WebAssemblyModuleRecord.cpp:158 >> + throwSyntaxError(exec, scope, makeString("Importing binding name 'default' cannot be resolved by star export entries.")); > > This message confused me. How can it happen? It is a bit complicated thing :) If "default" query hits with export *, and default export, this error happens. I'll add a test for this >> Source/JavaScriptCore/wasm/js/WebAssemblyModuleRecord.cpp:184 >> + value = jsUndefined(); > > Isn't it a TDZ in this case? IIUC that's where you have a circular import, and you're going to try using it in evaluate() ? That should fail wasm loading, just like TDZ does for JS. If it is TDZ, it's empty. And the following `if (!value.isFunction())` should fail. But right now, we cannot `isFunction()` to JSEmpty. (We do not perform such predicate for JSEmpty). So now, we just use undefined here. And it soon raises an error in the following switch statement. This meets the description in the proposal. https://github.com/WebAssembly/esm-integration/tree/master/proposals/esm-integration#instantiation >> Source/JavaScriptCore/wasm/js/WebAssemblyModuleRecord.cpp:230 >> + } > > Drop extra braces here and below. OK fixed. >> Tools/Scripts/run-jsc-stress-tests:1017 >> +def runWebAssemblyDirect > > Maybe it's better to rename runWebAssembly above? The one you're adding is really the "run the simple thing", whereas above is "run with all these extra imports". This could be a follow-up cleanup. OK, I'll do it in a follow-up patch.
Committed r230720: <https://trac.webkit.org/changeset/230720>
<rdar://problem/39496166>
Re-opened since this is blocked by bug 184717