1450263 - Basic generalized tables: add table-of-anyref, table.get, table.set, table.grow

Status: RESOLVED FIXED

(Core :: JavaScript: WebAssembly, enhancement, P2)

Comment 1

[Lars T Hansen [:lth]]

Does not block Milestone 1.

Comment 2

[Lars T Hansen [:lth]]

Attachment: bug1450263-table-of-anyref-part-1.patch

Part 1: table-of-anyref + changes to existing operations + test cases

Comment 3

[Lars T Hansen [:lth]]

Attachment: bug1450263-table-of-anyref-part-1-v2.patch

Table of anyref and related goodies but no instructions yet.  See commit comment for link to (evolving) spec.

Comment 4

[Lars T Hansen [:lth]]

Attachment: bug1450263-table-of-anyref-part-2.patch

And some new wasm instructions to operate on tables.  (Now that I think about it I could usefully test that get and set instructions can't operate on table-of-anyfunc yet, until we have anyfunc as a true type, but I can work on that in parallel with review.)

Comment 5

[Lars T Hansen [:lth]]

Attachment: bug1450263-table-of-anyref-part-2-v2.patch

Added more test cases, and added validation guards that were missing: table number out of range for all instructions; only allow table-of-anyref for table.get and table.set (for now).

Comment 6

[Lars T Hansen [:lth]]

I note that the reftypes proposal has different instruction encodings, having used a few of the valuable single-byte opcodes:

get_table is un-prefixed 0x25
set_table is un-prefixed 0x26

Otherwise the proposal seems poorly developed in terms of encoding (no accomodation for table indices from what I can tell) and execution.

Comment 7

[Lars T Hansen [:lth]]

The behavior of table.grow when it can't grow /should/ probably be to return -1, but IIRC the behavior is not in the draft spec, and the implementation here throws a RangeError, consistent with the JS API.

Comment 8

[Lars T Hansen [:lth]]

Rossberg also suggests that table.grow should take a value to initialize with, so that we can support non-nullable types.  This is a good idea, but the code here does not accept one.

(Again, no spec yet for table.grow.)

Comment 9

[Alex Crichton [:acrichto]]

I've now finished writing an initial version of supporting the anyref type in wasm-bindgen - https://github.com/rustwasm/wasm-bindgen/pull/1002, although it's naturally entirely untested other than me eyeing it manually against `wasm2wat` output. In any case though I can definitely help provide testing and feedback for this if necessary, and just wanted to be sure to mention it!

Comment 10

[Lars T Hansen [:lth]]

Comment on attachment 9018328 [details] [diff] [review]
bug1450263-table-of-anyref-part-1-v2.patch

Cancelling review for now; found some minor issues.

Comment 11

[Lars T Hansen [:lth]]

Comment on attachment 9018543 [details] [diff] [review]
bug1450263-table-of-anyref-part-2-v2.patch

Cancelling review for now, found some bugs.

Comment 12

[Lars T Hansen [:lth]]

Attachment: bug1450263-table-of-anyref-v2.patch

Comment 13

[Lars T Hansen [:lth]]

Attachment: bug1450263-table-operations-v2.patch

Comment 14

[Lars T Hansen [:lth]]

Work that remains here that will come in a follow-up patch or on a different bug:

- table.fill operation
- JS API for table.grow should accept an initializer value
- opcode assignments (awaiting some kind of agreement)
- instruction encodings (basically, flag vs no flag and if a flag, what value for the flag)

Comment 15

[Luke Wagner [:luke]]

Comment on attachment 9022655 [details] [diff] [review]
bug1450263-table-of-anyref-v2.patch

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

Nice and clean!

::: js/src/jit-test/tests/wasm/gc/tables-generalized.js
@@ +1,1 @@
> +// |jit-test| skip-if: !wasmGeneralizedTables()

TIL about skip-if.  Cool.

::: js/src/wasm/WasmJS.cpp
@@ +2265,5 @@
> +        } else {
> +            table.setNull(index);
> +        }
> +    } else if (table.kind() == TableKind::AnyRef) {
> +        RootedObject value(cx, ToObject(cx, args[1]));

I know there are already lots of these scattered about, but it'd be nice to have some syntactic placeholder for all the places where we do jsval-->anyref conversions so we don't miss one later when we do anyref properly.

::: js/src/wasm/WasmTable.cpp
@@ +241,5 @@
> +        // Realloc does not zero the delta for us.
> +        PodZero(newArray + length_, delta);
> +        length_ = newLength.value();
> +    } else if (kind_ == TableKind::AnyRef) {
> +        JS::Heap<JSObject*>* newArray = rt->pod_realloc(objectArray(), length_, newLength.value());

I think realloc of Heap<T>'s is problematic due to pre/post barriers embedding Heap<T>*'s in the store buffer, and this is the reason Heap<T> is marked as MOZ_NON_MEMMOVABLE.  Presumably you can ~Heap<T> the old and construct the new (I eternally wish there was a "realloc, but only if you can do it in-place" function).  It'd be nice to have a test for this that pounds on grow and storing objects and such.

Comment 16

[Luke Wagner [:luke]]

Comment on attachment 9022656 [details] [diff] [review]
bug1450263-table-operations-v2.patch

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

Looking good; some initial comments:

::: js/src/wasm/WasmInstance.cpp
@@ +695,5 @@
>      return -1;
>  }
>  
> +/* static */ void*
> +Instance::tableGet(Instance* instance, int32_t index)

