[analyzer] StdLibraryFunctionsChecker: fix bug with arg constraints

[martong]

Summary

Previously we induced a state split if there were multiple argument
constraints given for a function. This was because we called
addTransition inside the for loop.
The fix is to is to store the state and apply the next argument
constraint on that. And once the loop is finished we call addTransition.

[martong]

I was thinking about the below test, but then I reverted back because I don't want to add "fake" summaries for testing purposes. Perhaps adding a new checker option could enable these "fake" summaries, @Szelethus what's your opinion?

diff --git a/clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp
index 64a412bb4db..c1022492429 100644
--- a/clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp
@@ -736,6 +736,14 @@ void StdLibraryFunctionsChecker::initFunctionSummaries(
   };

   FunctionSummaryMap = {
+      {
+        "__test",
+        Summaries{
+          Summary(ArgTypes{IntTy, IntTy}, RetType{IntTy}, EvalCallAsPure)
+            .ArgConstraint(ArgumentCondition(0U, WithinRange, SingleValue(1)))
+            .ArgConstraint(ArgumentCondition(1U, WithinRange, SingleValue(1)))
+        }
+      },
       // The isascii() family of functions.
       // The behavior is undefined if the value of the argument is not
       // representable as unsigned char or is not equal to EOF. See e.g. C99
diff --git a/clang/test/Analysis/std-c-library-functions-arg-constraints.c b/clang/test/Analysis/std-c-library-functions-arg-constraints.c
index a20b90ad1cc..01109e28b99 100644
--- a/clang/test/Analysis/std-c-library-functions-arg-constraints.c
+++ b/clang/test/Analysis/std-c-library-functions-arg-constraints.c
@@ -85,3 +85,18 @@ void test_notnull_symbolic2(FILE *fp, int *buf) {
     // bugpath-warning{{Function argument constraint is not satisfied}} \
     // bugpath-note{{Function argument constraint is not satisfied}}
 }
+
+int __test(int, int);
+
+void test_multiple_constraints(int x, int y) {
+  __test(x, y);
+  clang_analyzer_eval(x == 1); // \
+  // report-warning{{TRUE}} \
+  // bugpath-warning{{TRUE}} \
+  // bugpath-note{{TRUE}}
+  clang_analyzer_eval(y == 1); // \
+  // report-warning{{TRUE}} \
+  // bugpath-warning{{TRUE}} \
+  // bugpath-note{{TRUE}}
+}
+

[Szelethus]

The fix looks great!

In D76790#1941857, @martong wrote:

I was thinking about the below test, but then I reverted back because I don't want to add "fake" summaries for testing purposes. Perhaps adding a new checker option could enable these "fake" summaries, @Szelethus what's your opinion?

The fake summary sounds great. You could hide it behind a debug checker, if we are to be that cautious.

[martong]

• Add tests with a subchecker

[martong]

• Test multiple constraints on the same arg

[martong]

Ping :)

[Szelethus]

Yup, looks great, sorry for the slack!

[NoQ]

In D76790#1957061, @Szelethus wrote:

Yup, looks great

Can confirm.

clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp

void StdLibraryFunctionsChecker::checkPreCall(const CallEvent &Call,
                                               CheckerContext &C) const {
   Optional<Summary> FoundSummary = findFunctionSummary(Call, C);
   if (!FoundSummary)
     return;
 
   const Summary &Summary = *FoundSummary;
   ProgramStateRef State = C.getState();
 
+  ProgramStateRef NewState = State;
   for (const ValueConstraintPtr& VC : Summary.ArgConstraints) {
-    ProgramStateRef SuccessSt = VC->apply(State, Call, Summary);
-    ProgramStateRef FailureSt = VC->negate()->apply(State, Call, Summary);
+    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(State))
+      if (ExplodedNode *N = C.generateErrorNode(NewState))
         reportBug(Call, N, C);
       break;
     } else {
-      // 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.
+      // 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);
-      C.addTransition(SuccessSt);
+      NewState = SuccessSt;
     }
   }
+  if (NewState && NewState != State)
+    C.addTransition(NewState);
 }
 
 void StdLibraryFunctionsChecker::checkPostCall(const CallEvent &Call,
                                                CheckerContext &C) const {
   Optional<Summary> FoundSummary = findFunctionSummary(Call, C);
   if (!FoundSummary)
     return;