Bug 193926 – [WebGPU] Fix and add validation to WebGPURenderPipeline and MTLVertexDescriptor

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

[Justin Fan]

[WebGPU] Fix and add validation to WebGPURenderPipeline and MTLVertexDescriptor

[Justin Fan]

<rdar://problem/47327648>

[Radar WebKit Bug Importer]

<rdar://problem/47612384>

[Radar WebKit Bug Importer]

<rdar://problem/47612385>

[Justin Fan]

Created attachment 360380 [details]
Patch

[Justin Fan]

<rdar://problem/47327648>

[Myles C. Maxfield]

Comment on attachment 360380 [details]
Patch

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

> Source/WebCore/platform/graphics/gpu/cocoa/GPURenderPipelineMetal.mm:123
> +    auto attributeArray = retainPtr(mtlVertexDescriptor.get().attributes);

Are you sure our style guide permits use of Objective-C dot syntax?

> Source/WebCore/platform/graphics/gpu/cocoa/GPURenderPipelineMetal.mm:128
> +        if (location > 30) {

I think this limit needs to be 16, not 30. It is/will be in the spec.

> Source/WebCore/platform/graphics/gpu/cocoa/GPURenderPipelineMetal.mm:133
> +        if (attributes[i].inputSlot > 30) {

Ditto.

> Source/WebCore/platform/graphics/gpu/cocoa/GPURenderPipelineMetal.mm:145
> +        mtlAttributeDesc.get().offset = attributes[i].offset; // FIXME: Setting this to an invalidly large value causes pipelineState creation to SIGABT.

This offset, plus the byte size of the data type, need to be less than or equal to the buffer's stride.

> Source/WebCore/platform/graphics/gpu/cocoa/GPURenderPipelineMetal.mm:156
> +        if (inputs[j].inputSlot > 30) {

Ditto about 16

> Source/WebCore/platform/graphics/gpu/cocoa/GPURenderPipelineMetal.mm:205
> +    populateMtlDescriptorSuccess = setFunctionsForPipelineDescriptor(functionName, mtlDescriptor.get(), descriptor)
> +        && setInputStateForPipelineDescriptor(functionName, mtlDescriptor.get(), descriptor);

I would separate these out into separate statements.

> Source/WebCore/platform/graphics/gpu/cocoa/GPURenderPipelineMetal.mm:219
> +    NSError *error = [NSError errorWithDomain:@"com.apple.WebKit.GPU" code:1 userInfo:nil];

wat

[Justin Fan]

Comment on attachment 360380 [details]
Patch

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

>> Source/WebCore/platform/graphics/gpu/cocoa/GPURenderPipelineMetal.mm:123
>> +    auto attributeArray = retainPtr(mtlVertexDescriptor.get().attributes);
> 
> Are you sure our style guide permits use of Objective-C dot syntax?

Our style guide doesn't mention ObjC dot vs bracket. I use [object methodCall] and object.property.

>> Source/WebCore/platform/graphics/gpu/cocoa/GPURenderPipelineMetal.mm:145
>> +        mtlAttributeDesc.get().offset = attributes[i].offset; // FIXME: Setting this to an invalidly large value causes pipelineState creation to SIGABT.
> 
> This offset, plus the byte size of the data type, need to be less than or equal to the buffer's stride.

I could process the vertex buffer layouts first to determine the stride of the attribute's buffer. However, we won't know the size of the vertex buffer, so I'm not sure how to validate stride, because it has the same issue as the offset value here.

>> Source/WebCore/platform/graphics/gpu/cocoa/GPURenderPipelineMetal.mm:219
>> +    NSError *error = [NSError errorWithDomain:@"com.apple.WebKit.GPU" code:1 userInfo:nil];
> 
> wat

error is an autoreleased NSError * created via the way we did it in WebMetal ¯\_(ツ)_/¯. Useful for debugging.

[Justin Fan]

Created attachment 360485 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 360485 [details]
Patch for landing

Clearing flags on attachment: 360485

Committed r240672: <https://trac.webkit.org/changeset/240672>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.