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

Validator: Check subtyping in CallIndirect [DO NOT LAND] #6336

Closed
wants to merge 2 commits into from

Conversation

kripken
Copy link
Member

@kripken kripken commented Feb 22, 2024

V8 requires this, e.g.

(module
 (rec
  (type $0 (sub (func)))
  (type $1 (sub (func)))
 )
 (type $2 (func))
 (table $0 1 1 (ref null $1))
 (func $0
  (call_indirect (type $0)
   (i32.const 0)
  )
 )
)

errors on

CompileError: WebAssembly.Module(): Compiling function #0 failed: call_indirect: Immediate signature #0 is not a subtype of immediate table #0 @+46

However, @tlively you wrote otherwise in a test, so I am confused:

(table $t 1 1 (ref null $super))
;; CHECK: (func $call-indirect-table (type $sub)
;; CHECK-NEXT: (call_indirect $t (type $sub)
;; CHECK-NEXT: (i32.const 0)
;; CHECK-NEXT: )
;; CHECK-NEXT: )
(func $call-indirect-table (type $sub)
;; This does *not* require $sub <: $super on its own, although if that
;; subtyping is not required at all, then it must be impossible for the table
;; to contain a $sub and this call will trap.
(call_indirect $t (type $sub)

Reading the main spec, it still mentions only funcref, so it is pre-typed-function-references I guess. Reading the typed function references spec I only see it change the requirement from funcref to ref null func. I also cannot find anything in the overviews, so I am lost as to what the correct behavior is.

cc @jakobkummerow for the V8 behavior - am I reading that error right, and if so where in the spec is that validation rule added?

@kripken kripken requested a review from tlively February 22, 2024 18:16
@kripken
Copy link
Member Author

kripken commented Feb 22, 2024

(found by fuzzing with #6327)

@tlively
Copy link
Member

tlively commented Feb 22, 2024

Sounds like there is a V8 bug here.

The validation rule for call_indirect deliberately requires that the table element type is any subtype of ref null func, even with typed function references and GC. This is safe because the typing and semantics of call_indirect are precisely equivalent to (table.get x) (ref.cast y) (call_ref y) and the cast will fail if the table element has an incompatible type.

The V8 error message here is extra interesting because it implies that V8 will also disallow call_indirect with a supertype of the table element type, which is perfectly safe and wouldn't even trap.

@kripken kripken changed the title Validator: Check subtyping in CallIndirect Validator: Check subtyping in CallIndirect [DO NOT LAND] Feb 22, 2024
@tlively
Copy link
Member

tlively commented Feb 22, 2024

I have a PR up adding spec tests for this: WebAssembly/gc#526

@jakobkummerow
Copy link
Contributor

Sounds like we'll have to update V8's implementation there, yes.
Our implementation clearly intends to do a subtype check there; however we added this code quite a long time ago and haven't touched it since: https://chromium-review.googlesource.com/c/v8/v8/+/2335190/7/src/wasm/function-body-decoder-impl.h
So presumably it was based on an early understanding of an early (and/or not fully fleshed out) version of the spec.
I agree that the current (and presumably final) wording at https://webassembly.github.io/function-references/core/valid/instructions.html#xref-syntax-instructions-syntax-instr-control-mathsf-call-indirect-x-y doesn't require the call instruction's immediate type y to be a subtype of the table type t.

@jakobkummerow
Copy link
Contributor

V8-side fix in flight: crrev.com/c/5319505

hubot pushed a commit to v8/v8 that referenced this pull request Feb 23, 2024
As discovered on WebAssembly/binaryen#6336,
we erroneously required that the immediate type of a call_indirect
instruction be a subtype of the referenced table's type, but there
is no such requirement in the spec. This patch drops the check.
This change is backwards-compatible because it makes V8's behavior
strictly more permissive.

Bug: v8:9495
Change-Id: I198822e3d1ef8d8dd349fa92ed9e49d043d5d192
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/5319505
Commit-Queue: Manos Koukoutos <[email protected]>
Commit-Queue: Jakob Kummerow <[email protected]>
Reviewed-by: Manos Koukoutos <[email protected]>
Auto-Submit: Jakob Kummerow <[email protected]>
Cr-Commit-Position: refs/heads/main@{#92509}
@kripken
Copy link
Member Author

kripken commented Feb 23, 2024

Great, thanks @jakobkummerow , let's close this PR then.

@kripken kripken closed this Feb 23, 2024
@kripken kripken deleted the call.indirect.subtyping branch February 23, 2024 16:16
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.

3 participants