[analyzer] StdLibraryFunctionsChecker: Add better diagnostics

[martong]

Summary

Title says it, but there is still place for further improvements.

[Szelethus]

Sure, this is an improvement because we display more information, but I'd argue that it isn't really a more readable warning message :) How about

<argno>th argument to the call to <function name>

• cannot be represented with a character
• is a null pointer
• ...

, which violates the function's preconditions.

WDYT?

[xazax.hun]

Also, I see no tests :)

[baloghadamsoftware]

In D79431#2020951, @Szelethus wrote:

Sure, this is an improvement because we display more information, but I'd argue that it isn't really a more readable warning message :) How about

<argno>th argument to the call to <function name>

• cannot be represented with a character
• is a null pointer
• ...

, which violates the function's preconditions.

WDYT?

+1 Warning messages should be as clear as possible. Although our users are programmers, they do not necessarily understand a message "Range constraint not satisfied.". Maybe the error messages could be stored together with the function summaries.

[baloghadamsoftware]

clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp

506

Instead of std::string we usually use llvm::SmallString and an llvm::raw_svector_ostream to print into it.

[balazske]

clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp

506

This would look better?
"Function argument constraint is not satisfied, constraint: <Name>, ArgN: <ArgN>"

[steakhal]

Could you add some tests demonstrating the refined diagnostics of the checker?
It would help to validate the quality of the emitted diagnostics.

clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp

87

Shouldn't we use using instead?

506

Maybe llvm::Twine would be a better choice to llvm::raw_svector_ostream for string concatenations.
docs:

Twine - A lightweight data structure for efficiently representing the concatenation of temporary values as strings.

[martong]

• Rebase to master

[martong]

In D79431#2020951, @Szelethus wrote:

Sure, this is an improvement because we display more information, but I'd argue that it isn't really a more readable warning message :) How about

<argno>th argument to the call to <function name>

• cannot be represented with a character
• is a null pointer
• ...

, which violates the function's preconditions.

WDYT?

I'd rather prefer @balazske's approach (see below) as that is similarly expressive but sparser.

clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp

87

Yes, we should. But it's legacy code from the times long before.

506

We can use a Twine to create the owning std::string without creating interim objects. But then we must pass an owning object to the ctor of PathSensitiveBugReport, otherwise we might end up with dangling pointers.
https://llvm.org/docs/ProgrammersManual.html#dss-twine

[martong]

• Use Twine

clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp

506

Yes, I like this.

[martong]

Ping.

[Szelethus]

Tests c:

I'm still not a huge fan of the warning message. Now it describes what kind of constraint was violated, but not how (too large? too small?). Also, while I respect that you didn't prefer my suggestion, I think the warning message there is still a lot more welcoming, even if it doesn't convey more information. But, I don't necessarily insist on it.

[balazske]

There is a TODO comment at the error message, it needs improvement. The current form is still something for developers, not for end users. For final version I would accept textual descriptions (as mentioned above), not names like "BufferSize" and words like "ArgN" inside it.

[Szelethus]

Ah, okay, silly me. Didn't catch that. Then the message is OK for now.

edit: Describing how the violation happened might be a valuable for development purposes as well.

[martong]

In D79431#2215610, @Szelethus wrote:

Ah, okay, silly me. Didn't catch that. Then the message is OK for now.

edit: Describing how the violation happened might be a valuable for development purposes as well.

What if we'd add a toString method to the constraints and we'd add this to Msg? This way we'd know the contents of the constraint, thus we we'd know how the constraint is violated.

[martong]

In D79431#2263690, @martong wrote:

In D79431#2215610, @Szelethus wrote:

Ah, okay, silly me. Didn't catch that. Then the message is OK for now.

edit: Describing how the violation happened might be a valuable for development purposes as well.

What if we'd add a toString method to the constraints and we'd add this to Msg? This way we'd know the contents of the constraint, thus we we'd know how the constraint is violated.

I mean we'd know what is not satisfied. But, to know why exactly that is not satisfied we should dump the whole State but that's obviously not an option. Perhaps we could track which symbols and expressions are participating in the assumption related to the constraint and we could dump only those, but this seems to be a very complex approach.

[Szelethus]

In D79431#2263693, @martong wrote:

In D79431#2263690, @martong wrote:

