Stream: git-wasmtime

Topic: wasmtime / PR #9520 threads: add `WasmCompositeInnerType...

Wasmtime GitHub notifications bot (Oct 29 2024 at 19:04):

abrown requested alexcrichton for a review on PR #9520.

Wasmtime GitHub notifications bot (Oct 29 2024 at 19:04):

abrown opened PR #9520 from abrown:composite-inner to bytecodealliance:main:

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.

<!--
Please make sure you include the following information:

• If this work has been discussed elsewhere, please include a link to that
  conversation. If it was discussed in an issue, just mention "issue #...".

• Explain why this change is needed. If the details are in an issue already,
  this can be brief.

Our development process is documented in the Wasmtime book:
https://docs.wasmtime.dev/contributing-development-process.html

Please ensure all communication follows the code of conduct:
https://github.com/bytecodealliance/wasmtime/blob/main/CODE_OF_CONDUCT.md
-->

Wasmtime GitHub notifications bot (Oct 29 2024 at 19:04):

abrown requested wasmtime-core-reviewers for a review on PR #9520.

Wasmtime GitHub notifications bot (Oct 29 2024 at 20:44):

github-actions[bot] commented on PR #9520:

Subscribe to Label Action

cc @fitzgen

<details>
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 <code>.github/subscribe-to-label.json</code> configuration file.

Learn more.
</details>

Wasmtime GitHub notifications bot (Oct 30 2024 at 15:12):

alexcrichton submitted PR review:

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.

Wasmtime GitHub notifications bot (Oct 30 2024 at 15:12):

alexcrichton created PR review comment:

This, and all other translation methods below, should I think return an error if the type is shared since we won't want to support that just yet.

Wasmtime GitHub notifications bot (Oct 30 2024 at 15:12):

alexcrichton created PR review comment:

Like gc/enabled.rs, this and below will want to panic/error on shared

Wasmtime GitHub notifications bot (Oct 30 2024 at 15:12):

alexcrichton created PR review comment:

For this accessor and those below I think we'll want to implicitly redefine is_func as is_unshared_func (although without the renaming for now). Eventually if needed we can add is_shared_func but otherwise these should all get extra boolean conditions and such to check if self.shared and act appropriately (e.g. return false, None, or panic!)

Wasmtime GitHub notifications bot (Oct 30 2024 at 15:12):

alexcrichton created PR review comment:

This I think we concluded might be able to be replaced with the trait helper method above.

Wasmtime GitHub notifications bot (Oct 30 2024 at 15:12):

alexcrichton created PR review comment:

I think this is correct to have, this is basically pattern-matching to only unshared structures for struct.new_default which is desired behavior at this time. This panic can then get weeded out in the future once support for struct.new_default comes around.

Wasmtime GitHub notifications bot (Oct 30 2024 at 15:12):

alexcrichton created PR review comment:

In this file I think it's fine to just set shared: false at the end. There'll be more plumbing eventually to handle this if needed but for now it's an accurate reflection that only unshared types are created here.

Wasmtime GitHub notifications bot (Oct 30 2024 at 15:12):

alexcrichton created PR review comment:

I think we'll want to panic on shared here

Wasmtime GitHub notifications bot (Oct 30 2024 at 15:12):

alexcrichton created PR review comment:

Here I think you'll want to propagate the shared-ness from self.types[for_func_ty] up above

Wasmtime GitHub notifications bot (Oct 30 2024 at 15:12):

alexcrichton created PR review comment:

Could this, and the matches below, continue to assert that the type is not-shared? I think the end state we'll want here for now is to introduce ConcreteShared{Array,Func,Struct}(...) but that might be best to do in a follow-up

Wasmtime GitHub notifications bot (Oct 30 2024 at 15:12):

alexcrichton created PR review comment:

I think this can probably be sub_ty.composite_type.shared to propagate the shared-ness?

Wasmtime GitHub notifications bot (Oct 30 2024 at 18:16):

abrown updated PR #9520.

Wasmtime GitHub notifications bot (Oct 30 2024 at 18:17):

abrown updated PR #9520.

Wasmtime GitHub notifications bot (Oct 30 2024 at 18:29):

abrown updated PR #9520.

Wasmtime GitHub notifications bot (Oct 30 2024 at 18:30):

abrown updated PR #9520.

Wasmtime GitHub notifications bot (Oct 30 2024 at 18:53):

abrown updated PR #9520.

Wasmtime GitHub notifications bot (Oct 30 2024 at 18:56):

abrown updated PR #9520.

Wasmtime GitHub notifications bot (Oct 30 2024 at 21:29):

abrown updated PR #9520.

Wasmtime GitHub notifications bot (Oct 30 2024 at 21:43):

abrown updated PR #9520.

Wasmtime GitHub notifications bot (Oct 30 2024 at 21:50):

abrown requested alexcrichton for a review on PR #9520.

Wasmtime GitHub notifications bot (Nov 01 2024 at 01:25):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Nov 01 2024 at 01:25):

alexcrichton created PR review comment:

Did this sneak in? Or was this to fix something else?

Wasmtime GitHub notifications bot (Nov 01 2024 at 01:25):

alexcrichton created PR review comment:

This line might be able to be deleted now?

Wasmtime GitHub notifications bot (Nov 01 2024 at 21:48):

abrown submitted PR review.

Wasmtime GitHub notifications bot (Nov 01 2024 at 21:48):

abrown created PR review comment:

This was due to the refactor in 8445481; in order to get rid of the duplication there we need to add the disabled version of this trait in bbce595.

Wasmtime GitHub notifications bot (Nov 02 2024 at 00:09):

abrown updated PR #9520.

Wasmtime GitHub notifications bot (Nov 02 2024 at 00:29):

abrown updated PR #9520.

Wasmtime GitHub notifications bot (Nov 02 2024 at 00:33):

abrown submitted PR review.

Wasmtime GitHub notifications bot (Nov 02 2024 at 00:33):

abrown created PR review comment:

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.

Wasmtime GitHub notifications bot (Nov 02 2024 at 00:34):

abrown submitted PR review.

Wasmtime GitHub notifications bot (Nov 02 2024 at 00:34):

abrown created PR review comment:

This is now gone as a result of fixing conflicts; some intervening commit started using an Option to solve this.

Wasmtime GitHub notifications bot (Nov 02 2024 at 00:35):

abrown has enabled auto merge for PR #9520.

Wasmtime GitHub notifications bot (Nov 02 2024 at 00:54):

abrown merged PR #9520.

Last updated: Nov 22 2024 at 17:03 UTC