[analyzer] ConditionBRVisitor: Remove GDM checking

[Charusso]

Summary

Removed the GDM checking what could prevent reports made by this visitor. Now we rely on constraint changes instead.

[NoQ]

Does this change the observed behavior? If so, a test would be great! If this is a part of D53076, please attach to D53076. If this could only be tested after D53076, please add a test anyway and get these patches committed in the respective order.

I guess it's going to be *less* notes emitted after the patch, because every update in constraint ranges is also a GDM update, so we'll be bailing out more often. But i doubt that there are many situations where something else changes in the GDM and it causes us to emit a note in *this* visitor, so i think the change makes sense.

[Charusso]

In D54811#1315431, @NoQ wrote:

Does this change the observed behavior? If so, a test would be great! If this is a part of D53076, please attach to D53076. If this could only be tested after D53076, please add a test anyway and get these patches committed in the respective order.

I am not really into the checker business as I started to work with the 'front-end', so I cannot write a test for now. I have ran CodeChecker with the default 'analyze' command before and after the patch on the following projects: bitcoin, curl, ffmpeg, git, memcached, nginx, openssl, postgresql, tinyxml2, tmux, twin, vim, xerces. After that I made a test that should not pass: the commit hash is different in every file (comparing the files in before/after folders), so I have changed the test with diffing '<(tail -n +8 "$file")' magic. There is no difference at all, so I think it should work fine to continue with other patches. (Z3-compatibility could be problematic as I have no experience with that and I cannot test it currently.)

[Szelethus]

I guess we used to have something that would result in an event duplication (which was the motivation for this patch), but it's long forgotten.

In D54811#1332093, @Charusso wrote:

(Z3-compatibility could be problematic as I have no experience with that and I cannot test it currently.)

That is my no1 concern too. Luckily, we do have lit tests for Z3, if you have the time to configure it (as I sadly don't :/)

[Charusso]

• Redownloaded and retested the following projects with the same technique as mentioned above with no difference in the plists: bitcoin, curl, ffmpeg, git, memcached, nginx, openssl, postgres, redis, sqlite, tmux, vim, xerces. (This is 8 GB of pure errors.)
• ninja check-clang passes well.
• ninja check-clang-analyzer-z3 passes as the trunk version.

[Charusso]

Thanks you @Szelethus! I found the lit tests.

The output from the trunk and after the patch running ninja check-clang-analyzer-z3:

Failing Tests (1):
    Clang :: Analysis/plist-macros.cpp

  Expected Passes    : 565
  Expected Failures  : 2
  Unsupported Tests  : 3
  Unexpected Failures: 1

The behaviour is the same before the patch. I have not found a way to eliminate this error as it is a plist-style test and it is impossible to inject #ifndef ANALYZER_CM_Z3 to differentiate the default test.

[NoQ]

Ok! I'll try to evaluate it and see if i an example of improvement shows up - this would probably be easier than to come up with one manually.

include/clang/StaticAnalyzer/Core/PathSensitive/ConstraintManager.h

82–85

I think that haveSameConstraints(S1, S2) is a slightly more friendly API.

[Charusso]

Better API.

[NoQ]

This indeed seems to be a no-functional-change patch. For now we don't have checkers that actively add transitions after eagerly-assume operators. evalAssume() doesn't count - as we've recently discussed, it does allow checkers to actively participate in modeling, but it doesn't allow adding transitions :) So we're pretty much stuck with only the uninitialized variable checker that finds binary operators with undefined results, but it doesn't add transitions either; it only generates fatal errors. So for now the invariant "on block edges, GDMs are different iff constraint ranges are different" seems to hold and the original code was working correctly, even if accidentally.

Let's commit! Do you have commit access? You might want to get commit access and commit yourself, 'cause that's a lot of patches!

[Charusso]

In D54811#1420868, @NoQ wrote:

Let's commit! Do you have commit access? You might want to get commit access and commit yourself, 'cause that's a lot of patches!

Thanks you! I will ask for commit access soon.

include/clang/StaticAnalyzer/Core/PathSensitive/ConstraintManager.h

