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

smith: Fix types recorded in ref.func #1965

Closed

Conversation

alexcrichton
Copy link
Member

This commit fixes a minor issue in wasm-smith when generating the ref.func instruction when the GC proposal is disabled but shared-everything-threads is enabled. In this situation all ref.func instructions were recorded as RefType::FUNCREF but this wasn't correct for shared function types. The logic here is applicable both with and without the GC proposal is enabled so the precise type is always pushed now.

This commit fixes a minor issue in `wasm-smith` when generating the
`ref.func` instruction when the GC proposal is disabled but
shared-everything-threads is enabled. In this situation all `ref.func`
instructions were recorded as `RefType::FUNCREF` but this wasn't correct
for shared function types. The logic here is applicable both with and
without the GC proposal is enabled so the precise type is always pushed
now.
It's no longer present on `main`
@alexcrichton alexcrichton requested a review from fitzgen January 8, 2025 17:45
@fitzgen
Copy link
Member

fitzgen commented Jan 13, 2025

when the GC proposal is disabled but shared-everything-threads is enabled

But shared-everything extends the GC proposal, so I don't think we should even allow this set up. It's like disabling bulk memory but enabling reference types.

Comment on lines +941 to +949
// If GC is disabled then favor the "top" type of the input type.
if !self.config.gc_enabled {
return Ok(ty);
return Ok(match ty {
HeapType::Concrete(i) => {
let CompositeType { inner, shared } = &self.ty(i).composite_type;
let ty = match inner {
CompositeInnerType::Array(_) => AbstractHeapType::Array,
CompositeInnerType::Struct(_) => AbstractHeapType::Struct,
CompositeInnerType::Func(_) => AbstractHeapType::Func,
Copy link
Member

Choose a reason for hiding this comment

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

The only case that this would be valid for is (ref null? func). If GC is disabled, then struct and array references will still be invalid.

Which again points me towards this trying to do something that doesn't make sense...

alexcrichton added a commit to alexcrichton/wasm-tools that referenced this pull request Jan 13, 2025
@alexcrichton
Copy link
Member Author

Seems reasonable to apply that fix too yeah. Personally the two proposals are only loosely coupled in my head because we'll probably want pretty separate gates/paths for both, but it doesn't really matter in the interim.

It's like disabling bulk memory but enabling reference types.

Technically we do allow this, but we don't allow disabling reference types and enabling gc. Or at least not in wasm-smith, I think we allow that in wasmparser.

The only case that this would be valid for is (ref null? func). If GC is disabled, then struct and array references will still be invalid.

True, but if GC is disabled, then the struct/array stuff won't show up. I gambled that an insertion of => unreachable!() would have fewer reviewer comments and lost heh

github-merge-queue bot pushed a commit that referenced this pull request Jan 13, 2025
* smith: Disable shared-everything-threads without GC

Alternative fix from #1965

* Review comments
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