Skip to content

Use ImportResolver for tag imports#8254

Open
stevenfontanella wants to merge 8 commits intomainfrom
tag-import
Open

Use ImportResolver for tag imports#8254
stevenfontanella wants to merge 8 commits intomainfrom
tag-import

Conversation

@stevenfontanella
Copy link
Member

@stevenfontanella stevenfontanella commented Jan 29, 2026

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).

@stevenfontanella stevenfontanella changed the title Refactor tag / tag import logic Use ImportResolver for tag imports Feb 3, 2026
@stevenfontanella stevenfontanella marked this pull request as ready for review February 3, 2026 04:43
virtual RuntimeTable* getTableOrNull(ImportNames name,
const Table& type) const = 0;

virtual Tag* getTagOrNull(ImportNames name, const Signature& type) const = 0;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, I think that will clarify things 👍

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants