Closed Bug 1450263 Opened 2 years ago Closed 1 year ago

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

Categories

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

enhancement

Tracking

()

RESOLVED FIXED
mozilla65
Tracking Status
firefox65 --- fixed

People

(Reporter: bbouvier, Assigned: lth)

References

(Blocks 1 open bug)

Details

(Keywords: dev-doc-needed)

Attachments

(2 files, 8 obsolete files)

No description provided.
Priority: -- → P1
Component: JavaScript Engine: JIT → Javascript: Web Assembly
Does not block Milestone 1.
No longer blocks: 1444925
Blocks: 1488199
Assignee: nobody → lhansen
Status: NEW → ASSIGNED
Depends on: 1496582
Summary: wasm: Add anyref support in tables → Basic generalized tables: add table-of-anyref, table.get, table.set, table.grow
Part 1: table-of-anyref + changes to existing operations + test cases
Depends on: 1500120
Table of anyref and related goodies but no instructions yet.  See commit comment for link to (evolving) spec.
Attachment #9018153 - Attachment is obsolete: true
Attachment #9018328 - Flags: review?(luke)
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.)
Attachment #9018329 - Flags: review?(luke)
Blocks: 1500167
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).
Attachment #9018329 - Attachment is obsolete: true
Attachment #9018329 - Flags: review?(luke)
Attachment #9018543 - Flags: review?(luke)
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.
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.
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.)
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 on attachment 9018328 [details] [diff] [review]
bug1450263-table-of-anyref-part-1-v2.patch

Cancelling review for now; found some minor issues.
Attachment #9018328 - Flags: review?(luke)
Comment on attachment 9018543 [details] [diff] [review]
bug1450263-table-of-anyref-part-2-v2.patch

Cancelling review for now, found some bugs.
Attachment #9018543 - Flags: review?(luke)
Attachment #9018328 - Attachment is obsolete: true
Attachment #9022655 - Flags: review?(luke)
Attachment #9018543 - Attachment is obsolete: true
Attachment #9022656 - Flags: review?(luke)
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 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.
Attachment #9022655 - Flags: review?(luke) → review+
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.
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.
(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 :)
Hah, yeah, GCVector sounds like the way to go here :)
(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.
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.
Attachment #9022655 - Attachment is obsolete: true
Attachment #9024745 - Flags: review+
Table operations, updated; this should address all concerns and has a stress test case for table-of-anyref GC operations.
Attachment #9022656 - Attachment is obsolete: true
Attachment #9022656 - Flags: review?(luke)
Attachment #9024746 - Flags: review?(luke)
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.
Attachment #9024745 - Flags: review+
Comment on attachment 9024746 [details] [diff] [review]
bug1450263-table-operations-v3.patch

Removing r? until I'm sure I know what's going on.
Attachment #9024746 - Flags: review?(luke)
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.
Attachment #9024745 - Attachment is obsolete: true
Attachment #9024784 - Flags: review+
Attachment #9024746 - Attachment is obsolete: true
Attachment #9024785 - Flags: review?(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"
Attachment #9024785 - Flags: review?(luke) → review+
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
https://hg.mozilla.org/mozilla-central/rev/01a7e880049a
https://hg.mozilla.org/mozilla-central/rev/5c97eca29dd7
Status: ASSIGNED → RESOLVED
Closed: 1 year ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla65
Note this is nightly-only until the feature has been fully specced.
Keywords: dev-doc-needed
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?
Flags: needinfo?(lhansen)
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.
Flags: needinfo?(lhansen)
You need to log in before you can comment on or make changes to this bug.