Status

()

enhancement
P3
normal
RESOLVED FIXED
10 months ago
9 months ago

People

(Reporter: bbouvier, Assigned: lth)

Tracking

Trunk
mozilla63
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox63 fixed)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

10 months ago
Same work as bug 1450261, but for any Ref in addition to AnyRef (!).

Some attention will be required when setting value (to take type system into account). (Also please be mindful that the postbarrier code checking if the set location is tenured has never been tested and could be awfully wrong)
(Assignee)

Updated

9 months ago
Assignee: nobody → lhansen
Status: NEW → ASSIGNED
Priority: -- → P3
(Assignee)

Comment 1

9 months ago
I think this should actually be good enough, given my existing patches for struct types.  The verifier is already doing the type checking as part of its normal operation with push/pop, so all we need to do is clean up a few places that handle AnyRef but not Ref.  I'll look around for more places and I'll write some more interesting test cases, but I don't expect to find any more problems really.  Input is very welcome.
Attachment #8994565 - Flags: feedback?(bbouvier)
(Reporter)

Comment 2

9 months ago
Comment on attachment 8994565 [details] [diff] [review]
bug1472178-ref-in-wasm-globals.patch

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

Sweet. So there can't be pointer attributes within a user-defined struct type yet, right? (otherwise i'd expect changes in codegen)
With more tests (including ones for pre- and post-barriers) this would be good to land.

::: js/src/wasm/WasmValidate.cpp
@@ +1763,1 @@
>              return d.fail("expected anyref as type for ref.null");

nit: update error message
Attachment #8994565 - Flags: feedback?(bbouvier) → feedback+
(Assignee)

Comment 3

9 months ago
With more test cases for import/export and with update to the error message.
Attachment #8994565 - Attachment is obsolete: true
Attachment #8994793 - Flags: feedback+
(Assignee)

Updated

9 months ago
Depends on: 1478616
(Assignee)

Comment 4

9 months ago
The test cases are a little lame because we don't have struct.new et al yet, only null pointers, but they did uncover a bug so they're not worthless.  I've preserved the better test cases from the WIP patches and will land them later with the struct code.
Attachment #8994793 - Attachment is obsolete: true
Attachment #8995227 - Flags: review?(bbouvier)
(Reporter)

Comment 5

9 months ago
Comment on attachment 8995227 [details] [diff] [review]
bug1472178-ref-in-wasm-globals-v3.patch

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

Nice.
Attachment #8995227 - Flags: review?(bbouvier) → review+

Comment 6

9 months ago
Pushed by lhansen@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c47ad4f17e19
Ref types for wasm globals. r=bbouvier

Comment 7

9 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/c47ad4f17e19
Status: ASSIGNED → RESOLVED
Last Resolved: 9 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
You need to log in before you can comment on or make changes to this bug.