Bug 194618 - [Web GPU] Replace 'unsigned long' with 'unsigned' when implementing u32 variables
Summary: [Web GPU] Replace 'unsigned long' with 'unsigned' when implementing u32 varia...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebGPU (show other bugs)
Version: Other
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Justin Fan
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-02-13 15:36 PST by Justin Fan
Modified: 2019-03-29 12:43 PDT (History)
8 users (show)

See Also:


Attachments
Patch (24.51 KB, patch)
2019-03-28 18:38 PDT, Justin Fan
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Justin Fan 2019-02-13 15:36:46 PST
In WebIDL, "unsigned long (aka u32 in Web GPU)" actually refers to a 32-bit unsigned integer. Replace all instances of "unsigned long" in Web GPU C++ meant to represent "u32" with "unsigned".
Comment 1 Radar WebKit Bug Importer 2019-02-13 15:38:03 PST
<rdar://problem/48055390>
Comment 2 Justin Fan 2019-02-13 15:38:09 PST
Silly me didn't think harder about why "u32" was the chosen typedef for "unsigned long".
Comment 3 Radar WebKit Bug Importer 2019-02-13 15:47:49 PST
<rdar://problem/48055796>
Comment 4 Justin Fan 2019-03-28 18:38:57 PDT
Created attachment 366231 [details]
Patch
Comment 5 WebKit Commit Bot 2019-03-28 19:19:43 PDT
Comment on attachment 366231 [details]
Patch

Clearing flags on attachment: 366231

Committed r243636: <https://trac.webkit.org/changeset/243636>
Comment 6 WebKit Commit Bot 2019-03-28 19:19:44 PDT
All reviewed patches have been landed.  Closing bug.
Comment 7 Darin Adler 2019-03-29 08:54:26 PDT
Comment on attachment 366231 [details]
Patch

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

> Source/WebCore/Modules/webgpu/WebGPUBuffer.cpp:45
> -void WebGPUBuffer::setSubData(unsigned long long offset, const JSC::ArrayBuffer& data)
> +void WebGPUBuffer::setSubData(unsigned long offset, const JSC::ArrayBuffer& data)

Possibly wrong, see comment below.

> Source/WebCore/Modules/webgpu/WebGPUBuffer.h:51
> -    void setSubData(unsigned long long, const JSC::ArrayBuffer&);
> +    void setSubData(unsigned long, const JSC::ArrayBuffer&);

Possibly wrong, see comment below.

> Source/WebCore/Modules/webgpu/WebGPUBufferBinding.h:38
> -    unsigned long long offset;
> -    unsigned long long size;
> +    unsigned long offset;
> +    unsigned long size;

This change seems wrong. It changes 64-bit to 32-bit, and uses the type "unsigned long" for 32-bit, when we would normally use "unsigned" or "uint32_t".

I am pretty sure this needs to change back to "unsigned long long".

Do these need to be 64-bit? If so, please change back to "unsigned long long". If not, please change to "unsigned".

> Source/WebCore/platform/graphics/gpu/GPUBindGroupLayout.h:50
> -    using BindingsMapType = HashMap<unsigned long long, GPUBindGroupLayoutBinding, WTF::IntHash<unsigned long long>, WTF::UnsignedWithZeroKeyHashTraits<unsigned long long>>;
> +    using BindingsMapType = HashMap<unsigned long, GPUBindGroupLayoutBinding, WTF::IntHash<unsigned long>, WTF::UnsignedWithZeroKeyHashTraits<unsigned long>>;

Same problem here.

> Source/WebCore/platform/graphics/gpu/GPUBufferBinding.h:38
> -    unsigned long long offset;
> -    unsigned long long size;
> +    unsigned long offset;
> +    unsigned long size;

And here.
Comment 8 Justin Fan 2019-03-29 11:16:04 PDT
Comment on attachment 366231 [details]
Patch

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

>> Source/WebCore/Modules/webgpu/WebGPUBufferBinding.h:38
>> +    unsigned long size;
> 
> This change seems wrong. It changes 64-bit to 32-bit, and uses the type "unsigned long" for 32-bit, when we would normally use "unsigned" or "uint32_t".
> 
> I am pretty sure this needs to change back to "unsigned long long".
> 
> Do these need to be 64-bit? If so, please change back to "unsigned long long". If not, please change to "unsigned".

Thanks for the catch. The rest of WebKit backs these with 'uint64_t' so I'll be changing these to that!
Comment 9 Justin Fan 2019-03-29 12:43:18 PDT
Addressing Darin's comments in https://bugs.webkit.org/show_bug.cgi?id=196401