Bug 195682 – [WHLSL] Add descriptive error messages

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

[Myles C. Maxfield]

Migrate compiler passes from returning Optional<>s to returning Expected<>s.

[Myles C. Maxfield]

*** Bug 195872 has been marked as a duplicate of this bug. ***

[Radar WebKit Bug Importer]

<rdar://problem/50746322>

[Myles C. Maxfield]

Created attachment 374272 [details]
WIP

[EWS Watchlist]

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.

[Saam Barati]

Spoke with Myles, ima take this over.

[Saam Barati]

Created attachment 374669 [details]
WIP

[Saam Barati]

Created attachment 374700 [details]
WIP

almost done

[Saam Barati]

Created attachment 374714 [details]
patch

[Saam Barati]

Created attachment 374717 [details]
patch

[Myles C. Maxfield]

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

👍

[Saam Barati]

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.

[Saam Barati]

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.

[Myles C. Maxfield]

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"

[Myles C. Maxfield]

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

[Saam Barati]

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()"?

[Saam Barati]

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.

[Saam Barati]

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

[Myles C. Maxfield]

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.