Bug 184689 – [WebAssembly][Modules] Implement function import from wasm modules

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Yusuke Suzuki]

[WebAssembly][Modules] Implement function import from wasm modules

[Yusuke Suzuki]

Created attachment 338105 [details]
Patch

[Yusuke Suzuki]

Created attachment 338106 [details]
Patch

[Yusuke Suzuki]

Created attachment 338107 [details]
Patch

[Yusuke Suzuki]

Created attachment 338108 [details]
Patch

[JF Bastien]

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.

[Yusuke Suzuki]

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.

[Yusuke Suzuki]

Committed r230720: <https://trac.webkit.org/changeset/230720>

[Radar WebKit Bug Importer]

<rdar://problem/39496166>

[WebKit Commit Bot]

Re-opened since this is blocked by bug 184717