Closed Bug 1243252 Opened 6 years ago Closed 6 years ago

Baldr: import section

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla47
Tracking Status
firefox47 --- fixed

People

(Reporter: luke, Assigned: luke)

References

Details

Attachments

(3 files)

This bug adds import declarations:
  https://github.com/WebAssembly/design/blob/master/Modules.md
which pretty much mirror asm.js FFI functions.
Otherwise, can't pass TwoByteChars (in next patch).
Attachment #8712493 - Flags: review?(jdemooij)
Attached patch refactor-exportsSplinter Review
This patch just refactors:
 - to fold createJSExport into dynamicallyLink since, from an external interface pov, these are both the same operation
 - WasmAstExport to use TwoByteChars instead of manual pointer and length.
Attachment #8712494 - Flags: review?(bbouvier)
Attached patch add-importsSplinter Review
Again, fairly straightforward.  Of course, they can't be *called* yet with this patch, but there's still enough to write some basic tests.  call/call_import will be in a separate patch.
Attachment #8712495 - Flags: review?(bbouvier)
Attachment #8712493 - Flags: review?(jdemooij) → review+
Blocks: 1243633
Comment on attachment 8712494 [details] [diff] [review]
refactor-exports

Review of attachment 8712494 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/asmjs/WasmModule.h
@@ +538,3 @@
>  
> +    bool dynamicallyLink(JSContext* cx,
> +                         Handle<WasmModuleObject*> moduleObj, 

nit: trailing space
Attachment #8712494 - Flags: review?(bbouvier) → review+
It sounds a bit arbitrary to me to have only two levels of depth in the module object {module: {name: ''}}. I've read a few issues on the WebAssembly repository but couldn't find a reason why we need two levels. In my opinion, there should either be one level (and users can use dots or whatever they want to represent submodules, e.g. "module.math.fround.call") or as many as we want (and then, we could join the levels by dots and use the dotted string as a unique identifier, e.g. {module:{math:{fround:{call: ()=>{}}}}} becomes "module.math.fround.call"). Can you explain me the rationale behind this choice, please?
Comment on attachment 8712495 [details] [diff] [review]
add-imports

Review of attachment 8712495 [details] [diff] [review]:
-----------------------------------------------------------------

Nice! According to https://github.com/WebAssembly/design/issues/522, we'll be able to re-export imports at the moment. I think that this patch doesn't implement this, but it can be done later in a subsequent patch/bug.

::: js/src/asmjs/Wasm.cpp
@@ +672,5 @@
> +            if (!GetProperty(cx, obj, name.func.get(), &v))
> +                return false;
> +        }
> +
> +        if (!IsFunctionObject(v))

Could you use IsFunctionObject in AsmJS.cpp ValidateFFI too, please?

::: js/src/asmjs/WasmModule.h
@@ -181,4 @@
>          return pod.interpExitCodeOffset_;
>      }
>      uint32_t jitExitCodeOffset() const {
> -        MOZ_ASSERT(pod.jitExitCodeOffset_);

Can't we keep these two assertions?

::: js/src/asmjs/WasmText.cpp
@@ +810,5 @@
> +            }
> +            WasmToken valueType;
> +            if (!c.ts.match(WasmToken::ValueType, &valueType, c.error))
> +                return nullptr;
> +            result = ToExprType(valueType.valueType());

These two cases are the exact same as the one in the switch in ParseFunc. Any chance this could be factored out?

::: js/src/jit-test/tests/wasm/binary.js
@@ +79,5 @@
>  }
>  
>  function moduleWithSections(sectionArray) {
>      var bytes = moduleHeaderThen();
>      for (section of sectionArray) {

preexisting nit: let section of sectionArray (or var, but not a global)

@@ +127,5 @@
> +
> +function importSection(imports) {
> +    var body = [];
> +    body.push(...varU32(imports.length));
> +    for (imp of imports) {

nit: let imp of imports
Attachment #8712495 - Flags: review?(bbouvier) → review+
(In reply to Benjamin Bouvier [:bbouvier] from comment #6)
> Nice! According to https://github.com/WebAssembly/design/issues/522, we'll
> be able to re-export imports at the moment.

Only in v8 (not in spec/design) and I think that issue will fix it in v8 to match spec/design.  The current consensus is that imports and functions are in separate index spaces and so if we were to want to re-export imports that would be a separate kind of export statement (which would fit into the binary format I've added here: instead of a "func" export, there'd be some new "import" export).

> Can't we keep these two assertions?

One of my test cases has no function bodies so the exit stubs end up at offset 0 :)

> These two cases are the exact same as the one in the switch in ParseFunc.
> Any chance this could be factored out?

Yes, and it actually cascades and allows even more sharing (between local/param) in ParseFunc!
(In reply to Benjamin Bouvier [:bbouvier] from comment #5)
> It sounds a bit arbitrary to me to have only two levels of depth in the
> module object {module: {name: ''}}.

The reasoning here is that to integrate with es6 modules, we want exactly two levels, mirroring the
  import foo from "bar";
statements ("foo" is the "function", "bar" is the "module").  However, wasm always wants to be loadable outside es6 modules so we have to decide how to interpret these two strings when all you're given is some import object.  The "" special case mirrors the (export "" index) case where "" means "the default export of the module".
(In reply to Luke Wagner [:luke] from comment #8)
> (In reply to Benjamin Bouvier [:bbouvier] from comment #5)
> > It sounds a bit arbitrary to me to have only two levels of depth in the
> > module object {module: {name: ''}}.
> 
> The reasoning here is that to integrate with es6 modules, we want exactly
> two levels, mirroring the
>   import foo from "bar";
> statements ("foo" is the "function", "bar" is the "module").  However, wasm
> always wants to be loadable outside es6 modules so we have to decide how to
> interpret these two strings when all you're given is some import object. 
> The "" special case mirrors the (export "" index) case where "" means "the
> default export of the module".

I see, thank you.
You need to log in before you can comment on or make changes to this bug.