Closed Bug 1546592 Opened 7 months ago Closed 29 days ago

Support anyref-typed passive element segments and table.init on those

Categories

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

enhancement

Tracking

()

RESOLVED FIXED
mozilla71
Tracking Status
firefox71 --- fixed

People

(Reporter: lth, Assigned: lth)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

Currently all our element segments have type funcref, but they can be type-limited to anyref and they should still be usable to initialize table-of-anyref using table.init.

For sure we need to fix the text->binary parser (this should be easy after bug 1546074 lands) but I expect it cuts deeper than that.

I expect the values in the segment will still be limited to ref.func and ref.null values.

I'll investigate if the reftypes proposal really requires this, and whether we have tests.

Assignee: nobody → lhansen
Depends on: 1546074

This is required, though the spec interpreter in the reference-types repo does not yet support it. See https://github.com/WebAssembly/reference-types/blob/master/proposals/reference-types/Overview.md#language-extensions and search for the next occurrence of table.init.

My thinking so far:

We can allow "anyref" in addition to "funcref" in the text syntax, but it's sugar, it basically means the same: the element values in the text must be reference expressions, they are not implicit function references. (If they're all function references they will still be encoded compactly.) The type name disappears during the encoding -- there's no space for it in the binary representation of the element segment at the moment. After all a (ref.func n) can be upcast to an anyref, so with only funcref and anyref as the option it won't matter. All the interesting type checking happens in table.init anyway.

But this opens a question about whether the element segment should carry a type in the future, to express more interesting function types at least. There are flag bits available in the element segment encoding so this seems doable. And then the reader of the element segment must check that each expression type conforms to the encoded type.

In readMemOrTableInit() there's this ominous part of a comment:

    // ... and funcref is not yet a subtype of anyref.

If we've implemented ref.func properly then that should not be true, so that needs investigation. Anyway, the table can be of type anyref or funcref here, but since the element segment can contain only ref.func and ref.null, the type checking should not be very interesting.

(In reply to Lars T Hansen [:lth] from comment #3)

But this opens a question about whether the element segment should carry a type in the future, to express more interesting function types at least. There are flag bits available in the element segment encoding so this seems doable. And then the reader of the element segment must check that each expression type conforms to the encoded type.

I may be misreading this, but don't element segments already contain a type? We just currently restrict it to funcref. [1]

[1] https://searchfox.org/mozilla-central/rev/01d1011ca4a460f751da030d455d35c267c3e210/js/src/wasm/WasmValidate.cpp#2395

In readMemOrTableInit() there's this ominous part of a comment:

    // ... and funcref is not yet a subtype of anyref.

If we've implemented ref.func properly then that should not be true, so that needs investigation. Anyway, the table can be of type anyref or funcref here, but since the element segment can contain only ref.func and ref.null, the type checking should not be very interesting.

That is an odd comment. It looks like our validation subtype algorithm should imply funcref <: anyref [2].

[2] https://searchfox.org/mozilla-central/rev/01d1011ca4a460f751da030d455d35c267c3e210/js/src/wasm/WasmValidate.h#229

(In reply to Ryan Hunt [:rhunt] from comment #4)

(In reply to Lars T Hansen [:lth] from comment #3)

But this opens a question about whether the element segment should carry a type in the future, to express more interesting function types at least. There are flag bits available in the element segment encoding so this seems doable. And then the reader of the element segment must check that each expression type conforms to the encoded type.

I may be misreading this, but don't element segments already contain a type? We just currently restrict it to funcref. [1]

Yes, my comment is wrong, it was based on my reading the Overview (stale) rather than the spec or the issues that will be incorporated in the spec.

Really this is a more general bug; I run into some assertions when I beef up the test cases and I think we have some holes here.

Initial patch; this has new test cases and passes them all. I still need to vet this against the official semantics and test cases, if there are any, and I'll consider more test cases in general. But the changes here feel natural:

  • allow element segments to be 'anyref' with ref.null and ref.func initializer expressions (both active and passive forms)
  • allow table-of-anyref to be initialized from segment-of-anyref (using active or passive initialization)
  • allow table-of-anyref to be initialized from segment-of-funcref, ditto
  • disallow table-of-funcref to be initialized from segment-of-anyref, ditto

A natural consequence is that a ref.func initializer in a segment-of-anyref makes the function reachable by a ref.func operator in the code, just as if the segment were a segment-of-funcref.

The patch is not big but there are many fiddly bits where we previously had some assumptions that are no longer true.

Attachment #9099860 - Attachment description: Bug 1546592 - Allow element segments to be 'anyref', allow table.init to use them. → Bug 1546592 - Allow element segments to be 'anyref', and generalize. r?rhunt
Attachment #9099860 - Attachment description: Bug 1546592 - Allow element segments to be 'anyref', and generalize. r?rhunt → Bug 1546592 - Allow element segments to be 'anyref', and generalize. r=rhunt
Pushed by lhansen@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/81babd897f9e
Allow element segments to be 'anyref', and generalize. r=rhunt
Status: NEW → RESOLVED
Closed: 29 days ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla71
You need to log in before you can comment on or make changes to this bug.