What if we'd add a toString method to the constraints and we'd add this to Msg? This way we'd know the contents of the constraint, thus we we'd know how the constraint is violated.

I mean we'd know what is not satisfied. But, to know why exactly that is not satisfied we should dump the whole State but that's obviously not an option. Perhaps we could track which symbols and expressions are participating in the assumption related to the constraint and we could dump only those, but this seems to be a very complex approach.

I realize that the how and why phrases in this context a bit too vague :) What do you mean under having to dump the whole State? I didn't mean to compress a bug path into a warning message, only what I mentioned in D79431#2020951. In any case, I think its okay to just move on with this patch. LGTM!

[martong]

In D79431#2265155, @Szelethus wrote:

In D79431#2263693, @martong wrote:

In D79431#2263690, @martong wrote:

What if we'd add a toString method to the constraints and we'd add this to Msg? This way we'd know the contents of the constraint, thus we we'd know how the constraint is violated.

I mean we'd know what is not satisfied. But, to know why exactly that is not satisfied we should dump the whole State but that's obviously not an option. Perhaps we could track which symbols and expressions are participating in the assumption related to the constraint and we could dump only those, but this seems to be a very complex approach.

I realize that the how and why phrases in this context a bit too vague :) What do you mean under having to dump the whole State? I didn't mean to compress a bug path into a warning message, only what I mentioned in D79431#2020951. In any case, I think its okay to just move on with this patch. LGTM!

First, thanks for the review!

Second, "dumping the whole State" is an overstatement. What I meant is this: to properly explain to the user how the constraint is failed is a hard task. Because we should dig up all symbols and expressions that are related to the constraint and we should dump their values. E.g. consider the violation of a range constraint here,

ssize_t send(int socket, const void *buffer, size_t length, int flags);
void foo() {
  if (socket < 0)
    send(socket, buf, buflen, flags); // constraint violation for 'socket' it should be in [0, IntMax]
}

We should print something like this: 'socket' it should be in [0, IntMax] but it is in [IntMin, -1]
In this example, the related symbol is only socket but this could be more complex, e.g. if the constraint is related to a buffer size ...

clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp

class StdLibraryFunctionsChecker
   /// 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;
 
   class ValueConstraint;
 
   // 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
bool checkValidity(const FunctionDecl *FD) const {
       assert(ValidArg && "Arg out of range!");
       if (!ValidArg)
         return false;
       // Subclasses may further refine the validation.
       return checkSpecificValidity(FD);
     }
     ArgNo getArgNo() const { return ArgN; }
 
+    virtual StringRef getName() const = 0;
+
   protected:
     ArgNo ArgN; // Argument to which we apply the constraint.
 
     /// Do polymorphic sanity check on the constraint.
     virtual bool checkSpecificValidity(const FunctionDecl *FD) const {
       return true;
     }
   };
 
   /// 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:
+    StringRef getName() const override { return "Range"; }
     RangeConstraint(ArgNo ArgN, RangeKind Kind, const IntRangeVector &Args)
         : ValueConstraint(ArgN), Kind(Kind), Args(Args) {}
 
     const IntRangeVector &getRanges() const {
       return Args;
     }
 
   private:
public:
     }
   };
 
   class ComparisonConstraint : public ValueConstraint {
     BinaryOperator::Opcode Opcode;
     ArgNo OtherArgN;
 
   public:
+    virtual StringRef getName() const override { return "Comparison"; };
     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,
                           CheckerContext &C) const override;
   };
 
   class NotNullConstraint : public ValueConstraint {
     using ValueConstraint::ValueConstraint;
     // This variable has a role when we negate the constraint.
     bool CannotBeNull = true;
 
   public:
+    StringRef getName() const override { return "NonNull"; }
     ProgramStateRef apply(ProgramStateRef State, const CallEvent &Call,
                           const Summary &Summary,
                           CheckerContext &C) const override {
       SVal V = getArgSVal(Call, getArgNo());
       if (V.isUndef())
         return State;
 
       DefinedOrUnknownSVal L = V.castAs<DefinedOrUnknownSVal>();
class BufferSizeConstraint : public ValueConstraint {
     // The argument which is a multiplier to size. This is set in case of
     // `fread` like functions where the size is computed as a multiplication of
     // two arguments.
     llvm::Optional<ArgNo> SizeMultiplierArgN;
     // The operator we use in apply. This is negated in negate().
     BinaryOperator::Opcode Op = BO_LE;
 
   public:
+    StringRef getName() const override { return "BufferSize"; }
     BufferSizeConstraint(ArgNo Buffer, ArgNo BufSize)
         : ValueConstraint(Buffer), SizeArgN(BufSize) {}
 
     BufferSizeConstraint(ArgNo Buffer, ArgNo BufSize, ArgNo BufSizeMultiplier)
         : ValueConstraint(Buffer), SizeArgN(BufSize),
           SizeMultiplierArgN(BufSizeMultiplier) {}
 
     ProgramStateRef apply(ProgramStateRef State, const CallEvent &Call,
Summary &setSignature(const Signature &S) {
       return *this;
     }
 
     Summary &Case(ConstraintSet&& CS) {
       CaseConstraints.push_back(std::move(CS));
       return *this;
     }
     Summary &ArgConstraint(ValueConstraintPtr VC) {
+      assert(VC->getArgNo() != Ret &&
+             "Arg constraint should not refer to the return value");
       ArgConstraints.push_back(VC);
       return *this;
     }
 
     InvalidationKind getInvalidationKd() const { return InvalidationKd; }
     const Cases &getCaseConstraints() const { return CaseConstraints; }
     const ConstraintSet &getArgConstraints() const { return ArgConstraints; }
 
private:
   Optional<Summary> findFunctionSummary(const FunctionDecl *FD,
                                         CheckerContext &C) const;
   Optional<Summary> findFunctionSummary(const CallEvent &Call,
                                         CheckerContext &C) const;
 
   void initFunctionSummaries(CheckerContext &C) const;
 
   void reportBug(const CallEvent &Call, ExplodedNode *N,
-                 CheckerContext &C) const {
+                 const ValueConstraint *VC, CheckerContext &C) const {
     if (!ChecksEnabled[CK_StdCLibraryFunctionArgsChecker])
       return;
-    // TODO Add detailed diagnostic.
-    StringRef Msg = "Function argument constraint is not satisfied";
+    // TODO Add more detailed diagnostic.
+    std::string Msg =
+        (Twine("Function argument constraint is not satisfied, constraint: ") +
+         VC->getName().data() + ", ArgN: " + Twine(VC->getArgNo()))
+            .str();
     if (!BT_InvalidArg)
       BT_InvalidArg = std::make_unique<BugType>(
           CheckNames[CK_StdCLibraryFunctionArgsChecker],
           "Unsatisfied argument constraints", categories::LogicError);
     auto R = std::make_unique<PathSensitiveBugReport>(*BT_InvalidArg, Msg, N);
-    bugreporter::trackExpressionValue(N, Call.getArgExpr(0), *R);
+    bugreporter::trackExpressionValue(N, Call.getArgExpr(VC->getArgNo()), *R);
+
+    // Highlight the range of the argument that was violated.
+    R->addRange(Call.getArgSourceRange(VC->getArgNo()));
+
     C.emitReport(std::move(R));
   }
 };
 
 const StdLibraryFunctionsChecker::ArgNo StdLibraryFunctionsChecker::Ret =
     std::numeric_limits<ArgNo>::max();
 
 } // end of anonymous namespace
void StdLibraryFunctionsChecker::checkPreCall(const CallEvent &Call,
   ProgramStateRef NewState = State;
   for (const ValueConstraintPtr &Constraint : Summary.getArgConstraints()) {
     ProgramStateRef SuccessSt = Constraint->apply(NewState, Call, Summary, C);
     ProgramStateRef FailureSt =
         Constraint->negate()->apply(NewState, Call, Summary, C);
     // The argument constraint is not satisfied.
     if (FailureSt && !SuccessSt) {
       if (ExplodedNode *N = C.generateErrorNode(NewState))
-        reportBug(Call, N, C);
+        reportBug(Call, N, Constraint.get(), C);
       break;
     } else {
       // We will apply the constraint even if we cannot reason about the
       // argument. This means both SuccessSt and FailureSt can be true. If we
       // weren't applying the constraint that would mean that symbolic
       // execution continues on a code whose behaviour is undefined.
       assert(SuccessSt);
       NewState = SuccessSt;