Summary: | [JSC] Improve Wasm binary test coverage | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Yusuke Suzuki <ysuzuki> | ||||||||||||
Component: | JavaScriptCore | Assignee: | Yusuke Suzuki <ysuzuki> | ||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||
Severity: | Normal | CC: | benjamin, cdumez, cmarcelo, darin, ews-watchlist, keith_miller, mark.lam, msaboff, saam, tzagallo, webkit-bug-importer, ysuzuki | ||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||
Hardware: | Unspecified | ||||||||||||||
OS: | Unspecified | ||||||||||||||
Attachments: |
|
Description
Yusuke Suzuki
2019-12-04 01:18:01 PST
Created attachment 414207 [details]
Patch
This patch modifies one of the wasm.json files. Please ensure that any changes in one have been mirrored to the other. You can find the wasm.json files at "Source/JavaScriptCore/wasm/wasm.json" and "JSTests/wasm/wasm.json". Created attachment 414208 [details]
Patch
Created attachment 414210 [details]
Patch
Created attachment 414412 [details]
Patch
Created attachment 414421 [details]
Patch
Comment on attachment 414421 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=414421&action=review > Source/JavaScriptCore/wasm/WasmSectionParser.cpp:185 > + WASM_PARSER_FAIL_IF(flags != 0x0 && flags != 0x1, "resiable limits flag should be 0x00 or 0x01 but ", flags); Typo here in the word resizable. Also, I don’t think “should be 0x00 or 0x01 but 5” makes sense so maybe add the word “is” and also seems like we’d want to format as 2-digit hex with a leading “0x” if that was easy, or just call the espected values 0 and 1. > Source/JavaScriptCore/wasm/WasmSectionParser.cpp:511 > + WASM_PARSER_FAIL_IF(mutability != 0x0 && mutability != 0x1, "invalid Global's mutability ", mutability); Might want a colon here to make it clear that the trailing number is the mutability, not just a stray number at the end of a line. > Source/WTF/wtf/LEBDecoder.h:78 > + static_assert(!std::is_unsigned_v<T>); Consider using is_signed? > Source/WTF/wtf/LEBDecoder.h:93 > + // This is not sign-extended, positive number. Then, remaining bits should be (lastByteMask<T>() >> 1). Grammar mistake here. Should be “... is a non-sign-extended, positive number. Thus, the remaining ...” > Source/WTF/wtf/LEBDecoder.h:94 > + // For example, in int32_t case, the last byte should be less than, 0b00000111, since 7 * 4 + 3 = 31. Extraneous comma here after “less than”. > Tools/TestWebKitAPI/Tests/WTF/LEBDecoder.cpp:40 > + out << static_cast<unsigned>(v) << ", "; I assume we want two-digit hex values here and could accomplish that with std::setfill and std::setw. Also, why is the cast to unsigned valuable? Comment on attachment 414421 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=414421&action=review >> Source/WTF/wtf/LEBDecoder.h:94 >> + // For example, in int32_t case, the last byte should be less than, 0b00000111, since 7 * 4 + 3 = 31. > > Extraneous comma here after “less than”. Oh, also should be “the int32_t case”. Comment on attachment 414421 [details]
Patch
lgtm as well
Comment on attachment 414421 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=414421&action=review Thanks! >> Source/JavaScriptCore/wasm/WasmSectionParser.cpp:185 >> + WASM_PARSER_FAIL_IF(flags != 0x0 && flags != 0x1, "resiable limits flag should be 0x00 or 0x01 but ", flags); > > Typo here in the word resizable. Also, I don’t think “should be 0x00 or 0x01 but 5” makes sense so maybe add the word “is” and also seems like we’d want to format as 2-digit hex with a leading “0x” if that was easy, or just call the espected values 0 and 1. Fixed for both. Generating hex formatted string by using `WTF::hex`. >> Source/JavaScriptCore/wasm/WasmSectionParser.cpp:511 >> + WASM_PARSER_FAIL_IF(mutability != 0x0 && mutability != 0x1, "invalid Global's mutability ", mutability); > > Might want a colon here to make it clear that the trailing number is the mutability, not just a stray number at the end of a line. Added. >> Source/WTF/wtf/LEBDecoder.h:78 >> + static_assert(!std::is_unsigned_v<T>); > > Consider using is_signed? Changed. >> Source/WTF/wtf/LEBDecoder.h:93 >> + // This is not sign-extended, positive number. Then, remaining bits should be (lastByteMask<T>() >> 1). > > Grammar mistake here. Should be “... is a non-sign-extended, positive number. Thus, the remaining ...” Fixed. >>> Source/WTF/wtf/LEBDecoder.h:94 >>> + // For example, in int32_t case, the last byte should be less than, 0b00000111, since 7 * 4 + 3 = 31. >> >> Extraneous comma here after “less than”. > > Oh, also should be “the int32_t case”. Fixed. >> Tools/TestWebKitAPI/Tests/WTF/LEBDecoder.cpp:40 >> + out << static_cast<unsigned>(v) << ", "; > > I assume we want two-digit hex values here and could accomplish that with std::setfill and std::setw. Also, why is the cast to unsigned valuable? Casting to unsigned is required to make it unsigned int. If we use uint8_t, upstream recognizes it as the same to char, and attempt to dump a broken character. Committed r269998: <https://trac.webkit.org/changeset/269998> (In reply to Yusuke Suzuki from comment #10) > >> Tools/TestWebKitAPI/Tests/WTF/LEBDecoder.cpp:40 > >> + out << static_cast<unsigned>(v) << ", "; > > > > I assume we want two-digit hex values here and could accomplish that with std::setfill and std::setw. Also, why is the cast to unsigned valuable? > > Casting to unsigned is required to make it unsigned int. If we use uint8_t, > upstream recognizes it as the same to char, and attempt to dump a broken > character. Wow it’s really surprising that ostream does that with uint8_t. I understand that happening for char, but I would not expect it for unsigned char. Makes me want to research it more. (In reply to Darin Adler from comment #13) > (In reply to Yusuke Suzuki from comment #10) > > >> Tools/TestWebKitAPI/Tests/WTF/LEBDecoder.cpp:40 > > >> + out << static_cast<unsigned>(v) << ", "; > > > > > > I assume we want two-digit hex values here and could accomplish that with std::setfill and std::setw. Also, why is the cast to unsigned valuable? > > > > Casting to unsigned is required to make it unsigned int. If we use uint8_t, > > upstream recognizes it as the same to char, and attempt to dump a broken > > character. > > Wow it’s really surprising that ostream does that with uint8_t. I understand > that happening for char, but I would not expect it for unsigned char. Makes > me want to research it more. Yeah, it seems this is some known problem... https://stackoverflow.com/questions/19562103/uint8-t-cant-be-printed-with-cout "uint8_t will most likely be a typedef for unsigned char. The ostream class has a special overload for unsigned char, i.e. it prints the character with the number 5, which is non-printable, hence the empty space." https://stackoverflow.com/questions/19562103/uint8-t-cant-be-printed-with-cout Possibly, this is the reason. C++ should have had char8_t at the first place... |