Summary: | [WHLSL] Track code locations correctly throughout the compiler to get good error messages | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Robin Morisset <rmorisset> | ||||||||||||
Component: | WebGPU | Assignee: | Robin Morisset <rmorisset> | ||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||
Severity: | Normal | CC: | commit-queue, ews-watchlist, mmaxfield, ryanhaddad, webkit-bug-importer | ||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||
Hardware: | Unspecified | ||||||||||||||
OS: | Unspecified | ||||||||||||||
Attachments: |
|
Description
Robin Morisset
2019-07-10 11:18:29 PDT
Created attachment 373851 [details]
Patch
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 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.
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.
Created attachment 373854 [details]
Patch
Actually, WTF::String is already limited to a 32-bit length, so that check is useless.
Comment on attachment 373854 [details] Patch Clearing flags on attachment: 373854 Committed r247316: <https://trac.webkit.org/changeset/247316> All reviewed patches have been landed. Closing bug. This change broke the Mojave build: https://build.webkit.org/builders/Apple%20Mojave%20Debug%20%28Build%29/builds/6946/steps/compile-webkit/logs/stdio Reverted r247316 for reason: Broke Mojave build Committed r247325: <https://trac.webkit.org/changeset/247325> Created attachment 373864 [details]
Patch
I had forgotten to mark two Token::stringView and CodeLocation::CodeLocation(const Token&) as "inline".
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 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 Created attachment 373865 [details]
Patch
Fixed the use-after-move problems.
Comment on attachment 373865 [details] Patch Clearing flags on attachment: 373865 Committed r247329: <https://trac.webkit.org/changeset/247329> All reviewed patches have been landed. Closing bug. |