nit: how about uint32_t index?

@@ +712,5 @@
> +{
> +    RootedObject obj(TlsContext.get(), (JSObject*)initValue);
> +    Table& table = *instance->tables()[0];
> +    MOZ_RELEASE_ASSERT(table.kind() == TableKind::AnyRef);
> +    if (delta < 0) {

Like Instance::growMemory_i32, the delta should be a uint32_t, I believe.

@@ +716,5 @@
> +    if (delta < 0) {
> +        JS_ReportErrorNumberASCII(TlsContext.get(), GetErrorMessage, nullptr, JSMSG_WASM_NEGATIVE_GROWTH);
> +        return -1;
> +    }
> +    uint32_t old_size = table.grow(uint32_t(delta), TlsContext.get());

nit: oldSize

@@ +719,5 @@
> +    }
> +    uint32_t old_size = table.grow(uint32_t(delta), TlsContext.get());
> +    if (old_size == uint32_t(-1)) {
> +        JS_ReportErrorNumberASCII(TlsContext.get(), GetErrorMessage, nullptr, JSMSG_WASM_BAD_GROW, "table");
> +        return -1;

memory.grow doesn't trap when grow fails, it just returns -1, so I think table.grow should do likewise.  That raises a maintenance hazard because some of these methods return -1 an internal error value that the compiled-code-caller will test-and-trap, while other methods return a -1 as a valid wasm result value.  It seems like we need some sort of syntactic convention + audit (ideally for all these Instance methods).  How about a comment after the return type saying, e.g., /* caller traps on (void*)-1 */ or /* infallible */ (if the caller doesn't branch on anything)?

@@ +730,5 @@
> +    return int32_t(old_size);
> +}
> +
> +/* static */ int32_t
> +Instance::tableSet(Instance* instance, int32_t index, void* value)

Similarly, uint32_t index.

Comment 17

[Lars T Hansen [:lth]]

Grumble, if `obj` is table-of-anyref then `obj.set(0, undefined)` dumps core because the ToObject operation throws yet we don't catch that properly.

Comment 18

[Lars T Hansen [:lth]]

(In reply to Luke Wagner [:luke] from comment #15)
> Comment on attachment 9022655 [details] [diff] [review]
> bug1450263-table-of-anyref-v2.patch
> 
> Review of attachment 9022655 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: js/src/wasm/WasmTable.cpp
> @@ +241,5 @@
> > +        // Realloc does not zero the delta for us.
> > +        PodZero(newArray + length_, delta);
> > +        length_ = newLength.value();
> > +    } else if (kind_ == TableKind::AnyRef) {
> > +        JS::Heap<JSObject*>* newArray = rt->pod_realloc(objectArray(), length_, newLength.value());
> 
> I think realloc of Heap<T>'s is problematic due to pre/post barriers
> embedding Heap<T>*'s in the store buffer, and this is the reason Heap<T> is
> marked as MOZ_NON_MEMMOVABLE.  Presumably you can ~Heap<T> the old and
> construct the new (I eternally wish there was a "realloc, but only if you
> can do it in-place" function).  It'd be nice to have a test for this that
> pounds on grow and storing objects and such.

I see.  And storing a null pointer in the cell removes the cell from the store buffer, so it's OK to delete the old array after destructing the elements individually.

However, I think this means (or might mean) we can't have FreePolicy just freeing the buffer when the Table is destructed; at a minimum we must destruct all the elements in it first.  And the values are no longer really POD, so pod_calloc is not really appropriate, though we can construct each element in turn when we alloc.

This is starting to sound like some data structure that already exists, or ought to exist, like GCVector :)

Comment 19

[Luke Wagner [:luke]]

Hah, yeah, GCVector sounds like the way to go here :)

Comment 20

[Lars T Hansen [:lth]]

