Bug 199675 - [WHLSL] Track code locations correctly throughout the compiler to get good error messages
Summary: [WHLSL] Track code locations correctly throughout the compiler to get good er...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebGPU (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Robin Morisset
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-07-10 11:18 PDT by Robin Morisset
Modified: 2019-07-10 15:34 PDT (History)
5 users (show)

See Also:


Attachments
Patch (248.63 KB, patch)
2019-07-10 11:21 PDT, Robin Morisset
mmaxfield: review+
mmaxfield: commit-queue-
Details | Formatted Diff | Diff
Patch (266.34 KB, patch)
2019-07-10 11:56 PDT, Robin Morisset
rmorisset: commit-queue-
Details | Formatted Diff | Diff
Patch (265.64 KB, patch)
2019-07-10 12:02 PDT, Robin Morisset
no flags Details | Formatted Diff | Diff
Patch (265.66 KB, patch)
2019-07-10 13:58 PDT, Robin Morisset
rmorisset: commit-queue-
Details | Formatted Diff | Diff
Patch (265.78 KB, patch)
2019-07-10 14:51 PDT, Robin Morisset
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Robin Morisset 2019-07-10 11:18:29 PDT
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).
Comment 1 Robin Morisset 2019-07-10 11:21:33 PDT
Created attachment 373851 [details]
Patch
Comment 2 Build Bot 2019-07-10 11:24:15 PDT
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.
Comment 3 Myles C. Maxfield 2019-07-10 11:31:30 PDT
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.
Comment 4 Robin Morisset 2019-07-10 11:56:00 PDT
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.
Comment 5 Robin Morisset 2019-07-10 12:02:20 PDT
Created attachment 373854 [details]
Patch

Actually, WTF::String is already limited to a 32-bit length, so that check is useless.
Comment 6 WebKit Commit Bot 2019-07-10 12:44:56 PDT
Comment on attachment 373854 [details]
Patch

Clearing flags on attachment: 373854

Committed r247316: <https://trac.webkit.org/changeset/247316>
Comment 7 WebKit Commit Bot 2019-07-10 12:44:57 PDT
All reviewed patches have been landed.  Closing bug.
Comment 8 Radar WebKit Bug Importer 2019-07-10 12:45:27 PDT
<rdar://problem/52907363>
Comment 10 Ryan Haddad 2019-07-10 13:37:34 PDT
Reverted r247316 for reason:

Broke Mojave build

Committed r247325: <https://trac.webkit.org/changeset/247325>
Comment 11 Robin Morisset 2019-07-10 13:58:32 PDT
Created attachment 373864 [details]
Patch

I had forgotten to mark two Token::stringView and CodeLocation::CodeLocation(const Token&) as "inline".
Comment 12 Ryan Haddad 2019-07-10 14:35:14 PDT
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
Comment 13 Robin Morisset 2019-07-10 14:44:28 PDT
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
Comment 14 Robin Morisset 2019-07-10 14:51:02 PDT
Created attachment 373865 [details]
Patch

Fixed the use-after-move problems.
Comment 15 WebKit Commit Bot 2019-07-10 15:34:04 PDT
Comment on attachment 373865 [details]
Patch

Clearing flags on attachment: 373865

Committed r247329: <https://trac.webkit.org/changeset/247329>
Comment 16 WebKit Commit Bot 2019-07-10 15:34:06 PDT
All reviewed patches have been landed.  Closing bug.