Fix parsing for typed function reference types #1889
Conversation
This file contains 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
|
Pinging @dbezhetskov in case you have any thoughts on this approach too. :) |
Previously an indexed ref type was parsed as a `Var`, assuming that regular types would be represented as index vars and named ref types would be named vars. Unfortunately, it's also possible for a ref type to be an indexed var that overlaps with a type (e.g., the type "any" which is 0x0 and the index 0). This commit restructures the code so that both a `Type` and `Var` are returned.
takikawa
force-pushed
the
fix-typed-funcref-crash
branch
from
fcc7e47
to
083e716
Compare
keithw
pushed a commit
that referenced
this pull request
Aug 15, 2022
Previously an indexed ref type was parsed as a `Var`, assuming that regular types would be represented as index vars and named ref types would be named vars. Unfortunately, it's also possible for a ref type to be an indexed var that overlaps with a type (e.g., the type "any" which is 0x0 and the index 0). This commit restructures the code so that both a `Type` and `Var` are returned. Closes #1889
keithw
pushed a commit
that referenced
this pull request
Aug 15, 2022
Previously an indexed ref type was parsed as a `Var`, assuming that regular types would be represented as index vars and named ref types would be named vars. Unfortunately, it's also possible for a ref type to be an indexed var that overlaps with a type (e.g., the type "any" which is 0x0 and the index 0). This commit restructures the code so that both a `Type` and `Var` are returned. Closes #1889
|
I think the idea is that builtin types should all have negative values .. so we know that thing >= to zero is a user type. The fact that we have this internal "Any" think value of zero sounds like a bug. |
sbc100
reviewed
Aug 15, 2022
| if (name.is_index()) { | ||
| *out_type = Type(Type::Reference, name.index()); | ||
| } else { | ||
| *out_type = Type(Type::Reference, kInvalidIndex); |
sbc100
reviewed
Aug 15, 2022
| Var type; | ||
| CHECK_RESULT(ParseValueType(&type)); | ||
| global->type = Type(type.index()); | ||
| CHECK_RESULT(ParseValueType(&global->type)); |
There was a problem hiding this comment.
I like the simplification we get from have ParseValueType return the Type directly.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Previously an indexed ref type was parsed as a
Var, assuming that regular types would be represented as index vars and indexed ref types would be named vars. The names would be resolved to actual indices later in a separate pass.Unfortunately, it's also possible for a ref type to be an indexed var that overlaps with a type (e.g., the type "any" which is 0x0 and the index 0 for the type
(ref 0)).This patch fixes the issue by returning both a
Typeand aVarfromParseValueType.Edit: I simplified this patch to just solve the issue at hand instead of introducing a bigger re-organization, which I realized was a bad idea.
This PR will close #1881.