Use ImportResolver for tag imports #8254
Conversation
stevenfontanella
force-pushed
the
tag-import
branch
from
f67ef57 to
04ab34d
Compare
stevenfontanella
force-pushed
the
tag-import
branch
from
d5af3e5 to
f050e53
Compare
stevenfontanella
changed the title
| 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.
|
|
||
| class Tag : public Importable { | ||
| public: | ||
| // TODO: This should ideally be const. |
There was a problem hiding this comment.
The immediate reason in this PR is that the ImportResolver returns a Tag*, so the importer would be free to mutate the type which would be wrong. We could have the ImportResolver return a const Tag* but that would prevent the caller from mutating other things within Tag if that ever makes sense in the future (probably not, but it makes more sense for the other types of imports like memories, tables and globals and return non-const pointers from ImportResolver).
Besides this PR, the classes in this file (Tag, Table, Memory, etc.) represent definitions and should never change, e.g. a Table's initial size never changes, only its runtime size changes. So I think most fields in these classes should be const.
There was a problem hiding this comment.
These classes represent the Wasm in the binary format, though, and optimizations may want to alter them.
For example, an optimization might actually want to reduce a Table's initial size - say it proves to itself that a smaller initial size is 100% safe and unnoticeable at runtime, then using less is just more efficient.
Part of #8180