Bug 199886

Summary: [WHLSL] The lexer should not choke on a single '/'
Product: WebKit Reporter: Robin Morisset <rmorisset>
Component: WebGPUAssignee: Robin Morisset <rmorisset>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, mmaxfield, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch
mmaxfield: review+, mmaxfield: commit-queue-
Patch for landing none

Robin Morisset
Reported 2019-07-17 15:30:38 PDT
I introduced a bug in the lexer when I last optimized its skipWhitespaceAndComments, and it now hangs whenever it encounters a single '/' (as in the function smoothstep in the stdlib).
Attachments
Patch (5.68 KB, patch)
2019-07-17 15:34 PDT, Robin Morisset 		no flags
DetailsFormatted DiffDiff
Patch (8.44 KB, patch)
2019-07-17 17:15 PDT, Robin Morisset 		no flags
DetailsFormatted DiffDiff
Patch (8.98 KB, patch)
2019-07-17 17:22 PDT, Robin Morisset 		mmaxfield: review+
mmaxfield: commit-queue-
DetailsFormatted DiffDiff
Patch for landing (8.56 KB, patch)
2019-07-17 18:17 PDT, Robin Morisset 		no flags
DetailsFormatted DiffDiff
Show Obsolete (3) View All Add attachment proposed patch, testcase, etc.
Robin Morisset
Comment 1 2019-07-17 15:34:44 PDT
Created attachment 374342 [details] Patch
Myles C. Maxfield
Comment 2 2019-07-17 15:38:25 PDT
Comment on attachment 374342 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=374342&action=review > Source/WebCore/Modules/webgpu/WHLSL/WHLSLPrepare.cpp:121 > + if (auto parseFailure = parser.parse(program, decompressAndDecodeStandardLibrary(), Parser::Mode::StandardLibrary)) { We probably want this to be able to use the static NeverDestroyed standard library in StandardLibraryUtilities.cpp so we don’t have a whole lot of copies of it and don’t have to decompress it multiple times
Myles C. Maxfield
Comment 3 2019-07-17 15:41:12 PDT
No test?
Robin Morisset
Comment 4 2019-07-17 17:15:43 PDT
Created attachment 374354 [details] Patch Added a test, and modified parseFullStandardLibrary so we only load and decompress the stdlib once, using the static NeverDestroyed<String> as suggested by Myles.
Robin Morisset
Comment 5 2019-07-17 17:22:19 PDT
Created attachment 374356 [details] Patch
Myles C. Maxfield
Comment 6 2019-07-17 17:57:12 PDT
Comment on attachment 374356 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=374356&action=review > Source/WebCore/Modules/webgpu/WHLSL/WHLSLPrepare.cpp:124 > + if (dumpPassFailure) > + dataLogLn("failed to parse the standard library: ", *parseFailure); > + return WTF::nullopt; > + } See comment below. > Source/WebCore/Modules/webgpu/WHLSL/WHLSLStandardLibraryUtilities.cpp:81 > + return parseFailure; The standard library should never be a parse failure. Instead of returning the error, we should ASSERT(). The return type of this function should be "void".
Robin Morisset
Comment 7 2019-07-17 18:17:10 PDT
Created attachment 374361 [details] Patch for landing
WebKit Commit Bot
Comment 8 2019-07-17 19:14:40 PDT
The commit-queue encountered the following flaky tests while processing attachment 374361 [details]: http/tests/IndexedDB/collect-IDB-objects.https.html bug 199881 (author: youennf@gmail.com) The commit-queue is continuing to process your patch.
WebKit Commit Bot
Comment 9 2019-07-17 19:15:03 PDT
The commit-queue encountered the following flaky tests while processing attachment 374361 [details]: storage/indexeddb/dont-wedge.html bug 199883 (authors: beidson@apple.com, commit-queue@webkit.org, dgrogan@chromium.org, and mark.lam@apple.com) The commit-queue is continuing to process your patch.
WebKit Commit Bot
Comment 10 2019-07-17 19:30:01 PDT
Comment on attachment 374361 [details] Patch for landing Clearing flags on attachment: 374361 Committed r247554: <https://trac.webkit.org/changeset/247554>
WebKit Commit Bot
Comment 11 2019-07-17 19:30:02 PDT
All reviewed patches have been landed. Closing bug.
Radar WebKit Bug Importer
Comment 12 2019-07-17 19:31:15 PDT
<rdar://problem/53233949>
Note You need to log in before you can comment on or make changes to this bug.