[analyzer] StdLibraryFunctionsChecker refactor w/ inheritance

[martong]

Summary

Currently, ValueRange is very hard to extend with new kind of constraints.
For instance, it forcibly encapsulates relations between arguments and the
return value (ComparesToArgument) besides handling the regular value
ranges (OutOfRange, WithinRange).
ValueRange in this form is not suitable to add new constraints on
arguments like "not-null".

This refactor introduces a new base class ValueConstraint with an
abstract apply function. Descendants must override this. There are 2
descendants: RangeConstraint and ComparisonConstraint. In the following
patches I am planning to add the NotNullConstraint, and additional
virtual functions like negate() and warning().

[martong]

clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp

158

Note here, we need a copyable, polymorphic and default initializable type (vector needs that). A raw pointer were good, however, we cannot default initialize that. unique_ptr makes the Summary class non-copyable, therefore not an option.
Releasing the copyablitly requirement would render the initialization of the Summary map infeasible.
Perhaps we could come up with a type erasure technique without inheritance once we consider the shared_ptr as restriction, but for now that seems to be overkill.

[balazske]

Is really more kind of constraint needed than range constraint? A non-null can be represented as range constraint too. The compare constraint is used only for the return value for which a special ReturnConstraint can be used to handle the return value not like a normal argument (and then the Ret special value is not needed). Or are there sometimes relations between arguments of a function?

[martong]

Is really more kind of constraint needed than range constraint?

Yes, there are other constraints I am planning to implement:

• Size requirements E.g.: asctime_s(char *buf, rsize_t bufsz, const struct tm *time_ptr); buf size must be at least bufsz.
• Not-null
• Not-uninitalized
• Not-tainted

A non-null can be represented as range constraint too.

Actually, to implement that we should have a branch in all ValueRange::apply* functions that handles Loc SVals. Unfortunately, a pointer cannot be handled as NonLoc, and the current Range based implementation handles NonLocs only.

The compare constraint is used only for the return value for which a special ReturnConstraint can be used to handle the return value not like a normal argument (and then the Ret special value is not needed).

The Compare constraint is already forced into a Range "concept" whereas it has nothing to do with ranges. By handling compare constraints separately, we attach a single responsibility to each constraint class, instead of having a monolithic god constraint class. Take a look at this coerced data representation that we have today in ValueRange:

BinaryOperator::Opcode getOpcode() const {
  assert(Kind == ComparesToArgument);
  assert(Args.size() == 1);
  BinaryOperator::Opcode Op =
      static_cast<BinaryOperator::Opcode>(Args[0].first);
  assert(BinaryOperator::isComparisonOp(Op) &&
         "Only comparison ops are supported for ComparesToArgument");
  return Op;
}

Subclasses are a good way to get rid of this not-so-intuitive structure and assertions.

Or are there sometimes relations between arguments of a function?

I can't recall now a direct relation by heart.
But there could be more subtle indirect relations, see the size requirements above. (Thought I'd rather implement that in a separate constraint class.)

[Szelethus]

FYI I've been seeing your patches to this checker and I will respond to them, but I need to do some learning on my own before having the confidence to accept or request changes. Working on it!

[martong]

In D74973#1889273, @Szelethus wrote:

FYI I've been seeing your patches to this checker and I will respond to them, but I need to do some learning on my own before having the confidence to accept or request changes. Working on it!

No worries, just take your time! :) In the meanwhile, I am trying to implement the "non-null" argument constraint based on these two patches.

[martong]

@balazske See https://reviews.llvm.org/D75063 about the simple independent implementation of the NotNull constraint.

[martong]

clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp

92

virtual dtor is missing.

[gamesh411]

clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp

158

std::variant (with std::monostate for the default constructibility) would also be an option (if c++17 were supported). But this is not really an issue, i agree with that.

[Szelethus]

I have some high level questions, you have spent far more time with this code and I'm happy to be proven wrong! :)

In D74973#1889188, @martong wrote:

Is really more kind of constraint needed than range constraint?

Yes, there are other constraints I am planning to implement:

• Size requirements E.g.: asctime_s(char *buf, rsize_t bufsz, const struct tm *time_ptr); buf size must be at least bufsz.
• Not-null
• Not-uninitalized
• Not-tainted

Are we really sure that we need to express that with constraints? Can't we just change the name of ValueRange (or encapsulate it in another class) and add more fields to it, such as taintedness or initializedness? Is there an incentive to keep ValueRange lean?

This doesn't look too bad:

auto Getc = [&]() {
  return Summary(ArgTypes{Irrelevant}, RetType{IntTy}, NoEvalCall)
      .Case(
          {ReturnValueDescription(RangeConstraints(WithinRange, {{EOFv, EOFv}, {0, UCharMax}},
                                  Tainted, Non_Uninitialized});
};

A non-null can be represented as range constraint too.

Actually, to implement that we should have a branch in all ValueRange::apply* functions that handles Loc SVals. Unfortunately, a pointer cannot be handled as NonLoc, and the current Range based implementation handles NonLocs only.

So, why didn't we take that route instead? Marking a pointer non-null seems to be a less invasive change.

The compare constraint is used only for the return value for which a special ReturnConstraint can be used to handle the return value not like a normal argument (and then the Ret special value is not needed).

The Compare constraint is already forced into a Range "concept" whereas it has nothing to do with ranges. By handling compare constraints separately, we attach a single responsibility to each constraint class, instead of having a monolithic god constraint class. Take a look at this coerced data representation that we have today in ValueRange:

BinaryOperator::Opcode getOpcode() const {
  assert(Kind == ComparesToArgument);
  assert(Args.size() == 1);
  BinaryOperator::Opcode Op =
      static_cast<BinaryOperator::Opcode>(Args[0].first);
  assert(BinaryOperator::isComparisonOp(Op) &&
         "Only comparison ops are supported for ComparesToArgument");
  return Op;
}

Subclasses are a good way to get rid of this not-so-intuitive structure and assertions.

Having more fields feels like another possible solution.

clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp

158

Ugh, we've historically been very hostile towards virtual functions. We don't mind them that much when they don't have to run a lot, like during bug report construction, but as a core part of the analysis, I'm not sure what the current stance is on it.

I'm not inherently (haha) against it, and I'm fine with leaving this as-is for the time being, though I'd prefer if you placed a TODO to revisit this issue.

[martong]

In D74973#1900852, @Szelethus wrote:

I have some high level questions, you have spent far more time with this code and I'm happy to be proven wrong! :)

In D74973#1889188, @martong wrote:

Is really more kind of constraint needed than range constraint?

Yes, there are other constraints I am planning to implement:

• Size requirements E.g.: asctime_s(char *buf, rsize_t bufsz, const struct tm *time_ptr); buf size must be at least bufsz.
• Not-null
• Not-uninitalized
• Not-tainted

Are we really sure that we need to express that with constraints? Can't we just change the name of ValueRange (or encapsulate it in another class) and add more fields to it, such as taintedness or initializedness? Is there an incentive to keep ValueRange lean?

Yes there is: separation of concerns, i.e. ValueRange should handle Values with int ranges. One class should handle only one well established responsibility.

This doesn't look too bad:

auto Getc = [&]() {
  return Summary(ArgTypes{Irrelevant}, RetType{IntTy}, NoEvalCall)
      .Case(
          {ReturnValueDescription(RangeConstraints(WithinRange, {{EOFv, EOFv}, {0, UCharMax}},
                                  Tainted, Non_Uninitialized});
};

A non-null can be represented as range constraint too.

Actually, to implement that we should have a branch in all ValueRange::apply* functions that handles Loc SVals. Unfortunately, a pointer cannot be handled as NonLoc, and the current Range based implementation handles NonLocs only.

So, why didn't we take that route instead? Marking a pointer non-null seems to be a less invasive change.

The answer is the same here. I think we should not mix too much implementation of different cases into one monumental function/class.

The compare constraint is used only for the return value for which a special ReturnConstraint can be used to handle the return value not like a normal argument (and then the Ret special value is not needed).

The Compare constraint is already forced into a Range "concept" whereas it has nothing to do with ranges. By handling compare constraints separately, we attach a single responsibility to each constraint class, instead of having a monolithic god constraint class. Take a look at this coerced data representation that we have today in ValueRange:

BinaryOperator::Opcode getOpcode() const {
  assert(Kind == ComparesToArgument);
  assert(Args.size() == 1);
  BinaryOperator::Opcode Op =
      static_cast<BinaryOperator::Opcode>(Args[0].first);
  assert(BinaryOperator::isComparisonOp(Op) &&
         "Only comparison ops are supported for ComparesToArgument");
  return Op;
}

Subclasses are a good way to get rid of this not-so-intuitive structure and assertions.

Having more fields feels like another possible solution.

Yes, having fields is another approach of having an enum and a union (i.e. tagged union). A tagged union is a variant basically. So adding more fields would finally be very similar to a variant based solution. And that is useful only if we know the set of classes beforehand and we want to gradually implement new operations on them.
In our case we know that we need 3 operations: apply, negate, warning and no more. And we want to gradually add new classes: NonNull, NotUninitialized, ...

clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp

158

std::variant (with std::monostate for the default constructibility) would also be an option (if c++17 were supported). But this is not really an issue, i agree with that.

Variant would be useful if we knew the set of classes prior and we wanted to add operations gradually. Class hierarchies (or run-time concepts [Sean Parent]) are very useful if we know the set of operations prior and we want to add classes gradually, and we have this case here.

158

Ugh, we've historically been very hostile towards virtual functions. We don't mind them that much when they don't have to run a lot, like during bug report construction, but as a core part of the analysis, I'm not sure what the current stance is on it.

I did not find any evidence for this statement. Consider as a counter example the ExternalASTSource interface in Clang, which is filled with virtual functions and is part of the C/C++ lookup mechanism, which is quite on the hot path of C/C++ parsing I think. Did not find any prohibition in LLVM coding guidelines neither. I do believe virtual functions have their use cases exactly where (runtime) polimorphism is needed, such as in this patch.

[Szelethus]

Ah, okay I think I got why you chose this direction. Summaries are little more then a collection of constraints, and at select points in the execution we check and apply them one-by-one. If we want to preserve this architecture (and it seems great, why not?), inheritance is indeed the correct design decision.

The costliest code to run according to my limited knowledge about C++ development is indeed the one that is hard to understand and maintain, and this seems to have a good bit of thought behind it. I'll check your followup patches to gain some confidence before formally accepting, but the general idea seems great.

clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp

87–88

Why did we remove the initialization here? Can we make this constexpr?

89

We should totally have a good bit of documentation here.

158

I consider myself proven wrong here then.

[martong]

clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp

158

Thanks for the review and for considering other alternatives! And please accept my apologies, maybe I was pushing too hard on inheritance.

[Szelethus]

clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp

158

We should definitely decorate this with a TODO: Can we change this to not use a shared_ptr?. Worst case scenario, there it will stay for eternity :)

[baloghadamsoftware]

clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp

101

I would move this into the class to encapsulate the values instead of contaminating namespace ento.

158

FIXME is the official, not TODO, afaik.

158

I think that inheritance is the right approach here. However, if it is unacceptable for performance reasons it could be replaced by a template-based solution.

[NoQ]

I like this! Please address the remaining nits^^

clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp

101

It's checker-local anyway and i suspect we write these a lot in the summaries, so let's keep it in the global scope.

[martong]

clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp

87–88

I am receiving an undefined reference linker error (I use gold) if the initialization is happening in-class.
Even if constexpr is used.

/usr/bin/ld.gold: error: tools/clang/lib/StaticAnalyzer/Checkers/CMakeFiles/obj.clangStaticAnalyzerCheckers.dir/StdLibraryFunctionsChecker.cpp.o: requires dynamic R_X86_64_PC32 reloc against '_ZN12_GLOBAL__N_126StdLibraryFunctionsChecker3RetE' which may overflow at runtime; recompile with -fPIC
tools/clang/lib/StaticAnalyzer/Checkers/CMakeFiles/obj.clangStaticAnalyzerCheckers.dir/StdLibraryFunctionsChecker.cpp.o:StdLibraryFunctionsChecker.cpp:function (anonymous namespace)::StdLibraryFunctionsChecker::initFunctionSummaries(clang::ento::CheckerContext&) const: error: undefined reference to '(anonymous namespace)::StdLibraryFunctionsChecker::Ret'

89

Ok, I added some comments to the class and to apply as well.

158

Actually, this is not necessary something that we need to fix or do. So, instead of the TODO I have added a comment that explains why do we use the shared_ptr.

[martong]

• Rebase to master
• Add comments to ValueConstraint
• Add virtual dtor
• Add comments to ValueConstraintPtr

[Szelethus]

Please have my post-commit approval :^) Nice work!

[martong]

In D74973#1926694, @Szelethus wrote:

Please have my post-commit approval :^) Nice work!

Thanks! :)

clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp

class StdLibraryFunctionsChecker : public Checker<check::PostCall, eval::Call> {
   /// We avoid nesting types here because each additional qualifier
   /// would need to be repeated in every function spec.
   struct Summary;
 
   /// Specify how much the analyzer engine should entrust modeling this function
   /// to us. If he doesn't, he performs additional invalidations.
   enum InvalidationKind { NoEvalCall, EvalCallAsPure };
 
-  /// A pair of ValueRangeKind and IntRangeVector would describe a range
-  /// imposed on a particular argument or return value symbol.
-  ///
-  /// Given a range, should the argument stay inside or outside this range?
-  /// The special `ComparesToArgument' value indicates that we should
-  /// impose a constraint that involves other argument or return value symbols.
-  enum ValueRangeKind { OutOfRange, WithinRange, ComparesToArgument };
-
   // The universal integral type to use in value range descriptions.
   // Unsigned to make sure overflows are well-defined.
   typedef uint64_t RangeInt;
 
   /// Normally, describes a single range constraint, eg. {{0, 1}, {3, 4}} is
   /// a non-negative integer, which less than 5 and not equal to 2. For
   /// `ComparesToArgument', holds information about how exactly to compare to
   /// the argument.
   typedef std::vector<std::pair<RangeInt, RangeInt>> IntRangeVector;
 
   /// A reference to an argument or return value by its number.
   /// ArgNo in CallExpr and CallEvent is defined as Unsigned, but
   /// obviously uint32_t should be enough for all practical purposes.
   typedef uint32_t ArgNo;
-  static const ArgNo Ret = std::numeric_limits<ArgNo>::max();
-
-  /// Incapsulates a single range on a single symbol within a branch.
-  class ValueRange {
-    ArgNo ArgN;          // Argument to which we apply the range.
-    ValueRangeKind Kind; // Kind of range definition.
-    IntRangeVector Args; // Polymorphic arguments.
+  static const ArgNo Ret;
 
+  /// Polymorphic base class that represents a constraint on a given argument
+  /// (or return value) of a function. Derived classes implement different kind
+  /// of constraints, e.g range constraints or correlation between two
+  /// arguments.
+  class ValueConstraint {
   public:
-    ValueRange(ArgNo ArgN, ValueRangeKind Kind, const IntRangeVector &Args)
-        : ArgN(ArgN), Kind(Kind), Args(Args) {}
-
+    ValueConstraint(ArgNo ArgN) : ArgN(ArgN) {}
+    virtual ~ValueConstraint() {}
+    /// Apply the effects of the constraint on the given program state. If null
+    /// is returned then the constraint is not feasible.
+    virtual ProgramStateRef apply(ProgramStateRef State, const CallEvent &Call,
+                                  const Summary &Summary) const = 0;
     ArgNo getArgNo() const { return ArgN; }
-    ValueRangeKind getKind() const { return Kind; }
 
-    BinaryOperator::Opcode getOpcode() const {
-      assert(Kind == ComparesToArgument);
-      assert(Args.size() == 1);
-      BinaryOperator::Opcode Op =
-          static_cast<BinaryOperator::Opcode>(Args[0].first);
-      assert(BinaryOperator::isComparisonOp(Op) &&
-             "Only comparison ops are supported for ComparesToArgument");
-      return Op;
-    }
-
-    ArgNo getOtherArgNo() const {
-      assert(Kind == ComparesToArgument);
-      assert(Args.size() == 1);
-      return static_cast<ArgNo>(Args[0].second);
-    }
+  protected:
+    ArgNo ArgN; // Argument to which we apply the constraint.
+  };
+
+  /// Given a range, should the argument stay inside or outside this range?
+  enum RangeKind { OutOfRange, WithinRange };
+
+  /// Encapsulates a single range on a single symbol within a branch.
+  class RangeConstraint : public ValueConstraint {
+    RangeKind Kind;      // Kind of range definition.
+    IntRangeVector Args; // Polymorphic arguments.
+
+  public:
+    RangeConstraint(ArgNo ArgN, RangeKind Kind, const IntRangeVector &Args)
+        : ValueConstraint(ArgN), Kind(Kind), Args(Args) {}
 
     const IntRangeVector &getRanges() const {
-      assert(Kind != ComparesToArgument);
       return Args;
     }
 
-    // We avoid creating a virtual apply() method because
-    // it makes initializer lists harder to write.
   private:
     ProgramStateRef applyAsOutOfRange(ProgramStateRef State,
                                       const CallEvent &Call,
                                       const Summary &Summary) const;
     ProgramStateRef applyAsWithinRange(ProgramStateRef State,
                                        const CallEvent &Call,
                                        const Summary &Summary) const;
-    ProgramStateRef applyAsComparesToArgument(ProgramStateRef State,
-                                              const CallEvent &Call,
-                                              const Summary &Summary) const;
-
   public:
     ProgramStateRef apply(ProgramStateRef State, const CallEvent &Call,
-                          const Summary &Summary) const {
+                          const Summary &Summary) const override {
       switch (Kind) {
       case OutOfRange:
         return applyAsOutOfRange(State, Call, Summary);
       case WithinRange:
         return applyAsWithinRange(State, Call, Summary);
-      case ComparesToArgument:
-        return applyAsComparesToArgument(State, Call, Summary);
       }
-      llvm_unreachable("Unknown ValueRange kind!");
+      llvm_unreachable("Unknown range kind!");
     }
   };
 
-  /// The complete list of ranges that defines a single branch.
-  typedef std::vector<ValueRange> ValueRangeSet;
+  class ComparisonConstraint : public ValueConstraint {
+    BinaryOperator::Opcode Opcode;
+    ArgNo OtherArgN;
+
+  public:
+    ComparisonConstraint(ArgNo ArgN, BinaryOperator::Opcode Opcode,
+                         ArgNo OtherArgN)
+        : ValueConstraint(ArgN), Opcode(Opcode), OtherArgN(OtherArgN) {}
+    ArgNo getOtherArgNo() const { return OtherArgN; }
+    BinaryOperator::Opcode getOpcode() const { return Opcode; }
+    ProgramStateRef apply(ProgramStateRef State, const CallEvent &Call,
+                          const Summary &Summary) const override;
+  };
+
+  // Pointer to the ValueConstraint. We need a copyable, polymorphic and
+  // default initialize able type (vector needs that). A raw pointer was good,
+  // however, we cannot default initialize that. unique_ptr makes the Summary
+  // class non-copyable, therefore not an option. Releasing the copyability
+  // requirement would render the initialization of the Summary map infeasible.
+  using ValueConstraintPtr = std::shared_ptr<ValueConstraint>;
+  /// The complete list of constraints that defines a single branch.
+  typedef std::vector<ValueConstraintPtr> ConstraintSet;
 
   using ArgTypes = std::vector<QualType>;
-  using Ranges = std::vector<ValueRangeSet>;
+  using Cases = std::vector<ConstraintSet>;
 
   /// Includes information about function prototype (which is necessary to
   /// ensure we're modeling the right function and casting values properly),
   /// approach to invalidation, and a list of branches - essentially, a list
   /// of list of ranges - essentially, a list of lists of lists of segments.
   struct Summary {
     const ArgTypes ArgTys;
     const QualType RetTy;
     const InvalidationKind InvalidationKd;
-    Ranges Cases;
-    ValueRangeSet ArgConstraints;
+    Cases CaseConstraints;
+    ConstraintSet ArgConstraints;
 
     Summary(ArgTypes ArgTys, QualType RetTy, InvalidationKind InvalidationKd)
         : ArgTys(ArgTys), RetTy(RetTy), InvalidationKd(InvalidationKd) {}
 
-    Summary &Case(ValueRangeSet VRS) {
-      Cases.push_back(VRS);
+    Summary &Case(ConstraintSet&& CS) {
+      CaseConstraints.push_back(std::move(CS));
       return *this;
     }
 
   private:
     static void assertTypeSuitableForSummary(QualType T) {
       assert(!T->isVoidType() &&
              "We should have had no significant void types in the spec");
       assert(T.isCanonical() &&

 
 private:
   Optional<Summary> findFunctionSummary(const FunctionDecl *FD,
                                         const CallExpr *CE,
                                         CheckerContext &C) const;
 
   void initFunctionSummaries(CheckerContext &C) const;
 };
+
+const StdLibraryFunctionsChecker::ArgNo StdLibraryFunctionsChecker::Ret =
+    std::numeric_limits<ArgNo>::max();
+
 } // end of anonymous namespace
 
-ProgramStateRef StdLibraryFunctionsChecker::ValueRange::applyAsOutOfRange(
+ProgramStateRef StdLibraryFunctionsChecker::RangeConstraint::applyAsOutOfRange(
     ProgramStateRef State, const CallEvent &Call,
     const Summary &Summary) const {
 
   ProgramStateManager &Mgr = State->getStateManager();
   SValBuilder &SVB = Mgr.getSValBuilder();
   BasicValueFactory &BVF = SVB.getBasicValueFactory();
   ConstraintManager &CM = Mgr.getConstraintManager();
   QualType T = getArgType(Summary, getArgNo());
for (size_t I = 0; I != E; ++I) {
       if (!State)
         break;
     }
   }
 
   return State;
 }
 
-ProgramStateRef StdLibraryFunctionsChecker::ValueRange::applyAsWithinRange(
+ProgramStateRef StdLibraryFunctionsChecker::RangeConstraint::applyAsWithinRange(
     ProgramStateRef State, const CallEvent &Call,
     const Summary &Summary) const {
 
   ProgramStateManager &Mgr = State->getStateManager();
   SValBuilder &SVB = Mgr.getSValBuilder();
   BasicValueFactory &BVF = SVB.getBasicValueFactory();
   ConstraintManager &CM = Mgr.getConstraintManager();
   QualType T = getArgType(Summary, getArgNo());
for (size_t I = 1; I != E; ++I) {
           return nullptr;
       }
     }
   }
 
   return State;
 }
 
-ProgramStateRef
-StdLibraryFunctionsChecker::ValueRange::applyAsComparesToArgument(
+ProgramStateRef StdLibraryFunctionsChecker::ComparisonConstraint::apply(
     ProgramStateRef State, const CallEvent &Call,
     const Summary &Summary) const {
 
   ProgramStateManager &Mgr = State->getStateManager();
   SValBuilder &SVB = Mgr.getSValBuilder();
   QualType CondT = SVB.getConditionType();
   QualType T = getArgType(Summary, getArgNo());
   SVal V = getArgSVal(Call, getArgNo());
void StdLibraryFunctionsChecker::checkPostCall(const CallEvent &Call,
   const CallExpr *CE = dyn_cast_or_null<CallExpr>(Call.getOriginExpr());
   if (!CE)
     return;
 
   Optional<Summary> FoundSummary = findFunctionSummary(FD, CE, C);
   if (!FoundSummary)
     return;
 
-  // Now apply ranges.
+  // Now apply the constraints.
   const Summary &Summary = *FoundSummary;
   ProgramStateRef State = C.getState();
 
   // Apply case/branch specifications.
-  for (const auto &VRS : Summary.Cases) {
+  for (const auto &VRS : Summary.CaseConstraints) {
     ProgramStateRef NewState = State;
     for (const auto &VR: VRS) {
-      NewState = VR.apply(NewState, Call, Summary);
+      NewState = VR->apply(NewState, Call, Summary);
       if (!NewState)
         break;
     }
 
     if (NewState && NewState != State)
       C.addTransition(NewState);
   }
 }
void StdLibraryFunctionsChecker::initFunctionSummaries(
   // and its return value, which is of type ssize_t, cannot be greater
   // than this argument. If we made a promotion, and the size argument
   // is equal to, say, 10, then we'd impose a range of [0, 10] on the
   // return value, however the correct range is [-1, 10].
   //
   // Please update the list of functions in the header after editing!
   //
 
-  // Below are helper functions to create the summaries.
-  auto ArgumentCondition = [](ArgNo ArgN, ValueRangeKind Kind,
-                              IntRangeVector Ranges) -> ValueRange {
-    ValueRange VR{ArgN, Kind, Ranges};
-    return VR;
-  };
-  auto ReturnValueCondition = [](ValueRangeKind Kind,
-                                 IntRangeVector Ranges) -> ValueRange {
-    ValueRange VR{Ret, Kind, Ranges};
-    return VR;
-  };
+  // Below are helpers functions to create the summaries.
+  auto ArgumentCondition = [](ArgNo ArgN, RangeKind Kind,
+                              IntRangeVector Ranges) {
+    return std::make_shared<RangeConstraint>(ArgN, Kind, Ranges);
+  };
+  struct {
+    auto operator()(RangeKind Kind, IntRangeVector Ranges) {
+      return std::make_shared<RangeConstraint>(Ret, Kind, Ranges);
+    }
+    auto operator()(BinaryOperator::Opcode Op, ArgNo OtherArgN) {
+      return std::make_shared<ComparisonConstraint>(Ret, Op, OtherArgN);
+    }
+  } ReturnValueCondition;
   auto Range = [](RangeInt b, RangeInt e) {
     return IntRangeVector{std::pair<RangeInt, RangeInt>{b, e}};
   };
   auto SingleValue = [](RangeInt v) {
     return IntRangeVector{std::pair<RangeInt, RangeInt>{v, v}};
   };
-  auto IsLessThan = [](ArgNo ArgN) { return IntRangeVector{{BO_LE, ArgN}}; };
+  auto LessThanOrEq = BO_LE;
 
   using RetType = QualType;
 
   // Templates for summaries that are reused by many functions.
   auto Getc = [&]() {
     return Summary(ArgTypes{Irrelevant}, RetType{IntTy}, NoEvalCall)
         .Case({ReturnValueCondition(WithinRange,
                                     {{EOFv, EOFv}, {0, UCharRangeMax}})});
   };
   auto Read = [&](RetType R, RangeInt Max) {
     return Summary(ArgTypes{Irrelevant, Irrelevant, SizeTy}, RetType{R},
                    NoEvalCall)
-        .Case({ReturnValueCondition(ComparesToArgument, IsLessThan(2)),
+        .Case({ReturnValueCondition(LessThanOrEq, ArgNo(2)),
                ReturnValueCondition(WithinRange, Range(-1, Max))});
   };
   auto Fread = [&]() {
     return Summary(ArgTypes{Irrelevant, Irrelevant, SizeTy, Irrelevant},
                    RetType{SizeTy}, NoEvalCall)
         .Case({
-            ReturnValueCondition(ComparesToArgument, IsLessThan(2)),
+            ReturnValueCondition(LessThanOrEq, ArgNo(2)),
         });
   };
   auto Getline = [&](RetType R, RangeInt Max) {
     return Summary(ArgTypes{Irrelevant, Irrelevant, Irrelevant}, RetType{R},
                    NoEvalCall)
         .Case({ReturnValueCondition(WithinRange, {{-1, -1}, {1, Max}})});
   };