Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Consider functions in elems of public tables to be public #6660

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

kripken
Copy link
Member

@kripken kripken commented Jun 13, 2024

If a table is public (imported or exported) then any function added to it can be called
from the outside, so any change to the type might cause observable differences.

Noticed while fuzzing wasm-split (unsubtyping considered the type of such a function
to be private, modified it, and things broke).

@kripken kripken requested a review from tlively June 13, 2024 23:21
@tlively
Copy link
Member

tlively commented Jun 13, 2024

This expands the definition of a public type from one which appears in a module's own external type to any type that is externalized during runtime (including during instantiation). I think this is a bridge we do not want to cross because the set of externalized types is undecidable.

How exactly did this come up in fuzzing?

@kripken
Copy link
Member Author

kripken commented Jun 13, 2024

As a concrete example see the testcase in this PR: the type $some there used to be in its own singleton rec group, and the outside called it that way. Specifically the outside was the other split part of the module, which called it through the table. If unsubtyping was run then that type ended up in the new big rec group. The call through the table then failed.

@tlively
Copy link
Member

tlively commented Jun 13, 2024

Ah, right, the "correct" fix is to make it so that any subtype of a public type is also public when running with an open world. That's more conservative that what is implemented here, but importantly it is also decidable. (This was a known bug at some point but I never fixed it because we weren't working on open-world optimizations. There may be other related bugs that I've forgotten about...)

@kripken
Copy link
Member Author

kripken commented Jun 14, 2024

Hmm, we already consider declared supertypes of public types to be public. Adding subtypes as well seems to suggest that huge swaths of the type tree would quickly become public? E.g. in Java, a single public subtype of java.lang.Object would turn essentially all data types public. And for the example in this PR, a single table of type funcref (i.e. a normal table) would make all function types public.

Maybe that's just how limited open world is. In that case maybe it makes sense to just say that wasm-split isn't compatible with such optimizations?

@tlively
Copy link
Member

tlively commented Jun 14, 2024

I don't think subtypes of abstract heap types like func need to automatically be made public, but otherwise yes, this would make a huge number of types public. Ideally we would have a @private annotation that could be applied to particular subtypes of public types to mark the subtrees they root as private.

@kripken
Copy link
Member Author

kripken commented Jun 14, 2024

I don't think subtypes of abstract heap types like func need to automatically be made public

If not, then how would it solve the testcase in this PR? Or were you not suggesting that it would be enough to fix that, and additional work would be needed?

@tlively
Copy link
Member

tlively commented Jun 14, 2024

Good point. I guess we either have to be extra conservative and make subtypes of abstract heap types (i.e. all types) public or we would have to add a @public annotation as well for use in cases like this one.

@kripken
Copy link
Member Author

kripken commented Jun 14, 2024

Yeah, makes sense. Well, for now I think this is not urgent as it doesn't impact our ability to optimize closed world, which matters a lot more. I'll mark this PR as draft/not-to-land.

@kripken kripken marked this pull request as draft June 14, 2024 19:38
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.

2 participants