Stream: git-wasmtime

Topic: wasmtime / PR #8450 Wasmtime(gc): Support `(ref.i31 (glob...

Wasmtime GitHub notifications bot (Apr 23 2024 at 21:29):

fitzgen opened PR #8450 from fitzgen:i31-global-initializers to bytecodealliance:main:

We had something hacked together to support (ref.i31 (i32.const N)). It wasn't a long-term solution. This is the first time that we have to really deal with multi-instruction const expressions.

This commit introduces a tiny interpreter to evaluate const expressions.

<!--
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 (Apr 23 2024 at 21:29):

fitzgen requested abrown for a review on PR #8450.

Wasmtime GitHub notifications bot (Apr 23 2024 at 21:29):

fitzgen requested wasmtime-compiler-reviewers for a review on PR #8450.

Wasmtime GitHub notifications bot (Apr 23 2024 at 21:29):

fitzgen requested alexcrichton for a review on PR #8450.

Wasmtime GitHub notifications bot (Apr 23 2024 at 21:29):

fitzgen requested wasmtime-core-reviewers for a review on PR #8450.

Wasmtime GitHub notifications bot (Apr 23 2024 at 21:29):

fitzgen requested wasmtime-default-reviewers for a review on PR #8450.

Wasmtime GitHub notifications bot (Apr 23 2024 at 21:30):

fitzgen updated PR #8450.

Wasmtime GitHub notifications bot (Apr 23 2024 at 22:05):

fitzgen updated PR #8450.

Wasmtime GitHub notifications bot (Apr 23 2024 at 22:19):

alexcrichton submitted PR review:

Looks good to me, I think the organization worked out well here.

My main comment is that the unsafe methods feel a little too unsafe and while I don't think there's a whole lot that can be done about that I still think it would be good to at least assert that Concrete heap types are functions for now to avoid accidental confusion in the future.

Wasmtime GitHub notifications bot (Apr 23 2024 at 22:19):

alexcrichton created PR review comment:

If this .to_ne_bytes() is required I think that means we're doing something wrong in ValRaw, perhaps a missing from_le_bytes in get_v128?

No matter what if this is the only one that does .to_ne_bytes() is should have docs as to why it's omitted on the others.

Wasmtime GitHub notifications bot (Apr 23 2024 at 22:19):

alexcrichton submitted PR review:

Looks good to me, I think the organization worked out well here.

My main comment is that the unsafe methods feel a little too unsafe and while I don't think there's a whole lot that can be done about that I still think it would be good to at least assert that Concrete heap types are functions for now to avoid accidental confusion in the future.

Wasmtime GitHub notifications bot (Apr 23 2024 at 22:19):

alexcrichton created PR review comment:

Bit of a side comment, but I get a little uncomfortable with Concrete(_) here since it seems very easy to overlook this in the future when the concrete type may be a function or may be a struct. Is it possible to perhaps restructure this or add an assert?

It seems especially relevant here and above given how unsafe the methods are

Wasmtime GitHub notifications bot (Apr 23 2024 at 22:19):

alexcrichton created PR review comment:

Do you think we can structure this as an enum or similar? E.g. a .classify() returning an enum of some kind. The differences between these functions feels really subtle and easy to get wrong, and getting it wrong feels like it could lead to unsafety pretty easily

Wasmtime GitHub notifications bot (Apr 23 2024 at 22:19):

alexcrichton created PR review comment:

8 seems pretty big here given how often we'll be storing this, could this perhaps be tuned to be the native size of SmallVec?

Wasmtime GitHub notifications bot (Apr 23 2024 at 22:32):

fitzgen submitted PR review.

Wasmtime GitHub notifications bot (Apr 23 2024 at 22:32):

fitzgen created PR review comment:

WasmHeapType::Extern

Wasmtime GitHub notifications bot (Apr 23 2024 at 22:32):

fitzgen edited PR review comment.

Wasmtime GitHub notifications bot (Apr 23 2024 at 23:40):

fitzgen submitted PR review.

Wasmtime GitHub notifications bot (Apr 23 2024 at 23:40):

fitzgen created PR review comment:

I was just doing this to get a [u8; 16] but it turns out that isn't necessary: we have a method on wasmtime_runtime::Global to get the value as a &mut u128, which I didn't see before.

