steakhal added a comment. Finally, we have this!
Can we have this https://godbolt.org/z/oferc6P5Y in addition to the one you proposed? I find it more readable. What performance hit will we suffer from this change? Please do a differential analysis. ================ Comment at: clang/include/clang/StaticAnalyzer/Core/PathSensitive/ConstraintManager.h:93 /// assumed to be true or false, respectively. - ProgramStatePair assumeDual(ProgramStateRef State, DefinedSVal Cond) { - ProgramStateRef StTrue = assume(State, Cond, true); - - // If StTrue is infeasible, asserting the falseness of Cond is unnecessary - // because the existing constraints already establish this. - if (!StTrue) { -#ifdef EXPENSIVE_CHECKS - assert(assume(State, Cond, false) && "System is over constrained."); -#endif - return ProgramStatePair((ProgramStateRef)nullptr, State); - } - - ProgramStateRef StFalse = assume(State, Cond, false); - if (!StFalse) { - // We are careful to return the original state, /not/ StTrue, - // because we want to avoid having callers generate a new node - // in the ExplodedGraph. - return ProgramStatePair(State, (ProgramStateRef)nullptr); - } - - return ProgramStatePair(StTrue, StFalse); - } + ProgramStatePair assumeDual(ProgramStateRef State, DefinedSVal Cond); ---------------- I would leave a note here that these two states might be equal in a very rare case (*); but one should not depend on that. ================ Comment at: clang/include/clang/StaticAnalyzer/Core/PathSensitive/CoreEngine.h:293 ExplodedNode *Pred) { - return generateNodeImpl(PP, State, Pred, false); + return generateNodeImpl(PP, State, Pred, State->isInfeasible()); } ---------------- ================ Comment at: clang/include/clang/StaticAnalyzer/Core/PathSensitive/ProgramState.h:86 GenericDataMap GDM; // Custom data stored by a client of this class. + bool Infeasible = false; unsigned refCount; ---------------- For the record, back then we had such flag. IDK when did we remove it, but we had it for sure. ================ Comment at: clang/include/clang/StaticAnalyzer/Core/PathSensitive/ProgramState.h:113-115 + ProgramStateRef cloneAsInfeasible() const; + bool isInfeasible() const { return Infeasible; } + ---------------- These should not be public. Make friends if required. ================ Comment at: clang/lib/StaticAnalyzer/Core/ConstraintManager.cpp:51 + ProgramStateRef StFalse = assume(State, Cond, false); + if (!StFalse) { // both infeasible + ProgramStateRef Infeasible = State->cloneAsInfeasible(); ---------------- Should we mark this `LLVM_UNLIKELY(cond)`? I would expect this function to be quite hot, and infeasible states rare. Could we measure this one? ================ Comment at: clang/lib/StaticAnalyzer/Core/ConstraintManager.cpp:52-54 + ProgramStateRef Infeasible = State->cloneAsInfeasible(); + assert(Infeasible->isInfeasible()); + return ProgramStatePair(Infeasible, Infeasible); ---------------- Add here some comments on why we return (`Infeasible`,`Infeasible`). I would rather see `StInfeasible` to better discriminate what we are talking about. We already use `StTrue` and `StFalse` in this context though. ================ Comment at: clang/test/Analysis/sink-infeasible.c:37-48 + /* The BASELINE passes these checks ('wrning' is used to avoid lit to match) + // The parent state is already infeasible, look at this contradiction: + clang_analyzer_eval(b > 0); // expected-wrning{{FALSE}} + clang_analyzer_eval(b <= 0); // expected-wrning{{FALSE}} + // Crashes with expensive checks. + if (b > 0) { + clang_analyzer_warnIfReached(); // no-warning, OK ---------------- You could use a non-default check prefix. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D124674/new/ https://reviews.llvm.org/D124674 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits