Bug 198600 – [WHLSL] Implement out-of-bounds and nullptr behavior

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

[Myles C. Maxfield]

3 options:
Trapping
Clamping
Zero reads / writes do nothing

[Saam Barati]

Gonna look at this today.

[Saam Barati]

Planning on doing:
"Zero reads / writes do nothing"

[Saam Barati]

Created attachment 371908 [details]
WIP

Has a bunch of Myles's patches applied locally so I can test.

[Saam Barati]

Created attachment 371912 [details]
WIP

[Saam Barati]

So the plan is:

1. The value stack in FunctionWriter turns into a stack of pairs: rvalues and lvalues.
2. All lvalues are pointers.
3. Anything that produces an lvalue must push a pointer to the stack. But not all things produce lvalues, so that field may be empty. However, all things that produce lvalues also produce rvalues. So, "*x = 42" works, but so does "foo(*x)".
4. Dereference just works, as dereference produces both an lvalue and rvalue. Dereference node's child must also be an lvalue. So we just forward that value along on the stack. For the rvalue, if we try to dereference nullptr, we just fill in zero bytes instead.
5. Assignment expressions check if the incoming lvalue is nullptr. If it is, we skip the assignment.
6. Then, operator&[] just returns nullptr on OOB. Then dereference works as expected, and assignment works as expected. So does "thread *foo = null; *foo = 42; *foo;".
7. MakePointerExpression just takes the last lvalue off the stack and returns it.
8. VariableReference will push both the variable value and a pointer to the variable.

[Saam Barati]

I believe the patch accomplishes this, just need to write some tests.

[Radar WebKit Bug Importer]

<rdar://problem/51668853>

[Saam Barati]

Created attachment 372013 [details]
WIP

[Saam Barati]

Created attachment 372074 [details]
patch

This is ready for review. It relies on the compute patch (which just got rolled out). So I will hold off on landing this until that re-lands.

[Saam Barati]

Comment on attachment 372074 [details]
patch

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

> Source/WebCore/platform/graphics/gpu/cocoa/GPUComputePipelineMetal.mm:97
> +        if (!computeLibrary)
> +            NSLog(@"%@", error);

I find these logs helpful during development, but I can remove them if that's preferred.

[Robin Morisset]

Comment on attachment 372074 [details]
patch

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

It looks good overall, I've made a few comments inline.

> Source/WebCore/Modules/webgpu/WHLSL/WHLSLPreserveVariableLifetimes.cpp:113
> +        rhs->setTypeAnnotation(AST::LeftValue { AST::AddressSpace::Thread });

I don't get this, why would the right side of an assignment be a left-value?

> Source/WebCore/Modules/webgpu/WHLSL/WHLSLPreserveVariableLifetimes.cpp:139
> +            makePointerExpression->setTypeAnnotation(AST::RightValue());

I agree with this one on the other hand

> Source/WebCore/Modules/webgpu/WHLSL/WHLSLPropertyResolver.cpp:692
> +        variableReference->setTypeAnnotation(AST::LeftValue { AST::AddressSpace::Thread }); // FIXME: https://bugs.webkit.org/show_bug.cgi?id=198169 Is this right?

No it's not, I made readModifyWrite expressions right values in the spec (it gets super confusing otherwise, e.g. what would (x += y) = z do?)

> Source/WebCore/Modules/webgpu/WHLSL/Metal/WHLSLFunctionWriter.cpp:612
> +    // FIXME: This needs to be made to work. It probably should be using the last leftValue too.

I agree that it should be using the leftValue.

> Source/WebCore/Modules/webgpu/WHLSL/Metal/WHLSLNativeFunctionWriter.cpp:84
> +String writeNativeFunction(AST::NativeFunctionDeclaration& nativeFunctionDeclaration, String& outputFunctionName, Intrinsics& intrinsics, TypeNamer& typeNamer, const char* memsetZeroFunctionName)

What is the reasoning behind passing memsetZeroFunctionName as an argument? Is it not a global constant?

>> Source/WebCore/platform/graphics/gpu/cocoa/GPUComputePipelineMetal.mm:97
>> +            NSLog(@"%@", error);
> 
> I find these logs helpful during development, but I can remove them if that's preferred.

I'm happy to see logs, but maybe behind a !ASSERT_DISABLED ? So they will be free in release mode.

[Saam Barati]

Comment on attachment 372074 [details]
patch

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

>> Source/WebCore/Modules/webgpu/WHLSL/WHLSLPreserveVariableLifetimes.cpp:113
>> +        rhs->setTypeAnnotation(AST::LeftValue { AST::AddressSpace::Thread });
> 
> I don't get this, why would the right side of an assignment be a left-value?

Spoke to Robin offline, but commenting here for posterity:

In the current implementation of the compiler, all VariableReferences are Lvalues, even if we don't use them as an lvalue. This patch makes this happen consistently everywhere. It simplifies how we generate code in metal. Alternatively, we could not rely on this, but metal code gen that produces Lvalues would have to branch to see if the current node is used in a lvalue context or not. I thought it'd be easier to just make this consistent everywhere. Now, the metal codegen will produce an lvalue irrespective of if the lvalue is uses. That's ok, because in the worst case, we just end up with unused variables, which LLVM will happily kill the various operations used to produce such Lvalues.

>> Source/WebCore/Modules/webgpu/WHLSL/Metal/WHLSLNativeFunctionWriter.cpp:84
>> +String writeNativeFunction(AST::NativeFunctionDeclaration& nativeFunctionDeclaration, String& outputFunctionName, Intrinsics& intrinsics, TypeNamer& typeNamer, const char* memsetZeroFunctionName)
> 
> What is the reasoning behind passing memsetZeroFunctionName as an argument? Is it not a global constant?

It's currently scoped to the class. I thought it is nicer to just pass it in.

>>> Source/WebCore/platform/graphics/gpu/cocoa/GPUComputePipelineMetal.mm:97
>>> +            NSLog(@"%@", error);
>> 
>> I find these logs helpful during development, but I can remove them if that's preferred.
> 
> I'm happy to see logs, but maybe behind a !ASSERT_DISABLED ? So they will be free in release mode.

Well, we shouldn't ever reach this. If we do, it's a bug in the compiler. We could put it under !NDEBUG or something like that.

[Robin Morisset]

Comment on attachment 372074 [details]
patch

r=me

[Saam Barati]

Created attachment 372127 [details]
patch for landing

[EWS Watchlist]

Attachment 372127 [details] did not pass style-queue:


ERROR: Source/WebCore/ChangeLog:40:  Line contains tab character.  [whitespace/tab] [5]
Total errors found: 1 in 15 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Saam Barati]

Created attachment 372128 [details]
patch for landing

[Saam Barati]

landed in:
https://trac.webkit.org/changeset/246438/webkit

[Saam Barati]

follow up landed in:
https://trac.webkit.org/changeset/246439/webkit