[analyzer][CallAndMessage][NFC] Split up checkPreCall

[Szelethus]

Summary

The patch aims to use CallEvents interface in a more principled manner, and also to highlight what this checker really does. It in fact checks for 5 different kinds of errors (from checkPreCall, that is):

• Invalid function pointer related errors
• Call of methods from an invalid C++ this object
• Function calls with incorrect amount of parameters
• Invalid arguments for operator delete
• Pass of uninitialized values to pass-by-value parameters

In a previous patch I complained that this checker is responsible for emitting a lot of different diagnostics all under core.CallAndMessage's name, and this patch shows where we could start to assign different diagnostics to different entities.

[balazske]

clang/lib/StaticAnalyzer/Checkers/CallAndMessageChecker.cpp

479

The one thing where I am not sure is if this condition is really needed for every case (for example can checkFunctionPointerCall be moved outside this if?). I see only that checkParameterCount has to be in this if block.

[balazske]

clang/lib/StaticAnalyzer/Checkers/CallAndMessageChecker.cpp

26

Are these new includes needed?

[Szelethus]

Fixes according to @balazske's comments, cheers!

[Szelethus]

clang/lib/StaticAnalyzer/Checkers/CallAndMessageChecker.cpp

26

Nope, you're right.

371

@balazske this is the corresponding condition that got changed. Now, your inline is fair, and I remember being puzzled by needing that condition to not crash. One thing to note is that CallAndMessage really tests every part of CallEvent, and since its registered very early (its also a dependency in addition to being a core checker) it runs before most other checkers. Anyways, I revisited each of the new functions I added, and wow, you totally nailed it. Nothing crashes if I remove that condition, but I vividly remember it breaking earlier.

So yeah, nice catch!

[balazske]

Looks good. Not sure if it work in all cases after the MallocChecker problems but we can fix problems if any later.

clang/lib/StaticAnalyzer/Checkers/CallAndMessageChecker.cpp

 #include "clang/StaticAnalyzer/Core/BugReporter/BugType.h"
 #include "clang/StaticAnalyzer/Core/Checker.h"
 #include "clang/StaticAnalyzer/Core/CheckerManager.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h"
 #include "llvm/ADT/SmallString.h"
 #include "llvm/ADT/StringExtras.h"
 #include "llvm/Support/Casting.h"
 #include "llvm/Support/raw_ostream.h"
 
 using namespace clang;
 using namespace ento;
 
 namespace {
 
 class CallAndMessageChecker
     : public Checker<check::PreObjCMessage, check::ObjCMessageNil,
public:
 
   /// Fill in the return value that results from messaging nil based on the
   /// return type and architecture and diagnose if the return value will be
   /// garbage.
   void checkObjCMessageNil(const ObjCMethodCall &msg, CheckerContext &C) const;
 
   void checkPreCall(const CallEvent &Call, CheckerContext &C) const;
 
+  ProgramStateRef checkFunctionPointerCall(const CallExpr *CE,
+                                           CheckerContext &C,
+                                           ProgramStateRef State) const;
+
+  ProgramStateRef checkCXXMethodCall(const CXXInstanceCall *CC,
+                                     CheckerContext &C,
+                                     ProgramStateRef State) const;
+
+  ProgramStateRef checkParameterCount(const CallEvent &Call, CheckerContext &C,
+                                      ProgramStateRef State) const;
+
+  ProgramStateRef checkCXXDeallocation(const CXXDeallocatorCall *DC,
+                                       CheckerContext &C,
+                                       ProgramStateRef State) const;
+
+  ProgramStateRef checkArgInitializedness(const CallEvent &Call,
+                                          CheckerContext &C,
+                                          ProgramStateRef State) const;
+
 private:
   bool PreVisitProcessArg(CheckerContext &C, SVal V, SourceRange ArgRange,
                           const Expr *ArgEx, int ArgumentNumber,
                           bool CheckUninitFields, const CallEvent &Call,
                           std::unique_ptr<BugType> &BT,
                           const ParmVarDecl *ParamDecl) const;
 
   static void emitBadCall(BugType *BT, CheckerContext &C, const Expr *BadE);
if (F.Find(D->getRegion())) {
       }
       return true;
     }
   }
 
   return false;
 }
 
