Bug 195682 - [WHLSL] Add descriptive error messages
Summary: [WHLSL] Add descriptive error messages
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebGPU (show other bugs)
Version: Other
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Saam Barati
URL:
Keywords: InRadar
: 195872 (view as bug list)
Depends on:
Blocks: 200049
  Show dependency treegraph
 
Reported: 2019-03-13 10:14 PDT by Myles C. Maxfield
Modified: 2019-07-25 16:24 PDT (History)
7 users (show)

See Also:


Attachments
WIP (16.17 KB, patch)
2019-07-16 19:38 PDT, Myles C. Maxfield
no flags Details | Formatted Diff | Diff
WIP (122.21 KB, patch)
2019-07-22 20:18 PDT, Saam Barati
no flags Details | Formatted Diff | Diff
WIP (137.34 KB, patch)
2019-07-23 12:59 PDT, Saam Barati
no flags Details | Formatted Diff | Diff
patch (152.42 KB, patch)
2019-07-23 15:26 PDT, Saam Barati
no flags Details | Formatted Diff | Diff
patch (152.50 KB, patch)
2019-07-23 15:29 PDT, Saam Barati
mmaxfield: review+
mmaxfield: commit-queue-
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-03-13 10:14:33 PDT
Migrate compiler passes from returning Optional<>s to returning Expected<>s.
Comment 1 Myles C. Maxfield 2019-05-13 16:31:34 PDT
*** Bug 195872 has been marked as a duplicate of this bug. ***
Comment 2 Radar WebKit Bug Importer 2019-05-13 17:08:19 PDT
<rdar://problem/50746322>
Comment 3 Myles C. Maxfield 2019-07-16 19:38:08 PDT
Created attachment 374272 [details]
WIP
Comment 4 Build Bot 2019-07-16 19:40:15 PDT
Attachment 374272 [details] did not pass style-queue:


ERROR: Source/WebCore/ChangeLog:8:  You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]
Total errors found: 1 in 7 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 5 Saam Barati 2019-07-22 14:23:53 PDT
Spoke with Myles, ima take this over.
Comment 6 Saam Barati 2019-07-22 20:18:13 PDT
Created attachment 374669 [details]
WIP
Comment 7 Saam Barati 2019-07-23 12:59:05 PDT
Created attachment 374700 [details]
WIP

almost done
Comment 8 Saam Barati 2019-07-23 15:26:26 PDT
Created attachment 374714 [details]
patch
Comment 9 Saam Barati 2019-07-23 15:29:01 PDT
Created attachment 374717 [details]
patch
Comment 10 Myles C. Maxfield 2019-07-23 19:07:01 PDT
Comment on attachment 374717 [details]
patch

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

> Source/WebCore/ChangeLog:13
> +        Passes that can fail no longer return a boolean. Instead, they return Expected<void, Error>.

Not Optional<Error>? Expected<void> seems weird

> Source/WebCore/ChangeLog:15
> +        requires an Error as an argument. So anywhere in Visitor that might fail is

šŸ‘
Comment 11 Saam Barati 2019-07-23 23:15:50 PDT
Comment on attachment 374717 [details]
patch

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

>> Source/WebCore/ChangeLog:13
>> +        Passes that can fail no longer return a boolean. Instead, they return Expected<void, Error>.
> 
> Not Optional<Error>? Expected<void> seems weird

I think Expected is nicer for this. It documents the intent of the API in a clearer way than Optional. I used to think this was a bit weird too, but we use this in JSC for Wasm code and it has worked out nicely there too. That said, Iā€™m not opposed to changing it.
Comment 12 Saam Barati 2019-07-24 15:40:53 PDT
Comment on attachment 374717 [details]
patch

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

>>> Source/WebCore/ChangeLog:13
>>> +        Passes that can fail no longer return a boolean. Instead, they return Expected<void, Error>.
>> 
>> Not Optional<Error>? Expected<void> seems weird
> 
> I think Expected is nicer for this. It documents the intent of the API in a clearer way than Optional. I used to think this was a bit weird too, but we use this in JSC for Wasm code and it has worked out nicely there too. That said, Iā€™m not opposed to changing it.

One other small nit picky reason I prefer it this way: I like truthy to mean success, and falsey to mean failure. If we switched to Optional, truthy means failure, and falsey means success.
Comment 13 Myles C. Maxfield 2019-07-24 17:45:46 PDT
Comment on attachment 374717 [details]
patch

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

