1247846 - Baldr: add indirect function table and call_indirect

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Luke Wagner [:luke]]

From:
  https://github.com/WebAssembly/design/blob/master/AstSemantics.md#calls
WebAssembly has a single heterogeneous table instead of N homogeneous tables (arising from the constraints of dynamic linking).  The initial impl will generate N homogeneous tables (so, for a given index, one table has the real function and all the others have throw stubs).  This could take a lot of memory in the worst case (many signatures, many elements) so later we may want to add heterogeneous tables as an option (or a hybrid scheme that uses homogeneous tables up to a certain memory limit and heterogeneous for the rest).

Comment 1

[Luke Wagner [:luke]]

Attachment: change-asmjs-func-ptr

This patch changes the binary format to match wasm (almost, except for using U32 instead of VarU32 for sig index) and the asm.js generator.

Comment 2

[Luke Wagner [:luke]]

Attachment: change-asmjs-func-ptr

Rebased

Comment 3

[Luke Wagner [:luke]]

Attachment: refactor-finish

Simple codemotion patch to to create StaticLinkData only at the end and make finish() shorter.

Comment 4

[Luke Wagner [:luke]]

Attachment: refactor-error-stubs

Share more code, including 1 more in the next patches.

Comment 5

[Luke Wagner [:luke]]

Attachment: add-type-decl

At type declarations to s-expr language.

Comment 6

[Luke Wagner [:luke]]

Attachment: func-ptr-table

Adds table sections and call_indirect with tests.

Comment 7

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8719079 [details] [diff] [review]
change-asmjs-func-ptr

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

