Conversation
Literals could previously represent externalized internal references and strings, but they could not represent externref values that actually represent external references. Fix this by using the `i32` field to hold externref payloads in the case where they are not externalized references or strings. Strings literals are already differentiated because they have type `(ref string)`, but that still leaves external references and externalized internal references to be differentiated. They were previously differentiated using the `type` field in GCData, but that field was otherwise redundant wit the `type` field in the outer Literal. Remove `type` from GCData and instead use the low bit of the i32/shared_ptr<GCData> union to mark external references. This depends on `shared_ptr<GCData>` not using that low bit.
src/literal.h
Outdated
| } | ||
| if (type.isMaybeShared(wasm::HeapType::any)) { | ||
| // This may be an extern string that was internalized to |any|. Undo | ||
| // This may be an extern string that was 9d to |any|. Undo |
There was a problem hiding this comment.
I don't even have a cat to blame it on 😿
src/literal.h
Outdated
| // | ||
| // Externref payloads are also stored here, with their low bit set to | ||
| // differentiate an externref with a payload from an externalized internal | ||
| // reference, which uses the gcData field instead. |
There was a problem hiding this comment.
Does "here" mean the i32 field?
Why can we assume externrefs can be stored as i32? (or really i31 as you reserve one bit?)
There was a problem hiding this comment.
For the optimizer, externrefs are totally opaque and only their identities matter, so i32s (or i31s, or i63s if we use the i64 field) are sufficient. For the spec tests, externrefs are conveniently already represented as values like ref.extern 42, so this is exactly what we need. For the fuzzer, I plan to import JS objects that look like { payload: 42 } and have fuzzing imports that can log those payloads. The fuzzing interpreter will similarly log the interpreted externref payloads.
There was a problem hiding this comment.
I see, then (1) let's document those limitations, but also (2) if spec tests have ref.extern 42 then don't we need to represent any integer? (i.e. we can't give up a bit, and should it be 32 or 64 bits?)
There was a problem hiding this comment.
I'm pretty sure it won't matter in practice. If we do encounter issues, we can just switch to using the i64 field of the union instead. (Or I could eagerly do that now, but I think using the i32 field is a more naturally first choice.)
There was a problem hiding this comment.
Ok, sgtm to use i32, but please document all this in this header.
| int32_t getPayload() const { | ||
| assert(hasPayload()); | ||
| return int32_t(uint32_t(i32) >> 1); | ||
| } |
There was a problem hiding this comment.
Perhaps hasExternrefPayload / getExternrefPayload etc., to clarify this isn't a generic payload?
| } | ||
| case HeapType::any: | ||
| // Internalized external reference. | ||
| new (&gcData) std::shared_ptr<GCData>(other.gcData); |
There was a problem hiding this comment.
Can we keep the code below that clarifies this is a string?
There was a problem hiding this comment.
I can expand the comment, but the gcData no longer has a type field for an assertion to look at. I could assert gcData.values[0].type.getHeapType().isMaybeShared(HeapType::ext), but I don't think it's worth it.
| assert(data->values.size() == 1); | ||
| o << "internalized " << literal.getGCData()->values[0]; | ||
| break; | ||
| } |
There was a problem hiding this comment.
Why did this change from printing a string?
There was a problem hiding this comment.
Anyref literals are now either internalized strings or other internalized extern references. We don't always have a string to print.
kripken
left a comment
kripken
left a comment
There was a problem hiding this comment.
lgtm, but not hitting approve as there is automerge and I would fuzz this one...
|
Feel free to disable my auto-merges! There should be a button for it. |
|
Fuzzer looks happy. |
2 participants
Literals could previously represent externalized internal references and
strings, but they could not represent externref values that actually
represent external references. Fix this by using the
i32field to holdexternref payloads in the case where they are not externalized
references or strings. Strings literals are already differentiated
because they have type
(ref string), but that still leaves externalreferences and externalized internal references to be differentiated.
They were previously differentiated using the
typefield in GCData,but that field was otherwise redundant wit the
typefield in the outerLiteral. Remove
typefrom GCData and instead use the low bit of thei32/shared_ptr union to mark external references. This depends
on
shared_ptr<GCData>not using that low bit.