> Source/WebCore/Modules/webgpu/WHLSL/WHLSLCheckDuplicateFunctions.cpp:130
> +                            return makeUnexpected(Error("Cannot define custom array ander."));

Array reference ander*

> Source/WebCore/Modules/webgpu/WHLSL/WHLSLCheckTextureReferences.cpp:90
> +        setError(Error("Cannot have a pointer to a texture.", pointerType.codeLocation()));

texture or sampler

> Source/WebCore/Modules/webgpu/WHLSL/WHLSLCheckTextureReferences.cpp:99
> +        setError(Error("Cannot have an array reference to a texture.", arrayReferenceType.codeLocation()));

Ditto

> Source/WebCore/Modules/webgpu/WHLSL/WHLSLCheckTextureReferences.cpp:108
> +        setError(Error("Cannot have an array of textures.", arrayType.codeLocation()));

Ditto

> Source/WebCore/Modules/webgpu/WHLSL/WHLSLCheckTextureReferences.cpp:121
> +    return textureReferencesChecker.expectedError();

Truthy means no error, so can we call this "success()" or something?

> Source/WebCore/Modules/webgpu/WHLSL/WHLSLChecker.cpp:300
>                      return false;

I think this function should preserve the error. We generate an error in PODChecker but disregard it here.

Also, is there another bug for finding all the Boolean functions and replacing those with returning Errors?

> Source/WebCore/Modules/webgpu/WHLSL/WHLSLChecker.cpp:593
> +        setError(Error("Entrypoint cannot be an operator.", functionDefinition.codeLocation()));

"Operator does not match expected signature"
Comment 14 Myles C. Maxfield 2019-07-24 19:31:46 PDT
Comment on attachment 374717 [details]
patch

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

> Source/WebCore/Modules/webgpu/WHLSL/WHLSLChecker.cpp:810
> +        setError(Error("Left hand side of assignment does not match the type of the right hand side.", readModifyWriteExpression.codeLocation()));

Assignment? Don't you mean read-modify-write?

> Source/WebCore/Modules/webgpu/WHLSL/WHLSLChecker.cpp:887
> +        setError(Error("Cannot take the address of a value without a type.", makeArrayReferenceExpression.codeLocation()));

Cannot make an array reference from a value without a type

> Source/WebCore/Modules/webgpu/WHLSL/WHLSLChecker.cpp:903
> +            setError(Error("Cannot take the address of a non lvalue.", makeArrayReferenceExpression.codeLocation()));

Cannot make an array reference from a non-left-value

> Source/WebCore/Modules/webgpu/WHLSL/WHLSLChecker.cpp:917
> +        setError(Error("Cannot take the address of a non lvalue.", makeArrayReferenceExpression.codeLocation()));

Ditto

> Source/WebCore/Modules/webgpu/WHLSL/WHLSLChecker.cpp:1102
> +        setError(Error("Cannot return nothing from a non-void function.", returnStatement.codeLocation()));

I think this is backwards. It has to return void.

> Source/WebCore/Modules/webgpu/WHLSL/WHLSLChecker.cpp:1434
> +        setError(Error("Cannot resolve function call to a concrete callee. Make sure you are using compatible types.", callExpression.codeLocation()));

I think this should be "Could not find any functions with appropriate name"

> Source/WebCore/Modules/webgpu/WHLSL/WHLSLChecker.cpp:1440
> +        setError(Error("Cannot resolve function call to a concrete callee. Make sure you are using compatible types.", callExpression.codeLocation()));

Can we add a FIXME and bug to add more detail here? It would be good to know why all the overrides were rejected.

> Source/WebCore/Modules/webgpu/WHLSL/WHLSLPrepare.cpp:154
> +Expected<RenderPrepareResult, String> prepare(String& whlslSource, RenderPipelineDescriptor& renderPipelineDescriptor)

I think the API should return an Error object, which can have some fields inside it. The code that actually outputs this stuff (into the inspector) might want to linkify stuff, etc, and serializing everything to a string loses that information.

> Source/WebCore/Modules/webgpu/WHLSL/WHLSLPropertyResolver.cpp:428
>          auto callExpression = getterCall(propertyAccessExpression, relevantAnder, previousLeftValue, indexVariable ? &*indexVariable : nullptr);

getterCall() doesn't actually appear to ever return nullopt :(

