[analyzer][StdLibraryFunctionsChecker] Add NoteTags for applied arg

constraints

In this patch I add a new NoteTag for each applied argument constraint.
This way, any other checker that reports a bug - where the applied
constraint is relevant - will display the corresponding note. With this
change we provide more information for the users to understand some
bug reports easier.

Differential Revision: https://reviews.llvm.org/D101526

Reviewed By: NoQ

clang/lib/StaticAnalyzer/Checkers/ExprInspectionChecker.cpp

@@ -115,7 +115,8 @@ bool ExprInspectionChecker::evalCall(const CallEvent &Call,
           .Case("clang_analyzer_hashDump",
                 &ExprInspectionChecker::analyzerHashDump)
           .Case("clang_analyzer_denote", &ExprInspectionChecker::analyzerDenote)
-          .Case("clang_analyzer_express",
+          .Case("clang_analyzer_express", // This also marks the argument as
+                                          // interesting.
                 &ExprInspectionChecker::analyzerExpress)
           .StartsWith("clang_analyzer_isTainted",
                       &ExprInspectionChecker::analyzerIsTainted)
@@ -530,14 +531,14 @@ void ExprInspectionChecker::analyzerExpress(const CallExpr *CE,
   SVal ArgVal = C.getSVal(CE->getArg(0));
   SymbolRef Sym = ArgVal.getAsSymbol();
   if (!Sym) {
-    reportBug("Not a symbol", C);
+    reportBug("Not a symbol", C, ArgVal);
     return;
   }
 
   SymbolExpressor V(C.getState());
   auto Str = V.Visit(Sym);
   if (!Str) {
-    reportBug("Unable to express", C);
+    reportBug("Unable to express", C, ArgVal);
     return;
   }
 

clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp

@@ -134,9 +134,18 @@ class StdLibraryFunctionsChecker
 
     virtual StringRef getName() const = 0;
 
+    // Represents that in which context do we require a description of the
+    // constraint.
+    enum class DescriptionKind {
+      // The constraint is violated.
+      Violation,
+      // We assume that the constraint is satisfied.
+      Assumption
+    };
+
     // Give a description that explains the constraint to the user. Used when
     // the bug is reported.
-    virtual std::string describe(ProgramStateRef State,
+    virtual std::string describe(DescriptionKind DK, ProgramStateRef State,
                                  const Summary &Summary) const {
       // There are some descendant classes that are not used as argument
       // constraints, e.g. ComparisonConstraint. In that case we can safely
@@ -174,7 +183,7 @@ class StdLibraryFunctionsChecker
     RangeConstraint(ArgNo ArgN, RangeKind Kind, const IntRangeVector &Ranges)
         : ValueConstraint(ArgN), Kind(Kind), Ranges(Ranges) {}
 
-    std::string describe(ProgramStateRef State,
+    std::string describe(DescriptionKind DK, ProgramStateRef State,
                          const Summary &Summary) const override;
 
     const IntRangeVector &getRanges() const { return Ranges; }
@@ -244,7 +253,7 @@ class StdLibraryFunctionsChecker
     bool CannotBeNull = true;
 
   public:
-    std::string describe(ProgramStateRef State,
+    std::string describe(DescriptionKind DK, ProgramStateRef State,
                          const Summary &Summary) const override;
     StringRef getName() const override { return "NonNull"; }
     ProgramStateRef apply(ProgramStateRef State, const CallEvent &Call,
@@ -316,7 +325,7 @@ class StdLibraryFunctionsChecker
       return Result;
     }
 
-    std::string describe(ProgramStateRef State,
+    std::string describe(DescriptionKind DK, ProgramStateRef State,
                          const Summary &Summary) const override;
 
     ProgramStateRef apply(ProgramStateRef State, const CallEvent &Call,
@@ -704,9 +713,12 @@ private:
     // Highlight the range of the argument that was violated.
     R->addRange(Call.getArgSourceRange(VC->getArgNo()));
 
-    // Describe the argument constraint in a note.
-    R->addNote(VC->describe(C.getState(), Summary), R->getLocation(),
-               Call.getArgSourceRange(VC->getArgNo()));
+    // Describe the argument constraint violation in a note.
+    std::string Descr = VC->describe(
+        ValueConstraint::DescriptionKind::Violation, C.getState(), Summary);
+    // Capitalize the first letter b/c we want a full sentence.
+    Descr[0] = toupper(Descr[0]);
+    R->addNote(Descr, R->getLocation(), Call.getArgSourceRange(VC->getArgNo()));
 
     C.emitReport(std::move(R));
   }
@@ -735,24 +747,26 @@ static BasicValueFactory &getBVF(ProgramStateRef State) {
 }
 
 std::string StdLibraryFunctionsChecker::NotNullConstraint::describe(
-    ProgramStateRef State, const Summary &Summary) const {
+    DescriptionKind DK, ProgramStateRef State, const Summary &Summary) const {
   SmallString<48> Result;
-  Result += "The ";
+  const auto Violation = ValueConstraint::DescriptionKind::Violation;
+  Result += "the ";
   Result += getArgDesc(ArgN);
-  Result += " should not be NULL";
+  Result += DK == Violation ? " should not be NULL" : " is not NULL";
   return Result.c_str();
 }
 
 std::string StdLibraryFunctionsChecker::RangeConstraint::describe(
-    ProgramStateRef State, const Summary &Summary) const {
+    DescriptionKind DK, ProgramStateRef State, const Summary &Summary) const {
 
   BasicValueFactory &BVF = getBVF(State);
 
   QualType T = Summary.getArgType(getArgNo());
   SmallString<48> Result;
-  Result += "The ";
+  const auto Violation = ValueConstraint::DescriptionKind::Violation;
+  Result += "the ";
   Result += getArgDesc(ArgN);
-  Result += " should be ";
+  Result += DK == Violation ? " should be " : " is ";
 
   // Range kind as a string.
   Kind == OutOfRange ? Result += "out of" : Result += "within";
@@ -784,16 +798,18 @@ StdLibraryFunctionsChecker::getArgDesc(StdLibraryFunctionsChecker::ArgNo ArgN) {
   SmallString<8> Result;
   Result += std::to_string(ArgN + 1);
   Result += llvm::getOrdinalSuffix(ArgN + 1);
-  Result += " arg";
+  Result += " argument";
   return Result;
 }
 
 std::string StdLibraryFunctionsChecker::BufferSizeConstraint::describe(
-    ProgramStateRef State, const Summary &Summary) const {
+    DescriptionKind DK, ProgramStateRef State, const Summary &Summary) const {
   SmallString<96> Result;
-  Result += "The size of the ";
+  const auto Violation = ValueConstraint::DescriptionKind::Violation;
+  Result += "the size of the ";
   Result += getArgDesc(ArgN);
-  Result += " should be equal to or less than the value of ";
+  Result += DK == Violation ? " should be " : " is ";
+  Result += "equal to or greater than the value of ";
   if (ConcreteSize) {
     ConcreteSize->toString(Result);
   } else if (SizeArgN) {
@@ -927,6 +943,7 @@ void StdLibraryFunctionsChecker::checkPreCall(const CallEvent &Call,
   ProgramStateRef State = C.getState();
 
   ProgramStateRef NewState = State;
+  ExplodedNode *NewNode = C.getPredecessor();
   for (const ValueConstraintPtr &Constraint : Summary.getArgConstraints()) {
     ProgramStateRef SuccessSt = Constraint->apply(NewState, Call, Summary, C);
     ProgramStateRef FailureSt =
@@ -936,17 +953,28 @@ void StdLibraryFunctionsChecker::checkPreCall(const CallEvent &Call,
       if (ExplodedNode *N = C.generateErrorNode(NewState))
         reportBug(Call, N, Constraint.get(), Summary, 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;
+    }
+    // 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;
+    if (NewState != State) {
+      SmallString<64> Msg;
+      Msg += "Assuming ";
+      Msg += Constraint->describe(ValueConstraint::DescriptionKind::Assumption,
+                                  NewState, Summary);
+      const auto ArgSVal = Call.getArgSVal(Constraint->getArgNo());
+      NewNode = C.addTransition(
+          NewState, NewNode,
+          C.getNoteTag([Msg = std::move(Msg), ArgSVal](
+                           PathSensitiveBugReport &BR, llvm::raw_ostream &OS) {
+            if (BR.isInteresting(ArgSVal))
+              OS << Msg;
+          }));
     }
   }
-  if (NewState && NewState != State)
-    C.addTransition(NewState);
 }
 
 void StdLibraryFunctionsChecker::checkPostCall(const CallEvent &Call,
@@ -2882,6 +2910,10 @@ void StdLibraryFunctionsChecker::initFunctionSummaries(
         Summary(EvalCallAsPure).ArgConstraint(NotNull(ArgNo(0))));
 
     // Test range values.
+    addToFunctionSummaryMap(
+        "__single_val_0", Signature(ArgTypes{IntTy}, RetType{IntTy}),
+        Summary(EvalCallAsPure)
+            .ArgConstraint(ArgumentCondition(0U, WithinRange, SingleValue(0))));
     addToFunctionSummaryMap(
         "__single_val_1", Signature(ArgTypes{IntTy}, RetType{IntTy}),
         Summary(EvalCallAsPure)

clang/test/Analysis/std-c-library-functions-arg-constraints-note-tags.cpp

@@ -0,0 +1,51 @@
+// RUN: %clang_analyze_cc1 %s \
+// RUN:   -analyzer-checker=core \
+// RUN:   -analyzer-checker=apiModeling.StdCLibraryFunctions \
+// RUN:   -analyzer-checker=alpha.unix.StdCLibraryFunctionArgs \
+// RUN:   -analyzer-checker=debug.StdCLibraryFunctionsTester \
+// RUN:   -analyzer-config apiModeling.StdCLibraryFunctions:DisplayLoadedSummaries=true \
+// RUN:   -analyzer-checker=debug.ExprInspection \
+// RUN:   -analyzer-config eagerly-assume=false \
+// RUN:   -triple i686-unknown-linux \
+// RUN:   -analyzer-output=text \
+// RUN:   -verify
+
+template <typename T>
+void clang_analyzer_express(T x);
+void clang_analyzer_eval(bool);
+int clang_analyzer_getExtent(void *);
+
+
+// Check NotNullConstraint assumption notes.
+int __not_null(int *);
+int test_not_null_note(int *x, int y) {
+  __not_null(x);      // expected-note{{Assuming the 1st argument is not NULL}}
+  if (x)              // expected-note{{'x' is non-null}} \
+                      // expected-note{{Taking true branch}}
+    if (!y)           // expected-note{{Assuming 'y' is 0}} \
+                      // expected-note{{Taking true branch}}
+      return 1 / y;   // expected-warning{{Division by zero}} \
+                      // expected-note{{Division by zero}}
+
+  return 0;
+}
+
+// Check the RangeConstraint assumption notes.
+int __single_val_0(int);      // [0, 0]
+int test_range_constraint_note(int x, int y) {
+  __single_val_0(x);  // expected-note{{Assuming the 1st argument is within the range [0, 0]}}
+  return y / x;       // expected-warning{{Division by zero}} \
+                      // expected-note{{Division by zero}}
+}
+
+// Check the BufferSizeConstraint assumption notes.
+int __buf_size_arg_constraint_concrete(const void *buf); // size of buf must be >= 10
+void test_buffer_size_note(char *buf, int y) {
+  __buf_size_arg_constraint_concrete(buf); // expected-note {{Assuming the size of the 1st argument is equal to or greater than the value of 10}}
+  clang_analyzer_eval(clang_analyzer_getExtent(buf) >= 10); // expected-warning{{TRUE}} \
+                                                            // expected-note{{TRUE}}
+
+  // clang_analyzer_express marks the argument as interesting.
+  clang_analyzer_express(buf); // expected-warning {{}} // the message does not really matter \
+                               // expected-note {{}}
+}

clang/test/Analysis/std-c-library-functions-arg-constraints-notes.cpp

@@ -15,7 +15,7 @@
 int __not_null(int *);
 void test_not_null(int *x) {
   __not_null(nullptr); // \
-  // expected-note{{The 1st arg should not be NULL}} \
+  // expected-note{{The 1st argument should not be NULL}} \
   // expected-warning{{}}
 }
 
@@ -29,21 +29,21 @@ void test_buffer_size(int x) {
   case 1: {
     char buf[9];
     __buf_size_arg_constraint_concrete(buf); // \
-    // expected-note{{The size of the 1st arg should be equal to or less than the value of 10}} \
+    // expected-note{{The size of the 1st argument should be equal to or greater than the value of 10}} \
     // expected-warning{{}}
     break;
   }
   case 2: {
     char buf[3];
     __buf_size_arg_constraint(buf, 4); // \
-    // expected-note{{The size of the 1st arg should be equal to or less than the value of the 2nd arg}} \
+    // expected-note{{The size of the 1st argument should be equal to or greater than the value of the 2nd arg}} \
     // expected-warning{{}}
     break;
   }
   case 3: {
     char buf[3];
     __buf_size_arg_constraint_mul(buf, 4, 2); // \
-    // expected-note{{The size of the 1st arg should be equal to or less than the value of the 2nd arg times the 3rd arg}} \
+    // expected-note{{The size of the 1st argument should be equal to or greater than the value of the 2nd argument times the 3rd argument}} \
     // expected-warning{{}}
     break;
   }
@@ -56,7 +56,7 @@ int __range_1_2(int);         // [1, 2]
 int __range_1_2__4_5(int);    // [1, 2], [4, 5]
 void test_range(int x) {
     __single_val_1(2); // \
-    // expected-note{{The 1st arg should be within the range [1, 1]}} \
+    // expected-note{{The 1st argument should be within the range [1, 1]}} \
     // expected-warning{{}}
 }
 // Do more specific check against the range strings.

clang/test/Analysis/std-c-library-functions-arg-constraints.c

@@ -239,6 +239,7 @@ void test_constraints_on_multiple_args(int x, int y) {
   // State split should not happen here. I.e. x == 1 should not be evaluated
   // FALSE.
   __two_constrained_args(x, y);
+  //NOTE! Because of the second `clang_analyzer_eval` call we have two bug
   clang_analyzer_eval(x == 1); // \
   // report-warning{{TRUE}} \
   // bugpath-warning{{TRUE}} \
@@ -252,7 +253,6 @@ void test_constraints_on_multiple_args(int x, int y) {
 int __arg_constrained_twice(int);
 void test_multiple_constraints_on_same_arg(int x) {
   __arg_constrained_twice(x);
-  // Check that both constraints are applied and only one branch is there.
   clang_analyzer_eval(x < 1 || x > 2); // \
   // report-warning{{TRUE}} \
   // bugpath-warning{{TRUE}} \