(In reply to Luke Wagner [:luke] from comment #15)
> Comment on attachment 9022655 [details] [diff] [review]
> bug1450263-table-of-anyref-v2.patch
> 
> Review of attachment 9022655 [details] [diff] [review]:
> -----------------------------------------------------------------
> > ::: js/src/wasm/WasmTable.cpp
> @@ +241,5 @@
> > +        // Realloc does not zero the delta for us.
> > +        PodZero(newArray + length_, delta);
> > +        length_ = newLength.value();
> > +    } else if (kind_ == TableKind::AnyRef) {
> > +        JS::Heap<JSObject*>* newArray = rt->pod_realloc(objectArray(), length_, newLength.value());
> 
> I think realloc of Heap<T>'s is problematic ... It'd be nice to have a test for this that
> pounds on grow and storing objects and such.

TC part of the second patch here, coming shortly.  It did find a bug: GCVector<> is not right; GCVector<JS::Heap<>> is right.

Comment 21

[Lars T Hansen [:lth]]

Attachment: bug1450263-table-of-anyref-v3.patch

Table of anyref, updated.  The substantive change here is how we represent the table storage for table of anyref, since it needs proper tracing.  The function tables are left the way they were; this will change in the multiple-tables patch.

Comment 22

[Lars T Hansen [:lth]]

Attachment: bug1450263-table-operations-v3.patch

Table operations, updated; this should address all concerns and has a stress test case for table-of-anyref GC operations.

Comment 23

[Lars T Hansen [:lth]]

Comment on attachment 9024745 [details] [diff] [review]
bug1450263-table-of-anyref-v3.patch

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

Un-carrying r+ because there are some problems with the new representation; the Table method base() slipped through but can't work in its current form.

Comment 24

[Lars T Hansen [:lth]]

Comment on attachment 9024746 [details] [diff] [review]
bug1450263-table-operations-v3.patch

Removing r? until I'm sure I know what's going on.

Comment 25

[Lars T Hansen [:lth]]

Attachment: bug1450263-table-of-anyref-v4.patch

OK, the issue with the base pointer wasn't breaking anything but I've cleaned it up a little.  Still very small changes here apart from the GCVector for the object table.  Carrying r+ still.

Comment 26

[Lars T Hansen [:lth]]

Attachment: bug1450263-table-operations-v4.patch

Comment 27

[Luke Wagner [:luke]]

Comment on attachment 9024785 [details] [diff] [review]
bug1450263-table-operations-v4.patch

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

Great!  I really appreciate all the Instance method annotations.

::: js/src/js.msg
@@ +401,5 @@
>  MSG_DEF(JSMSG_WASM_DEREF_NULL,         0, JSEXN_WASMRUNTIMEERROR, "dereferencing null pointer")
>  MSG_DEF(JSMSG_WASM_BAD_RANGE ,         2, JSEXN_RANGEERR,    "bad {0} {1}")
>  MSG_DEF(JSMSG_WASM_BAD_GROW,           1, JSEXN_RANGEERR,    "failed to grow {0}")
> +MSG_DEF(JSMSG_WASM_TABLE_OUT_OF_BOUNDS, 0, JSEXN_RANGEERR,   "table index out of bounds")
> +MSG_DEF(JSMSG_WASM_NEGATIVE_GROWTH,    0, JSEXN_RANGEERR,    "attempting to grow table by negative amount")

You can kill the second error message now.

::: js/src/wasm/WasmInstance.cpp
@@ +715,5 @@
> +    MOZ_RELEASE_ASSERT(table.kind() == TableKind::AnyRef);
> +
> +    uint32_t oldSize = table.grow(delta, TlsContext.get());
> +    if (oldSize != uint32_t(-1) && initValue != nullptr) {
> +        for(uint32_t i=0; i < delta; i++) {

spacing nit: "i = 0"

Comment 28

[Lars T Hansen [:lth]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/01a7e880049ac624ea2b094898a1a888230e4884
Bug 1450263 - Basic generalized table type, part 1: table-of-anyref. r=luke

https://hg.mozilla.org/integration/mozilla-inbound/rev/5c97eca29dd76b2ea6a5e4146b685d6b5b821526
Bug 1450263 - Basic generalized table type, part 2: table.get/set/grow/size. r=luke

Comment 29

[Razvan Maries]

https://hg.mozilla.org/mozilla-central/rev/01a7e880049a
https://hg.mozilla.org/mozilla-central/rev/5c97eca29dd7

Comment 30

[Lars T Hansen [:lth]]

Note this is nightly-only until the feature has been fully specced.

Comment 31

[Chris Mills [:cmills]]

Since this is Nightly-only for now, I'll not add a note to the Fx65 rel notes.

It is not really very clear from this bug what has been added. LTH, can you give me quick summary of what the new developer-facing features are?

Comment 32

[Lars T Hansen [:lth]]

Several things have been added:

(1) New wasm feature, the ability to have multiple Table objects per Module (previously only one Table).  This means that instructions that already operate on tables are generalized (in a backward-compatible way) to be able to specify which table they operate on, both in the text and binary formats.  Only one instruction, "call_indirect", is affected.

(2) Tables of base type "anyref"; previously only "anyfunc" was allowed.  This adds a new error mode to "call_indirect" since it can now be applied to table-of-anyref; I believe it's a verification-time error for call_indirect to reference a table-of-anyref.

(3) New wasm instructions for operating on tables, roughly like this:

  <index-value> (table.get <table-index>) -> slot-value
  <index-value> <new-slot-value> (table.set <table-index>) -> void
  (table.size <table-index>) -> i32
  <delta-value> <slot-default> (table.grow <table-index>) -> old-size-value

The proposal that these bits are all part of is not quite stable yet, I think we're hoping to stabilize and (if we're lucky) let this ride the trains in 2019Q1.