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

threads: add WasmCompositeInnerType layer #9520

Merged
merged 12 commits into from
Nov 2, 2024

Conversation

abrown
Copy link
Contributor

@abrown abrown commented Oct 29, 2024

As in wasm-tools, push the type down a layer so that WasmCompositeType can be marked shared. This leaves several holes to be filled in by future commits (all marked with TODO: handle shared); in effect, this change allows WasmCompositeType to be marked shared but mostly does not wire up the sharedness during translation.

As in `wasm-tools`, push the type down a layer so that
`WasmCompositeType` can be marked shared. This leaves several holes to
be filled in by future commits (all marked with `TODO: handle shared`);
in effect, this change allows `WasmCompositeType` to be marked shared
but mostly does not wire up the sharedness during translation.
@abrown abrown requested a review from a team as a code owner October 29, 2024 19:04
@abrown abrown requested review from alexcrichton and removed request for a team October 29, 2024 19:04
@github-actions github-actions bot added wasmtime:api Related to the API of the `wasmtime` crate itself wasmtime:ref-types Issues related to reference types and GC in Wasmtime labels Oct 29, 2024
Copy link

Subscribe to Label Action

cc @fitzgen

This issue or pull request has been labeled: "wasmtime:api", "wasmtime:ref-types"

Thus the following users have been cc'd because of the following labels:

  • fitzgen: wasmtime:ref-types

To subscribe or unsubscribe from this label, edit the .github/subscribe-to-label.json configuration file.

Learn more.

Copy link
Member

@alexcrichton alexcrichton left a comment

Choose a reason for hiding this comment

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

I talked about this a bit with @abrown yesterday but I wanted to paste the conclusion here as well. My general worry as we propagate this up further is that it's accidentally easy to forget to check .shared which means that further down the road we might have a bug of "oh forgot to check .shared there". I'd prefer to continue to panic/error in variuos places where shared isn't handled today and I've commented a few of them here and there.

crates/cranelift/src/gc/enabled.rs Outdated Show resolved Hide resolved
crates/cranelift/src/gc/enabled/drc.rs Outdated Show resolved Hide resolved
crates/environ/src/gc.rs Show resolved Hide resolved
crates/environ/src/types.rs Outdated Show resolved Hide resolved
crates/wasmtime/src/runtime/type_registry.rs Show resolved Hide resolved
crates/wasmtime/src/runtime/types.rs Outdated Show resolved Hide resolved
crates/wasmtime/src/runtime/vm/const_expr.rs Outdated Show resolved Hide resolved
crates/wasmtime/src/runtime/type_registry.rs Outdated Show resolved Hide resolved
crates/environ/src/compile/module_types.rs Show resolved Hide resolved
crates/environ/src/compile/module_types.rs Outdated Show resolved Hide resolved
crates/wasmtime/src/runtime/vm/gc/disabled.rs Outdated Show resolved Hide resolved
crates/environ/src/types.rs Outdated Show resolved Hide resolved
Comment on lines 812 to 829
assert!(!ty.composite_type.shared);
let gc_layout = match &ty.composite_type.inner {
wasmtime_environ::WasmCompositeInnerType::Func(_) => None,
wasmtime_environ::WasmCompositeInnerType::Array(a) => Some(
gc_runtime
.expect("must have a GC runtime to register array types")
.layouts()
.array_layout(a)
.into(),
),
wasmtime_environ::WasmCompositeInnerType::Struct(s) => Some(
gc_runtime
.expect("must have a GC runtime to register array types")
.layouts()
.struct_layout(s)
.into(),
),
};
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I ended up removing the refactor we used in this PR. Though indeed we could have done gc_runtime.map(...).flatten() and ended up with the same result, I'm concerned that @fitzgen probably is using these expects as an early warning sign for winch, e.g.

@abrown abrown enabled auto-merge November 2, 2024 00:35
@abrown abrown added this pull request to the merge queue Nov 2, 2024
Merged via the queue into bytecodealliance:main with commit 38845a0 Nov 2, 2024
40 checks passed
@abrown abrown deleted the composite-inner branch November 2, 2024 00:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
wasmtime:api Related to the API of the `wasmtime` crate itself wasmtime:ref-types Issues related to reference types and GC in Wasmtime
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants