[Analyzer][solver] Fix inconsistent equivalence class data

[martong]

Summary

https://bugs.llvm.org/show_bug.cgi?id=51109

When we merged two classes, *this became an obsolete representation of
the new State. This is b/c the member relations had changed during the
previous merge of another member of the same class in a way that *this
had no longer any members. (mergeImpl might keep the member relations
to Other and could dissolve *this.)

[martong]

• Remove unnecessary comment (the API tells it all)

[vsavchenko]

Ah, I see now! I think we could've put together a somewhat easier test knowing what's wrong, but it's not important at all.
Thanks for addressing this issue!

[martong]

Ping

[vsavchenko]

Oh, I didn't accept it? Sorry!

[martong]

In D106285#2899545, @vsavchenko wrote:

Oh, I didn't accept it? Sorry!

No problem, thanks for the review!

clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp

public:
 
   LLVM_NODISCARD static inline Optional<bool> areEqual(ProgramStateRef State,
                                                        EquivalenceClass First,
                                                        EquivalenceClass Second);
   LLVM_NODISCARD static inline Optional<bool>
   areEqual(ProgramStateRef State, SymbolRef First, SymbolRef Second);
 
   /// Iterate over all symbols and try to simplify them.
-  LLVM_NODISCARD ProgramStateRef simplify(SValBuilder &SVB,
+  LLVM_NODISCARD static inline ProgramStateRef simplify(SValBuilder &SVB,
                                                         RangeSet::Factory &F,
-                                          ProgramStateRef State);
+                                                        ProgramStateRef State,
+                                                        EquivalenceClass Class);
 
   void dumpToStream(ProgramStateRef State, raw_ostream &os) const;
   LLVM_DUMP_METHOD void dump(ProgramStateRef State) const {
     dumpToStream(State, llvm::errs());
   }
 
   /// Check equivalence data for consistency.
   LLVM_NODISCARD LLVM_ATTRIBUTE_UNUSED static bool

 
 bool ConstraintAssignor::assignSymExprToConst(const SymExpr *Sym,
                                               const llvm::APSInt &Constraint) {
   llvm::SmallSet<EquivalenceClass, 4> SimplifiedClasses;
   // Iterate over all equivalence classes and try to simplify them.
   ClassMembersTy Members = State->get<ClassMembers>();
   for (std::pair<EquivalenceClass, SymbolSet> ClassToSymbolSet : Members) {
     EquivalenceClass Class = ClassToSymbolSet.first;
-    State = Class.simplify(Builder, RangeFactory, State);
+    State = EquivalenceClass::simplify(Builder, RangeFactory, State, Class);
     if (!State)
       return false;
     SimplifiedClasses.insert(Class);
   }
 
   // Trivial equivalence classes (those that have only one symbol member) are
   // not stored in the State. Thus, we must skim through the constraints as
   // well. And we try to simplify symbols in the constraints.
   ConstraintRangeTy Constraints = State->get<ConstraintRange>();
   for (std::pair<EquivalenceClass, RangeSet> ClassConstraint : Constraints) {
     EquivalenceClass Class = ClassConstraint.first;
     if (SimplifiedClasses.count(Class)) // Already simplified.
       continue;
-    State = Class.simplify(Builder, RangeFactory, State);
+    State = EquivalenceClass::simplify(Builder, RangeFactory, State, Class);
     if (!State)
       return false;
   }
 
   return true;
 }
 
 bool ConstraintAssignor::assignSymSymExprToRangeSet(const SymSymExpr *Sym,
inline Optional<bool> EquivalenceClass::areEqual(ProgramStateRef State,
   return llvm::None;
 }
 
 // Iterate over all symbols and try to simplify them. Once a symbol is
 // simplified then we check if we can merge the simplified symbol's equivalence
 // class to this class. This way, we simplify not just the symbols but the
 // classes as well: we strive to keep the number of the classes to be the
 // absolute minimum.
-LLVM_NODISCARD ProgramStateRef EquivalenceClass::simplify(
-    SValBuilder &SVB, RangeSet::Factory &F, ProgramStateRef State) {
-  SymbolSet ClassMembers = getClassMembers(State);
+LLVM_NODISCARD ProgramStateRef
+EquivalenceClass::simplify(SValBuilder &SVB, RangeSet::Factory &F,
+                           ProgramStateRef State, EquivalenceClass Class) {
+  SymbolSet ClassMembers = Class.getClassMembers(State);
   for (const SymbolRef &MemberSym : ClassMembers) {
     SymbolRef SimplifiedMemberSym = ento::simplify(State, MemberSym);
     if (SimplifiedMemberSym && MemberSym != SimplifiedMemberSym) {
-      EquivalenceClass ClassOfSimplifiedSym =
-          EquivalenceClass::find(State, SimplifiedMemberSym);
       // The simplified symbol should be the member of the original Class,
       // however, it might be in another existing class at the moment. We
       // have to merge these classes.
-      State = merge(F, State, ClassOfSimplifiedSym);
+      State = merge(F, State, MemberSym, SimplifiedMemberSym);
       if (!State)
         return nullptr;
     }
   }
   return State;
 }
 
 inline ClassSet EquivalenceClass::getDisequalClasses(ProgramStateRef State,

clang/test/Analysis/solver-sym-simplification-no-crash2.c

+// RUN: %clang_analyze_cc1 %s \
+// RUN:   -analyzer-checker=core \
+// RUN:   -analyzer-checker=debug.ExprInspection \
+// RUN:   -verify
+
+// Here, we test that symbol simplification in the solver does not produce any
+// crashes.
+// https://bugs.llvm.org/show_bug.cgi?id=51109
+
+// expected-no-diagnostics
+
+int a, b, c, d;
+void f() {
+  a = -1;
+  d = b * a;
+  a = d / c;
+  if (a < 7 / b)
+    return;
+  if (d *a / c < 7 / b)
+    return;
+  if (b == 1 && c == -1)
+    return;
+}