Bug 193926 - [WebGPU] Fix and add validation to WebGPURenderPipeline and MTLVertexDescriptor
Summary: [WebGPU] Fix and add validation to WebGPURenderPipeline and MTLVertexDescriptor
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebGPU (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Justin Fan
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-01-28 14:56 PST by Justin Fan
Modified: 2019-01-29 12:50 PST (History)
9 users (show)

See Also:


Attachments
Patch (11.43 KB, patch)
2019-01-28 15:09 PST, Justin Fan
no flags Details | Formatted Diff | Diff
Patch for landing (11.44 KB, patch)
2019-01-29 12:11 PST, 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-01-28 14:56:46 PST
[WebGPU] Fix and add validation to WebGPURenderPipeline and MTLVertexDescriptor
Comment 1 Justin Fan 2019-01-28 14:58:47 PST
<rdar://problem/47327648>
Comment 2 Radar WebKit Bug Importer 2019-01-28 14:59:04 PST
<rdar://problem/47612384>
Comment 3 Radar WebKit Bug Importer 2019-01-28 14:59:04 PST
<rdar://problem/47612385>
Comment 4 Justin Fan 2019-01-28 15:09:29 PST
Created attachment 360380 [details]
Patch
Comment 5 Justin Fan 2019-01-28 15:12:18 PST
<rdar://problem/47327648>
Comment 6 Myles C. Maxfield 2019-01-28 19:03:21 PST
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
Comment 7 Justin Fan 2019-01-29 11:49:29 PST
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.
Comment 8 Justin Fan 2019-01-29 12:11:15 PST
Created attachment 360485 [details]
Patch for landing
Comment 9 WebKit Commit Bot 2019-01-29 12:50:23 PST
Comment on attachment 360485 [details]
Patch for landing

Clearing flags on attachment: 360485

Committed r240672: <https://trac.webkit.org/changeset/240672>
Comment 10 WebKit Commit Bot 2019-01-29 12:50:24 PST
All reviewed patches have been landed.  Closing bug.