[analyzer] StdLibraryFunctionsChecker: match signature based on FunctionDecl

[martong]

Summary

Currently we match the summary signature based on the arguments in the CallExpr.
There are a few problems with this approach.

1. Variadic arguments are handled badly. Consider the below code:

int foo(void *stream, const char *format, ...);
void test_arg_constraint_on_variadic_fun() {
   foo(0, "%d%d", 1, 2); // CallExpr
}

Here the call expression holds 4 arguments, whereas the function declaration
has only 2 ParmVarDecls. So there is no way to create a summary that
matches the call expression, because the discrepancy in the number of
arguments causes a mismatch.

2. The call expression does not handle the restrict type qualifier. In C99, fwrite's signature is the following:

size_t fwrite(const void *restrict, size_t, size_t, FILE *restrict);

However, in a call expression, like below, the type of the argument does not
have the restrict qualifier.

void test_fread_fwrite(FILE *fp, int *buf) {
  size_t x = fwrite(buf, sizeof(int), 10, fp);
}

The type of the 1st argument in the CallExpr is

PointerType 0x55b7168af2e0 'const void *'
`-QualType 0x55b7168373c1 'const void' const
  `-BuiltinType 0x55b7168373c0 'void'

However, the type in the signature of the FunctionDecl and in the summary map is