> Source/WebCore/Modules/webgpu/WHLSL/WHLSLPropertyResolver.cpp:451
>      auto modificationResult = modification(WTFMove(lastGetterCallExpression));

Similarly, modification() seems to only return nullopt if getterCall() returns nullopt, which seems to never happen.

> Source/WebCore/Modules/webgpu/WHLSL/WHLSLPropertyResolver.cpp:471
>          }, indexVariable ? &*indexVariable : nullptr);

setterCall() doesn't actually appear to ever return nullopt :(

> Source/WebCore/Modules/webgpu/WHLSL/WHLSLPropertyResolver.cpp:498
>          }, indexVariables[indexVariables.size() - 1] ? &*(indexVariables[indexVariables.size() - 1]) : nullptr);

Ditto

> Source/WebCore/Modules/webgpu/WHLSL/WHLSLPropertyResolver.cpp:665
> +        setError(Error("Unexpected lhs value for read modify write expression.", readModifyWriteExpression.leftValue().codeLocation()));

Base of read modify write expression is a right-value

> Source/WebCore/Modules/webgpu/WHLSL/WHLSLStatementBehaviorChecker.cpp:234
> +                return makeUnexpected(Error("void functions must return nothing."));

Or not return?

> Source/WebCore/Modules/webgpu/WHLSL/WHLSLStatementBehaviorChecker.cpp:237
> +                return makeUnexpected(Error("Non-void functions must return something."));

in all code paths
Comment 15 Saam Barati 2019-07-25 11:39:04 PDT
Comment on attachment 374717 [details]
patch

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

>> Source/WebCore/Modules/webgpu/WHLSL/WHLSLCheckTextureReferences.cpp:121
>> +    return textureReferencesChecker.expectedError();
> 
> Truthy means no error, so can we call this "success()" or something?

What about just "result()"?
Comment 16 Saam Barati 2019-07-25 12:16:32 PDT
Comment on attachment 374717 [details]
patch

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

>> Source/WebCore/Modules/webgpu/WHLSL/WHLSLChecker.cpp:300
>>                      return false;
> 
> I think this function should preserve the error. We generate an error in PODChecker but disregard it here.
> 
> Also, is there another bug for finding all the Boolean functions and replacing those with returning Errors?

filed: https://bugs.webkit.org/show_bug.cgi?id=200132

It's a bit difficult here since the only caller of this is checker for property access instructions. We'd need to find a nice way to thread the error message through, since it's not obvious which of the many errors we could see are relevant.

>> Source/WebCore/Modules/webgpu/WHLSL/WHLSLPrepare.cpp:154
>> +Expected<RenderPrepareResult, String> prepare(String& whlslSource, RenderPipelineDescriptor& renderPipelineDescriptor)
> 
> I think the API should return an Error object, which can have some fields inside it. The code that actually outputs this stuff (into the inspector) might want to linkify stuff, etc, and serializing everything to a string loses that information.

We can fix this in the future. Right now, we rely on a helper in the lexer to translate CodeLocation => line/column. As we need more information in the future, we can easily refactor this.

>> Source/WebCore/Modules/webgpu/WHLSL/WHLSLPropertyResolver.cpp:428
>>          auto callExpression = getterCall(propertyAccessExpression, relevantAnder, previousLeftValue, indexVariable ? &*indexVariable : nullptr);
> 
> getterCall() doesn't actually appear to ever return nullopt :(

will remove the Optional here and below.
Comment 17 Saam Barati 2019-07-25 12:43:06 PDT
landed in:
https://trac.webkit.org/changeset/247834/webkit
Comment 18 Myles C. Maxfield 2019-07-25 16:24:57 PDT
Comment on attachment 374717 [details]
patch

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

>>> Source/WebCore/Modules/webgpu/WHLSL/WHLSLChecker.cpp:300
>>>                      return false;
>> 
>> I think this function should preserve the error. We generate an error in PODChecker but disregard it here.
>> 
>> Also, is there another bug for finding all the Boolean functions and replacing those with returning Errors?
> 
> filed: https://bugs.webkit.org/show_bug.cgi?id=200132
> 
> It's a bit difficult here since the only caller of this is checker for property access instructions. We'd need to find a nice way to thread the error message through, since it's not obvious which of the many errors we could see are relevant.

The JS implementation concatenated all the constituent errors together to explain in lots of detail what was going wrong.