Validate type references by index #2584
Conversation
| } | ||
|
|
||
| Result SharedValidator::EndTypeSection() { | ||
| Result result = Result::Ok; |
There was a problem hiding this comment.
This is a new event in SharedValidator. This check could be done in OnFuncType, but then we needed the OnTypeCount event, which is also not used by the validator.
Since the Google v8 engine has separate reference types for array/struct, we might need to check the reference type in the future as well, and we cannot do that before reading all types.
|
|
||
| for (auto func_type : func_types_) { | ||
| for (auto type : func_type.second.params) { | ||
| result |= CheckReferenceType(Location(), type, "params"); |
There was a problem hiding this comment.
We don't have the locations at this point.
|
The parser also validates the references to provide nice error locations, which is a double check when the validator is used. However the check in the parser has an advantage: implicit function type indicies are not accepted, which I think is a user friendly thing. |
|
Btw, I agree that the current |
547b2db to
5f9d5a3
Compare
|
Thank you for the review, I have updated the patch. |
Leaving as is, for consistency, seems fine to me. |
|
Is there anything I need to do? |
| if (type.is_index()) { | ||
| // TODO: Incorrect values can be misinterpreted by the parser. | ||
| if (type.index() >= static_cast<Index>(Type::Void)) { | ||
| if (IsIndexLikelyType(type.index())) { |
There was a problem hiding this comment.
From the code that follows it looks like we are trying to distinguish reference types from function types? Is that right? How about something like IsReferenceType? I'm not really sure what IsIndexLikelyType means here... perhaps I'm misunderstanding?
There was a problem hiding this comment.
It is not just reference type, it is currently any type. The ParseValueType encodes the value type as index.
For example: the binary code of (func $f (param i64 (ref 0xfffffffe))) is
; func type 0
000000b: 60 ; func
000000c: 02 ; num params
000000d: 7e ; i64
000000e: 7e ; i64
000000f: 00 ; num results
The i64 is encoded as 0xfffffffe, which is stored in the index. Also, the (ref 0xfffffffe) is converted to i64, because currently it is impossible to tell the difference (at the moment). The function_ref patch will fix this issue.
|
Rebased this patch (no change). Can you land it? Thank you! |
Also fix a reference parsing bug when named locals are used.
|
@sbc100 could you land this approved patch? Thank you! |
This patch is another bugfix.