1501201 - table.init should disregard the table index of the segment it's operating on

Status: RESOLVED FIXED

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

Description

[Lars T Hansen [:lth]]

From Instance::tableInit():

    MOZ_RELEASE_ASSERT(!seg.active());
    const Table& table = *instance->tables()[seg.tableIndex];

This happens to work because we only have table 0 at this point, but really, "seg.tableIndex" above should just be "0", we should never be looking at the segment's table index.

I am also thinking that passive segments should be required to have a table index of value zero, or some other fixed value (like -1).  Will have to read the specs more closely, and try to file a PR if necessary.  The spec needs real work anyhow, and we should try to push it along since we've spent the effort to implement.

Comment 1

[Lars T Hansen [:lth]]

I see that we do not read a tableIndex for passive segments, which seems like the right thing.  Now to correlate that to the spec...

Comment 2

[Lars T Hansen [:lth]]

Attachment: bug1501201-ignore-table-index.patch

Comment 3

[Lars T Hansen [:lth]]

The table at the bottom of https://github.com/WebAssembly/bulk-memory-operations/blob/30092f19832756cdd3a8b3de98fc66a9ec1dfe8c/proposals/bulk-memory-operations/Overview.md#design states that there is no index read for passive segments, so that's OK (even if I can't find anything like that in the executable spec at the moment).

Comment 4

[Julian Seward [:jseward]]

Comment on attachment 9019292 [details] [diff] [review]
bug1501201-ignore-table-index.patch

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

Ok to land.  Some part of me would prefer to leave it as-is and instead add
MOZ_ASSERT(seg.tableIndex == 0) so that we don't have to back out the change
should we ever move to a multi-table world.  But in that case we'd have to
revisit this and other table-init-y places anyway, so this is fine.

Comment 5

[Luke Wagner [:luke]]

Is this just an optimization?  Otherwise, validation criteria should ensure that tableIndex is a valid index into the Tables array and, when we have multi-table, the before-state will be what we need.

Comment 6

[Lars T Hansen [:lth]]

This is a temporary clarity fix only.  Multi-table will change this code, but IMO it is currently incorrect; the segment does not carry a table index.

Comment 7

[Lars T Hansen [:lth]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d4bfab51a097d87e78e6701b5efd9b474785755a
Bug 1501201 - do not look at table index of passive segments. r=jseward

Comment 8

[Arthur Iakab [arthur_iakab]]

https://hg.mozilla.org/mozilla-central/rev/d4bfab51a097