public:
   }
 };
 
 class ConstraintManager {
 public:
   ConstraintManager() = default;
   virtual ~ConstraintManager();
 
+  virtual bool haveEqualConstraints(ProgramStateRef S1,
+                                    ProgramStateRef S2) const = 0;
+
   virtual ProgramStateRef assume(ProgramStateRef state,
                                  DefinedSVal Cond,
                                  bool Assumption) = 0;
 
   using ProgramStatePair = std::pair<ProgramStateRef, ProgramStateRef>;
 
   /// Returns a pair of states (StTrue, StFalse) where the given condition is
   /// assumed to be true or false, respectively.
   ProgramStatePair assumeDual(ProgramStateRef State, DefinedSVal Cond) {

include/clang/StaticAnalyzer/Core/PathSensitive/ProgramState.h

public:
   void iterBindings(ProgramStateRef state, StoreManager::BindingsHandler& F) {
     StoreMgr->iterBindings(state->getStore(), F);
   }
 
   ProgramStateRef getPersistentState(ProgramState &Impl);
   ProgramStateRef getPersistentStateWithGDM(ProgramStateRef FromState,
                                            ProgramStateRef GDMState);
 
-  bool haveEqualEnvironments(ProgramStateRef S1, ProgramStateRef S2) {
+  bool haveEqualConstraints(ProgramStateRef S1, ProgramStateRef S2) const {
+    return ConstraintMgr->haveEqualConstraints(S1, S2);
+  }
+
+  bool haveEqualEnvironments(ProgramStateRef S1, ProgramStateRef S2) const {
     return S1->Env == S2->Env;
   }
 
-  bool haveEqualStores(ProgramStateRef S1, ProgramStateRef S2) {
+  bool haveEqualStores(ProgramStateRef S1, ProgramStateRef S2) const {
     return S1->store == S2->store;
   }
 
   //==---------------------------------------------------------------------==//
   // Generic Data Map methods.
   //==---------------------------------------------------------------------==//
   //
   // ProgramStateManager and ProgramState support a "generic data map" that allows

include/clang/StaticAnalyzer/Core/PathSensitive/SMTConstraintManager.h

void print(ProgramStateRef St, raw_ostream &OS, const char *nl,
     OS << nl << sep << "Constraints:";
     for (auto I = CZ.begin(), E = CZ.end(); I != E; ++I) {
       OS << nl << ' ' << I->first << " : ";
       I->second.print(OS);
     }
     OS << nl;
   }
 
+  bool haveEqualConstraints(ProgramStateRef S1,
+                            ProgramStateRef S2) const override {
+    return S1->get<ConstraintSMT>() == S2->get<ConstraintSMT>();
+  }
+
   bool canReasonAbout(SVal X) const override {
     const TargetInfo &TI = getBasicVals().getContext().getTargetInfo();
 
     Optional<nonloc::SymbolVal> SymVal = X.getAs<nonloc::SymbolVal>();
     if (!SymVal)
       return true;
 
     const SymExpr *Sym = SymVal->getSymbol();

lib/StaticAnalyzer/Core/BugReporterVisitors.cpp

ConditionBRVisitor::VisitNode(const ExplodedNode *N,
   return piece;
 }
 
 std::shared_ptr<PathDiagnosticPiece>
 ConditionBRVisitor::VisitNodeImpl(const ExplodedNode *N,
                                   BugReporterContext &BRC, BugReport &BR) {
   ProgramPoint progPoint = N->getLocation();
   ProgramStateRef CurrentState = N->getState();
-  ProgramStateRef PrevState = N->getFirstPred()->getState();
+  ProgramStateRef PreviousState = N->getFirstPred()->getState();
 
-  // Compare the GDMs of the state, because that is where constraints
-  // are managed.  Note that ensure that we only look at nodes that
-  // were generated by the analyzer engine proper, not checkers.
-  if (CurrentState->getGDM().getRoot() ==
-      PrevState->getGDM().getRoot())
+  // If the constraint information does not changed there is no assumption.
+  if (BRC.getStateManager().haveEqualConstraints(CurrentState, PreviousState))
     return nullptr;
 
   // If an assumption was made on a branch, it should be caught
   // here by looking at the state transition.
   if (Optional<BlockEdge> BE = progPoint.getAs<BlockEdge>()) {
     const CFGBlock *srcBlk = BE->getSrc();
     if (const Stmt *term = srcBlk->getTerminator())
       return VisitTerminator(term, N, srcBlk, BE->getDst(), BR, BRC);

lib/StaticAnalyzer/Core/RangeConstraintManager.cpp

 public:
   RangeConstraintManager(SubEngine *SE, SValBuilder &SVB)
       : RangedConstraintManager(SE, SVB) {}
 
   //===------------------------------------------------------------------===//
   // Implementation for interface from ConstraintManager.
   //===------------------------------------------------------------------===//
 
+  bool haveEqualConstraints(ProgramStateRef S1,
+                            ProgramStateRef S2) const override {
+    return S1->get<ConstraintRange>() == S2->get<ConstraintRange>();
+  }
+
   bool canReasonAbout(SVal X) const override;
 
   ConditionTruthVal checkNull(ProgramStateRef State, SymbolRef Sym) override;
 
   const llvm::APSInt *getSymVal(ProgramStateRef State,
                                 SymbolRef Sym) const override;
 
   ProgramStateRef removeDeadBindings(ProgramStateRef State,