QualType 0x55b7168af2e2 'const void *__restrict' __restrict
`-PointerType 0x55b7168af2e0 'const void *'
  `-QualType 0x55b7168373c1 'const void' const
    `-BuiltinType 0x55b7168373c0 'void'

The restrict qualifier is not in the call expression (actually, why would it be?). So, this results in an unmatched signature and the summary is not applied.

The solution is to match the summary against the referened callee
FunctionDecl that we can query from the CallExpr.

Further patches will continue with additional refactoring where I am going to
do a lookup during the checker initialization and the signature match will
happen there. That way, we will not check the signature during every call,
rather we will compare only two FunctionDecl pointers.

[NoQ]

Excellent! No objections. It should have absolutely been like that from the start: summaries are of functions, not of call sites.

[balazske]

clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp

552

This assert can be removed, after the previous getCanonicalType call it looks redundant.

[Szelethus]

LGTM! Maybe its worth glancing over the rest of the code (comments in particular) whether anything would be outdated by this change.

[martong]

In D77410#1963288, @Szelethus wrote:

LGTM! Maybe its worth glancing over the rest of the code (comments in particular) whether anything would be outdated by this change.

Yeah, seems like the comments are still aligned. Also, I am going to submit the next patch, which will scramble the current structures (and probably the related comments as well).

clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp

552

Thanks, good catch!

clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp

public:
     QualType getArgType(ArgNo ArgN) const {
       QualType T = (ArgN == Ret) ? RetTy : ArgTys[ArgN];
       assertTypeSuitableForSummary(T);
       return T;
     }
 
     /// Try our best to figure out if the call expression is the call of
     /// *the* library function to which this specification applies.
-    bool matchesCall(const CallExpr *CE) const;
+    bool matchesCall(const FunctionDecl *FD) const;
   };
 
   // The same function (as in, function identifier) may have different
   // summaries assigned to it, with different argument and return value types.
   // We call these "variants" of the function. This can be useful for handling
   // C++ function overloads, and also it can be used when the same function
   // may have different definitions on different platforms.
   typedef std::vector<Summary> Summaries;
enum CheckKind {
     CK_StdCLibraryFunctionsTesterChecker,
     CK_NumCheckKinds
   };
   DefaultBool ChecksEnabled[CK_NumCheckKinds];
   CheckerNameRef CheckNames[CK_NumCheckKinds];
 
 private:
   Optional<Summary> findFunctionSummary(const FunctionDecl *FD,
-                                        const CallExpr *CE,
                                         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 {
case NoEvalCall:
     // Summary tells us to avoid performing eval::Call. The function is possibly
     // evaluated by another checker, or evaluated conservatively.
     return false;
   }
   llvm_unreachable("Unknown invalidation kind!");
 }
 
 bool StdLibraryFunctionsChecker::Summary::matchesCall(
-    const CallExpr *CE) const {
+    const FunctionDecl *FD) const {
   // Check number of arguments:
-  if (CE->getNumArgs() != ArgTys.size())
+  if (FD->param_size() != ArgTys.size())
     return false;
 
   // Check return type if relevant:
-  if (!RetTy.isNull() && RetTy != CE->getType().getCanonicalType())
+  if (!RetTy.isNull() && RetTy != FD->getReturnType().getCanonicalType())
     return false;
 
   // Check argument types when relevant:
   for (size_t I = 0, E = ArgTys.size(); I != E; ++I) {
     QualType FormalT = ArgTys[I];
     // Null type marks irrelevant arguments.
     if (FormalT.isNull())
       continue;
 
     assertTypeSuitableForSummary(FormalT);
 
-    QualType ActualT = StdLibraryFunctionsChecker::getArgType(CE, I);
-    assert(ActualT.isCanonical());
+    QualType ActualT = FD->getParamDecl(I)->getType().getCanonicalType();
     if (ActualT != FormalT)
       return false;
   }
 
   return true;
 }
 
 Optional<StdLibraryFunctionsChecker::Summary>
 StdLibraryFunctionsChecker::findFunctionSummary(const FunctionDecl *FD,
-                                                const CallExpr *CE,
                                                 CheckerContext &C) const {
-  // Note: we cannot always obtain FD from CE
-  // (eg. virtual call, or call by pointer).
-  assert(CE);
-
   if (!FD)
     return None;
 
   initFunctionSummaries(C);
 
   IdentifierInfo *II = FD->getIdentifier();
   if (!II)
     return None;
   StringRef Name = II->getName();
   if (Name.empty() || !C.isCLibraryFunction(FD, Name))
     return None;
 
   auto FSMI = FunctionSummaryMap.find(Name);
   if (FSMI == FunctionSummaryMap.end())
     return None;
 
   // Verify that function signature matches the spec in advance.
   // Otherwise we might be modeling the wrong function.
   // Strict checking is important because we will be conducting
   // very integral-type-sensitive operations on arguments and
   // return values.
   const Summaries &SpecVariants = FSMI->second;
   for (const Summary &Spec : SpecVariants)
-    if (Spec.matchesCall(CE))
+    if (Spec.matchesCall(FD))
       return Spec;
 
   return None;
 }
 
 Optional<StdLibraryFunctionsChecker::Summary>
 StdLibraryFunctionsChecker::findFunctionSummary(const CallEvent &Call,
                                                 CheckerContext &C) const {
   const FunctionDecl *FD = dyn_cast_or_null<FunctionDecl>(Call.getDecl());
   if (!FD)
     return None;
-  const CallExpr *CE = dyn_cast_or_null<CallExpr>(Call.getOriginExpr());
-  if (!CE)
-    return None;
-  return findFunctionSummary(FD, CE, C);
+  return findFunctionSummary(FD, C);
 }
 
 void StdLibraryFunctionsChecker::initFunctionSummaries(
     CheckerContext &C) const {
   if (!FunctionSummaryMap.empty())
     return;
 
   SValBuilder &SVB = C.getSValBuilder();
   BasicValueFactory &BVF = SVB.getBasicValueFactory();
   const ASTContext &ACtx = BVF.getContext();
 
   // These types are useful for writing specifications quickly,
   // New specifications should probably introduce more types.
   // Some types are hard to obtain from the AST, eg. "ssize_t".
   // In such cases it should be possible to provide multiple variants
   // of function summary for common cases (eg. ssize_t could be int or long
   // or long long, so three summary variants would be enough).
   // Of course, function variants are also useful for C++ overloads.
   const QualType
       Irrelevant{}; // A placeholder, whenever we do not care about the type.
   const QualType IntTy = ACtx.IntTy;
   const QualType LongTy = ACtx.LongTy;
   const QualType LongLongTy = ACtx.LongLongTy;
   const QualType SizeTy = ACtx.getSizeType();
-  const QualType VoidPtrTy = ACtx.VoidPtrTy; // void *T
+  const QualType VoidPtrTy = ACtx.VoidPtrTy; // void *
+  const QualType VoidPtrRestrictTy =
+      ACtx.getRestrictType(VoidPtrTy); // void *restrict
   const QualType ConstVoidPtrTy =
-      ACtx.getPointerType(ACtx.VoidTy.withConst()); // const void *T
+      ACtx.getPointerType(ACtx.VoidTy.withConst()); // const void *
+  const QualType ConstCharPtrTy =
+      ACtx.getPointerType(ACtx.CharTy.withConst()); // const char *
+  const QualType ConstVoidPtrRestrictTy =
+      ACtx.getRestrictType(ConstVoidPtrTy); // const void *restrict
 
   const RangeInt IntMax = BVF.getMaxValue(IntTy).getLimitedValue();
   const RangeInt LongMax = BVF.getMaxValue(LongTy).getLimitedValue();
   const RangeInt LongLongMax = BVF.getMaxValue(LongLongTy).getLimitedValue();
 
   // Set UCharRangeMax to min of int or uchar maximum value.
   // The C standard states that the arguments of functions like isalpha must
   // be representable as an unsigned char. Their type is 'int', so the max
void StdLibraryFunctionsChecker::initFunctionSummaries(
   };
   auto Read = [&](RetType R, RangeInt Max) {
     return Summary(ArgTypes{Irrelevant, Irrelevant, SizeTy}, RetType{R},
                    NoEvalCall)
         .Case({ReturnValueCondition(LessThanOrEq, ArgNo(2)),
                ReturnValueCondition(WithinRange, Range(-1, Max))});
   };
   auto Fread = [&]() {
-    return Summary(ArgTypes{VoidPtrTy, Irrelevant, SizeTy, Irrelevant},
+    return Summary(ArgTypes{VoidPtrRestrictTy, Irrelevant, SizeTy, Irrelevant},
                    RetType{SizeTy}, NoEvalCall)
         .Case({
             ReturnValueCondition(LessThanOrEq, ArgNo(2)),
         })
         .ArgConstraint(NotNull(ArgNo(0)));
   };
   auto Fwrite = [&]() {
-    return Summary(ArgTypes{ConstVoidPtrTy, Irrelevant, SizeTy, Irrelevant},
-                   RetType{SizeTy}, NoEvalCall)
+    return Summary(
+               ArgTypes{ConstVoidPtrRestrictTy, Irrelevant, SizeTy, Irrelevant},
         .Case({
             ReturnValueCondition(LessThanOrEq, ArgNo(2)),
         })
         .ArgConstraint(NotNull(ArgNo(0)));
   };
   auto Getline = [&](RetType R, RangeInt Max) {
     return Summary(ArgTypes{Irrelevant, Irrelevant, Irrelevant}, RetType{R},
                    NoEvalCall)
llvm::StringMap<Summaries> TestFunctionSummaryMap = {
          Summaries{Summary(ArgTypes{IntTy}, RetType{IntTy}, EvalCallAsPure)
                        .ArgConstraint(
                            ArgumentCondition(0U, OutOfRange, SingleValue(1)))
                        .ArgConstraint(
                            ArgumentCondition(0U, OutOfRange, SingleValue(2)))}},
         {"__defaultparam", Summaries{Summary(ArgTypes{Irrelevant, IntTy},
                                              RetType{IntTy}, EvalCallAsPure)
                                          .ArgConstraint(NotNull(ArgNo(0)))}},
-    };
+        {"__variadic", Summaries{Summary(ArgTypes{VoidPtrTy, ConstCharPtrTy},
+                                         RetType{IntTy}, EvalCallAsPure)
+                                     .ArgConstraint(NotNull(ArgNo(0)))
+                                     .ArgConstraint(NotNull(ArgNo(1)))}}};
     for (auto &E : TestFunctionSummaryMap) {
       auto InsertRes =
           FunctionSummaryMap.insert({std::string(E.getKey()), E.getValue()});
       assert(InsertRes.second &&
              "Test functions must not clash with modeled functions");
       (void)InsertRes;
     }
   }

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

if (x > 255) { // \
     // bugpath-note{{Function argument constraint is not satisfied}}
 
     (void)ret;
   }
 }
 
 typedef struct FILE FILE;
 typedef typeof(sizeof(int)) size_t;
-size_t fread(void *, size_t, size_t, FILE *);
+size_t fread(void *restrict, size_t, size_t, FILE *);
 void test_notnull_concrete(FILE *fp) {
   fread(0, sizeof(int), 10, fp); // \
   // report-warning{{Function argument constraint is not satisfied}} \
   // bugpath-warning{{Function argument constraint is not satisfied}} \
   // bugpath-note{{Function argument constraint is not satisfied}}
 }
 void test_notnull_symbolic(FILE *fp, int *buf) {
   fread(buf, sizeof(int), 10, fp);
void test_multiple_constraints_on_same_arg(int 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}} \
   // bugpath-note{{TRUE}} \
   // bugpath-note{{Assuming 'x' is < 1}} \
   // bugpath-note{{Left side of '||' is true}}
 }
+
+int __variadic(void *stream, const char *format, ...);
+void test_arg_constraint_on_variadic_fun() {
+  __variadic(0, "%d%d", 1, 2); // \
+  // report-warning{{Function argument constraint is not satisfied}} \
+  // bugpath-warning{{Function argument constraint is not satisfied}} \
+  // bugpath-note{{Function argument constraint is not satisfied}}
+}

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

if (y >= 0) {
       // -1 overflows on promotion!
       clang_analyzer_eval(y <= sizeof(glob)); // expected-warning{{FALSE}}
     }
   } else {
     clang_analyzer_eval(x == -1); // expected-warning{{TRUE}}
   }
 }
 
-size_t fread(void *, size_t, size_t, FILE *);
+size_t fread(void *restrict, size_t, size_t, FILE *);
 size_t fwrite(const void *restrict, size_t, size_t, FILE *restrict);
 void test_fread_fwrite(FILE *fp, int *buf) {
 
   size_t x = fwrite(buf, sizeof(int), 10, fp);
   clang_analyzer_eval(x <= 10); // expected-warning{{TRUE}}
 
   size_t y = fread(buf, sizeof(int), 10, fp);
   clang_analyzer_eval(y <= 10); // expected-warning{{TRUE}}