[PATCH] D76790: [analyzer] StdLibraryFunctionsChecker: fix bug with arg constraints

[Gabor Marton via Phabricator via cfe-commits]

martong created this revision.
martong added reviewers: NoQ, Szelethus, baloghadamsoftware.
Herald added subscribers: cfe-commits, ASDenysPetrov, steakhal, Charusso, gamesh411, dkrupp, donat.nagy, mikhail.ramalho, a.sidorin, rnkovacs, szepet, xazax.hun, whisperity.
Herald added a project: clang.
martong added a comment.

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}}
  +}
  +


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`.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D76790

Files:
  clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp


Index: clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp
===================================================================
--- clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp
+++ clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp
@@ -452,23 +452,26 @@
   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,


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D76790.252631.patch
Type: text/x-patch
Size: 1871 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20200325/bb0ce062/attachment.bin>