Repository navigation
Conversation
There was a problem hiding this comment.
Code Review
This pull request adds type validation checks for various WebAssembly string operations (such as string.measure, string.concat, string.eq, string.test, string.wtf16_get, and string.slice_wtf) in wasm-validator.cpp, ensuring inputs are subtypes of externref and index arguments are i32. It also updates associated tests to use noextern instead of none. The reviewer identified that validation for string.compare is missing and should be added. Additionally, the reviewer suggested simplifying redundant Type(Type::i32) constructor calls to Type::i32 in visitStringWTF16Get and visitStringSliceWTF for better readability.
| shouldBeSubTypeIgnoringShared(curr->left->type, | ||
| Type(HeapType::ext, Nullable), | ||
| curr, | ||
| "string.eq left should be an externref"); | ||
| shouldBeSubTypeIgnoringShared(curr->right->type, | ||
| Type(HeapType::ext, Nullable), | ||
| curr, | ||
| "string.eq right should be an externref"); |
There was a problem hiding this comment.
It appears that visitStringCompare is missing from both the FunctionValidator class declaration and its implementation. Since string.compare is a standard string operation (and is used in the tests), it should be validated similarly to string.eq to ensure that the strings feature is enabled and that both operands are subtypes of externref (or stringref). Please add the declaration to FunctionValidator and implement visitStringCompare.
There was a problem hiding this comment.
StringCompare does not exist in the IR, so visitStringCompare is irrelevant. It is implemented as a StringEq opcode.
| shouldBeSubType(curr->pos->type, | ||
| Type(Type::i32), | ||
| curr, | ||
| "string.wtf16_get pos should be an i32"); |
There was a problem hiding this comment.
The explicit construction Type(Type::i32) is redundant here. Type::i32 is an unscoped enum value that is implicitly convertible to Type and is already used directly elsewhere in this file (e.g., in ref.i31 validation). Simplifying it to Type::i32 improves readability.
| shouldBeSubType(curr->pos->type, | |
| Type(Type::i32), | |
| curr, | |
| "string.wtf16_get pos should be an i32"); | |
| shouldBeSubType(curr->pos->type, | |
| Type::i32, | |
| curr, | |
| "string.wtf16_get pos should be an i32"); |
There was a problem hiding this comment.
This is not the case due to C++ type conversion shenanigans.
| shouldBeSubType(curr->start->type, | ||
| Type(Type::i32), | ||
| curr, | ||
| "string.slice_wtf start should be an i32"); | ||
| shouldBeSubType(curr->end->type, | ||
| Type(Type::i32), | ||
| curr, | ||
| "string.slice_wtf end should be an i32"); |
There was a problem hiding this comment.
The explicit construction Type(Type::i32) is redundant here. Simplifying it to Type::i32 improves readability and maintains consistency with other parts of the validator.
| shouldBeSubType(curr->start->type, | |
| Type(Type::i32), | |
| curr, | |
| "string.slice_wtf start should be an i32"); | |
| shouldBeSubType(curr->end->type, | |
| Type(Type::i32), | |
| curr, | |
| "string.slice_wtf end should be an i32"); | |
| shouldBeSubType(curr->start->type, | |
| Type::i32, | |
| curr, | |
| "string.slice_wtf start should be an i32"); | |
| shouldBeSubType(curr->end->type, | |
| Type::i32, | |
| curr, | |
| "string.slice_wtf end should be an i32"); |
| shouldBeSubTypeIgnoringShared(curr->right->type, | ||
| Type(HeapType::ext, Nullable), | ||
| curr, | ||
| "string.concat right should be an externref"); |
There was a problem hiding this comment.
We should check that the LHS and RHS sharedness match as well. Same for StringEq below.
| shouldBeSubType(curr->pos->type, | ||
| Type(Type::i32), | ||
| curr, | ||
| "string.wtf16_get pos should be an i32"); |
There was a problem hiding this comment.
This is not the case due to C++ type conversion shenanigans.
No description provided.