[clang] c7b266d - [analyzer] Return null from makeNode on PosteriorlyOverconstrained (#218472)

[via cfe-commits]

Author: Donát Nagy
Date: 2026-08-25T15:28:44+02:00
New Revision: c7b266d8f7ab42ef36794c8e32757115aa6c2d2c

URL: https://github.com/llvm/llvm-project/commit/c7b266d8f7ab42ef36794c8e32757115aa6c2d2c
DIFF: https://github.com/llvm/llvm-project/commit/c7b266d8f7ab42ef36794c8e32757115aa6c2d2c.diff

LOG: [analyzer] Return null from makeNode on PosteriorlyOverconstrained (#218472)

Previously node creation methods like `ExprEngine::makeNode` had two
ways to signal failure: they returned `nullptr` when they would have
reached an already existing node (through a second path) and created a
sink node (returning it) when the state was `PosteriorlyOverconstrained`
(i.e. self-contradictory, not corresponding to a real possibility).

Both kinds of failures need to be handled in the same way (return
early), so this commit ensures that `makeNode` returns `nullptr` after
creating a node with `PosteriorlyOverconstrained` state. (It still
creates the nodes with the `PosteriorlyOverconstrained` state to ensure
that those infeasible execution paths end in sinks.)

For motivation and further plans see the discussion at:

https://discourse.llvm.org/t/simplifying-failure-modes-in-explodednode-creation/91542/5

As node creation could already return `nullptr` when "caching out"
(which can happen anywhere), theoretically all code that creates nodes
should be ready to handle the new `nullptr`s. In practice, this commit
might expose buggy code that fails to handle the `nullptr` -- but
`PosteriorlyOverconstrained` states are _very_ rare (even "caching out"
is more common), so we won't see a troublesome amount of crashes.

I analyzed a dozen open source projects with this change and there were
no new crashes or other changes in the behavior of the analyzer.

This commit also removes two `isSink()` tests that became irrelevant now
that `makeNode` cannot return a sink node when its `MarkAsSink`
parameter is false (the default).

Added: 
    

Modified: 
    clang/lib/StaticAnalyzer/Core/CoreEngine.cpp
    clang/lib/StaticAnalyzer/Core/ExprEngine.cpp

Removed: 
    


################################################################################
diff  --git a/clang/lib/StaticAnalyzer/Core/CoreEngine.cpp b/clang/lib/StaticAnalyzer/Core/CoreEngine.cpp
index 45087198da27d..307c96b23b206 100644
--- a/clang/lib/StaticAnalyzer/Core/CoreEngine.cpp
+++ b/clang/lib/StaticAnalyzer/Core/CoreEngine.cpp
@@ -561,13 +561,13 @@ void CoreEngine::HandleVirtualBaseBranch(const CFGBlock *B,
 ExplodedNode *CoreEngine::makeNode(const ProgramPoint &Loc,
                                    ProgramStateRef State, ExplodedNode *Pred,
                                    bool MarkAsSink) const {
-  MarkAsSink = MarkAsSink || State->isPosteriorlyOverconstrained();
+  bool IsPO = State->isPosteriorlyOverconstrained();
 
   bool IsNew;
-  ExplodedNode *N = G.getNode(Loc, State, MarkAsSink, &IsNew);
+  ExplodedNode *N = G.getNode(Loc, State, MarkAsSink || IsPO, &IsNew);
   N->addPredecessor(Pred, G);
 
-  return IsNew ? N : nullptr;
+  return (IsNew && !IsPO) ? N : nullptr;
 }
 
 void CoreEngine::enqueueStmtNode(ExplodedNode *N,

diff  --git a/clang/lib/StaticAnalyzer/Core/ExprEngine.cpp b/clang/lib/StaticAnalyzer/Core/ExprEngine.cpp
index a8fa11b078993..530fae9ee2dee 100644
--- a/clang/lib/StaticAnalyzer/Core/ExprEngine.cpp
+++ b/clang/lib/StaticAnalyzer/Core/ExprEngine.cpp
@@ -1138,8 +1138,7 @@ void ExprEngine::ProcessLoopExit(const Stmt* S, ExplodedNode *Pred) {
     NewState = processLoopEnd(S, NewState);
 
   LoopExit PP(S, Pred->getStackFrame());
-  ExplodedNode *N = Engine.makeNode(PP, NewState, Pred);
-  if (N && !N->isSink())
+  if (ExplodedNode *N = Engine.makeNode(PP, NewState, Pred))
     Engine.enqueueStmtNode(N, getCurrBlock(), currStmtIdx);
 }
 
@@ -1566,10 +1565,9 @@ void ExprEngine::ProcessTemporaryDtor(const CFGTemporaryDtor D,
   }
 
   ExplodedNode *CleanPred = Engine.makePostStmtNode(BTE, State, Pred);
-  if (!CleanPred || CleanPred->isSink()) {
+  if (!CleanPred) {
     // FIXME: We can get a null node here due to temporaries being
     // bound to default parameters.
-    // Sink check is just PosteriorlyOverconstrained paranoia.
     CleanPred = Pred;
   }