github-actions[bot] commented on issue #5288:
Subscribe to Label Action
cc @peterhuene
<details>
This issue or pull request has been labeled: "cranelift", "cranelift:wasm", "wasmtime:api", "wasmtime:config"Thus the following users have been cc'd because of the following labels:
- peterhuene: wasmtime:api
To subscribe or unsubscribe from this label, edit the <code>.github/subscribe-to-label.json</code> configuration file.
Learn more.
</details>
github-actions[bot] commented on issue #5288:
Label Messager: wasmtime:config
It looks like you are changing Wasmtime's configuration options. Make sure to
complete this check list:
[ ] If you added a new
Config
method, you wrote extensive documentation for
it.<details>
Our documentation should be of the following form:
```text
Short, simple summary sentence.More details. These details can be multiple paragraphs. There should be
information about not just the method, but its parameters and results as
well.Is this method fallible? If so, when can it return an error?
Can this method panic? If so, when does it panic?
Example
Optional example here.
```</details>
[ ] If you added a new
Config
method, or modified an existing one, you
ensured that this configuration is exercised by the fuzz targets.<details>
For example, if you expose a new strategy for allocating the next instance
slot inside the pooling allocator, you should ensure that at least one of our
fuzz targets exercises that new strategy.Often, all that is required of you is to ensure that there is a knob for this
configuration option in [wasmtime_fuzzing::Config
][fuzzing-config] (or one
of its nestedstruct
s).Rarely, this may require authoring a new fuzz target to specifically test this
configuration. See [our docs on fuzzing][fuzzing-docs] for more details.</details>
[ ] If you are enabling a configuration option by default, make sure that it
has been fuzzed for at least two weeks before turning it on by default.[fuzzing-config]: https://github.com/bytecodealliance/wasmtime/blob/ca0e8d0a1d8cefc0496dba2f77a670571d8fdcab/crates/fuzzing/src/generators.rs#L182-L194
[fuzzing-docs]: https://docs.wasmtime.dev/contributing-fuzzing.html
<details>
To modify this label's message, edit the <code>.github/label-messager/wasmtime-config.md</code> file.
To add new label messages or remove existing label messages, edit the
<code>.github/label-messager.json</code> configuration file.</details>
bjorn3 commented on issue #5288:
call_ref is simply translated as a cranelift indirect_call. A faster cranelift indirect call that does not check the callee signature can (and should) eventually be added and used. Further, no null check is elided.
call_indirect doesn't check the function signature, nor does it check for null pointers.
dhil commented on issue #5288:
I'm interested in resuming work on this patch and dedicating the necessary time to bring it into an acceptable state. I have a patch which merges this PR with the recent changes to
main
(c.f. effect-handlers/wasmtime#13). I will merge that patch into this PR later today.I think the current status is as follows:
- Compatible with latest wasm-tools
- All new function reference instructions have been implemented (save for
return_call_ref
) (tested on x86_64).- 10 test case failures:
failures: wast::Cranelift::spec::function_references::br_table wast::Cranelift::spec::function_references::br_table_pooling wast::Cranelift::spec::function_references::linking wast::Cranelift::spec::function_references::linking_pooling wast::Cranelift::spec::function_references::return_call_ref wast::Cranelift::spec::function_references::return_call_ref_pooling wast::Cranelift::spec::function_references::table wast::Cranelift::spec::function_references::table_pooling wast::Cranelift::spec::function_references::type_equivalence wast::Cranelift::spec::function_references::type_equivalence_pooling test result: FAILED. 872 passed; 10 failed; 10 ignored; 0 measured; 0 filtered out; finished in 77.91s
Here
return_call_ref
is expected to fail as we have not implemented the tail calls proposal. I haven't worked out whylinking
andtype_equivalence
fail yet. The testbr_table
fails because of how thewast
crate ofwasm-tools
elaborates single element tables(module (type $t (func)) (func $tf) (table $t (ref null $t) (elem $tf)))
The
table
test fails due to the incompleteness of the type reconstruction procedurewasmtime::values::Val::ty()
. The procedure has to infer the nullability of anExternRef
orFuncRef
by looking at their respective shape, which is impossible.I will probably need some help with this latter issue as it seems to require a slight redesign/refactor to keep source value type information around, at least in the
wast
crate of wasmtime. I will probably also need some help with diagnosing the remaining failure cases. I am also positive that there are several code quality improvements that need to be implemented in order to make this PR satisfactory.
abrown commented on issue #5288:
This review got auto-assigned to me but I think @fitzgen and @alexcrichton have the right context for the review here.
dhil commented on issue #5288:
@alexcrichton @fitzgen I have reverted the changes to the wasmtime public API. The end result is not too bad; there are a couple of more test failures now. I haven't looked into the failures in detail yet.
dhil edited a comment on issue #5288:
@alexcrichton @fitzgen I have reverted the changes to the wasmtime public API. The end result is not too bad; there are a couple of more test failures now. I haven't looked into the failures in detail yet.
If you want I can reopen this PR from my personal fork such that you can get write permissions. I think GitHub do not grant write permissions on PRs originating from an organisation account.
alexcrichton commented on issue #5288:
Some things I'm seeing:
- I sent https://github.com/effect-handlers/wasmtime/pull/19 to surface the failures here on this PR
- One test failure,
wast::Cranelift::spec::function_references::linking
, relates to this function which needs to be souped up for typed function references. Note though that this function will need to grow&ModuleTypes
arguments to perform the subtyping check between two different modules (this is basically the same code that's inwasmparser
, just at link-time instead of validation-time).wast::Cranelift::spec::function_references::table
looks like it may be a bug in table initialization?wast::Cranelift::spec::function_references::br_table
is failing due to https://github.com/bytecodealliance/wasm-tools/pull/952 I thinkwast::Cranelift::spec::function_references::type_equivalence
looks like it may be some sort of code generator bug? Unsure.There are additionally a number of tests as you've found which all panic due to the one you inserted to avoid changes to the public API. Those tests will need to be skipped as-is from the upstream testsuite, but they can be copied to to
tests/misc_testsuite
with updates to run in Wasmtime.
dhil commented on issue #5288:
Some things I'm seeing:
* I sent [Get more CI passing effect-handlers/wasmtime#19](https://github.com/effect-handlers/wasmtime/pull/19) to surface the failures here on this PR * One test failure, `wast::Cranelift::spec::function_references::linking`, relates to [this function](https://github.com/bytecodealliance/wasmtime/blob/ef7af28ef095277f5979dcbc6ff03964c1e0ba12/crates/wasmtime/src/types/matching.rs#L183-L193) which needs to be souped up for typed function references. Note though that this function will need to grow `&ModuleTypes` arguments to perform the subtyping check between two different modules (this is basically the same code that's in `wasmparser`, just at link-time instead of validation-time). * `wast::Cranelift::spec::function_references::table` looks like it may be a bug in table initialization? * `wast::Cranelift::spec::function_references::br_table` is failing due to [Support for Wast table initialisation syntactic sugar with typeful references wasm-tools#952](https://github.com/bytecodealliance/wasm-tools/pull/952) I think * `wast::Cranelift::spec::function_references::type_equivalence` looks like it may be some sort of code generator bug? Unsure.
There are additionally a number of tests as you've found which all panic due to the one you inserted to avoid changes to the public API. Those tests will need to be skipped as-is from the upstream testsuite, but they can be copied to to
tests/misc_testsuite
with updates to run in Wasmtime.Excellent, thank you! I am down to two failures now
type_equivalence
andtable
.
dhil edited a comment on issue #5288:
Excellent, thank you! I am down to two failures now
type_equivalence
andtable
.There are additionally a number of tests as you've found which all panic due to the one you inserted to avoid changes to the public API. Those tests will need to be skipped as-is from the upstream testsuite, but they can be copied to to
tests/misc_testsuite
with updates to run in Wasmtime.I will move some of the content of the ignored tests to the misc testsuite too.
dhil commented on issue #5288:
I have been trying to track down the remaining two failures. In the
table.wast
file, the problem is that tables aren't properly initialised. Looking at the code it indeed seems like table initialisers get ignored altogether (https://github.com/bytecodealliance/wasmtime/blob/main/crates/environ/src/module_environ.rs#L303-L307), which would explain why table entries always seem to be initialised tonull
(c.f. https://github.com/bytecodealliance/wasmtime/blob/main/crates/runtime/src/instance.rs#L872-L874). From reading the code, I understand that great care is being taken to ensure lazy initialisation of tables, and thus, I wonder what semantics you want for tables with initialising expressions? @alexcrichton @fitzgen
alexcrichton commented on issue #5288:
Looking at the code it indeed seems like table initialisers get ignored altogether
Indeed! Table intializers were added with the function-references proposal so that's why they're "ignored". When we updated
wasmparser
we probably should have made it an error if a non-null initializer was indicated.I think what you'll want to do is to update
TableInitialization
to store the default initializer and then updatetry_func_table_init
to not useFuncTable
if there's a non-null initializer. We can probably lazy-initialize with initializers but that's fine to to in a future PR. It's probably best to get the base set of functionality working first.
dhil commented on issue #5288:
Commit 5de9a246fc0c8a0b9ae7a4a60b2ce74cceba86ce implements a fix for the table initialisation problem, though, it is ended up a bit more roundabout than I initially anticipated. If I understand correctly,
TableInitialization
is global per module, so to avoid penalising pre-function-references code I introduced a new table initialisation strategyEagerFuncTable
, which eagerly initialises all table fields. I am not sure whether this aligns with what you had in mind.As for the final failing test
type-equivalence
, it fails, seemingly, because the type test forcall_indirect
is nominal rather than structural (c.f. https://github.com/bytecodealliance/wasmtime/blob/6dc3eb717d7cf9b3539557ea7b393dd51225336d/crates/cranelift/src/func_environ.rs#L1662C1-L1665). Here is a minimal example to reproduce:(module (type $s0 (func (param i32))) (type $s1 (func (param i32 (ref $s0)))) (type $t1 (func (param (ref $s1)))) (func $s1 (type $s1)) (table funcref (elem $s1)) (func (export "run") (call_indirect (type $t1) (ref.func $s1) (i32.const 0)) ) ) (assert_return (invoke "run"))
I am not quite sure what the best course of action is here. I would have expected type/type signature canonicalisation to solve this problem.
alexcrichton commented on issue #5288:
which eagerly initialises all table fields. I am not sure whether this aligns with what you had in mind.
Seems reasonable to me! One adjustment I might recommend though is to reuse the existing
TableInitialization::Segments
initializer. That's the "eager" mode which is optimized, if possible, toTableInitialization::FuncTable
which is lazy initialization. During that optimization you can add a check that if anything is not null-initialized then the optimization can be skipped. I think this'll all have the same effect as what you've implemented but avoidsEagerTableInitializer
andSegments
being sort of duplicates of each other.I'll look more into the
call_indirect
issue now.
alexcrichton commented on issue #5288:
Ok so I can definitely imagine that canonicalization has a bug in it and something isn't working as expected. My guess is that something is hashing a module-local u32 when it actually needs to hash something module-independent, or something like that.
That being said I'm not sure that your reduction showcases the issue? You're invoking, indirectly, a function that takes two parameters with only one parameter. In that sense I think a runtime error is expected there?
dhil commented on issue #5288:
Ok so I can definitely imagine that canonicalization has a bug in it and something isn't working as expected. My guess is that something is hashing a module-local u32 when it actually needs to hash something module-independent, or something like that.
That being said I'm not sure that your reduction showcases the issue? You're invoking, indirectly, a function that takes two parameters with only one parameter. In that sense I think a runtime error is expected there?
Ah yes, sorry. I deleted a little bit too much. Here is a (valid) repro:
(module (type $s0 (func (param i32))) (type $s1 (func (param i32 (ref $s0)))) (type $s2 (func (param i32 (ref $s0)))) (type $t1 (func (param (ref $s1)))) (type $t2 (func (param (ref $s2)))) (func $s1 (type $s1)) (func $f1 (type $t1)) (func $f2 (type $t2)) (table funcref (elem $f1 $f2 $s1)) (func (export "run") (call_indirect (type $t1) (ref.func $s1) (i32.const 1)) ) )
It is extracted from
type-equivalence.wast
, the;; Indirect types
section.
dhil edited a comment on issue #5288:
Ok so I can definitely imagine that canonicalization has a bug in it and something isn't working as expected. My guess is that something is hashing a module-local u32 when it actually needs to hash something module-independent, or something like that.
That being said I'm not sure that your reduction showcases the issue? You're invoking, indirectly, a function that takes two parameters with only one parameter. In that sense I think a runtime error is expected there?
Ah yes, sorry. I deleted a little bit too much. Here is a (valid) repro:
(module (type $s0 (func (param i32))) (type $s1 (func (param i32 (ref $s0)))) (type $s2 (func (param i32 (ref $s0)))) (type $t1 (func (param (ref $s1)))) (type $t2 (func (param (ref $s2)))) (func $s1 (type $s1)) (func $f1 (type $t1)) (func $f2 (type $t2)) (table funcref (elem $f1 $f2 $s1)) (func (export "run") (call_indirect (type $t1) (ref.func $s1) (i32.const 1)) ) ) (assert_return (invoke "run"))
It is extracted from
type-equivalence.wast
, the;; Indirect types
section.
dhil edited a comment on issue #5288:
Ok so I can definitely imagine that canonicalization has a bug in it and something isn't working as expected. My guess is that something is hashing a module-local u32 when it actually needs to hash something module-independent, or something like that.
That being said I'm not sure that your reduction showcases the issue? You're invoking, indirectly, a function that takes two parameters with only one parameter. In that sense I think a runtime error is expected there?
Ah yes, sorry. I deleted a little bit too much. Here is a (valid) repro:
(module (type $s0 (func (param i32))) (type $s1 (func (param i32 (ref $s0)))) (type $s2 (func (param i32 (ref $s0)))) (type $t1 (func (param (ref $s1)))) (type $t2 (func (param (ref $s2)))) (func $s1 (type $s1)) (func $f2 (type $t2)) (table funcref (elem $f2 $s1)) (func (export "run") (call_indirect (type $t1) (ref.func $s1) (i32.const 0)) ) ) (assert_return (invoke "run"))
It is extracted from
type-equivalence.wast
, the;; Indirect types
section.
dhil commented on issue #5288:
which eagerly initialises all table fields. I am not sure whether this aligns with what you had in mind.
Seems reasonable to me! One adjustment I might recommend though is to reuse the existing
TableInitialization::Segments
initializer. That's the "eager" mode which is optimized, if possible, toTableInitialization::FuncTable
which is lazy initialization. During that optimization you can add a check that if anything is not null-initialized then the optimization can be skipped. I think this'll all have the same effect as what you've implemented but avoidsEagerTableInitializer
andSegments
being sort of duplicates of each other.I'll look more into the
call_indirect
issue now.I have attempted to clean this up in the latest commit. Is this closer to what you had in mind?
alexcrichton commented on issue #5288:
Ok so I think the issue is in two locations:
- Here in the engine-level signature registry, which is a hash map of types across modules registered in an engine.
- Here where there's a module-level map of types-to-indexes which should be deduplicating the above types but isn't.
This bug is due to
derive(Hash)
onWasmFuncType
which delegates all the way through toWasmHeapType
which hashes the raw index. I think the best solution would be to remove the auto-derived trait impls fromWasmType
about equality and hashing, and then work backwards from there. The rawWasmHeapType::Index
should probably take aSignatureIndex
to assist in deduplicating within a module. That should solve the second bullet above.The first bullet above, however, is much more difficult. That'll need to perform a "deep hash" of the entire structural type since it must be module-independent (and
SignatureIndex
is module-local). If the second bullet is enough to get tests passing for this PR though then that seems ok to defer to an issue and get this landed instead.(to clarify, at this point I think it's fine to land this PR so long as it passes CI, it's been long enough and I'm sure y'all are tired of rebasing. It's ok to defer work so long as there's issues on file and the work is gated, which it all is)
dhil commented on issue #5288:
Thanks! I see the culprit now. Just to clarify, when you say
The raw WasmHeapType::Index should probably take a SignatureIndex to assist in deduplicating within a module.
Do you mean that
WasmHeapType
should be parameterised by aSignatureIndex
rather thanu32
? I can see how that would make the interning easier, though, how does this affect conversion to/fromwasmparser
types?
alexcrichton commented on issue #5288:
No need to worry about conversion into a
wasmparser
type, that in theory shouldn't happen (or at least not that I know of). For conversion from awasmparser
type it may mean that aFrom
impl needs to be removed in favor of a method onModuleTypes
or something like that where the indexing/hashing context is available.So thinking more on this, sort of what this wants is
WasmType<T>
where aModule
storeWasmType<SignatureIndex>
but anEngine
stores aWasmType<VMSharedSignatureIndex>
. I think then thederive(Hash)
should continue to work withPartialEq
as well?
dhil commented on issue #5288:
I am OK with trying this approach, but it is a sizeable refactoring I think as
WasmType<T>
implies a type bound onTable
,Global
, andEntityType
(at least). I suppose for each of those the same logic applies: at the module level we wantT = SignatureIndex
and at the engine levelT = VMSharedSignatureIndex
(if relevant).The
From
trait will need to be removed, or at least supported by an alternative to support conversion from typed references.Should I give this refactoring a go?
dhil edited a comment on issue #5288:
I am OK with trying this approach, but it is a sizeable refactoring I think as
WasmType<T>
implies a type bound onTable
,Global
, andEntityType
(I suppose if they are only module-local then they can instantiateT = SignatureIndex
internally).The
From
trait will need to be removed, or at least supported by an alternative to support conversion from typed references.Should I give this refactoring a go?
CosineP commented on issue #5288:
This needs to happen for typed continuations support as well. Currently we assume everything is a function, but if we do this refactor I think we can look up in the types table whether a ref is a function or a continuation, which we need to know in a number of places in Wasmtime (at least in the embedding API; we managed to fudge the table layout stuff). But as a result I can say firsthand it is absolutely a hefty undertaking which is why we resorted to our current hack. I may still have a local branch with part of this refactoring underway.
alexcrichton commented on issue #5288:
If you're ok with it, I think it's ok to set the test to ignore and land this. It's true that this refactoring will require a bit of finesse, but it's one I'm happy to take on myself rather than having y'all do yet-more work on top of what you've already got here.
Otherwise I'd be happy to merge this with a green CI run (which will require ignoring tests) and a list of items to tackle tracked in an issue (such as fixing the failing tests, this possible refactoring, the embedder API, etc)
dhil commented on issue #5288:
I have added the test
type-equivalence.wast
to the ignore set. How should we go about documenting the issues? Should I open each issue as a separate issue here on GitHub?
alexcrichton commented on issue #5288:
I think one tracking issue should be good to start off with and it can branch from there as necessary. I'll work on giving this a final pass today. Thanks again for y'all's work here!
alexcrichton commented on issue #5288:
@dhil to answer this question (over here for a bit more visibility) my thinking is that at the wasm AST layer there's only "this index" as a type but at the Wasmtime layer we'll be able to be more principled than that by having, for example:
enum WasmHeapType { Func, Extern, TypedFunc(SignatureIndex), Array, TypedArray(StorageType), Struct, TypedStruct(StructIndex), // .. }
or something along the lines of that where
WasmHeapType
stores effectively the discriminant of what it refers to rather than requiring the indexing operation to then verify the type of the result.One of the things I was worried about was that there was the pervasive assumption that
WasmHeapType::Index(_)
was a function which won't be true once the GC proposal was implemented. I wanted to head off issues there by having the Wasmtime-level name be calledTypedFunc
so it's clear that all the cases handlingFunc
andTypedFunc
are only there for handling function-related things. In the future when more variants are added it'll require adding morematch
arms to handle in all these places.
dhil commented on issue #5288:
I've opened a issue to track the unresolved bits of this PR. See #6455. I hopefully I managed to cover them all.
fitzgen commented on issue #5288:
Seems like there are some conflicts and this needs a rebase/merge.
dhil commented on issue #5288:
Seems like there are some conflicts and this needs a rebase/merge.
I have synchronised with upstream again. Hopefully everything ticks green again :-)
alexcrichton commented on issue #5288:
:confetti:
Last updated: Dec 23 2024 at 12:05 UTC