-void CallAndMessageChecker::checkPreCall(const CallEvent &Call,
-                                         CheckerContext &C) const {
-  ProgramStateRef State = C.getState();
-  if (const CallExpr *CE = dyn_cast_or_null<CallExpr>(Call.getOriginExpr())) {
-    const Expr *Callee = CE->getCallee()->IgnoreParens();
-    const LocationContext *LCtx = C.getLocationContext();
-    SVal L = State->getSVal(Callee, LCtx);
+ProgramStateRef CallAndMessageChecker::checkFunctionPointerCall(
+    const CallExpr *CE, CheckerContext &C, ProgramStateRef State) const {
+
 
-    if (L.isUndef()) {
-      if (!BT_call_undef)
-        BT_call_undef.reset(new BuiltinBug(
-            this, "Called function pointer is an uninitialized pointer value"));
-      emitBadCall(BT_call_undef.get(), C, Callee);
-      return;
-    }
+    return nullptr;
 
-    ProgramStateRef StNonNull, StNull;
-    std::tie(StNonNull, StNull) =
-        State->assume(L.castAs<DefinedOrUnknownSVal>());
+  std::tie(StNonNull, StNull) = State->assume(L.castAs<DefinedOrUnknownSVal>());
 
-    if (StNull && !StNonNull) {
-      if (!BT_call_null)
-        BT_call_null.reset(new BuiltinBug(
-            this, "Called function pointer is null (null dereference)"));
-      emitBadCall(BT_call_null.get(), C, Callee);
-      return;
-    }
+    return nullptr;
 
-    State = StNonNull;
-  }
-
-  // If this is a call to a C++ method, check if the callee is null or
-  // undefined.
-  if (const CXXInstanceCall *CC = dyn_cast<CXXInstanceCall>(&Call)) {
-    SVal V = CC->getCXXThisVal();
-    if (V.isUndef()) {
-      if (!BT_cxx_call_undef)
-        BT_cxx_call_undef.reset(
-            new BuiltinBug(this, "Called C++ object pointer is uninitialized"));
-      emitBadCall(BT_cxx_call_undef.get(), C, CC->getCXXThisExpr());
-      return;
-    }
-
-    ProgramStateRef StNonNull, StNull;
-    std::tie(StNonNull, StNull) =
-        State->assume(V.castAs<DefinedOrUnknownSVal>());
-
-    if (StNull && !StNonNull) {
-      if (!BT_cxx_call_null)
-        BT_cxx_call_null.reset(
-            new BuiltinBug(this, "Called C++ object pointer is null"));
-      emitBadCall(BT_cxx_call_null.get(), C, CC->getCXXThisExpr());
-      return;
-    }
+  return StNonNull;
 
-    State = StNonNull;
-  }
+ProgramStateRef CallAndMessageChecker::checkParameterCount(
+    const CallEvent &Call, CheckerContext &C, ProgramStateRef State) const {
 
-  const Decl *D = Call.getDecl();
-  if (D && (isa<FunctionDecl>(D) || isa<BlockDecl>(D))) {
-    // If we have a function or block declaration, we can make sure we pass
-    // enough parameters.
-    unsigned Params = Call.parameters().size();
-    if (Call.getNumArgs() < Params) {
-      ExplodedNode *N = C.generateErrorNode();
-      if (!N)
-        return;
+  if (Call.getNumArgs() >= Params)
+    return State;
+
+    return nullptr;
 
-      LazyInit_BT("Function call with too few arguments", BT_call_few_args);
 
-      SmallString<512> Str;
-      llvm::raw_svector_ostream os(Str);
-      if (isa<FunctionDecl>(D)) {
-        os << "Function ";
-      } else {
-        assert(isa<BlockDecl>(D));
-        os << "Block ";
-      }
-      os << "taking " << Params << " argument"
-         << (Params == 1 ? "" : "s") << " is called with fewer ("
-         << Call.getNumArgs() << ")";
+  if (isa<AnyFunctionCall>(Call)) {
+    assert(isa<BlockCall>(Call));
+  os << "taking " << Params << " argument" << (Params == 1 ? "" : "s")
+     << " is called with fewer (" << Call.getNumArgs() << ")";
 
-      C.emitReport(std::make_unique<PathSensitiveBugReport>(*BT_call_few_args,
-                                                            os.str(), N));
-    }
+  C.emitReport(
+      std::make_unique<PathSensitiveBugReport>(*BT_call_few_args, os.str(), N));
+  return nullptr;
+
+ProgramStateRef CallAndMessageChecker::checkCXXMethodCall(
+    const CXXInstanceCall *CC, CheckerContext &C, ProgramStateRef State) const {
+
+  SVal V = CC->getCXXThisVal();
+  if (V.isUndef()) {
+    if (!BT_cxx_call_undef)
+      BT_cxx_call_undef.reset(
+          new BuiltinBug(this, "Called C++ object pointer is uninitialized"));
+    emitBadCall(BT_cxx_call_undef.get(), C, CC->getCXXThisExpr());
+    return nullptr;
   }
 
-  if (const auto *DC = dyn_cast<CXXDeallocatorCall>(&Call)) {
-    const CXXDeleteExpr *DE = DC->getOriginExpr();
-    assert(DE);
-    SVal Arg = C.getSVal(DE->getArgument());
-    if (Arg.isUndef()) {
-      StringRef Desc;
-      ExplodedNode *N = C.generateErrorNode();
-      if (!N)
-        return;
-      if (!BT_cxx_delete_undef)
-        BT_cxx_delete_undef.reset(
-            new BuiltinBug(this, "Uninitialized argument value"));
-      if (DE->isArrayFormAsWritten())
-        Desc = "Argument to 'delete[]' is uninitialized";
-      else
-        Desc = "Argument to 'delete' is uninitialized";
-      BugType *BT = BT_cxx_delete_undef.get();
-      auto R = std::make_unique<PathSensitiveBugReport>(*BT, Desc, N);
-      bugreporter::trackExpressionValue(N, DE, *R);
-      C.emitReport(std::move(R));
-      return;
-    }
-  }
+  ProgramStateRef StNonNull, StNull;
+  std::tie(StNonNull, StNull) = State->assume(V.castAs<DefinedOrUnknownSVal>());
+
+  if (StNull && !StNonNull) {
+    if (!BT_cxx_call_null)
+      BT_cxx_call_null.reset(
+          new BuiltinBug(this, "Called C++ object pointer is null"));
+    emitBadCall(BT_cxx_call_null.get(), C, CC->getCXXThisExpr());
+    return nullptr;
+  }
+
+  return StNonNull;
+}
+
+ProgramStateRef
+CallAndMessageChecker::checkCXXDeallocation(const CXXDeallocatorCall *DC,
+                                            CheckerContext &C,
+                                            ProgramStateRef State) const {
+  if (!Arg.isUndef())
+    return State;
+
+    return nullptr;
+  return nullptr;
 
+ProgramStateRef CallAndMessageChecker::checkArgInitializedness(
+    const CallEvent &Call, CheckerContext &C, ProgramStateRef State) const {
+
+  const Decl *D = Call.getDecl();
+
   // Don't check for uninitialized field values in arguments if the
   // caller has a body that is available and we have the chance to inline it.
   // This is a hack, but is a reasonable compromise betweens sometimes warning
   // and sometimes not depending on if we decide to inline a function.
   const bool checkUninitFields =
       !(C.getAnalysisManager().shouldInlineCall() && (D && D->getBody()));
 
   std::unique_ptr<BugType> *BT;
   if (isa<ObjCMethodCall>(Call))
     BT = &BT_msg_arg;
   else
     BT = &BT_call_arg;
 
   const FunctionDecl *FD = dyn_cast_or_null<FunctionDecl>(D);
   for (unsigned i = 0, e = Call.getNumArgs(); i != e; ++i) {
     const ParmVarDecl *ParamDecl = nullptr;
-    if(FD && i < FD->getNumParams())
+    if (FD && i < FD->getNumParams())
       ParamDecl = FD->getParamDecl(i);
     if (PreVisitProcessArg(C, Call.getArgSVal(i), Call.getArgSourceRange(i),
                            Call.getArgExpr(i), i, checkUninitFields, Call, *BT,
                            ParamDecl))
-      return;
+      return nullptr;
   }
+  return State;
+}
+
+void CallAndMessageChecker::checkPreCall(const CallEvent &Call,
+                                         CheckerContext &C) const {
+  ProgramStateRef State = C.getState();
+
+  if (const CallExpr *CE = dyn_cast_or_null<CallExpr>(Call.getOriginExpr()))
+    State = checkFunctionPointerCall(CE, C, State);
+
+  if (!State)
+    return;
+
+  if (Call.getDecl())
+    State = checkParameterCount(Call, C, State);
+
+  if (!State)
+    return;
+
+  if (const auto *CC = dyn_cast<CXXInstanceCall>(&Call))
+    State = checkCXXMethodCall(CC, C, State);
+
+  if (!State)
+    return;
+
+  if (const auto *DC = dyn_cast<CXXDeallocatorCall>(&Call))
+    State = checkCXXDeallocation(DC, C, State);
+
+  if (!State)
+    return;
+
+  State = checkArgInitializedness(Call, C, State);
 
   // If we make it here, record our assumptions about the callee.
   C.addTransition(State);
 }
 
 void CallAndMessageChecker::checkPreObjCMessage(const ObjCMethodCall &msg,
                                                 CheckerContext &C) const {
   SVal recVal = msg.getReceiverSVal();