Bug 199675 – [WHLSL] Track code locations correctly throughout the compiler to get good error messages

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

[Robin Morisset]

Currently we only track the first token of an AST node. This is both insufficient (doesn't tell us the end of the production) and wastes space (tokens are larger than just a pair of offsets telling us the beginning and end of a node).

[Robin Morisset]

Created attachment 373851 [details]
Patch

[EWS Watchlist]

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


ERROR: Source/WebCore/Modules/webgpu/WHLSL/AST/WHLSLUnsignedIntegerLiteralType.h:44:  The parameter name "location" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/WebCore/Modules/webgpu/WHLSL/WHLSLParser.cpp:1535:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebCore/Modules/webgpu/WHLSL/WHLSLParser.cpp:1610:  Multi line control clauses should use braces.  [whitespace/braces] [4]
ERROR: Source/WebCore/Modules/webgpu/WHLSL/AST/WHLSLFloatLiteralType.h:44:  The parameter name "location" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/WebCore/Modules/webgpu/WHLSL/WHLSLLexer.h:51:  Missing space inside { }.  [whitespace/braces] [5]
ERROR: Source/WebCore/Modules/webgpu/WHLSL/WHLSLLexer.h:52:  The parameter name "token" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/WebCore/Modules/webgpu/WHLSL/WHLSLLexer.h:56:  Missing space inside { }.  [whitespace/braces] [5]
ERROR: Source/WebCore/Modules/webgpu/WHLSL/WHLSLLexer.h:185:  The parameter name "lexer" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/WebCore/Modules/webgpu/WHLSL/WHLSLLexer.h:196:  Missing space inside { }.  [whitespace/braces] [5]
ERROR: Source/WebCore/Modules/webgpu/WHLSL/AST/WHLSLIntegerLiteralType.h:44:  The parameter name "location" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/WebCore/Modules/webgpu/WHLSL/AST/WHLSLTypeReference.h:59:  The parameter name "location" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 11 in 81 files


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

[Myles C. Maxfield]

Comment on attachment 373851 [details]
Patch

Now that we’re downgrading string indexes to 32-bits, can we do an early check that the string isn’t too long, and early-out if it is?

Also, because there’s no ChangeLog, it isn’t clear what you were hoping to accomplish with this patch. Because there’s not really a behavior change, please include some results in the ChangeLog (performance results, or memory size reduction, etc.).

r=me assuming you do that.

[Robin Morisset]

Created attachment 373853 [details]
Patch

Three changes:
- Added the Changelog I had forgotten
- Fixed style issues
- Added a check that the size of the input is < UINT32_MAX as suggested by Myles.

[Robin Morisset]

Created attachment 373854 [details]
Patch

Actually, WTF::String is already limited to a 32-bit length, so that check is useless.

[WebKit Commit Bot]

Comment on attachment 373854 [details]
Patch

Clearing flags on attachment: 373854

Committed r247316: <https://trac.webkit.org/changeset/247316>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/52907363>

[Ryan Haddad]

This change broke the Mojave build:
https://build.webkit.org/builders/Apple%20Mojave%20Debug%20%28Build%29/builds/6946/steps/compile-webkit/logs/stdio

[Ryan Haddad]

Reverted r247316 for reason:

Broke Mojave build

Committed r247325: <https://trac.webkit.org/changeset/247325>

[Robin Morisset]

Created attachment 373864 [details]
Patch

I had forgotten to mark two Token::stringView and CodeLocation::CodeLocation(const Token&) as "inline".

[Ryan Haddad]

It looks like it also caused some webgpu layout tests to crash on Mojave:
https://build.webkit.org/results/Apple%20Mojave%20Release%20WK2%20(Tests)/r247324%20(5237)/results.html

Thread 0 Crashed:: Dispatch queue: com.apple.main-thread
0   com.apple.WebCore             	0x000000010a8baa8d WebCore::WHLSL::Parser::parseExpression() + 477 (WHLSLParser.cpp:1520)
1   com.apple.WebCore             	0x000000010a8c2d15 WebCore::WHLSL::Parser::parseTerm() + 261 (WHLSLParser.cpp:2037)
2   com.apple.WebCore             	0x000000010a8c22c0 WebCore::WHLSL::Parser::parsePossibleSuffix(bool*) + 752 (WHLSLParser.cpp:1948)
3   com.apple.WebCore             	0x000000010a8be7d3 WebCore::WHLSL::Parser::parsePossiblePrefix(bool*) + 579
4   com.apple.WebCore             	0x000000010a8bd7fe WebCore::WHLSL::Parser::parsePossibleTernaryConditional() + 30 (WHLSLParser.cpp:1608)
5   com.apple.WebCore             	0x000000010a8ba8ce WebCore::WHLSL::Parser::parseExpression() + 30 (WHLSLParser.cpp:1508)
6   com.apple.WebCore             	0x000000010a8bcfab WebCore::WHLSL::Parser::parseDoWhileLoop() + 155 (WHLSLParser.cpp:1206)
7   com.apple.WebCore             	0x000000010a8b9bd0 WebCore::WHLSL::Parser::parseStatement() + 1040 (WHLSLParser.cpp:1281)
8   com.apple.WebCore             	0x000000010a8b961f WebCore::WHLSL::Parser::parseBlockBody() + 127 (WHLSLParser.cpp:1066)
9   com.apple.WebCore             	0x000000010a8b7c63 WebCore::WHLSL::Parser::parseBlock() + 67 (WHLSLParser.cpp:1054)
10  com.apple.WebCore             	0x000000010a8b0e8f WebCore::WHLSL::Parser::parseFunctionDefinition() + 63 (WHLSLParser.cpp:955)
11  com.apple.WebCore             	0x000000010a8af152 WebCore::WHLSL::Parser::parse(WebCore::WHLSL::Program&, WTF::StringView, WebCore::WHLSL::Parser::Mode) + 626 (WHLSLParser.cpp:116)
12  com.apple.WebCore             	0x000000010a8c3ab9 WebCore::WHLSL::prepareShared(WTF::String&) + 281 (WHLSLPrepare.cpp:113)
13  com.apple.WebCore             	0x000000010a8c3586 WebCore::WHLSL::prepare(WTF::String&, WebCore::WHLSL::RenderPipelineDescriptor&) + 38 (WHLSLPrepare.cpp:153)
14  com.apple.WebCore             	0x000000010a1b8b83 WebCore::GPURenderPipeline::tryCreate(WebCore::GPUDevice const&, WebCore::GPURenderPipelineDescriptor const&) + 3555 (GPURenderPipelineMetal.mm:410)
15  com.apple.WebCore             	0x000000010b29dd4e WebCore::GPUDevice::tryCreateRenderPipeline(WebCore::GPURenderPipelineDescriptor const&) const + 14 (GPUDevice.cpp:85)
16  com.apple.WebCore             	0x000000010a8922f3 WebCore::WebGPUDevice::createRenderPipeline(WebCore::WebGPURenderPipelineDescriptor const&) const + 67 (WebGPUDevice.cpp:156)
17  com.apple.WebCore             	0x000000010a6160a0 WebCore::jsWebGPUDevicePrototypeFunctionCreateRenderPipeline(JSC::ExecState*) + 304 (JSWebGPUDevice.cpp:390)
18  ???                           	0x00002544d620116b 0 + 40977580429675
19  com.apple.JavaScriptCore      	0x000000010e94eec1 llint_entry + 93096
20  com.apple.JavaScriptCore      	0x000000010e94eec1 llint_entry + 93096

[Robin Morisset]

Thanks, I used a value that had been WTFMove-d away accidentally. It did not cause problems on any of the tests I ran locally, because they did not have comma expressions.

(In reply to Ryan Haddad from comment #12)
> It looks like it also caused some webgpu layout tests to crash on Mojave:
> https://build.webkit.org/results/Apple%20Mojave%20Release%20WK2%20(Tests)/
> r247324%20(5237)/results.html
> 
> Thread 0 Crashed:: Dispatch queue: com.apple.main-thread
> 0   com.apple.WebCore             	0x000000010a8baa8d
> WebCore::WHLSL::Parser::parseExpression() + 477 (WHLSLParser.cpp:1520)
> 1   com.apple.WebCore             	0x000000010a8c2d15
> WebCore::WHLSL::Parser::parseTerm() + 261 (WHLSLParser.cpp:2037)
> 2   com.apple.WebCore             	0x000000010a8c22c0
> WebCore::WHLSL::Parser::parsePossibleSuffix(bool*) + 752
> (WHLSLParser.cpp:1948)
> 3   com.apple.WebCore             	0x000000010a8be7d3
> WebCore::WHLSL::Parser::parsePossiblePrefix(bool*) + 579
> 4   com.apple.WebCore             	0x000000010a8bd7fe
> WebCore::WHLSL::Parser::parsePossibleTernaryConditional() + 30
> (WHLSLParser.cpp:1608)
> 5   com.apple.WebCore             	0x000000010a8ba8ce
> WebCore::WHLSL::Parser::parseExpression() + 30 (WHLSLParser.cpp:1508)
> 6   com.apple.WebCore             	0x000000010a8bcfab
> WebCore::WHLSL::Parser::parseDoWhileLoop() + 155 (WHLSLParser.cpp:1206)
> 7   com.apple.WebCore             	0x000000010a8b9bd0
> WebCore::WHLSL::Parser::parseStatement() + 1040 (WHLSLParser.cpp:1281)
> 8   com.apple.WebCore             	0x000000010a8b961f
> WebCore::WHLSL::Parser::parseBlockBody() + 127 (WHLSLParser.cpp:1066)
> 9   com.apple.WebCore             	0x000000010a8b7c63
> WebCore::WHLSL::Parser::parseBlock() + 67 (WHLSLParser.cpp:1054)
> 10  com.apple.WebCore             	0x000000010a8b0e8f
> WebCore::WHLSL::Parser::parseFunctionDefinition() + 63 (WHLSLParser.cpp:955)
> 11  com.apple.WebCore             	0x000000010a8af152
> WebCore::WHLSL::Parser::parse(WebCore::WHLSL::Program&, WTF::StringView,
> WebCore::WHLSL::Parser::Mode) + 626 (WHLSLParser.cpp:116)
> 12  com.apple.WebCore             	0x000000010a8c3ab9
> WebCore::WHLSL::prepareShared(WTF::String&) + 281 (WHLSLPrepare.cpp:113)
> 13  com.apple.WebCore             	0x000000010a8c3586
> WebCore::WHLSL::prepare(WTF::String&,
> WebCore::WHLSL::RenderPipelineDescriptor&) + 38 (WHLSLPrepare.cpp:153)
> 14  com.apple.WebCore             	0x000000010a1b8b83
> WebCore::GPURenderPipeline::tryCreate(WebCore::GPUDevice const&,
> WebCore::GPURenderPipelineDescriptor const&) + 3555
> (GPURenderPipelineMetal.mm:410)
> 15  com.apple.WebCore             	0x000000010b29dd4e
> WebCore::GPUDevice::tryCreateRenderPipeline(WebCore::
> GPURenderPipelineDescriptor const&) const + 14 (GPUDevice.cpp:85)
> 16  com.apple.WebCore             	0x000000010a8922f3
> WebCore::WebGPUDevice::createRenderPipeline(WebCore::
> WebGPURenderPipelineDescriptor const&) const + 67 (WebGPUDevice.cpp:156)
> 17  com.apple.WebCore             	0x000000010a6160a0
> WebCore::jsWebGPUDevicePrototypeFunctionCreateRenderPipeline(JSC::
> ExecState*) + 304 (JSWebGPUDevice.cpp:390)
> 18  ???                           	0x00002544d620116b 0 + 40977580429675
> 19  com.apple.JavaScriptCore      	0x000000010e94eec1 llint_entry + 93096
> 20  com.apple.JavaScriptCore      	0x000000010e94eec1 llint_entry + 93096

[Robin Morisset]

Created attachment 373865 [details]
Patch

Fixed the use-after-move problems.

[WebKit Commit Bot]

Comment on attachment 373865 [details]
Patch

Clearing flags on attachment: 373865

Committed r247329: <https://trac.webkit.org/changeset/247329>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.