Bug 198096 – WHLSL: Parsing negative int literals parses the positive value instead

• Status: RESOLVED
• Resolution: FIXED
• Component: WebGPU

[Saam Barati]

...

[Saam Barati]

Created attachment 370365 [details]
patch

[Myles C. Maxfield]

Comment on attachment 370365 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=370365&action=review

> Source/WebCore/Modules/webgpu/WHLSL/WHLSLParser.cpp:233
> +        int64_t intResult = -static_cast<int64_t>(result);

Don't we still need a static assert, though? C++ doesn't guarantee int64_t is wider than int.

[Saam Barati]

Comment on attachment 370365 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=370365&action=review

>> Source/WebCore/Modules/webgpu/WHLSL/WHLSLParser.cpp:233
>> +        int64_t intResult = -static_cast<int64_t>(result);
> 
> Don't we still need a static assert, though? C++ doesn't guarantee int64_t is wider than int.

Few reasons I removed it:
1. This is not the only place relying on sizeof(int) == 4. All of WK relies on this. It's not super helpful to just have it here.
2. We could also take this further. I don't believe sizeof(unsigned) is guaranteed to be 4. So let's say sizeof(unsigned) == 8, then the static_cast from unsigned to int64_t is also broken. I could add another static_assert, but the same as above: lots of things in WK would be broken if sizeof(unsigned) != 4

[Saam Barati]

(In reply to Saam Barati from comment #3)
> Comment on attachment 370365 [details]
> patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=370365&action=review
> 
> >> Source/WebCore/Modules/webgpu/WHLSL/WHLSLParser.cpp:233
> >> +        int64_t intResult = -static_cast<int64_t>(result);
> > 
> > Don't we still need a static assert, though? C++ doesn't guarantee int64_t is wider than int.
> 
> Few reasons I removed it:
> 1. This is not the only place relying on sizeof(int) == 4. All of WK relies
> on this. It's not super helpful to just have it here.
> 2. We could also take this further. I don't believe sizeof(unsigned) is
> guaranteed to be 4. So let's say sizeof(unsigned) == 8, then the static_cast
> from unsigned to int64_t is also broken. I could add another static_assert,
> but the same as above: lots of things in WK would be broken if
> sizeof(unsigned) != 4

The nicer thing could be to switch to uint32_t/int32_t/int64_t everywhere.

[Myles C. Maxfield]

> > Few reasons I removed it:
> > 1. This is not the only place relying on sizeof(int) == 4. All of WK relies
> > on this.
Don't we want this code to be easily separable from WebKit?

[Saam Barati]

(In reply to Myles C. Maxfield from comment #5)
> > > Few reasons I removed it:
> > > 1. This is not the only place relying on sizeof(int) == 4. All of WK relies
> > > on this.
> Don't we want this code to be easily separable from WebKit?

Not sure. I’ll defer to you on that. 

Anyways, this isn’t a super interesting thing to worry about. It sounds like we should use int32_t since that is probably what’s specified by WHLSL. (And also use uint32_t/int64_t, etc, in surrounding code.)

[Saam Barati]

(In reply to Saam Barati from comment #6)
> (In reply to Myles C. Maxfield from comment #5)
> > > > Few reasons I removed it:
> > > > 1. This is not the only place relying on sizeof(int) == 4. All of WK relies
> > > > on this.
> > Don't we want this code to be easily separable from WebKit?
> 
> Not sure. I’ll defer to you on that. 
> 
> Anyways, this isn’t a super interesting thing to worry about. It sounds like
> we should use int32_t since that is probably what’s specified by WHLSL. (And
> also use uint32_t/int64_t, etc, in surrounding code.)

This is somewhat annoying to do. I'll just add back a similar static_assert. Anyways, we should consider using types that specify width in the areas that's specified by the specification. This isn't really that interesting because no one is going to compile C programs with sizeof(int)/sizeof(unsigned) not equal to 4.

[Saam Barati]

Created attachment 370372 [details]
patch for landing

[WebKit Commit Bot]

Comment on attachment 370372 [details]
patch for landing

Clearing flags on attachment: 370372

Committed r245612: <https://trac.webkit.org/changeset/245612>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/51012964>