Bug 204843

Summary: [JSC] Improve Wasm binary test coverage
Product: WebKit Reporter: Yusuke Suzuki <ysuzuki>
Component: JavaScriptCoreAssignee: Yusuke Suzuki <ysuzuki>
Status: RESOLVED FIXED    
Severity: Normal CC: benjamin, cdumez, cmarcelo, darin, ews-watchlist, keith_miller, mark.lam, msaboff, sbarati, tzagallo, webkit-bug-importer, ysuzuki
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch darin: review+

Description Yusuke Suzuki 2019-12-04 01:18:01 PST
...
Comment 1 Yusuke Suzuki 2020-11-16 02:09:38 PST
Created attachment 414207 [details]
Patch
Comment 2 EWS Watchlist 2020-11-16 02:10:34 PST
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".
Comment 3 Yusuke Suzuki 2020-11-16 02:11:42 PST
Created attachment 414208 [details]
Patch
Comment 4 Yusuke Suzuki 2020-11-16 02:29:41 PST
Created attachment 414210 [details]
Patch
Comment 5 Yusuke Suzuki 2020-11-17 21:50:55 PST
Created attachment 414412 [details]
Patch
Comment 6 Yusuke Suzuki 2020-11-17 23:55:09 PST
Created attachment 414421 [details]
Patch
Comment 7 Darin Adler 2020-11-18 08:58:04 PST
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 8 Darin Adler 2020-11-18 09:00:26 PST
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 9 Keith Miller 2020-11-18 10:45:30 PST
Comment on attachment 414421 [details]
Patch

lgtm as well
Comment 10 Yusuke Suzuki 2020-11-18 15:45:09 PST
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.
Comment 11 Yusuke Suzuki 2020-11-18 15:51:06 PST
Committed r269998: <https://trac.webkit.org/changeset/269998>
Comment 12 Radar WebKit Bug Importer 2020-11-18 15:52:20 PST
<rdar://problem/71559933>
Comment 13 Darin Adler 2020-11-18 16:07:04 PST
(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.
Comment 14 Yusuke Suzuki 2020-11-18 16:09:46 PST
(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
Comment 15 Yusuke Suzuki 2020-11-18 16:12:33 PST
"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...