::: js/src/asmjs/AsmJS.cpp
@@ +2047,5 @@
>      {
> +        if (mask > MaxTableElems)
> +            return failCurrentOffset("function pointer table too big");
> +        uint32_t sigIndex;
> +        if (!newSig(Move(sig), &sigIndex))

Why creating a new signature all the time here? Why can't reuse the signature, if it were already existing? I don't see anything in ModuleGenerator preventing this, but maybe I'm overlooking something...

::: js/src/asmjs/WasmGenerator.cpp
@@ +895,5 @@
>  
> +    // Convert the function pointer table elements from function-indices to code
> +    // offsets that static linking will convert to absolute addresses.
> +    for (uint32_t sigIndex = 0; sigIndex < numSigs_; sigIndex++) {
> +        const TableModuleGeneratorData& table = shared_->sigToTable[sigIndex];

Could we directly continue if table.elemFuncIndices.length() == 0?

@@ +903,5 @@
> +            return false;
> +
> +        for (size_t i = 0; i < table.elemFuncIndices.length(); i++) {
> +            uint32_t funcIndex = table.elemFuncIndices[i];
> +            MOZ_ASSERT(funcIsDefined(funcIndex));

nit: funcEntry called just below already asserts this.

::: js/src/asmjs/WasmIonCompile.cpp
@@ +1602,5 @@
>      uint32_t sigIndex = f.readU32();
>  
> +    MDefinition* index;
> +    if (!EmitExpr(f, ExprType::I32, &index))
> +        return false;

I preferred to have this below the MOZ_ASSERT_IF, so that the sigIndex definition and uses were closer and all the Emit* calls were closer to each other, but it's really personal taste.

Comment 8

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8719081 [details] [diff] [review]
refactor-error-stubs

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

Nice!

Comment 9

[Luke Wagner [:luke]]

(In reply to Benjamin Bouvier [:bbouvier] from comment #7)
> Why creating a new signature all the time here? Why can't reuse the
> signature, if it were already existing? I don't see anything in
> ModuleGenerator preventing this, but maybe I'm overlooking something...

The problem is that, in asm.js, there can be multiple tables for a single signature, so declareSig would cause a collision.  (Technically we could reuse up to one existing signature, but that's more complexity and probably not worth it.)

> Could we directly continue if table.elemFuncIndices.length() == 0?

Yes, and that even saves a wasted element being added to static link data, so a good idea.

Comment 10

[Dan Gohman [:sunfish]]

Comment on attachment 8719082 [details] [diff] [review]
add-type-decl

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

::: js/src/asmjs/WasmText.cpp
@@ +72,5 @@
>      WasmAstValTypeVector args_;
>      ExprType ret_;
>  
>    public:
> +    WasmAstSig(LifoAlloc& lifo)

This should be `explicit`.

Comment 11

[Dan Gohman [:sunfish]]

Comment on attachment 8719083 [details] [diff] [review]
func-ptr-table

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

::: js/src/asmjs/WasmGenerator.h
@@ +98,5 @@
>      DeclaredSigPtrVector            funcSigs;
>      ImportModuleGeneratorDataVector imports;
>      AsmJSGlobalVariableVector       globals;
> +
> +    uint32_t funcSigIndex(uint32_t funcIndex) {

This member function can be `const`.

@@ +102,5 @@
> +    uint32_t funcSigIndex(uint32_t funcIndex) {
> +        return funcSigs[funcIndex] - sigs.begin();
> +    }
> +
> +    ModuleGeneratorData(ModuleKind kind = ModuleKind::Wasm) : kind(kind), numTableElems(0) {}

This constructor should be `explicit`.

::: js/src/asmjs/WasmIonCompile.cpp
@@ +769,5 @@
> +            curBlock_->add(maskedIndex);
> +            ptrFun = MAsmJSLoadFuncPtr::New(alloc(), maskedIndex, globalDataOffset);
> +            curBlock_->add(ptrFun);
> +        } else {
> +            MOZ_ASSERT(!length || length == mg_.numTableElems());

This line is the main thing I'm confused about. Why is length allowed to be zero here? That seems to be the thing which ends up requiring the special alwaysThrow logic in several places.

::: js/src/asmjs/WasmModule.cpp
@@ +53,5 @@
>  using mozilla::PodZero;
>  using mozilla::Swap;
>  using JS::GenericNaN;
>  
> +/* static */ const uint32_t ExportMap::MemoryExport;

This comment is to emphasize that MemoryExport is a static data member? I initially read it as though it were suggesting MemoryExport has internal name linkage (one of the many meanings of `static`) which is being disabled by being commented out. One can't declare non-static data members with this syntax, so I think the code is clear without the comment.

::: js/src/asmjs/WasmText.cpp
@@ +395,5 @@
> +{
> +    WasmAstTableElemVector elems_;
> +
> +  public:
> +    WasmAstTable(WasmAstTableElemVector&& elems) : elems_(Move(elems)) {}

`explicit`

@@ +2402,5 @@
>      WasmAstValTypeVector vars(c.lifo);
>      WasmAstValTypeVector args(c.lifo);
>      ExprType result = ExprType::Void;
>  
> +    uint32_t sigIndex = UINT32_MAX;

It'd be nice to give this UINT32_MAX usage a name, similar to BadIndirectCall.

Comment 12

[Luke Wagner [:luke]]

(In reply to Dan Gohman [:sunfish] from comment #11)
> This line is the main thing I'm confused about. Why is length allowed to be
> zero here? That seems to be the thing which ends up requiring the special
> alwaysThrow logic in several places.

Yes, that's exactly right: we don't want to allocate a sizeof(indirect table) array for *every* signature, only the ones that have at least 1 indirectly-callable function.  It's a hack, but I think it'll save significant memory.  Perhaps what you're saying is that some comments are in order :) ?

Agreed on other comments.

Comment 13

[Luke Wagner [:luke]]

Attachment: func-ptr-table

Rebased and with comment.

Comment 14

[Dan Gohman [:sunfish]]

Comment on attachment 8719945 [details] [diff] [review]
func-ptr-table

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

The comment makes sense. Thanks!

Comment 15

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c79666816060
https://hg.mozilla.org/integration/mozilla-inbound/rev/94cd9c9cf218
https://hg.mozilla.org/integration/mozilla-inbound/rev/31ba23efb586
https://hg.mozilla.org/integration/mozilla-inbound/rev/68c3d87a7cc9
https://hg.mozilla.org/integration/mozilla-inbound/rev/9fc23b671400

Comment 16

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/c79666816060
https://hg.mozilla.org/mozilla-central/rev/94cd9c9cf218
https://hg.mozilla.org/mozilla-central/rev/31ba23efb586
https://hg.mozilla.org/mozilla-central/rev/68c3d87a7cc9
https://hg.mozilla.org/mozilla-central/rev/9fc23b671400