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

Fuzzer: Ignore our current bug with the type of br_if #3775

Merged
merged 1 commit into from
Apr 6, 2021
Merged

Conversation

kripken
Copy link
Member

@kripken kripken commented Apr 6, 2021

We give br_if a too specific type: #3767

This is only noticeable with GC, and in rare cases where the type of br_if
is actually used - which realistically it never is, so really just fuzzer testcases.

@kripken kripken requested review from tlively and aheejin April 6, 2021 17:16
Copy link
Member

@tlively tlively left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it be better to just relax our validation rules? That way at least we could have tests demonstrating our implemented behavior.

@kripken
Copy link
Member Author

kripken commented Apr 6, 2021

The validation error comes from v8 in this case. We emit code that is valid if the br_if's type is more specific, then v8 (or any other compliant VM) sees a too-general type.

Getting us to emit the proper general type is not trivial (it's not present on the br_if - it's only known by the block we branch to). But also as mentioned in the linked issue, we probably want to keep the specific type (as with tee)....

@tlively
Copy link
Member

tlively commented Apr 6, 2021

Oh, I see, we would have to insert extra downcasts to make V8 happy, but that's not worth it considering that V8 will probably be updated to accept what we are emitting. Makes sense 👍

@kripken kripken merged commit cb8cc08 into main Apr 6, 2021
@kripken kripken deleted the fuzzignore branch April 6, 2021 20:54
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.

2 participants