[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

323

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

323

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

88

Shouldn't we use using instead?

323

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

88

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

323

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

323

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
public:
     /// is returned then the constraint is not feasible.
     virtual ProgramStateRef apply(ProgramStateRef State, const CallEvent &Call,
                                   const Summary &Summary) const = 0;
     virtual ValueConstraintPtr negate() const {
       llvm_unreachable("Not implemented");
     };
     ArgNo getArgNo() const { return ArgN; }
 
+    virtual StringRef getName() const = 0;
+
   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:
+    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) 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) const override {
       SVal V = getArgSVal(Call, getArgNo());
       if (V.isUndef())
         return State;
 
       DefinedOrUnknownSVal L = V.castAs<DefinedOrUnknownSVal>();
       if (!L.getAs<Loc>())
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 =
+        std::string("Function argument constraint is not satisfied, ") +
+        VC->getName().data() + ", ArgN: " + std::to_string(VC->getArgNo());
     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,
   if (!FoundSummary)
     return;
 
   const Summary &Summary = *FoundSummary;
   ProgramStateRef State = C.getState();
 
   ProgramStateRef NewState = State;
   for (const ValueConstraintPtr& VC : Summary.ArgConstraints) {
+    assert(VC->getArgNo() != Ret &&
+           "Arg constraint should not refer to the return value");
     ProgramStateRef SuccessSt = VC->apply(NewState, Call, Summary);
     ProgramStateRef FailureSt = VC->negate()->apply(NewState, Call, Summary);
     // The argument constraint is not satisfied.
     if (FailureSt && !SuccessSt) {
       if (ExplodedNode *N = C.generateErrorNode(NewState))
-        reportBug(Call, N, C);
+        reportBug(Call, N, VC.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;