Conversation
f67ef57 to
04ab34d
Compare
d5af3e5 to
f050e53
Compare
| virtual RuntimeTable* getTableOrNull(ImportNames name, | ||
| const Table& type) const = 0; | ||
|
|
||
| virtual Tag* getTagOrNull(ImportNames name, const Signature& type) const = 0; |
There was a problem hiding this comment.
We're also going to need some kind of RuntimeTag abstraction, since multiple instances of the same modules generate distinct instantiations of the same tag definition.
Would that be better handled now or in a follow-up?
There was a problem hiding this comment.
This PR implements the correct behavior when there are multiple instances of the same tag definition. I added instance.wast which passes from this PR but not from main (not for the whole spec-test run including transformation but with bin/wasm-shell test/spec/instance.wast).
The 'runtime tag' is basically Tag* right now since we use pointer comparison. Otherwise it could be a RuntimeTag class that contains a Tag or Tag*. It gets a little confusing because we could return RuntimeTag* and do pointer comparison on that (and the class either has Tag or Tag*), or we could return RuntimeTag and do pointer comparison on a Tag* field that it has. The extra layer of wrapping is strange to me so I decided to keep it as Tag* even though it doesn't look like a 'runtime' object like the other cases. WDYT?
There was a problem hiding this comment.
Basically a runtime tag is nothing more than an identity/symbol so currently it makes sense to me to implement this as a Tag*, but I'm not sure if there are other features in the future that might change this.
There was a problem hiding this comment.
Based on Alon's comment, I think I'd add a RuntimeTag type with a const Tag* const as a member, does that sound good?
There was a problem hiding this comment.
Thanks, I think that will clarify things 👍
There was a problem hiding this comment.
I'll continue adding that in the next PR to keep the footprint lower if that sounds ok. Started it here.
| // internal name. | ||
| std::vector<Literals> definedGlobals; | ||
| std::vector<std::unique_ptr<RuntimeTable>> definedTables; | ||
| std::vector<Tag> definedTags; |
There was a problem hiding this comment.
Why do we need std::unique_ptr for definedTables but not for definedGLobals or definedTags? Or could we get rid of the unique ptr in a separate change?
There was a problem hiding this comment.
definedTables are unique_ptrs because RuntimeTable is a virtual type. It also makes a little more sense for tables than for globals and tags since tables are more like 'control' while globals and tags are more like plain data (setting aside what we discussed about tags having an identity).
We could potentially remove the indirection by changing it to RealRuntimeTable but it isn't possible right now because we inject a fake RuntimeTable in ctor eval: link. Once we resolve the TODO mentioned there, we could change it. On that note: what's the reasoning for wanting to remove the unique_ptr here? Is it speed or something else?
There was a problem hiding this comment.
Makes sense, thanks. My motivation for asking is that I think it would be nice to be as uniform as possible with how we treat the different kinds of module fields and their corresponding runtime objects here. So I would have expected the vector of defined tags to look very much like the vector of defined tables, i.e. both or neither using unique_ptrs. (This is also why I would expect the tags to be represented by RuntimeTag objects containing pointers to the instantiated Tag definitions, just like RuntimeTable has a pointer to a Table.)
I would go as far as to say that RuntimeTag doesn't need to support an equality operator or do anything except wrap a Tag*. We can tell if two RuntimeTags are the same if they physically have the same address.
Part of #8180 and #8261. Fixes the semantics/spec test when the same tag is imported in different instances, in which case the tag should behave as a new identity, which was previously not the case (see the tags in the modified instance.wast in this PR).