Wasmtime GitHub notifications bot (Apr 23 2024 at 23:43):

fitzgen submitted PR review.

Wasmtime GitHub notifications bot (Apr 23 2024 at 23:43):

fitzgen created PR review comment:

Unfortunately there isn't a good way to resolve this, given the wasmtime vs wasmtime-runtime crate split. We would need to take a wasmtime::Engine here and then look up the type index in the engine's types registry and match on the resulting value (which can only be a function type right now). But we can't get an engine inside the wasmtime-runtime crate.

Wasmtime GitHub notifications bot (Apr 24 2024 at 14:36):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Apr 24 2024 at 14:36):

alexcrichton created PR review comment:

We have to solve this in the near future though right?

Originally I was hoping we could have Concrete renamed to ConcreteFunc and then also have Concrete{Struct,Array} or something like that, but I'm not sure if that's still possible.

Do you have other ideas though on what to do here once arrays/structs are implemented?

Wasmtime GitHub notifications bot (Apr 24 2024 at 15:00):

fitzgen submitted PR review.

Wasmtime GitHub notifications bot (Apr 24 2024 at 15:00):

fitzgen created PR review comment:

My plan was to start passing in a function to these sorts of methods that need to do the index-to-type resolution. Between that, fuzzing, and using LSP to find all uses of Concrete(_) I wasn't too too worried about missing a spot to update.

The other thing we could do is try moving the type registry into wasmtime-runtime.

Wasmtime GitHub notifications bot (Apr 24 2024 at 15:21):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Apr 24 2024 at 15:21):

alexcrichton created PR review comment:

Even without a const fn could this perhaps be smaller to start? A ConstOp is 32 bytes large and 8 of them inline here is 256 bytes for a single expression which seems pretty egregious especially if a table is initialized with a list of constant expressions.

I'm basically pretty fearful of the impact of using this everywhere when the representation is 8x larger than it is today

Wasmtime GitHub notifications bot (Apr 24 2024 at 15:25):

alexcrichton created PR review comment:

I may be blowing things out of proportion here perhaps, but I'm personally pretty worried about this. I just ran across another instance of where type information will be needed as well.

I'm basically pretty worried that we've pretty deeply baked in the assumption that "Concrete is always a function" and I'd like to avoid baking it in any further, especially here where it's necessary for safety.

I suppose for the sake of this PR could you add an assertion to the constructors of Concrete that it's always a function? (or double-check such an assertion already exists?) More-or-less we need to re-audit all usages of Concrete when the GC implementation gets further along.

Wasmtime GitHub notifications bot (Apr 24 2024 at 15:25):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Apr 24 2024 at 15:25):

fitzgen submitted PR review.

Wasmtime GitHub notifications bot (Apr 24 2024 at 15:25):

fitzgen created PR review comment:

Yeah I dropped it to 2 locally, just haven't pushed the updates yet until I finish addressing the other comments.

Wasmtime GitHub notifications bot (Apr 24 2024 at 19:27):

fitzgen submitted PR review.

Wasmtime GitHub notifications bot (Apr 24 2024 at 19:27):

fitzgen created PR review comment:

(We talked offline about this and decided to just rename the predicates).

Wasmtime GitHub notifications bot (Apr 24 2024 at 19:30):

fitzgen submitted PR review.

Wasmtime GitHub notifications bot (Apr 24 2024 at 19:30):

fitzgen created PR review comment:

(We talked offline about this and decided that renaming Concrete(_) to ConcreteFunc(_) actually is okay, and will do it in a future PR. It wasn't workable for wasmparser, since that crate needs to be able to parse opcodes without having a validated types section where we can look things up in and determine whether we are talking about a concrete func or e.g. struct, but that constraint doesn't apply here.)

Wasmtime GitHub notifications bot (Apr 24 2024 at 19:37):

fitzgen updated PR #8450.

Wasmtime GitHub notifications bot (Apr 24 2024 at 19:38):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Apr 24 2024 at 19:40):

fitzgen has enabled auto merge for PR #8450.

Wasmtime GitHub notifications bot (Apr 24 2024 at 20:23):

fitzgen merged PR #8450.

Last updated: Nov 22 2024 at 17:03 UTC