Closed Bug 1284155 Opened 4 years ago Closed 3 years ago

Baldr: add WebAssembly.(Memory, Table)

Categories

(Core :: JavaScript Engine, defect, P1)

defect

Tracking

()

RESOLVED FIXED
mozilla51
Tracking Status
firefox50 --- wontfix
firefox51 --- fixed

People

(Reporter: luke, Assigned: luke)

References

Details

Attachments

(32 files, 13 obsolete files)

31.89 KB, patch
sunfish
: review+
Details | Diff | Splinter Review
37.08 KB, patch
bbouvier
: review+
Details | Diff | Splinter Review
6.90 KB, patch
bbouvier
: review+
Details | Diff | Splinter Review
30.92 KB, patch
bbouvier
: review+
Details | Diff | Splinter Review
61.45 KB, patch
bbouvier
: review+
Details | Diff | Splinter Review
56.20 KB, patch
bbouvier
: review+
Details | Diff | Splinter Review
18.97 KB, patch
bbouvier
: review+
Details | Diff | Splinter Review
19.35 KB, patch
bbouvier
: review+
Details | Diff | Splinter Review
28.43 KB, patch
bbouvier
: review+
Details | Diff | Splinter Review
3.31 KB, patch
bbouvier
: review+
Details | Diff | Splinter Review
33.30 KB, patch
bbouvier
: review+
Details | Diff | Splinter Review
55.75 KB, patch
bbouvier
: review+
Details | Diff | Splinter Review
19.04 KB, patch
bbouvier
: review+
Details | Diff | Splinter Review
52.27 KB, patch
bbouvier
: review+
Details | Diff | Splinter Review
40.96 KB, patch
bbouvier
: review+
Details | Diff | Splinter Review
2.75 KB, patch
bbouvier
: review+
Details | Diff | Splinter Review
17.84 KB, patch
bbouvier
: review+
Details | Diff | Splinter Review
6.59 KB, patch
bbouvier
: review+
Details | Diff | Splinter Review
18.23 KB, patch
bbouvier
: review+
Details | Diff | Splinter Review
14.79 KB, patch
bbouvier
: review+
terrence
: feedback+
Details | Diff | Splinter Review
37.84 KB, patch
bbouvier
: review+
Details | Diff | Splinter Review
43.38 KB, patch
bbouvier
: review+
Details | Diff | Splinter Review
17.16 KB, patch
bbouvier
: review+
Details | Diff | Splinter Review
36.56 KB, patch
bbouvier
: review+
Details | Diff | Splinter Review
14.96 KB, patch
bbouvier
: review+
Details | Diff | Splinter Review
5.70 KB, patch
terrence
: review+
Details | Diff | Splinter Review
26.89 KB, patch
bbouvier
: review+
Details | Diff | Splinter Review
6.67 KB, patch
bbouvier
: review+
Details | Diff | Splinter Review
19.98 KB, patch
bbouvier
: review+
Details | Diff | Splinter Review
4.16 KB, patch
terrence
: review+
Details | Diff | Splinter Review
15.94 KB, patch
bbouvier
: review+
Details | Diff | Splinter Review
11.71 KB, patch
terrence
: review+
Details | Diff | Splinter Review
These objects were recently merged into JS.md [1][2] and are the last major missing piece of the wasm JS API.  This requires breaking binary/JS API changes so, to avoid breaking the current demo, the plan is for Wasm.instantiateModule() to keep doing what is always has (it's a temporary API) but for WebAssembly.(Module,Instance) to use the new binary format and export Memory/Table.  This bug will do everything but resizing, which will be in a dependent bug.

[1] https://github.com/WebAssembly/design/blob/master/JS.md#webassemblymemory-objects
[2] https://github.com/WebAssembly/design/blob/master/JS.md#webassemblytable-objects
Blocks: 1284156
Attached patch change-import-export-format (obsolete) — Splinter Review
This first step is to change the binary format for imports/exports and have a way to land and test that without breaking current demo/Emscripten users.
Attachment #8767721 - Flags: review?(sunfish)
Tweaked to put the bool into Assumptions so it will later be available in Metadata.
Attachment #8767797 - Flags: review?(sunfish)
Attachment #8767721 - Attachment is obsolete: true
Attachment #8767721 - Flags: review?(sunfish)
Simple renaming patch.  Some ArrayBuffer-y things are left 'heap' and will be renamed in the next patch.
Attachment #8767798 - Flags: review?(bbouvier)
The next patch will need a prototype and it doesn't have a ctor handy so this patch switches to the normal scheme of allocating a JSProto_* entry and stashing in reserved slots of the global object.  The only quirk is that, since these constructors are namespaced, we can't use the normal lazy resolution mechanism, so that's why they are flagged as 'imaginary' and have 'dummy' init/class functions.
Attachment #8767799 - Flags: review?(bbouvier)
Attached patch memory-js-apiSplinter Review
This patch adds the initial WebAssembly.Memory class, and its 'buffer' getter.  WebAssembly.Memory is only created as the export when Module/Instance are used; Wasm.instantiateModule continues to return an ArrayBuffer.  Despite what I was thinking earlier, it's not so trivial to have Memory own the memory and have ArrayBuffer point into Memory, so I sticking with the current scheme of having the buffers own the memory and, when we implement resizing, we'll detach and steal it (like the old ArrayBuffer.transfer).
Attachment #8767801 - Flags: review?(bbouvier)
Comment on attachment 8767797 [details] [diff] [review]
change-import-export-format

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

Looks good!
Attachment #8767797 - Flags: review?(sunfish) → review+
Comment on attachment 8767798 [details] [diff] [review]
rename-heap-to-memory

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

Cool, thank you!

::: js/src/asmjs/WasmModule.cpp
@@ +334,5 @@
>  
>      // asm.js module instantiation supplies its own heap, but for wasm, create
>      // and initialize the heap if one is requested.
>  
>      Rooted<ArrayBufferObjectMaybeShared*> heap(cx, asmJSHeap);

Maybe this can be renamed too? (or it will be in a subsequent patch)

@@ +340,2 @@
>          MOZ_ASSERT(!metadata_->isAsmJS());
> +        heap = ArrayBufferObject::createForWasm(cx, metadata_->initialMemoryLength, 

pre-existing: trailing space (maybe already noted in another review)
Attachment #8767798 - Flags: review?(bbouvier) → review+
Comment on attachment 8767799 [details] [diff] [review]
change-prototypes

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

Not sure to understand all the consequences of this patch. Can you please detail a few things here:

- marking these two items as Imaginary just means that they won't have a ctor at the global namespace level, right? But the slots will be there in every single global, right?
- is this done just for symmetry with Memory happening in the next patch? or is there any other real benefit taken from doing this?
- it looks like the ctor's prototype is still set in LinkConstructorAndPrototype, meaning we'd now reference the prototype from two different locations: once in the constructor, once in the global. Is it ok to do so?
- is there any way to store the Memory prototype somewhere else entirely?
1. correct; we just want the reserved slots in the global.  Later, we might want to generalize the prototype mechanism so that it could deal with standard constructors that were on a namespace object ('WebAssembly') instead of the global object.
2. just putting it in the "normal" place for standard prototypes
3. yes, the standard does actually want a 'prototype' property, regardless, and this is symmetric to how other standard prototypes behave: the reserved slot on the global is basically a fast path that avoids property lookup and the need to have a ctor on hand
4. we certainly could, but this seems like the "right" place for standard prototypes; if the resolution mechanism was more general, we could use it for the lookup too instead of eagerly defining both ctors when 'WebAssembly' is initialized
Comment on attachment 8767801 [details] [diff] [review]
memory-js-api

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

Nice tests. Thanks for the patch.

::: js/src/asmjs/WasmJS.cpp
@@ +531,5 @@
> +    if (!ToInteger(cx, initialVal, &initialDbl))
> +        return false;
> +
> +    if (initialDbl < 0 || initialDbl > INT32_MAX / PageSize) {
> +        ReportOutOfMemory(cx);

Could this be a more precise error message? Report OOM here would make it look like an internal error of the engine rather than an unwanted parameter.

@@ +579,5 @@
> +
> +ArrayBufferObjectMaybeShared&
> +WasmMemoryObject::buffer() const
> +{
> +    return (ArrayBufferObjectMaybeShared&)getReservedSlot(BUFFER_SLOT).toObject();

No way to use .as<ArrayBufferObjectMaybeShared>() instead?

::: js/src/asmjs/WasmJS.h
@@ +137,5 @@
> +    static const Class class_;
> +    static const JSPropertySpec properties[];
> +
> +    static WasmMemoryObject* create(ExclusiveContext* cx,
> +                                    Handle<ArrayBufferObjectMaybeShared*> buffer,

can use the alias HandleArrayBufferObjectMaybeShared

::: js/src/asmjs/WasmModule.cpp
@@ +325,5 @@
>  
>  bool
>  Module::instantiate(JSContext* cx,
>                      Handle<FunctionVector> funcImports,
> +                    Handle<ArrayBufferObjectMaybeShared*> asmJSBuffer,

Can use the alias here too.

@@ +341,5 @@
> +        if (metadata_->isAsmJS()) {
> +            MOZ_ASSERT(asmJSBuffer);
> +            buffer = asmJSBuffer;
> +        } else {
> +            MOZ_ASSERT(!metadata_->isAsmJS());

That's tautologic since we're in the else branch of if (metadata_->isAsmJS()). You can probably MOZ_ASSERT(!asmJSBuffer) here, though. Or just replace these two by MOZ_ASSERT(metadata_->isAsmJS() == !!asmJSBuffer);

@@ +369,3 @@
>      // Create a new, specialized CodeSegment for the new Instance (for now).
>  
> +    uint8_t* memoryBase = buffer ? buffer->dataPointerEither().unwrap(/* code patching */) : 0;

nullptr would be more idiomatic than 0 here.

@@ +369,4 @@
>      // Create a new, specialized CodeSegment for the new Instance (for now).
>  
> +    uint8_t* memoryBase = buffer ? buffer->dataPointerEither().unwrap(/* code patching */) : 0;
> +    uint32_t memoryLength = buffer ? buffer->byteLength() : 0;

If these two declarations were hoisted above the if (metadata_->usesMemory()), then we could avoid these two ternaries following each other, and reuse memoryBase which is also set with the same value on the `if` branch.

::: js/src/jit-test/tests/wasm/jsapi.js
@@ +144,5 @@
> +assertEq(bufferDesc.set, undefined);
> +assertEq(bufferDesc.enumerable, false);
> +assertEq(bufferDesc.configurable, true);
> +
> +// 'WebAssembly.Memory.prototype.buffer' setter

nit: setter -> getter
Attachment #8767801 - Flags: review?(bbouvier) → review+
Comment on attachment 8767799 [details] [diff] [review]
change-prototypes

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

Thanks for the explanation, it now makes more sense to me. I think you could define the `resolve` hook on the WebAssembly object to lazily define the other prototypes, but that is probably premature optimization until somebody complains that all global objects have grown a few bytes :)

(fwiw, this happened with SIMD objects defining lots of new global slots, increasing the size of each global, and that triggered a talos alert on a test that creates a big amount of tabs. SIMD type ctors are also namespaced, so we use the resolve hook to lazily instantiate them, so that this doesn't have an effect on most users. Adding only a handful slots is probably fine.)
Attachment #8767799 - Flags: review?(bbouvier) → review+
(In reply to Benjamin Bouvier [:bbouvier] from comment #7)
> >      Rooted<ArrayBufferObjectMaybeShared*> heap(cx, asmJSHeap);
> 
> Maybe this can be renamed too? (or it will be in a subsequent patch)

indeed!

(In reply to Benjamin Bouvier [:bbouvier] from comment #11)
> Thanks for the explanation, it now makes more sense to me. I think you could
> define the `resolve` hook on the WebAssembly object to lazily define the
> other prototypes, but that is probably premature optimization until somebody
> complains that all global objects have grown a few bytes :)

agreed

(In reply to Benjamin Bouvier [:bbouvier] from comment #10)
Great points, thanks!

> can use the alias HandleArrayBufferObjectMaybeShared

I would've, but that requires #including ArrayBufferObject.h which I'd like to avoid in this pretty widespread .h file.

> > +                    Handle<ArrayBufferObjectMaybeShared*> asmJSBuffer,
> Can use the alias here too.

But here I can b/c in the .cpp we have everything.
Whiteboard: [leave open]
Pushed by lwagner@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7711cc8ad793
Baldr: add 'newFormat' binary encoding flag (r=sunfish)
https://hg.mozilla.org/integration/mozilla-inbound/rev/71449118bfe8
Baldr: rename 'heap' to 'memory' to better match wasm terminiology (r=bbouvier)
https://hg.mozilla.org/integration/mozilla-inbound/rev/5b783da72df1
Baldr: store prototypes in GlobalObject with JSProtoKey (r=bbouvier)
https://hg.mozilla.org/integration/mozilla-inbound/rev/0e5d81aae722
Baldr: add initial pieces of WebAssembly.Memory API (r=bbouvier)
Pure renaming patch:
 Import => FuncImport (b/c it only describes function imports)
 ImportName => Import (b/c it describes all imports)
 ImportModuleGeneratorData => FuncImportDesc (b/c it's only for func imports)
Attachment #8768617 - Flags: review?(bbouvier)
Attached patch memory-imports (obsolete) — Splinter Review
This patch generalizes imports to now accept memory.  As described in Modules.md#imports and Modules.md#linear-memory-section, memories optionally declare that they are defaults and there can only be 1 default (obvs) and, in the MVP, *every* import/definition must declare itself default, so thus we still only have 0 or 1 memories for a given module.  This patch is actually kinda nice b/c it makes wasm more like asm.js (which can "import" 0 or 1 memories).
Attachment #8768619 - Flags: review?(bbouvier)
Attached patch data-segments (obsolete) — Splinter Review
This patch mostly updates the text->binary to allow segments to be declared outside of a (memory) section (which allows a module to initialize imported memory).  This patch also updates the binary format to match Modules.md#data-section although it doesn't add support for const global variable initializers (that's for a separate patch that adds get_global/set_global).
Attachment #8768620 - Flags: review?(bbouvier)
Attached patch memory-importsSplinter Review
Updated to split Module::instantiateMemory so Module::instantiate doesn't get too big (with addition of table imports too).
Attachment #8768619 - Attachment is obsolete: true
Attachment #8768619 - Flags: review?(bbouvier)
Attachment #8769019 - Flags: review?(bbouvier)
Attached patch data-segmentsSplinter Review
Rebased
Attachment #8768620 - Attachment is obsolete: true
Attachment #8768620 - Flags: review?(bbouvier)
Attachment #8769034 - Flags: review?(bbouvier)
Working on the next few patches it was clear that it was awkward to split instantiation logic between Module and Instance so this patch is pure code motion, consolidating in WasmModule.cpp (and shortening the code).
Attachment #8769035 - Flags: review?(bbouvier)
The first step for Table is to move element initialization into its own proper ElemSegment, symmetric to DataSegment that (in a later patch) can be applied to an imported table.  Later patches will change DataSegment more significantly, this one mostly just refactors.
Attachment #8769036 - Flags: review?(bbouvier)
I'll get to the reviews today quickly, but a spec test showed an edge-case: we shouldn't be able to export memory when there is no memory in the module. With this and your previous patches, it seems wasm/spec/exports.wast.js would pass.
Comment on attachment 8768617 [details] [diff] [review]
rename-import-types

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

Very nice, thanks.

::: js/src/asmjs/WasmGenerator.h
@@ +48,5 @@
>  };
>  
>  typedef Vector<TableModuleGeneratorData, 0, SystemAllocPolicy> TableModuleGeneratorDataVector;
>  
> +struct FuncImportDesc

That is such a better name. If you want to rename TableModuleGeneratorData to FuncTableDesc too, go ahead (or maybe this happens in another patch already?).

Thinking about it, it was nice that it contained "Generator", since it helped reasoning about during which phase of the compilation, which was a bit useful (and helped disambiguate this struct against the others). Do you think it'd be worth while to add something in the name related to this? FuncImportGenDesc? FuncImportGeneratorDesc? FuncImportBuildDesc?

::: js/src/asmjs/WasmInstance.cpp
@@ +233,3 @@
>          return true;
>  
> +    FuncImportExit& exit = funcImportToExit(fi);

If you hoist this up, you could reuse it in fval's definition.

::: js/src/asmjs/WasmTypes.h
@@ +817,1 @@
>  // import. This is accessed directly from JIT code and mutated by Instance as

Should we precise "for a *function* import", here?
Attachment #8768617 - Flags: review?(bbouvier) → review+
(In reply to Benjamin Bouvier [:bbouvier] from comment #23)
Yes, FuncImportGenDesc/TableGenDesc sound perfect, thanks.
Comment on attachment 8769019 [details] [diff] [review]
memory-imports

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

A few questions below, but this overall looks great! A few more test cases and this should be good to land. Thanks.

::: js/src/asmjs/WasmBinaryToExperimentalText.cpp
@@ +1429,5 @@
>      return true;
>  }
>  
>  static bool
>  PrintImport(WasmPrintContext& c, AstImport& import, const AstModule::SigVector& sigs)

Same comment as in WasmBinaryToText.

::: js/src/asmjs/WasmBinaryToText.cpp
@@ +1111,5 @@
>      return true;
>  }
>  
>  static bool
>  RenderImport(WasmRenderContext& c, AstImport& import, const AstModule::SigVector& sigs)

This would need an update to handle memory imports too, right?

::: js/src/asmjs/WasmCompile.cpp
@@ +697,5 @@
> +
> +        maxBytes = maxPages;
> +        maxBytes *= PageSize;
> +        if (!maxBytes.isValid())
> +            return Fail(d, "maximum memory size too big");

Could we also check maxBytes >= initialBytes, please? (and have a test case for that)

@@ +705,5 @@
> +        return Fail(d, "already have default memory");
> +
> +    init->memoryUsage = MemoryUsage::Unshared;
> +    init->minMemoryLength = initialBytes.value();
> +    init->maxMemoryLength = maxBytes.value();

If maxBytes hasn't been set, this will be 0, correct? Won't this provoke a false positive in the test that the actual given memory's size is bigger than the maxMemoryLength, in instantiateMemory? If so, can you add a test case, please?

@@ +847,5 @@
>  
>          *exported = u8;
> +        MOZ_ASSERT(init->memoryUsage == MemoryUsage::None);
> +        init->memoryUsage = MemoryUsage::Unshared;
> +        init->minMemoryLength = initialSize.value();

Don't we need to set init->maxMemoryLength here too?

::: js/src/asmjs/WasmModule.cpp
@@ +338,5 @@
> +    RootedArrayBufferObjectMaybeShared buffer(cx);
> +    if (memory) {
> +        buffer = &memory->buffer();
> +        uint32_t length = buffer->byteLength();
> +        if (length < metadata_->minMemoryLength || length > metadata_->maxMemoryLength) {

Just an FYI: this is correct here, but Module.md is being a bit less conservative: WasmMemoryObject could have both a min and max memory values too, and the provided memory's initial size must be >= minMemoryLength, and its max size <= maxMemoryLength.

`The host environment must only allow imports of WebAssembly linear memories that have initial length greater-or-equal than the initial length declared in the import and that have maximum length less-or-equal than the maximum length declared in the import.`

I am realizing that the wording makes it unclear whether the imported linear memory must have the two fields or not; being more conservative as in the patch works here.

@@ +346,2 @@
>  
> +        // This can't happen except via the shell toggling signals.enabled.

Should we also check the memory hasn't been neutered? (what happens if a first module exports its memory to a second module, and someone neuters the memory (be it one of the modules or an outside observer)?)

(I seem to recall that neutered buffers have a length of 0, so the above length checks would fail, but better to double-check this)

::: js/src/asmjs/WasmTextToBinary.cpp
@@ +2990,3 @@
>          if (!r.registerImportName(imp->name(), i))
>              return r.fail("duplicate import");
> +        switch (imp->kind()) {

nit: can you add a new line above this one please?

::: js/src/jit-test/tests/wasm/import-export.js
@@ +44,1 @@
>  

As stated somewhere else, we need a few test case where we don't set maxSize.

::: js/src/jit-test/tests/wasm/signals-enabled.js
@@ +29,5 @@
> +disable();
> +assertErrorMessage(() => new Instance(m, {x:{y:mem}}), Error, /signals/);
> +var m = new Module(code);
> +enable();
> +assertEq(new Instance(m, {x:{y:mem}}) instanceof Instance, true);

instanceof Instance, now we're getting meta
Attachment #8769019 - Flags: review?(bbouvier) → review+
Comment on attachment 8769034 [details] [diff] [review]
data-segments

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

This looks good, but the BinaryToX transformations won't know about the new Segment sections. It'd be nice to have a follow-up patch/bug for all this work to be done, if you want to postpone it.
Attachment #8769034 - Flags: review?(bbouvier) → review+
Comment on attachment 8769035 [details] [diff] [review]
move-instantiation-code

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

Nice, thanks.

::: js/src/asmjs/WasmInstance.cpp
@@ +25,5 @@
>  #include "asmjs/WasmBinaryToExperimentalText.h"
>  #include "asmjs/WasmJS.h"
>  #include "asmjs/WasmModule.h"
>  #include "builtin/SIMD.h"
> +#include "jit/BaselineJIT.h"

Why are we dependent on BaselineJIT, here?
Attachment #8769035 - Flags: review?(bbouvier) → review+
Comment on attachment 8769036 [details] [diff] [review]
hoist-elem-segments

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

Looks good, thanks!

::: js/src/asmjs/WasmGenerator.cpp
@@ +916,5 @@
>  
> +    // Convert function indices to offsets into the code section.
> +    // WebAssembly's tables are (currently) all untyped and point to the table
> +    // entry. asm.js tables are all typed and thus point to the normal entry.
> +    bool tableEntry = metadata_->kind == ModuleKind::Wasm;

Can use !isAsmJS() on the RHS, or directly inline.

::: js/src/asmjs/WasmModule.cpp
@@ +381,5 @@
>  
>      return true;
>  }
>  
> +bool

Although I like the symmetry with other instantiate* methods, if this is not fallible, we might as well make it void instead.

@@ +382,5 @@
>      return true;
>  }
>  
> +bool
> +Module::instantiateTable(JSContext* cx, const CodeSegment& cs) const

cx is a dead arg here.

::: js/src/asmjs/WasmModule.h
@@ +134,5 @@
>  
>  typedef Vector<DataSegment, 0, SystemAllocPolicy> DataSegmentVector;
>  
> +// ElemSegment represents an element segment in the module where each element's
> +// function index has been translated to it's offset in the code section.

nit: it's => its
Attachment #8769036 - Flags: review?(bbouvier) → review+
(In reply to Benjamin Bouvier [:bbouvier] from comment #25)
Great points!

> This would need an update to handle memory imports too, right?

Right.  I was planning to do this when 'new-format' became the only format (that's when all the text test cases would otherwise start failing) so that way we don't have to maintain an additional 'newFormat' fork in these two paths as well.

> If maxBytes hasn't been set, this will be 0, correct? Won't this provoke a
> false positive in the test that the actual given memory's size is bigger
> than the maxMemoryLength, in instantiateMemory? If so, can you add a test
> case, please?

Correct and added!

> > +        init->minMemoryLength = initialSize.value();
> 
> Don't we need to set init->maxMemoryLength here too?

Oops; maxMemoryLength's initialization to UINT32_MAX was covering this up.

> Just an FYI: this is correct here, but Module.md is being a bit less
> conservative: WasmMemoryObject could have both a min and max memory values
> too, and the provided memory's initial size must be >= minMemoryLength, and
> its max size <= maxMemoryLength.

That's a good spot and it is intentional.  Right now WasmMemoryObject simply doesn't have a max so I'll need to add that in a separate patch (I was planning to do it as part of resizing).

> Should we also check the memory hasn't been neutered? (what happens if a
> first module exports its memory to a second module, and someone neuters the
> memory (be it one of the modules or an outside observer)?)

asm.js/wasm set isWasm() (via WASM_MALLOCED or WASM_MAPPED) on the ArrayBuffer which makes detachment (née neutering) throw.  (This is a cheat from an asm.js pov; we justify via "out of memory" :)  In the asm.js case, AsmJS.cpp has already called ArrayBuffer::prepareForAsmJS() which produces an isWasm() buffer (which we do assert here, below).

> instanceof Instance, now we're getting meta

goodness help us if wasm gets an 'instanceof' operator and JS bytecode reflection API leading to `instanceOf instanceof InstanceOf` :)
(In reply to Benjamin Bouvier [:bbouvier] from comment #27)
> Why are we dependent on BaselineJIT, here?

BaselineScript, for the Jit exits.
(In reply to Benjamin Bouvier [:bbouvier] from comment #28)
> Although I like the symmetry with other instantiate* methods, if this is not
> fallible, we might as well make it void instead.
...
> cx is a dead arg here.

In a few patches it'll be fallible and using 'cx' :)
Pushed by lwagner@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8abd128aac1e
Baldr: rename some types and fields to better reflect reality (r=bbouvier)
https://hg.mozilla.org/integration/mozilla-inbound/rev/03a7c6a84d6a
Baldr: allow modules to import WebAssembly.Memory (r=bbouvier)
https://hg.mozilla.org/integration/mozilla-inbound/rev/2d7c84eed728
Baldr: allow data segments outside any memory definition (r=bbouvier)
https://hg.mozilla.org/integration/mozilla-inbound/rev/2263561d21e3
Baldr: consolidate instantiation code in Module::instantiate (r=bbouvier)
https://hg.mozilla.org/integration/mozilla-inbound/rev/4a1ad717fe1d
Baldr: hoist ElemSegment out of static link data (r=bbouvier)
Pushed by lwagner@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/43438ba43aa1
add #include to silence 32-bit unified compiler warning (r=me)
Attached patch opt-armSplinter Review
Found this tiny ARM optimization, splitting it out from the next patch.
Attachment #8769397 - Flags: review?(bbouvier)
Attached patch allocate-tables (obsolete) — Splinter Review
This patch moves tables out of global memory and into separate allocations. This will allow tables to be resizes and also have their pointers stored in TLS.  This does introduce 1 extra indirection (although x64 and ARM maintain the same number of instructions), but I think that's ok:
 - func-pointer calls are rather expensive anyway; the overhead is amortized
 - later we can split out a separate MTableBasePointer MIR node that will be super-hoistable (only killed by calls)
Attachment #8769398 - Flags: review?(bbouvier)
Attached patch allocate-tables (obsolete) — Splinter Review
Oops, didn't mean to disable unified builds :)
Attachment #8769398 - Attachment is obsolete: true
Attachment #8769398 - Flags: review?(bbouvier)
Attachment #8769399 - Flags: review?(bbouvier)
Comment on attachment 8769397 [details] [diff] [review]
opt-arm

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

Nice! Fwiw, I think the same can be done on x64, and then LIR nodes (resp. lowering) will be the same on x86/x64/ARM and can be hoisted up in shared LIR (resp. Lowering.cpp).
Attachment #8769397 - Flags: review?(bbouvier) → review+
(In reply to Benjamin Bouvier [:bbouvier] from comment #38)
Good observation!  There was the slight userRegister()/useRegisterAtStart() difference between x86 and the rest, but that went away in the next patch so they do become completely identical.
Attached patch refactor-for-table (obsolete) — Splinter Review
This patch just does some simple refactoring and renaming so that the element section (added in the next patch) can easily reuse what's already in place for the data section (since both are highly symmetric).
Attachment #8769944 - Flags: review?(bbouvier)
qref'd
Attachment #8769944 - Attachment is obsolete: true
Attachment #8769944 - Flags: review?(bbouvier)
Attachment #8769945 - Flags: review?(bbouvier)
Attached patch elem-segmentSplinter Review
This patch adds an "elem" section to the binary format as well as 'elem' to the s-expr format.  Additionally, (table) sections can now, instead of defining all their elements, like memory, declare only and initial and maximum size via (table (resizable initial maximum)).  (The 'resizable' token is used to disambiguate new vs. old tables and may change later, but it allows testing for now.)

This patch also adds the ability for a table element to have the 'sentinel' value described in Modules.md#elements-section so a simple call stub is added which is filled in for uninitialized table elements.
Attachment #8769946 - Flags: review?(bbouvier)
Attached patch table-objectSplinter Review
This patch adds the basic WebAssembly.Table object; other patches will add the ability to export/get/set.
Attachment #8769948 - Flags: review?(bbouvier)
Comment on attachment 8769399 [details] [diff] [review]
allocate-tables

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

I think there's a file missing in this patch; if you could upload it (or an interdiff) in a separate patch, that'd be great, thanks.

::: js/src/asmjs/WasmGenerator.cpp
@@ +831,5 @@
>  
> +    TableDesc& table = shared_->tables[sigIndex];
> +    table.kind = TableKind::TypedFunction;
> +    table.length = length;
> +    return allocateGlobalBytes(sizeof(void*), sizeof(void*), &table.globalDataOffset);

This is already allocated in init(). Can you instead MOZ_ASSERT(table.globalDataOffset != 0)?

::: js/src/asmjs/WasmGenerator.h
@@ +74,5 @@
>      {}
> +
> +    bool isAsmJS() const {
> +        return kind == ModuleKind::AsmJS;
> +    }

Yes!

::: js/src/asmjs/WasmInstance.cpp
@@ +749,5 @@
>               codeSegment_->globalDataLength() +
> +             metadata_->sizeOfIncludingThisIfNotSeen(mallocSizeOf, seenMetadata);
> +
> +    for (const SharedTable& table : tables_)
> +         table->sizeOfIncludingThisIfNotSeen(mallocSizeOf, seenTables);

This returns a size_t which is unused. It should be accumulated and added to data, right?

::: js/src/asmjs/WasmIonCompile.cpp
@@ +891,5 @@
>              return true;
>          }
>  
>          MAsmJSCall::Callee callee;
> +        if (mg().isAsmJS()) {

(There might be more in the load/store functions, now)

::: js/src/asmjs/WasmTable.h
@@ +1,4 @@
> +/* -*- Mode: C++; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 4 -*-
> + * vim: set ts=8 sts=4 et sw=4 tw=99:
> + *
> + * Copyright 2015 Mozilla Foundation

nit: already 2016 (and no Marty McFly yet).

@@ +49,5 @@
> +WASM_DECLARE_POD_VECTOR(TableDesc, TableDescVector)
> +
> +// A Table is an indexable array of opaque values. Tables are first-class
> +// stateful objects exposed to WebAssembly. asm.js also uses Tables to represent
> +// the its homogeneous function-pointer tables.

nit: s/the//

@@ +51,5 @@
> +// A Table is an indexable array of opaque values. Tables are first-class
> +// stateful objects exposed to WebAssembly. asm.js also uses Tables to represent
> +// the its homogeneous function-pointer tables.
> +
> +class Table : public ShareableBase<Table>

It seems that Table and TableDesc are somehow redundant (they share the kind/length attributes which seem to be equal all the time)(except I don't know what Table::create does :)). It also seems that every owner of Tables (Module/Instance) has access to Metadata which owns TableDescs. So maybe kind/length could be shared, and Table could just be a wrapper for the void* array?

@@ +58,5 @@
> +    UniquePtr<void*> array_;
> +    uint32_t length_;
> +
> +  public:
> +    static RefPtr<Table> create(JSContext* cx, const TableDesc& desc);

Where is it defined? Is there a missing WasmTable.cpp in this patch?

@@ +60,5 @@
> +
> +  public:
> +    static RefPtr<Table> create(JSContext* cx, const TableDesc& desc);
> +
> +    TableKind kind() const { return kind_; }

This is unused in this patch.

::: js/src/jit/x64/CodeGenerator-x64.cpp
@@ +1259,5 @@
>          masm.branch32(Assembler::Condition::AboveOrEqual, index, Imm32(mir->limit()),
>                        wasm::JumpTarget::OutOfBounds);
>      }
>  
> +    CodeOffset label = masm.loadRipRelativeInt64(tmp);

pre-existing, but x64 has a ScratchReg it could use instead of the tmp. Or it could use `out` instead of `tmp`, as does x86. (and probably the temporary in LAsmJSLoadFuncPtr could be removed then!)
Comment on attachment 8769945 [details] [diff] [review]
refactor-for-table

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

Looks good, thanks!

::: js/src/asmjs/WasmAST.h
@@ +599,5 @@
> +    LifoAlloc&           lifo_;
> +    SigVector            sigs_;
> +    SigMap               sigMap_;
> +    ImportVector         imports_;
> +    AstTable*            table_;

Could use a Maybe<> too.
Attachment #8769945 - Flags: review?(bbouvier) → review+
(In reply to Benjamin Bouvier [:bbouvier] from comment #44)
> This is already allocated in init(). Can you instead
> MOZ_ASSERT(table.globalDataOffset != 0)?

Oops, this is the isAsmJS() path; the allocateGlobalBytes() in init() should only be happening for !isAsmJS(), so that's the bug.  Good spot!

> This returns a size_t which is unused. It should be accumulated and added to
> data, right?

Heh, right again, good spot.

> It seems that Table and TableDesc are somehow redundant (they share the
> kind/length attributes which seem to be equal all the time)(except I don't
> know what Table::create does :)). It also seems that every owner of Tables
> (Module/Instance) has access to Metadata which owns TableDescs. So maybe
> kind/length could be shared, and Table could just be a wrapper for the void*
> array?

Yeah, it bugs me a little too, but Module has no Table (they are created (later imported) only at instantiation-time and handed to Instances).  For this reason I initially put the TableDesc in Module (not Metadata), but Instance needs to know tableDesc.globalDataOffset.  If there were millions of these things we could save a few bytes by storing only that, but there's going to be 0 or 1 for a long time so I thought it simplest to put it in the Metadata :)

> Where is it defined? Is there a missing WasmTable.cpp in this patch?

Correctamundo, sorry!
Attached patch allocate-tablesSplinter Review
Now with more WasmTable.cpp!
Attachment #8769399 - Attachment is obsolete: true
Attachment #8769399 - Flags: review?(bbouvier)
Attachment #8770222 - Flags: review?(bbouvier)
Comment on attachment 8769946 [details] [diff] [review]
elem-segment

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

Looks great, cheers! The BadIndirectCall should be removed, see comment below.

::: js/src/asmjs/WasmAST.h
@@ +604,5 @@
>      LifoAlloc&           lifo_;
>      SigVector            sigs_;
>      SigMap               sigMap_;
>      ImportVector         imports_;
> +    Maybe<AstResizable>  table_;

Ha, please ignore my comment in the previous review.

::: js/src/asmjs/WasmBinaryIterator.h
@@ -106,5 @@
>        : base(base), offset(offset), align(align)
>      {}
>  };
>  
> -struct Nothing {};

Ha, reusing mozilla::Nothing instead, right?

::: js/src/asmjs/WasmCompile.cpp
@@ +1164,5 @@
> +        if (!d.readVarU32(&seg.offset))
> +            return Fail(d, "expected elem segment destination offset");
> +
> +        if (seg.offset < prevEnd)
> +            return Fail(d, "elem segments must be disjoint and ordered");

At the moment, there's only one table. But when there are more than 1, do the segments need to be ordered in a lexicographical order on (table, offset), too? If so, we could have the full check right now.

@@ +1174,5 @@
> +        uint32_t tableLength = mg.tables()[seg.tableIndex].length;
> +        if (seg.offset > tableLength || tableLength - seg.offset < numElems)
> +            return Fail(d, "element segment does not fit");
> +
> +        if (!seg.elems.resize(numElems))

In the worst case, numElems is MaxTableElems, and there can be only one table at the moment. I wonder if in the future (>1 table) we'd want a finer check, to avoid the worst case of MaxTableElems * MaxTable seg.elems mallocs...

::: js/src/asmjs/WasmStubs.cpp
@@ +827,5 @@
> +    GenerateExitPrologue(masm, 0, ExitReason::BadTableElem, &offsets);
> +
> +    masm.jump(JumpTarget::BadIndirectCall);
> +
> +    GenerateExitEpilogue(masm, 0, ExitReason::BadTableElem, &offsets);

The exit epilogue is unreachable! There is actually already an exit (generated by GenerateTrapStub called by GenerateJumpTarget) for the BadIndirectCall, so you can reuse the offset of this exit instead of generating a new one. And then you don't need the new ExitReason / CodeRange and this exit generator.

::: js/src/asmjs/WasmTextToBinary.cpp
@@ +3681,4 @@
>              return false;
> +    } else {
> +        if (module.elemSegments().length() != 1)
> +            return false;

This can happen in the old format, when we use both a table *and* an Elem section, right? In this case, can you Fail() with an explicit error message here, instead of failing as OOM, please?
Attachment #8769946 - Flags: review?(bbouvier) → review+
Comment on attachment 8770222 [details] [diff] [review]
allocate-tables

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

Interdiff looks good, thanks!

::: js/src/asmjs/WasmTable.cpp
@@ +39,5 @@
> +
> +size_t
> +Table::sizeOfExcludingThis(MallocSizeOf mallocSizeOf) const
> +{
> +    return mallocSizeOf(this) + mallocSizeOf(array_.get());

Not too sure how these functions sizeOfExcludingThis and mallocSizeOf work, but it definitely looks strange to have a function called "size of excluding this" and then adding "mallocSizeOf(this)" here. Is it normal?
Attachment #8770222 - Flags: review?(bbouvier) → review+
Comment on attachment 8769948 [details] [diff] [review]
table-object

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

Approved! Thanks.

::: js/src/asmjs/WasmJS.cpp
@@ +656,5 @@
> +        return false;
> +
> +    if (!args.requireAtLeast(cx, "WebAssembly.Table", 1))
> +        return false;
> +    

nit: trailing spaces

::: js/src/jit-test/tests/wasm/jsapi.js
@@ +173,5 @@
> +assertErrorMessage(() => new Table(1), TypeError, "first argument must be a table descriptor");
> +assertErrorMessage(() => new Table({initial:{valueOf() { throw new Error("here")}}}), Error, "here");
> +assertErrorMessage(() => new Table({initial:-1}), TypeError, /bad Table initial size/);
> +assertErrorMessage(() => new Table({initial:Math.pow(2,32)}), TypeError, /bad Table initial size/);
> +assertErrorMessage(() => new Table({initial:Math.pow(2,32)}), TypeError, /bad Table initial size/);

These two tests are the same. Can you put a test with a Number non-integer value here, instead?
Attachment #8769948 - Flags: review?(bbouvier) → review+
(In reply to Benjamin Bouvier [:bbouvier] from comment #49)
Ugh, that used to be sizeOf*Including*This() before I switched to using ShareableBase to de-dupe with the SeenSet which required changing to sizeOf*Excluding*This().  Good catch again!
(In reply to Benjamin Bouvier [:bbouvier] from comment #48)
> Ha, reusing mozilla::Nothing instead, right?

Correct (and avoiding naming ambiguity).

> At the moment, there's only one table. But when there are more than 1, do
> the segments need to be ordered in a lexicographical order on (table,
> offset), too? If so, we could have the full check right now.

Good observation, I was thinking about this too.  For simplicity, I think that's what we'd do, but I'm not sure and I think this would involve adding some mostly-dead code, so I'd rather just wait for now.

> In the worst case, numElems is MaxTableElems, and there can be only one
> table at the moment. I wonder if in the future (>1 table) we'd want a finer
> check, to avoid the worst case of MaxTableElems * MaxTable seg.elems
> mallocs...

Hehe, true.  Or we could have a very low MaxNumTables :)

> The exit epilogue is unreachable! There is actually already an exit
> (generated by GenerateTrapStub called by GenerateJumpTarget) for the
> BadIndirectCall, so you can reuse the offset of this exit instead of
> generating a new one. And then you don't need the new ExitReason / CodeRange
> and this exit generator.

My motivation here was to have the profiling prologue/epilogue set WasmActivation.fp (and avoid bug 1277973), but probably this is fine for now and bug 1277973 will introduce a thunks for setting fp.

> This can happen in the old format, when we use both a table *and* an Elem
> section, right?

This patch adds elem sections, so no :)
Pushed by lwagner@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e3a3d7665277
Baldr: remove temp register allocation on ARM/x64 AsmJSLoadFuncPtr (r=bbouvier)
https://hg.mozilla.org/integration/mozilla-inbound/rev/d14dd0573e8a
Baldr: extract wasm::Table objects from the global data segment (r=bbouvier)
https://hg.mozilla.org/integration/mozilla-inbound/rev/7f8a63d08f90
Baldr: make Memory/Data more symmetric with Table/Elem (r=bbouvier)
https://hg.mozilla.org/integration/mozilla-inbound/rev/03596c2b00a4
Baldr: add 'elem' section to binary format (r=bbouvier)
https://hg.mozilla.org/integration/mozilla-inbound/rev/2dc915c67fb0
Baldr: add WebAssembly.Table constructor and object (r=bbouvier)
Attached patch rename-exportSplinter Review
Simple patch that, similar to previous patch for imports, renames Export=>FuncExport and ExportMap=>Export/ExportVector.  This leads to a really nice symmetry between Import/Export and FuncImport/FuncExport.
Attachment #8770347 - Flags: review?(bbouvier)
D'oh!  (It's a race against the fuzzers)
Attachment #8770376 - Flags: review?(bbouvier)
Attached patch table-exportSplinter Review
Adding Table exports is pretty easy with the preceding patch.
Attachment #8770392 - Flags: review?(bbouvier)
Table.prototype.length getter property
Attachment #8770394 - Flags: review?(bbouvier)
Comment on attachment 8770376 [details] [diff] [review]
check-elem-in-bounds

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

Wow.
Attachment #8770376 - Flags: review?(bbouvier) → review+
Comment on attachment 8770347 [details] [diff] [review]
rename-export

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

Looks good, thanks! Where does the startFuncIndex go, in this patch? (in the Metadata?)

::: js/src/asmjs/WasmModule.cpp
@@ +437,4 @@
>  }
>  
>  static JSFunction*
> +NewExportedFunction(JSContext* cx, Handle<WasmInstanceObject*> instanceObj, uint32_t funcExportIndex)

pre-existing: you can use HandleWasmInstanceObject here

::: js/src/asmjs/WasmModule.h
@@ +107,5 @@
> +// export object. For functions, Export stores an index into the
> +// FuncExportVector in Metadata. For memory and table exports, there is
> +// at most one (default) memory/table so no index is needed. Note: a single
> +// definition can be exported by multiple Exports in the ExportVector.
> +// 

nit: trailing space
Attachment #8770347 - Flags: review?(bbouvier) → review+
Comment on attachment 8770392 [details] [diff] [review]
table-export

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

Looks good!

::: js/src/asmjs/WasmCompile.cpp
@@ +980,5 @@
>        }
> +      case DefinitionKind::Table: {
> +        uint32_t tableIndex;
> +        if (!d.readVarU32(&tableIndex))
> +            return Fail(d, "expected memory index");

copy-pasto: memory => table

::: js/src/asmjs/WasmJS.cpp
@@ +110,5 @@
>                  return false;
>  
>              break;
> +          case DefinitionKind::Table:
> +            MOZ_CRASH("NYI");

If you land this before it's implemented, fuzzers might trip on it!

::: js/src/asmjs/WasmTextToBinary.cpp
@@ +3529,5 @@
>          if (!e.writeVarU32(imp.funcSig().index()))
>              return false;
>          break;
> +      case DefinitionKind::Table:
> +        MOZ_CRASH("NYI");

(if the grammar-based fuzzers are very very very lucky, they could also trip on this)

::: js/src/jit-test/tests/wasm/import-export.js
@@ +55,5 @@
>  assertEq(new Instance(m5, {a:{b:mem4Page}}) instanceof Instance, true);
>  
>  assertErrorMessage(() => new Module(textToBinary('(module (memory 2 1))')), TypeError, /maximum length less than initial length/);
>  assertErrorMessage(() => new Module(textToBinary('(module (import "a" "b" (memory 2 1)))')), TypeError, /maximum length less than initial length/);
> +assertErrorMessage(() => new Module(textToBinary('(module (table (resizable 2 1)))')), TypeError, /maximum length less than initial length/);

Can you add a test where we try to export a non-existing table?
Attachment #8770392 - Flags: review?(bbouvier) → review+
Comment on attachment 8770394 [details] [diff] [review]
table-length-property

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

Nice.
Attachment #8770394 - Flags: review?(bbouvier) → review+
(In reply to Benjamin Bouvier [:bbouvier] from comment #59)
> Looks good, thanks! Where does the startFuncIndex go, in this patch? (in the
> Metadata?)

Oh right, I totally forgot about that conflict.  MetadataCacheablePod would be even easier and achieve the same effect :)
(In reply to Benjamin Bouvier [:bbouvier] from comment #60)
> > +          case DefinitionKind::Table:
> > +            MOZ_CRASH("NYI");
> 
> If you land this before it's implemented, fuzzers might trip on it!
...
> (if the grammar-based fuzzers are very very very lucky, they could also trip
> on this)

Not atm, since we don't decode or parse table imports yet; the table-import patch will fill in these case bodies :)
Pushed by lwagner@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/89d77652a5d9
Baldr: check element segment function is in range (r=bbouvier)
Pushed by lwagner@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9111bfb50532
Baldr: refactor/rename exports to be more symmetric with imports (r=bbouvier)
https://hg.mozilla.org/integration/mozilla-inbound/rev/81e2f8a3193e
Baldr: add Table exports (r=bbouvier)
https://hg.mozilla.org/integration/mozilla-inbound/rev/7f9ce3eb7b9b
Baldr: add Table.prototype.length getter (r=bbouvier)
Depends on: 1287240
This simple patch removes the EXPORT_SLOT reserved slot in instances.  This slot isn't necessary since there is already a property on the instance object.  The slot was used to communicate the exports object to asm.js, but this can be done instead by a simple outparam.  This simplifies Module::instance() a bit.
Attachment #8771697 - Flags: review?(bbouvier)
Attached patch export-mapSplinter Review
This patch prepares for the next two by adding a weak map (from integers to live function objects) to WasmInstanceObject.  The idea is that whenever you want an exported function for i, you ask the WasmInstanceObject and it either gives you back a live one or creates one for you.  This may look like overkill for the current Module::instantiate(), but the real point is to prepare for Table.prototype.get() which uses the weak map at runtime (after instantiation).

Terrence, could you double-check the usage of GCHashMap in WasmJS.(h,cpp)?
Attachment #8771701 - Flags: review?(bbouvier)
Attachment #8771701 - Flags: feedback?(terrence)
This patch removes "function export index" as a concept so that we exclusively use function indices.  To efficiently map from a function index to the necessary FuncExport, the FuncExportVector is stored in sorted order so binary search can be used.  This makes the code a bit simpler but, more importantly, in the next patch, it allows us to create an exported function given only the function-index.
Attachment #8771702 - Flags: review?(bbouvier)
Attached patch table-getSplinter Review
This patch adds Table.prototype.get().  Using the functionality added in the last two patches, it's pretty easy.  The patch also anticipates table imports a bit by showing where the imports will be passed in; this helps point to the "right" place is to put the code.

While a naive impl of what's described in JS.md would eagerly create a function export object for each table element, I think this isn't acceptable since it'll make instantiation a lot slower and create potentially a lot of garbage.  Instead, the getter lazily creates function objects.  To do this, a vector of WasmInstanceObject* is kept in the table object which lets us (1) keep the instance alive, (2) create exported function objects on demand, (3) lookup functions by table-entry PC instead of additionally storing the function index.
Attachment #8771704 - Flags: review?(bbouvier)
Attached patch table-setSplinter Review
This patch handles the simple case of same-instance assignment to a Table.  Cross-instance assignment is next and will be the hard one.
Attachment #8771709 - Flags: review?(bbouvier)
Attached patch table-import (obsolete) — Splinter Review
Given previous refactoring work, table imports is also pretty easy.
Attachment #8771711 - Flags: review?(bbouvier)
Comment on attachment 8771697 [details] [diff] [review]
tweak-instantiate-signature

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

Nice.

::: js/src/asmjs/WasmJS.h
@@ +114,1 @@
>      void init(wasm::UniqueInstance module);

pre-existing: UniqueInstance module

::: js/src/asmjs/WasmModule.cpp
@@ +527,5 @@
>                      Handle<FunctionVector> funcImports,
>                      HandleWasmMemoryObject memImport,
> +                    HandleObject proto,
> +                    MutableHandleWasmInstanceObject instanceObj,
> +                    MutableHandleObject exportObj) const

Another idea: the caller would not pass the outparam exportObj, and asm.js could just do a JS_GetProperty on the Instance when defining exports. This would avoid passing the exportObj down here (it could just be a RootedObject on the stack in this function), but there'd be a very small hit for asm.js.

::: js/src/asmjs/WasmModule.h
@@ +214,5 @@
>  
>      bool instantiate(JSContext* cx,
>                       Handle<FunctionVector> funcImports,
>                       HandleWasmMemoryObject memoryImport,
> +                     HandleObject proto,

Can we rename this parameter to instanceProto, to erase all confusion?
Attachment #8771697 - Flags: review?(bbouvier) → review+
Comment on attachment 8771701 [details] [diff] [review]
export-map

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

The wasm bits look good to me.

::: js/src/asmjs/WasmJS.cpp
@@ +416,5 @@
>  
>  /* static */ void
>  WasmInstanceObject::finalize(FreeOp* fop, JSObject* obj)
>  {
> +    fop->delete_(&obj->as<WasmInstanceObject>().exports());

Does it need to be traced too?
Attachment #8771701 - Flags: review?(bbouvier) → review+
Comment on attachment 8771702 [details] [diff] [review]
rm-func-export-index

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

Nice.

::: js/src/asmjs/WasmGenerator.cpp
@@ +337,5 @@
>  
>  bool
> +ModuleGenerator::finishFuncExports()
> +{
> +    // ModuleGenerator::exportedFucs_ is an unordered HashSet. The

nit: exportedFuncs_

@@ +384,5 @@
>      {
>          TempAllocator alloc(&lifo_);
>          MacroAssembler masm(MacroAssembler::AsmJSToken(), alloc);
>  
> +        if (!entries.resize(metadata_->funcExports.length()))

Three instances of metadata_->funcExports.length() in this function; can you use a local, please?

@@ -694,5 @@
>  
>  bool
>  ModuleGenerator::setStartFunction(uint32_t funcIndex)
>  {
> -    MOZ_ASSERT(!metadata_->hasStartFunction());

Can we keep this assertion here? One Start Function To Rule Them All, etc.

::: js/src/asmjs/WasmInstance.cpp
@@ +424,2 @@
>  {
> +    auto& func = metadata_->lookupFuncExport(funcIndex);

Can you keep the non-auto explicit qualifier, please?
Attachment #8771702 - Flags: review?(bbouvier) → review+
Comment on attachment 8771704 [details] [diff] [review]
table-get

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

Looks good, thanks. Interesting tests!

::: js/src/asmjs/WasmJS.cpp
@@ +876,5 @@
> +
> +    uint32_t index = uint32_t(indexDbl);
> +    MOZ_ASSERT(double(index) == indexDbl);
> +
> +    if (!tableObj->initialized()) {

How can this happen in the wild? There should have been a reported OOM when creating the WasmTableObject, right?

@@ +886,5 @@
> +    MOZ_ASSERT(instanceVector.length() == table.length());
> +
> +    RootedWasmInstanceObject instanceObj(cx, instanceVector[index]);
> +    const CodeRange* codeRange = instanceObj->instance().lookupCodeRange(table.array()[index]);
> +    if (!codeRange || !codeRange->isFunction()) {

Can you please add a comment like

// Happens for unset table entries.

::: js/src/asmjs/WasmJS.h
@@ +190,5 @@
> +    static void trace(JSTracer* trc, JSObject* obj);
> +    static bool lengthGetterImpl(JSContext*, const CallArgs&);
> +    static bool lengthGetter(JSContext*, unsigned, Value*);
> +    static bool getImpl(JSContext*, const CallArgs&);
> +    static bool get(JSContext*, unsigned argc, Value*);

It's weird to name some arguments and let the other unnamed. Or do the same for lengthGetter, in this case?

::: js/src/jit-test/tests/wasm/jsapi.js
@@ +221,5 @@
> +assertErrorMessage(() => get.call(), TypeError, /called on incompatible undefined/);
> +assertErrorMessage(() => get.call({}), TypeError, /called on incompatible Object/);
> +assertEq(get.call(tbl1, 0), null);
> +assertEq(get.call(tbl1, 1), null);
> +assertErrorMessage(() => get.call(tbl1, 2), RangeError, /out-of-range index/);

Can you also test with an object which implements a valueOf that throws?
And also a valid test with a double non-integer value (like 0.5)?

::: js/src/jit-test/tests/wasm/tables.js
@@ +51,5 @@
>  assertEq(call(4), 0);
>  assertEq(call(5), 2);
>  assertErrorMessage(() => call(6), Error, /bad wasm indirect call/);
>  assertErrorMessage(() => call(10), Error, /out-of-range/);
>  

Can you add some tests for profiling? That is:
1. create a module with a non-empty exported table;
2. call a function in the table from outside the module;
3. enable profiling;
4. check that calling into the table still works;
5. call into the module (which will effectively toggle profiling mode on the functions);
6. then call into the table to ensure it still works;
7. then disable profiling and repeat steps 4 to 6 included.

@@ +137,5 @@
> +i = null;
> +gc();
> +assertEq(finalizeCount(), 1);
> +t = null;
> +gc();

Missing assert at the end? (or remove the gc() call?)
Attachment #8771704 - Flags: review?(bbouvier) → review+
(In reply to Benjamin Bouvier [:bbouvier] from comment #75)
> Another idea: the caller would not pass the outparam exportObj, and asm.js
> could just do a JS_GetProperty on the Instance when defining exports. This
> would avoid passing the exportObj down here (it could just be a RootedObject
> on the stack in this function), but there'd be a very small hit for asm.js.

I had considered this and decided against since Anything(TM) can happen during a plain JS property get.  However, I can't actually think of a corner case (given we just created this instance and the property), so I'll settle for the cleaner interface and a MOZ_RELEASE_ASSERT :)
(In reply to Benjamin Bouvier [:bbouvier] from comment #76)
> Does it need to be traced too?

Good question: no; because it is a WeakCache, it doesn't participate in tracing, but, rather, sweeping and JS::WeakCache takes care of registering the object in whatever internal lists that get swept.  So all we have to do is manage the C++ lifetime of the JS::WeakCache (iiuc from terrence :).
(In reply to Benjamin Bouvier [:bbouvier] from comment #77)
> >  ModuleGenerator::setStartFunction(uint32_t funcIndex)
> >  {
> > -    MOZ_ASSERT(!metadata_->hasStartFunction());
> 
> Can we keep this assertion here? One Start Function To Rule Them All, etc.

Even better: MetadataCacheablePod::initStartFuncIndex() already asserts this.
Comment on attachment 8771701 [details] [diff] [review]
export-map

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

Looks fine to me.

::: js/src/asmjs/WasmJS.cpp
@@ +442,5 @@
>      auto* obj = NewObjectWithGivenProto<WasmInstanceObject>(cx, proto);
>      if (!obj)
>          return nullptr;
>  
> +    obj->setReservedSlot(EXPORTS_SLOT, PrivateValue(exports.release()));

Ah, I didn't realize you were storing these in an object slots! It will work fine, but be a bit space inefficient. I have a new concept in my queue that will allow us to do this much more space efficiently.
Attachment #8771701 - Flags: feedback?(terrence) → feedback+
(In reply to Benjamin Bouvier [:bbouvier] from comment #78)
Thanks, great points!

> > +    if (!tableObj->initialized()) {
> 
> How can this happen in the wild? There should have been a reported OOM when
> creating the WasmTableObject, right?

WasmTableObject::construct() does not call init() on the returned table since it has no instance to init with (and also no badIndirectCall stub).  Thus, a Table object stays in this !initialized() state until its first set() or import.
Pushed by lwagner@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/09e1f272ac8d
Baldr: simplify Module::instantiate interface (r=bbouvier)
https://hg.mozilla.org/integration/mozilla-inbound/rev/de1b69faee63
Baldr: give each WasmInstanceObject a weak map of func-index-to-live-function-object (r=bbouvier)
https://hg.mozilla.org/integration/mozilla-inbound/rev/b4575688fc9f
Baldr: remove use of func-export-index, use func-index instead (r=bbouvier)
https://hg.mozilla.org/integration/mozilla-inbound/rev/4a992c10189f
Baldr: add Table.prototype.get (r=bbouvier)
Attached patch table-importSplinter Review
The previous patch was technically correct given no cross-module tables, but as soon as those work, it would be wrong because it always clobbered the imported tables' elements :)  The updated patch adds an additional "initialized" state to Table (matching the initialized state to WasmTableObject) so that we can only initialize uninitialized tables in initElems().
Attachment #8771711 - Attachment is obsolete: true
Attachment #8771711 - Flags: review?(bbouvier)
Attachment #8772128 - Flags: review?(bbouvier)
Priority: -- → P1
Attached patch elem-kind (obsolete) — Splinter Review
As just proposed in https://github.com/WebAssembly/design/pull/727 (so that other table element types can be added in the future).
Attachment #8772195 - Flags: review?(bbouvier)
Comment on attachment 8772195 [details] [diff] [review]
elem-kind

Waiting to see what gets decided in https://github.com/WebAssembly/design/pull/727.
Attachment #8772195 - Attachment is obsolete: true
Attachment #8772195 - Flags: review?(bbouvier)
Comment on attachment 8771709 [details] [diff] [review]
table-set

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

Nice.

::: js/src/asmjs/WasmJS.cpp
@@ +601,5 @@
> +wasm::ExportedFunctionToInstanceObject(JSFunction* fun)
> +{
> +    MOZ_ASSERT(IsExportedFunction(fun));
> +    const Value& v = fun->getExtendedSlot(FunctionExtended::WASM_INSTANCE_SLOT);
> +    return &v.toObject().as<WasmInstanceObject>();

You could simplify ExportedFunctionToInstance(JSFunction* fun) to return ExportedFunctionToInstanceObject(fun)->instance();

@@ +941,5 @@
> +    double indexDbl;
> +    if (!ToInteger(cx, args[0], &indexDbl))
> +        return false;
> +
> +    if (indexDbl < 0 || indexDbl >= table.length()) {

maybe pre-existing in the getImpl too: it'd be nicer to do this comparison *after* the conversion of the index to uint32_t, to not trigger unexpected implicit conversions.

@@ +966,5 @@
> +        if (!tableObj->init(cx, instanceObj))
> +            return false;
> +    }
> +
> +

nit: two blank lines

@@ +1007,4 @@
>  const JSFunctionSpec WasmTableObject::methods[] =
>  {
>      JS_FN("get", WasmTableObject::get, 1, 0),
> +    JS_FN("set", WasmTableObject::set, 1, 0),

nit: 2, not 1

::: js/src/asmjs/WasmJS.h
@@ +198,5 @@
>      static bool lengthGetter(JSContext*, unsigned, Value*);
>      static bool getImpl(JSContext*, const CallArgs&);
>      static bool get(JSContext*, unsigned argc, Value*);
> +    static bool setImpl(JSContext*, const CallArgs&);
> +    static bool set(JSContext*, unsigned argc, Value*);

nit (as in the other patch): mixed named and unnamed parameters

::: js/src/jit-test/tests/wasm/jsapi.js
@@ +235,5 @@
> +// 'WebAssembly.Table.prototype.set' method
> +const set = setDesc.value;
> +assertErrorMessage(() => set.call(), TypeError, /called on incompatible undefined/);
> +assertErrorMessage(() => set.call({}), TypeError, /called on incompatible Object/);
> +assertErrorMessage(() => set.call(tbl1, 2, null), RangeError, /out-of-range index/);

Can you add a test like set.call(tbl1, 2) to check that the right number of arguments is passed? And a test that passes two arguments but the second one is an explicit undefined?

@@ +237,5 @@
> +assertErrorMessage(() => set.call(), TypeError, /called on incompatible undefined/);
> +assertErrorMessage(() => set.call({}), TypeError, /called on incompatible Object/);
> +assertErrorMessage(() => set.call(tbl1, 2, null), RangeError, /out-of-range index/);
> +assertErrorMessage(() => set.call(tbl1, -1, null), RangeError, /out-of-range index/);
> +assertErrorMessage(() => set.call(tbl1, Math.pow(2,33), null), RangeError, /out-of-range index/);

Can you add a test where the index is an object which implements a valueOf that throws?

@@ +239,5 @@
> +assertErrorMessage(() => set.call(tbl1, 2, null), RangeError, /out-of-range index/);
> +assertErrorMessage(() => set.call(tbl1, -1, null), RangeError, /out-of-range index/);
> +assertErrorMessage(() => set.call(tbl1, Math.pow(2,33), null), RangeError, /out-of-range index/);
> +assertErrorMessage(() => set.call(tbl1, 0, {}), TypeError, /second argument must be null or an exported WebAssembly Function object/);
> +assertErrorMessage(() => set.call(tbl1, 0, ()=>{}), TypeError, /second argument must be null or an exported WebAssembly Function object/);

Can you add tests for a regular (non arrow) function? A standard library function (say, any of the WebAssembly ctors)?

::: js/src/jit-test/tests/wasm/tables.js
@@ +182,5 @@
> +assertEq(finalizeCount(), 2);
> +t.set(0, null);
> +assertEq(t.get(0), null);
> +gc();
> +assertEq(finalizeCount(), 2);

At this point, the Table only element is null, so we *could* release the Instance. I assume we still hold onto the Instance because 1. it's cheaper than uninitializing the Table, and 2. this shouldn't happen in practice, right?
Attachment #8771709 - Flags: review?(bbouvier) → review+
Comment on attachment 8772128 [details] [diff] [review]
table-import

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

Looks good, thanks!

::: js/src/asmjs/WasmCode.h
@@ +236,1 @@
>      explicit TableDesc(TableKind kind) : kind(kind) {}

pre-existing: mind to initialize the other members here too, or add the annotation that they're initialized somewhere else?
(in particular, maximum could be initialized to UINT32_MAX in both ctors)

::: js/src/asmjs/WasmJS.cpp
@@ +980,5 @@
>              return true;
>          }
>  
>          RootedWasmInstanceObject instanceObj(cx, ExportedFunctionToInstanceObject(value));
> +        tableObj->init(cx, instanceObj);

nit: This one is still fallible, iiuc. Maybe make it MOZ_MUST_USE?

::: js/src/asmjs/WasmTable.cpp
@@ +38,5 @@
>      return table;
>  }
>  
> +void
> +Table::init(JSContext* cx, const CodeSegment& codeSegment)

cx is dead here

::: js/src/asmjs/WasmTable.h
@@ +23,5 @@
>  
>  namespace js {
>  namespace wasm {
>  
> +class Instance;

Was this used in this header before?

@@ +45,2 @@
>      bool isTypedFunction() const { return kind_ == TableKind::TypedFunction; }
>      void** array() const { return array_.get(); }

Nice to have, but not mandatory: a second array() method that returns an const void**, in which we'd assert initialized(), for read-only accesses to array() (there are a few in the code).

@@ +48,5 @@
>  
> +    // A Table must be initialized before any dependent instance can execute.
> +
> +    bool initialized() const { return initialized_; }
> +    void init(JSContext* cx, const CodeSegment& codeSegment);

Another interface idea: the common usage is if (!initialized()) init(...), here and for the TableObject too. We could have ensureInitialized() functions, instead, to shorten this pattern (and have a few single line loop bodies and such).
Attachment #8772128 - Flags: review?(bbouvier) → review+
(In reply to Benjamin Bouvier [:bbouvier] from comment #88)
> > +    if (indexDbl < 0 || indexDbl >= table.length()) {
> 
> maybe pre-existing in the getImpl too: it'd be nicer to do this comparison
> *after* the conversion of the index to uint32_t, to not trigger unexpected
> implicit conversions.

I think it needs to happen before, though, since the double returned by ToInteger() can be outside the [0, UINT32_MAX] range.  (The goal here is that if you specify some crazy-big number, it doesn't wrap around to be in this uint32_t range and then get accepted.)

> At this point, the Table only element is null, so we *could* release the
> Instance. I assume we still hold onto the Instance because 1. it's cheaper
> than uninitializing the Table, and 2. this shouldn't happen in practice,
> right?

I think what we ultimately want is for Table to own its own bad-element code stub so assigning null can set the WasmInstanceObject* to nullptr.  That way, a Table will hold an Instance alive precisely as long as you'd expect if you assumped the Table was a simple array of Function values.  I expect we'll probably need to do that if we make wasm::Table shareable between threads; I'm just taking a short-cut here for expediency.
(In reply to Benjamin Bouvier [:bbouvier] from comment #89)
> >      void** array() const { return array_.get(); }
> 
> Nice to have, but not mandatory: a second array() method that returns an
> const void**, in which we'd assert initialized(), for read-only accesses to
> array() (there are a few in the code).

That's a good idea, but unfortunately we can't overload on the return value and the current array() already is specified as a 'const' member function.

> Another interface idea: the common usage is if (!initialized()) init(...),
> here and for the TableObject too. We could have ensureInitialized()
> functions, instead, to shorten this pattern (and have a few single line loop
> bodies and such).

It looks like only the Module::initElems function does the if (!initialized() init() pattern and in WasmJS.cpp, we either know we need to initialize (thereby asserting an invariant that Table::init() happens at the same time as WasmTableObject::init()) or we need to branch on initialized() separately.
Pushed by lwagner@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/42550e82d1d2
Baldr: add Table.set support for the same-instance case (r=bbouvier)
https://hg.mozilla.org/integration/mozilla-inbound/rev/dbbe25db2d61
Baldr: add support for same-instance Table imports (r=bbouvier)
this change seemed to make a large improvement (i.e. reduction) in our number of constructors:
https://treeherder.mozilla.org/perf.html#/alerts?id=1823
(In reply to Luke Wagner [:luke] from comment #90)
> (In reply to Benjamin Bouvier [:bbouvier] from comment #88)
> > > +    if (indexDbl < 0 || indexDbl >= table.length()) {
> > 
> > maybe pre-existing in the getImpl too: it'd be nicer to do this comparison
> > *after* the conversion of the index to uint32_t, to not trigger unexpected
> > implicit conversions.
> 
> I think it needs to happen before, though, since the double returned by
> ToInteger() can be outside the [0, UINT32_MAX] range.  (The goal here is
> that if you specify some crazy-big number, it doesn't wrap around to be in
> this uint32_t range and then get accepted.)

Out of curiosity, could you use the (fallible or infallible) ToUint32 functions maybe, instead?
(In reply to Benjamin Bouvier [:bbouvier] from comment #95)
The problem is that they'll wrap <0 and >UINT32_MAX back into [0, UINT32].
Depends on: 1288222
Depends on: 1288483
Depends on: 1288944
Attached patch trace-wasm-compartment (obsolete) — Splinter Review
Terrence: I have a weak set but, under certain conditions (described in the patch's comment), I want to strongly root all the elements.  I could *almost* simply call set.trace(), but this ends up calling TraceEdge() with a ReadBarriered<T> and TraceEdge() only takes a WriteBarriered<T>.
Attachment #8774089 - Flags: review?(terrence)
Attached patch allow-cross-instance-elems (obsolete) — Splinter Review
Given the 3 blocking bugs, we can allow multiple instances in the same table.  This patch removes the restriction, fixes two trivial issues this exposes and adds a bunch of tests.
Attachment #8774090 - Flags: review?(bbouvier)
Attached patch element-typeSplinter Review
To match JS.md and design/#727.

Other than initializer expressions in segment offsets (which is in bug 1286517) and binary encoding tweaks, I think this is the last patch :)
Attachment #8774091 - Flags: review?(bbouvier)
(In reply to Luke Wagner [:luke] from comment #97)
> Created attachment 8774089 [details] [diff] [review]
> trace-wasm-compartment
> 
> Terrence: I have a weak set but, under certain conditions (described in the
> patch's comment), I want to strongly root all the elements.  I could
> *almost* simply call set.trace(), but this ends up calling TraceEdge() with
> a ReadBarriered<T> and TraceEdge() only takes a WriteBarriered<T>.

This is only because we have not needed it yet. You'll note that there are already ReadBarriered implementations of TraceRoot and TraceNullableRoot.

This patch adds an implementation of TraceEdge for ReadBarriered. Please fold it in and see if you can just call trace.
Attachment #8774423 - Flags: feedback?(luke)
Attachment #8774089 - Flags: review?(terrence)
Ah great, that's much nicer.  Here's the two folded with the addition of an explicit template specialization for the new TraceEdge.
Attachment #8774089 - Attachment is obsolete: true
Attachment #8774423 - Attachment is obsolete: true
Attachment #8774423 - Flags: feedback?(luke)
Attachment #8774465 - Flags: review?(terrence)
Comment on attachment 8774465 [details] [diff] [review]
trace-wasm-compartment

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

::: js/src/gc/Marking.cpp
@@ +535,5 @@
>  
>  // Instantiate a copy of the Tracing templates for each derived type.
>  #define INSTANTIATE_ALL_VALID_TRACE_FUNCTIONS(type) \
>      template void js::TraceEdge<type>(JSTracer*, WriteBarrieredBase<type>*, const char*); \
> +    template void js::TraceEdge<type>(JSTracer*, ReadBarriered<type>*, const char*); \

Ah, new I forgot something!
Attachment #8774465 - Flags: review?(terrence) → review+
Comment on attachment 8774090 [details] [diff] [review]
allow-cross-instance-elems

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

Yayyyyyy

::: js/src/jit-test/tests/wasm/table-gc.js
@@ +224,5 @@
> +    inst.edge = makeFinalizeObserver();
> +    tbl.set(i, inst.exports.f);
> +}
> +inst = null;
> +assertEq(tbl.get(N - 1)(N - 1), 109);

Can the RHS be 100 + N - 1, instead?

::: js/src/jit-test/tests/wasm/tables.js
@@ +98,5 @@
> +tbl.set(1, e2.g);
> +tbl.set(2, e3.h);
> +var e4 = evalText(`(module (import "a" "b" (table 3)) ${caller})`, {a:{b:tbl}}).exports;
> +assertEq(e4.call(0), 42);
> +assertErrorMessage(() => e4.call(1), Error, /bad wasm indirect call/);

Why would this one fail? tbl.get(1) === e2.g, which is defined as far as I can tell.
Attachment #8774090 - Flags: review?(bbouvier) → review+
Comment on attachment 8774091 [details] [diff] [review]
element-type

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

Looks good to me.
Attachment #8774091 - Flags: review?(bbouvier) → review+
Pushed by lwagner@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/da7179ab3873
Baldr: trace all wasm instances in a compartment if it is active (r=terrence)
Pushed by lwagner@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/feceaaf64da6
Baldr: add element type to Tables (r=bbouvier)
In preparation for cross-instance *table* calls, this patch implements the simpler case of cross-instance *import* calls.
Attachment #8777432 - Flags: review?(bbouvier)
Attachment #8777432 - Attachment is patch: true
Oops, we mustn't throw instantiation errors for asm.js signature mismatch.
Attachment #8777432 - Attachment is obsolete: true
Attachment #8777432 - Flags: review?(bbouvier)
Attachment #8777597 - Flags: review?(bbouvier)
Comment on attachment 8777597 [details] [diff] [review]
direct-cross-instance-import-calls

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

Two worlds are colliding, and new test cases arise. Thanks for the patch, it's actually quite simple.

::: js/src/asmjs/WasmInstance.cpp
@@ +298,3 @@
>          const FuncImport& fi = metadata().funcImports[i];
>          FuncImportTls& import = funcImportTls(fi);
> +        if (IsExportedFunction(f) && !metadata().isAsmJS()) {

So this means wasm can import asm.js functions with signature checks, but not the other way around. Can you add tests for the asm.js -> wasm and wasm -> asm.js inter-module paths, please?

@@ +404,5 @@
>      // necessary strong edge.
>      TraceEdge(trc, &object_, "wasm object");
>  
>      for (const FuncImport& fi : metadata().funcImports)
> +        TraceNullableEdge(trc, &funcImportTls(fi).obj, "wasm import");

So I assume this is what ensures that a WasmInstanceObject referenced only by a module that imports one of its exported functions remain alive?

::: js/src/asmjs/WasmModule.cpp
@@ +494,5 @@
> +{
> +    MOZ_ASSERT(funcImports.length() == metadata_->funcImports.length());
> +
> +    if (metadata().isAsmJS())
> +        return true;

If I understand correctly, an asm.js module won't do the signature check when importing wasm functions. When instantiating, we check that we're not an asm.js module and use the interpreter path in this case, so we're fine here...

@@ +505,5 @@
> +        uint32_t funcIndex = ExportedFunctionToIndex(f);
> +        Instance& instance = ExportedFunctionToInstance(f);
> +        const FuncExport& funcExport = instance.metadata().lookupFuncExport(funcIndex);
> +
> +        if (funcExport.sig() != metadata_->funcImports[i].sig()) {

... but a wasm module importing asm.js functions will trigger the signature check, though, and nothing prevents it from calling directly into asm.js if the signature is correct.

We have a choice here: we *can* have the signature check for wasm->asm.js calls and report an error if they are not the same. But we also can not have the signature check and just decide that later in instantiateFunctions. Can you add test cases for the case where wasm modules import asm.js functions, please?

Actually, we could just use the more optimized path in all cases (asm.js or wasm) when the signatures match, or use the interpreter exit when they don't. That being said, the behavior you implement is more restrictive, so it's easier to relax in the future. What do you think?

::: js/src/asmjs/WasmTextToBinary.cpp
@@ +3216,5 @@
>      for (size_t i = 0; i < numImports; i++) {
>          AstImport* imp = module->imports()[i];
>          switch (imp->kind()) {
>            case DefinitionKind::Function:
> +            if (!r.registerImportName(imp->name(), lastFuncImportIndex++))

Nice catch!

::: js/src/jit-test/tests/wasm/import-export.js
@@ +83,5 @@
> +var f2vm = new Module(textToBinary('(module (import "a" "b" (param f32)))'));
> +assertEq(new Instance(i2vm, {a:{b:e.i2v}}) instanceof Instance, true);
> +assertErrorMessage(() => new Instance(i2vm, {a:{b:e.f2v}}), TypeError, /imported function signature mismatch/);
> +assertErrorMessage(() => new Instance(f2vm, {a:{b:e.i2v}}), TypeError, /imported function signature mismatch/);
> +assertEq(new Instance(f2vm, {a:{b:e.f2v}}) instanceof Instance, true);

Can you add tests where a module M1 imports a function $f from a module M2, where $f is not the first function in M2? (testing that we're taking the right function index, and not the function export index, in the many lookups)

@@ +356,5 @@
> +)`));
> +var e = {call:() => 1000};
> +for (var i = 0; i < 10; i++)
> +    e = new Instance(m, {a:{val:i, next:e.call}}).exports;
> +assertEq(e.call(), 1090);

Can you add a test (maybe in another file) that:
- creates a module M1 that exports a function $foo
- imports $foo from another module M2, call it in $bar
- instantiate M1 and M2
- dereference the instance of M1 and M1 (so they don't have anymore explicit references)
- gc();
- check that calling $bar still calls into $foo
Attachment #8777597 - Flags: review?(bbouvier) → review+
(In reply to Benjamin Bouvier [:bbouvier] from comment #111)
Thanks!

> > +        if (IsExportedFunction(f) && !metadata().isAsmJS()) {
> 
> So this means wasm can import asm.js functions with signature checks, but
> not the other way around. Can you add tests for the asm.js -> wasm and wasm
> -> asm.js inter-module paths, please?

You bet.  Writing the tests made me realize that wasm-importing-asm.js should work too, meaning that the direct import *only* happens for wasm->wasm.

> > +        TraceNullableEdge(trc, &funcImportTls(fi).obj, "wasm import");
> 
> So I assume this is what ensures that a WasmInstanceObject referenced only
> by a module that imports one of its exported functions remain alive?

Correct!

> Actually, we could just use the more optimized path in all cases (asm.js or
> wasm) when the signatures match, or use the interpreter exit when they
> don't. That being said, the behavior you implement is more restrictive, so
> it's easier to relax in the future. What do you think?

You're right, we could actually use the direct call when the asm.js signatures happen to match.  To keep things simple and avoid any unexpected interactions, I'd like only use the fast path for wasm->wasm for the time being, but you're right we can reevaluate later.
Pushed by lwagner@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/62134495d26b
Baldr: make direct calls for wasm->wasm imports (r=bbouvier)
Working on a subsequent patch, I wanted to make this simplification in how Instance/WasmInstanceObject are created.
Attachment #8778085 - Flags: review?(bbouvier)
... and this simplification to how table instantiation happens.  In retrospect, the current way is pretty goofy.
Attachment #8778086 - Flags: review?(bbouvier)
This improves the current situation with tracing WasmInstanceObject and wasm::Instance slightly.  In a subsequent patch, wasm::Table will call wasm::Instance::trace() which, without this patch, would go right back and call wasm::Table::trace(), so iloop.
Attachment #8778087 - Flags: review?(terrence)
... and with the patches in the dependent bugs, I think this is the last patch for this bug.
Attachment #8778111 - Flags: review?(bbouvier)
Attachment #8774090 - Attachment is obsolete: true
Attachment #8778085 - Flags: review?(bbouvier) → review+
Comment on attachment 8778086 [details] [diff] [review]
tidy-table-instantiation

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

It's much better to not have the wasm table creation not scattered anymore. Thanks!

::: js/src/asmjs/WasmJS.cpp
@@ +852,1 @@
>      if (tableObj.initialized())

Just objecting to naming here: isNewborn and initialized sound very close to me (almost synonyms?). How about renaming isNewborn to hasTable? And maybe explain in a comment somewhere what the order of initialization is, with respect to these functions.

::: js/src/asmjs/WasmModule.cpp
@@ +592,5 @@
> +    } else {
> +        for (const TableDesc& tableDesc : metadata_->tables) {
> +            SharedTable table;
> +            if (tableDesc.external) {
> +                MOZ_ASSERT(!tableObj);

nit: tautological, we're in the else branch of if (tableObj).

@@ -681,5 @@
> -                MOZ_ASSERT(instance.tables().length() == 1);
> -                tableObj.set(WasmTableObject::create(cx, *instance.tables()[0]));
> -                if (!tableObj)
> -                    return false;
> -            }

Nice to see this go!
Attachment #8778086 - Flags: review?(bbouvier) → review+
Comment on attachment 8778111 [details] [diff] [review]
allow-cross-instance-elems

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

\o/

::: js/src/jit-test/tests/wasm/import-export.js
@@ +278,5 @@
> +var code = textToBinary('(module (import "a" "b" (table 2 2)) (func $foo) (elem (i32.const 0) $foo) (export "foo" $foo))');
> +var tbl = new Table({initial:2, element:"anyfunc"});
> +var e1 = new Instance(new Module(code), {a:{b:tbl}}).exports;
> +tbl.set(1, e1.foo);
> +assertEq(e1.foo, tbl.get(0));

Could you move this assert above the call to set() above?
Attachment #8778111 - Flags: review?(bbouvier) → review+
Comment on attachment 8778087 [details] [diff] [review]
improve-instance-trace

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

::: js/src/asmjs/WasmInstance.cpp
@@ +401,5 @@
> +    // TraceEdge is called is so that the pointer can be updated during a moving
> +    // GC. TraceWeakEdge may sound better, but it is less efficient given that
> +    // we know object_ is already marked.
> +    MOZ_ASSERT(!IsAboutToBeFinalized(&object_));
> +    TraceEdge(trc, &object_, "wasm instance object");

You can also use |object_ = gc::MaybeForwarded(object_);| here, since you know you only need to update the pointer.
Attachment #8778087 - Flags: review?(terrence) → review+
(In reply to (PTO until 29th August) Benjamin Bouvier [:bbouvier] from comment #119)
> >      if (tableObj.initialized())
> 
> Just objecting to naming here: isNewborn and initialized sound very close to
> me (almost synonyms?). How about renaming isNewborn to hasTable? And maybe
> explain in a comment somewhere what the order of initialization is, with
> respect to these functions.

I see what you're getting at.  However, isNewborn() borrows from an older SM GC naming where isNewborn() is a sortof degenerate state where construction failed but you still have this allocated GC thing which must be traceable/finalizable, but nothing else.  initialized(), otoh, is an actual general state that has to be handled on all paths (like a HashMap's initialized() state).  As such, isNewborn() is a private method, only tested in the GC hooks whereas initialized() is public and (should be) tested everywhere.

> nit: tautological, we're in the else branch of if (tableObj).

Almost: we're in a loop :)  Thus, this assert is asserting that there is not more than one external table.
(In reply to Terrence Cole [:terrence] from comment #121)
> You can also use |object_ = gc::MaybeForwarded(object_);| here, since you
> know you only need to update the pointer.

Ooh, much nicer.  That also simplifies the comment :)
Pushed by lwagner@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/31ace86ee3e5
Baldr: tidy instantiation logic (r=bbouvier)
https://hg.mozilla.org/integration/mozilla-inbound/rev/214d6ce2943a
Baldr: tidy table instantiation logic (r=bbouvier)
https://hg.mozilla.org/integration/mozilla-inbound/rev/5f8ac144a7ac
Baldr: allow cross-element Table elements (r=bbouvier)
Whiteboard: [leave open]
Attached patch improve-table-trace (obsolete) — Splinter Review
This patch does the same thing to Table/WasmTableObject as the 'improve-instance-trace' patch did for Instance/WasmInstanceObject.  I had thought it wasn't necessary, but I realized later that if we have N instances which import 1 table (which is the expected case for dynamic linking), we'll otherwise mark the Table N times (and walk its array N times for aggregate O(N*M)).  Forwarding to WasmTableObject means we only mark each Table once.

One extra complication is that, unlike instances, tables don't necessarily have an object (when they are not imported or exported).  In this case, though, there is necessarily a single owning Instance so calling tracePrivate() directly is fine.
Attachment #8778597 - Flags: review?(terrence)
Oops, forgot to qref to include test changes.
Attachment #8778599 - Flags: review?(terrence)
Attachment #8778597 - Attachment is obsolete: true
Attachment #8778597 - Flags: review?(terrence)
Whiteboard: [leave open]
Comment on attachment 8778599 [details] [diff] [review]
improve-table-trace

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

That makes much more sense.
Attachment #8778599 - Flags: review?(terrence) → review+
Pushed by lwagner@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/bc162276db8a
Baldr: improve tracing of wasm::Table (r=terrence)
Whiteboard: [leave open]
https://hg.mozilla.org/mozilla-central/rev/bc162276db8a
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
You need to log in before you can comment on or make changes to this bug.