Bug 204843 – [JSC] Improve Wasm binary test coverage

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Yusuke Suzuki]

...

[Yusuke Suzuki]

Created attachment 414207 [details]
Patch

[EWS Watchlist]

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".

[Yusuke Suzuki]

Created attachment 414208 [details]
Patch

[Yusuke Suzuki]

Created attachment 414210 [details]
Patch

[Yusuke Suzuki]

Created attachment 414412 [details]
Patch

[Yusuke Suzuki]

Created attachment 414421 [details]
Patch

[Darin Adler]

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?

[Darin Adler]

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”.

[Keith Miller]

Comment on attachment 414421 [details]
Patch

lgtm as well

[Yusuke Suzuki]

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.

[Yusuke Suzuki]

Committed r269998: <https://trac.webkit.org/changeset/269998>

[Radar WebKit Bug Importer]

<rdar://problem/71559933>

[Darin Adler]

(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.

[Yusuke Suzuki]

(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

[Yusuke Suzuki]

"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...