Require RefFunc to have the proper type#7376
Merged
Conversation
As a holdout from before GC was implemented, we previously allowed RefFunc expressions to have type `funcref` rather than a specific signature type matching that of the referenced function. Remove this allowance and start requiring the types to be correct and precise to eliminate the possibility of stale types inhibiting (or invalidating!) optimizations. Update various older passes to update the types of RefFuncs, including those in tables, to keep their output passing validation. Also update the kitchen sink example test to construct RefFunc expressions with the correct type via the C API.
kripken
approved these changes
Mar 17, 2025
Member
kripken
left a comment
kripken
left a comment
There was a problem hiding this comment.
Do we have ref.func-using tests for FuncCastEmulation, I64ToI32Lowering, and LegalizeJSInterface?
| harnesses where they only want Binaryen to modify their given testcases, not | ||
| generate new things in them). | ||
| - Require the type of RefFunc expressions to match the type of the referenced | ||
| function. It is no longer valid to type them as anyref in the IR. |
Member
There was a problem hiding this comment.
Suggested change
| function. It is no longer valid to type them as anyref in the IR. | |
| function. It is no longer valid to type them as funcref in the IR. |
Member
Author
Yes, via indirect call tables. |
kripken
approved these changes
Mar 21, 2025
Member
|
Fuzzer found an issue here: (module
(type $0 (func (param f32 i64) (result i32)))
(type $1 (func (param i32 i32)))
(type $2 (func (result i64)))
(type $3 (func))
(import "fuzzing-support" "call-export" (func $fimport$0 (param i32 i32)))
(global $global$0 i32 (i32.const 0))
(memory $0 16 17)
(table $0 7 7 funcref)
(elem $0 (i32.const 0) $0 $1)
(export "func_17_invoker" (func $2))
(func $0 (param $0 f32) (param $1 i64) (result i32)
(unreachable)
)
(func $1 (result i64)
(unreachable)
)
(func $2
(drop
(call_indirect (type $0)
(f32.const 0)
(i64.const 0)
(i32.const 0)
)
)
)
) |
Member
Author
|
Thanks, will investigate. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
As a holdout from before GC was implemented, we previously allowed
RefFunc expressions to have type
funcrefrather than a specificsignature type matching that of the referenced function. Remove this
allowance and start requiring the types to be correct and precise to
eliminate the possibility of stale types inhibiting (or invalidating!)
optimizations.
Update various older passes to update the types of RefFuncs, including
those in tables, to keep their output passing validation. Also update
the kitchen sink example test to construct RefFunc expressions with the
correct type via the C API.