NEW189591
Add ASSERT() to FTL::DFG::LowerDFGToB3::compileAtomicsReadModifyWrite() to fix clang static analyzer warnings 
https://bugs.webkit.org/show_bug.cgi?id=189591
Summary Add ASSERT() to FTL::DFG::LowerDFGToB3::compileAtomicsReadModifyWrite() to fi...
David Kilzer (:ddkilzer)
Reported 2018-09-13 10:46:31 PDT
FTL::DFG::LowerDFGToB3::compileAtomicsReadModifyWrite() assumes `numExtraArgs` is always greater than zero, but the clang static analyzer assumes it could be zero and emits warnings. To fix this, add an `ASSERT(numExtraArgs)` statement so that the static analyzer doesn't assume `numExtraArgs` is zero below that statement.
Attachments
Patch v1 (2.44 KB, patch)
2018-09-13 10:48 PDT, David Kilzer (:ddkilzer) 		saam: review-
ews-watchlist: commit-queue-
DetailsFormatted DiffDiff
Archive of layout-test-results from ews124 for ios-simulator-wk2 (2.62 MB, application/zip)
2018-09-13 13:54 PDT, EWS Watchlist 		no flags
Details
View All Add attachment proposed patch, testcase, etc.
David Kilzer (:ddkilzer)
Comment 1 2018-09-13 10:48:15 PDT
Created attachment 349682 [details] Patch v1
Saam Barati
Comment 2 2018-09-13 10:53:50 PDT
Comment on attachment 349682 [details] Patch v1 I don't understand why we need to make this change. Even if numExtraArgs is zero, we only end up accessing the uninitialized variable when it's non-zero. Seems more like a static analyzer bug than a bug in our code
EWS Watchlist
Comment 3 2018-09-13 13:54:49 PDT
Comment on attachment 349682 [details] Patch v1 Attachment 349682 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: https://webkit-queues.webkit.org/results/9205534 New failing tests: fast/animation/css-animation-resuming-when-visible-with-style-change2.html
EWS Watchlist
Comment 4 2018-09-13 13:54:50 PDT
Created attachment 349697 [details] Archive of layout-test-results from ews124 for ios-simulator-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews124 Port: ios-simulator-wk2 Platform: Mac OS X 10.13.4
David Kilzer (:ddkilzer)
Comment 5 2018-10-25 11:14:05 PDT
(In reply to Saam Barati from comment #2) > Comment on attachment 349682 [details] > Patch v1 > > I don't understand why we need to make this change. Even if numExtraArgs is > zero, we only end up accessing the uninitialized variable when it's > non-zero. Seems more like a static analyzer bug than a bug in our code The static analyzer found multiple paths (that end in the `switch (m_node->op())` statement) where `numExtraArgs == 0 && !storageEdge == false`. Are you saying that if `numExtraArgs == 0` that `!storageEdge` will always be true? If so, the ASSERT needs to be something like this (and moved down after storageEdge is defined): ASSERT(numExtraArgs || !storageEdge); Which is reduced from: (numExtraArgs == 0 && !storageEdge == true) || numExtraArgs != 0)
Saam Barati
Comment 6 2019-01-20 13:34:55 PST
Comment on attachment 349682 [details] Patch v1 View in context: https://bugs.webkit.org/attachment.cgi?id=349682&action=review > Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:3217 > + ASSERT(numExtraArgs); If you look at numExtraAtomicsArgs implementation, this can return zero for AtomicsLoad.
Saam Barati
Comment 7 2019-01-20 13:37:02 PST
Comment on attachment 349682 [details] Patch v1 View in context: https://bugs.webkit.org/attachment.cgi?id=349682&action=review > Source/JavaScriptCore/ChangeLog:15 > + Add ASSERT() to fix clang static analyzer warnings about use of > + uninitialized values if `numExtraArgs == 0`. The assert tells > + the static analyzer that `numExtraArgs` will be non-zero below > + that line of code. What's the uninitialized value it's complaining about?
Note You need to log in before you can comment on or make changes to this bug.