llvmorg-github-actions[bot] wrote:
<!--LLVM PR SUMMARY COMMENT--> @llvm/pr-subscribers-clang-static-analyzer-1 Author: Marco Milanese (marco-milanese-sonarsource) <details> <summary>Changes</summary> This PR fixes a false positive on `UnreachableCodeChecker` occurring when STU exploration exhausts the steps budget and the CTU phase completes its exploration (i.e., has no more work to do). There are three code pieces to consider: * When we transition from STU exploration to CTU exploration ([`CoreEngine.h:173`](https://github.com/llvm/llvm-project/blob/main/clang/lib/StaticAnalyzer/Core/CoreEngine.cpp#L173)) we move `CTUWList` to `WList`: ```c++ if (CTUWList) { NumSTUSteps += STUSteps; const unsigned MinCTUSteps = this->ExprEng.getAnalysisManager().options.CTUMaxNodesMin; const unsigned Pct = this->ExprEng.getAnalysisManager().options.CTUMaxNodesPercentage; unsigned MaxCTUSteps = std::max(STUSteps * Pct / 100, MinCTUSteps); WList = std::move(CTUWList); // remaining STU work gets discarded const unsigned CTUSteps = ProcessWList(MaxCTUSteps); NumCTUSteps += CTUSteps; } ``` * At the end of the analysis `UnreachableCodeChecker` checks for code that was not explored. This is corresponds to actual dead code only as long as the exploration is complete, thus (correctly) the checker calls `hasWorkRemaining` ([`CoreEngine.h:149`](https://github.com/llvm/llvm-project/blob/main/clang/include/clang/StaticAnalyzer/Core/PathSensitive/CoreEngine.h#L149-L151)). If such predicate is true it immediately bails out as any finding may be due to the uncomplete exploration. * `hasWorkRemaining` relies on `WList` being empty or not to determine if there is work remaining (i.e., complete/uncomplete exploration): ```c++ bool hasWorkRemaining() const { return wasBlocksExhausted() || WList->hasWork() || wasBlockAborted(); } ``` Now, consider the scenario where: 1. STU exploration was *not* exhaustive (i.e., some code was not analyzed because we did not have enough steps budget). 2. CTU exploration was exhaustive. In this case `hasWorkRemaining` should return `true` as STU exploration had more work to do, but because we override it with the CTU worklist (and the latter is fully emptied), then `hasWorkRemaining` will return `false`, raising the false positive. This can be simulated in the regression test by setting a very low STU budget and enabling CTU mode. Actually, we don't need to do any CTU work or even provide extra TUs; this is because the switch to the CTU worklist (empty) is always done when CTU mode is enabled. --- Full diff: https://github.com/llvm/llvm-project/pull/219225.diff 3 Files Affected: - (modified) clang/include/clang/StaticAnalyzer/Core/PathSensitive/CoreEngine.h (+8-3) - (modified) clang/lib/StaticAnalyzer/Core/CoreEngine.cpp (+1) - (added) clang/test/Analysis/ctu/stu-workremaining.cpp (+21) ``````````diff diff --git a/clang/include/clang/StaticAnalyzer/Core/PathSensitive/CoreEngine.h b/clang/include/clang/StaticAnalyzer/Core/PathSensitive/CoreEngine.h index d26c0d9257b0f..46399793306b8 100644 --- a/clang/include/clang/StaticAnalyzer/Core/PathSensitive/CoreEngine.h +++ b/clang/include/clang/StaticAnalyzer/Core/PathSensitive/CoreEngine.h @@ -82,6 +82,10 @@ class CoreEngine { /// usually because it could not reason about something. BlocksAborted blocksAborted; + /// Whether the single-TU phase ran out of budget with work left over. + /// The CTU phase replaces \c WList, so this has to be remembered separately. + bool STUHadWorkRemaining = false; + /// The information about functions shared by the whole translation unit. /// (This data is owned by AnalysisConsumer.) FunctionSummariesTy *FunctionSummaries; @@ -146,9 +150,10 @@ class CoreEngine { // Functions for external checking of whether we have unfinished work. bool wasBlockAborted() const { return !blocksAborted.empty(); } bool wasBlocksExhausted() const { return !blocksExhausted.empty(); } - bool hasWorkRemaining() const { return wasBlocksExhausted() || - WList->hasWork() || - wasBlockAborted(); } + bool hasWorkRemaining() const { + return wasBlocksExhausted() || WList->hasWork() || STUHadWorkRemaining || + wasBlockAborted(); + } /// Inform the CoreEngine that a basic block was aborted because /// it could not be completely analyzed. diff --git a/clang/lib/StaticAnalyzer/Core/CoreEngine.cpp b/clang/lib/StaticAnalyzer/Core/CoreEngine.cpp index 307c96b23b206..85fddd16057a9 100644 --- a/clang/lib/StaticAnalyzer/Core/CoreEngine.cpp +++ b/clang/lib/StaticAnalyzer/Core/CoreEngine.cpp @@ -161,6 +161,7 @@ bool CoreEngine::ExecuteWorkList(const StackFrame *SF, unsigned MaxSteps, return MaxSteps - Steps; }; const unsigned STUSteps = ProcessWList(MaxSteps); + STUHadWorkRemaining = WList->hasWork(); if (CTUWList) { NumSTUSteps += STUSteps; diff --git a/clang/test/Analysis/ctu/stu-workremaining.cpp b/clang/test/Analysis/ctu/stu-workremaining.cpp new file mode 100644 index 0000000000000..1524a98ec9a93 --- /dev/null +++ b/clang/test/Analysis/ctu/stu-workremaining.cpp @@ -0,0 +1,21 @@ +// RUN: %clang_analyze_cc1 -std=c++20 \ +// RUN: -analyzer-checker=core,alpha.deadcode.UnreachableCode \ +// RUN: -analyzer-config experimental-enable-naive-ctu-analysis=true \ +// RUN: -analyzer-config max-nodes=10 \ +// RUN: -verify=ctu-on %s +// ctu-on-no-diagnostics + +// RUN: %clang_analyze_cc1 -std=c++20 \ +// RUN: -analyzer-checker=core,alpha.deadcode.UnreachableCode \ +// RUN: -analyzer-config experimental-enable-naive-ctu-analysis=false \ +// RUN: -analyzer-config max-nodes=10 \ +// RUN: -verify=ctu-off %s +// ctu-off-no-diagnostics + +#define NOP ((void)0) + +void entrypoint(int x) { + NOP; NOP; NOP; NOP; NOP; + NOP; NOP; NOP; NOP; NOP; + if (x) NOP; +} `````````` </details> https://github.com/llvm/llvm-project/pull/219225 _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
