Bug 198600 - [WHLSL] Implement out-of-bounds and nullptr behavior
Summary: [WHLSL] Implement out-of-bounds and nullptr behavior
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebGPU (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P1 Normal
Assignee: Saam Barati
URL:
Keywords: InRadar
Depends on: 198644
Blocks:
  Show dependency treegraph
 
Reported: 2019-06-05 20:51 PDT by Myles C. Maxfield
Modified: 2019-06-14 11:59 PDT (History)
9 users (show)

See Also:


Attachments
WIP (316.51 KB, patch)
2019-06-11 18:55 PDT, Saam Barati
no flags Details | Formatted Diff | Diff
WIP (349.04 KB, patch)
2019-06-11 19:20 PDT, Saam Barati
no flags Details | Formatted Diff | Diff
WIP (182.92 KB, patch)
2019-06-12 19:04 PDT, Saam Barati
no flags Details | Formatted Diff | Diff
patch (46.25 KB, patch)
2019-06-13 12:49 PDT, Saam Barati
rmorisset: review+
Details | Formatted Diff | Diff
patch for landing (46.35 KB, patch)
2019-06-14 10:54 PDT, Saam Barati
no flags Details | Formatted Diff | Diff
patch for landing (46.35 KB, patch)
2019-06-14 10:57 PDT, Saam Barati
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Myles C. Maxfield 2019-06-05 20:51:47 PDT
3 options:
Trapping
Clamping
Zero reads / writes do nothing
Comment 2 Saam Barati 2019-06-11 11:53:26 PDT
Gonna look at this today.
Comment 3 Saam Barati 2019-06-11 13:06:33 PDT
Planning on doing:
"Zero reads / writes do nothing"
Comment 4 Saam Barati 2019-06-11 18:55:21 PDT
Created attachment 371908 [details]
WIP

Has a bunch of Myles's patches applied locally so I can test.
Comment 5 Saam Barati 2019-06-11 19:20:11 PDT
Created attachment 371912 [details]
WIP
Comment 6 Saam Barati 2019-06-11 19:33:36 PDT
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.
Comment 7 Saam Barati 2019-06-11 19:34:05 PDT
I believe the patch accomplishes this, just need to write some tests.
Comment 8 Radar WebKit Bug Importer 2019-06-12 08:54:18 PDT
<rdar://problem/51668853>
Comment 9 Saam Barati 2019-06-12 19:04:42 PDT
Created attachment 372013 [details]
WIP
Comment 10 Saam Barati 2019-06-13 12:49:17 PDT
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.
Comment 11 Saam Barati 2019-06-13 12:50:11 PDT
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.
Comment 12 Robin Morisset 2019-06-13 13:39:51 PDT
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.
Comment 13 Saam Barati 2019-06-13 14:29:12 PDT
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.
Comment 14 Robin Morisset 2019-06-13 15:33:55 PDT
Comment on attachment 372074 [details]
patch

r=me
Comment 15 Saam Barati 2019-06-14 10:54:13 PDT
Created attachment 372127 [details]
patch for landing
Comment 16 Build Bot 2019-06-14 10:57:04 PDT
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.
Comment 17 Saam Barati 2019-06-14 10:57:13 PDT
Created attachment 372128 [details]
patch for landing
Comment 18 Saam Barati 2019-06-14 11:01:27 PDT
landed in:
https://trac.webkit.org/changeset/246438/webkit
Comment 19 Saam Barati 2019-06-14 11:59:33 PDT
follow up landed in:
https://trac.webkit.org/changeset/246439/webkit