[analyzer] VirtualCallChecker: Improve warning messages.

[NoQ]

Summary

• Mention the name of the virtual function. This should help when the warning appears in a complicated expression and it's unclear what part of the expression it is about.
• Calling a pure virtual function always sinks the analysis, even if the checker is not in pure-only mode.
• Refactor/modernize the code a bit.

[NoQ]

• Print only class name::method name instead of a fully qualified name, because fully qualified names may get pretty long.
• Remove more dead code.

[NoQ]

• Hmm, also i'd rather say "method" than "function".

[Szelethus]

Yay!

[NoQ]

One more thing: explain the consequences of the bug, so that it was obvious what the problem is.

[Charusso]

When I started programming I read that the programming is British English, but LLVM project has more 'behavior' than 'behaviour', so as being democratic it should be fine. Thanks!

[a_sidorin]

Hi Artem,
The patch looks good to me. I prefer a fully qualified name, however, but it is a matter of taste.

[NoQ]

• Fix checker names for consumers that display them (eg., clang-tidy). Add a test.
• Change bug descriptions: "Call to virtual function during construction or destruction" -> "Pure virtual method call" | "Unexpected loss of virtual dispatch"
• Remove the suggestion to explicitly qualify the call. It is incorrect for nested stack frames, as it may affect the behavior when the same function is called from elsewhere.
• Make the category reusable, fix capitalization to stay similar to other bug categories.

[NoQ]

Merged into D64274.

clang/include/clang/StaticAnalyzer/Core/BugReporter/CommonBugCategories.h

 namespace clang {
   namespace ento {
     namespace categories {
       extern const char * const CoreFoundationObjectiveC;
       extern const char * const LogicError;
       extern const char * const MemoryRefCount;
       extern const char * const MemoryError;
       extern const char * const UnixAPI;
+      extern const char * const CXXObjectLifecycle;
     }
   }
 }
 #endif
 

clang/lib/StaticAnalyzer/Checkers/VirtualCallChecker.cpp

 //=======- VirtualCallChecker.cpp --------------------------------*- C++ -*-==//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 //
-//  This file defines a checker that checks virtual function calls during
+//  This file defines a checker that checks virtual method calls during
 //  construction or destruction of C++ objects.
 //
 //===----------------------------------------------------------------------===//
 
 #include "clang/StaticAnalyzer/Checkers/BuiltinCheckerRegistration.h"
 #include "clang/AST/DeclCXX.h"
 #include "clang/StaticAnalyzer/Core/BugReporter/BugReporter.h"
 #include "clang/StaticAnalyzer/Core/BugReporter/BugType.h"
static inline void Profile(ObjectState X, FoldingSetNodeID &ID) {
     ID.AddInteger(static_cast<int>(X));
   }
 };
 } // end namespace llvm
 
 namespace {
 class VirtualCallChecker
     : public Checker<check::BeginFunction, check::EndFunction, check::PreCall> {
-  mutable std::unique_ptr<BugType> BT;
-
 public:
-  // The flag to determine if pure virtual functions should be issued only.
-  bool IsPureOnly = true;
+  // These are going to be null if the respective check is disabled.
+  mutable std::unique_ptr<BugType> BT_Pure, BT_Impure;
 
   void checkBeginFunction(CheckerContext &C) const;
   void checkEndFunction(const ReturnStmt *RS, CheckerContext &C) const;
   void checkPreCall(const CallEvent &Call, CheckerContext &C) const;
 
 private:
   void registerCtorDtorCallInState(bool IsBeginFunction,
                                    CheckerContext &C) const;
-  void reportBug(StringRef Msg, bool PureError, const MemRegion *Reg,
-                 CheckerContext &C) const;
 };
 } // end namespace
 
 // GDM (generic data map) to the memregion of this for the ctor and dtor.
 REGISTER_MAP_WITH_PROGRAMSTATE(CtorDtorMap, const MemRegion *, ObjectState)
 
-// The function to check if a callexpr is a virtual function.
+// The function to check if a callexpr is a virtual method call.
 static bool isVirtualCall(const CallExpr *CE) {
   bool CallIsNonVirtual = false;
 
   if (const MemberExpr *CME = dyn_cast<MemberExpr>(CE->getCallee())) {
     // The member access is fully qualified (i.e., X::F).
     // Treat this as a non-virtual call and do not warn.
     if (CME->getQualifier())
       CallIsNonVirtual = true;
void VirtualCallChecker::checkPreCall(const CallEvent &Call,
                                       CheckerContext &C) const {
   const auto MC = dyn_cast<CXXMemberCall>(&Call);
   if (!MC)
     return;
 
   const CXXMethodDecl *MD = dyn_cast_or_null<CXXMethodDecl>(Call.getDecl());
   if (!MD)
     return;
+
   ProgramStateRef State = C.getState();
   const CallExpr *CE = dyn_cast_or_null<CallExpr>(Call.getOriginExpr());
-
-  if (IsPureOnly && !MD->isPure())
-    return;
   if (!isVirtualCall(CE))
     return;
 
   const MemRegion *Reg = MC->getCXXThisVal().getAsRegion();
   const ObjectState *ObState = State->get<CtorDtorMap>(Reg);
   if (!ObState)
     return;
-  // Check if a virtual method is called.
-  // The GDM of constructor and destructor should be true.
-  if (*ObState == ObjectState::CtorCalled) {
-    if (IsPureOnly && MD->isPure())
-      reportBug("Call to pure virtual function during construction", true, Reg,
-                C);
-    else if (!MD->isPure())
-      reportBug("Call to virtual function during construction", false, Reg, C);
-    else
-      reportBug("Call to pure virtual function during construction", false, Reg,
-                C);
-  }
 
-  if (*ObState == ObjectState::DtorCalled) {
-    if (IsPureOnly && MD->isPure())
-      reportBug("Call to pure virtual function during destruction", true, Reg,
-                C);
-    else if (!MD->isPure())
-      reportBug("Call to virtual function during destruction", false, Reg, C);
-    else
-      reportBug("Call to pure virtual function during construction", false, Reg,
-                C);
+  bool IsPure = MD->isPure();
+
+  // At this point we're sure that we're calling a virtual method
+  // during construction or destruction, so we'll emit a report.
+  SmallString<128> Msg;
+  llvm::raw_svector_ostream OS(Msg);
+  OS << "Call to ";
+  if (IsPure)
+    OS << "pure ";
+  OS << "virtual method '" << MD->getParent()->getNameAsString()
+     << "::" << MD->getNameAsString() << "' during ";
+  if (*ObState == ObjectState::CtorCalled)
+    OS << "construction ";
+  else
+    OS << "destruction ";
+  if (IsPure)
+    OS << "has undefined behavior";
+    OS << "bypasses virtual dispatch";
+
+  ExplodedNode *N =
+      IsPure ? C.generateErrorNode() : C.generateNonFatalErrorNode();
+  if (!N)
+    return;
+
+  const std::unique_ptr<BugType> &BT = IsPure ? BT_Pure : BT_Impure;
+  if (!BT) {
+    assert(BT == BT_Impure && "Pure-only check must be enabled!");
+    return;
   }
+
+  auto Report = llvm::make_unique<BugReport>(*BT, OS.str(), N);
+  C.emitReport(std::move(Report));
 }
 
 void VirtualCallChecker::registerCtorDtorCallInState(bool IsBeginFunction,
                                                      CheckerContext &C) const {
   const auto *LCtx = C.getLocationContext();
   const auto *MD = dyn_cast_or_null<CXXMethodDecl>(LCtx->getDecl());
   if (!MD)
     return;
if (isa<CXXDestructorDecl>(MD)) {
     else
       State = State->remove<CtorDtorMap>(Reg);
 
     C.addTransition(State);
     return;
   }
 }
 
-void VirtualCallChecker::reportBug(StringRef Msg, bool IsSink,
-                                   const MemRegion *Reg,
-                                   CheckerContext &C) const {
-  ExplodedNode *N;
-  if (IsSink)
-    N = C.generateErrorNode();
-  else
-    N = C.generateNonFatalErrorNode();
-
-  if (!N)
-    return;
-  if (!BT)
-    BT.reset(new BugType(
-        this, "Call to virtual function during construction or destruction",
-        "C++ Object Lifecycle"));
-
-  auto Reporter = llvm::make_unique<BugReport>(*BT, Msg, N);
-  C.emitReport(std::move(Reporter));
-}
-
 void ento::registerPureVirtualCallChecker(CheckerManager &Mgr) {
-  Mgr.registerChecker<VirtualCallChecker>();
+  auto *Chk = Mgr.registerChecker<VirtualCallChecker>();
+  Chk->BT_Pure = llvm::make_unique<BugType>(
+      Mgr.getCurrentCheckName(), "Pure virtual method call",
+      categories::CXXObjectLifecycle);
 }
 
 void ento::registerVirtualCallChecker(CheckerManager &Mgr) {
   auto *Chk = Mgr.getChecker<VirtualCallChecker>();
-  Chk->IsPureOnly = Mgr.getAnalyzerOptions().getCheckerBooleanOption(
-      Mgr.getCurrentCheckName(), "PureOnly");
+  if (!Mgr.getAnalyzerOptions().getCheckerBooleanOption(
+          Mgr.getCurrentCheckName(), "PureOnly")) {
+    Chk->BT_Impure = llvm::make_unique<BugType>(
+        Mgr.getCurrentCheckName(), "Unexpected loss of virtual dispatch",
+        categories::CXXObjectLifecycle);
+  }
 }
 
 bool ento::shouldRegisterPureVirtualCallChecker(const LangOptions &LO) {
   return true;
 }
 
 bool ento::shouldRegisterVirtualCallChecker(const LangOptions &LO) {
   return true;
 }

clang/lib/StaticAnalyzer/Core/CommonBugCategories.cpp

 namespace clang { namespace ento { namespace categories {
 
 const char * const CoreFoundationObjectiveC = "Core Foundation/Objective-C";
 const char * const LogicError = "Logic error";
 const char * const MemoryRefCount =
   "Memory (Core Foundation/Objective-C/OSObject)";
 const char * const MemoryError = "Memory error";
 const char * const UnixAPI = "Unix API";
+const char * const CXXObjectLifecycle = "C++ object lifecycle";
 }}}

clang/test/Analysis/virtualcall-plist.cpp

+// RUN: %clang_analyze_cc1 -analyzer-checker=core,optin.cplusplus \
+// RUN:                    -analyzer-output=plist -o %t.plist -w -verify %s
+// RUN: cat %t.plist | FileCheck %s
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,cplusplus,optin.cplusplus \
+// RUN:                    -analyzer-output=plist -o %t.plist -w -verify %s
+// RUN: cat %t.plist | FileCheck %s
+
+struct S {
+  virtual void foo();
+  virtual void bar() = 0;
+
+  S() {
+    // CHECK: Call to virtual method 'S::foo' during construction bypasses virtual dispatch
+    // CHECK: optin.cplusplus.VirtualCall
+    foo(); // expected-warning{{Call to virtual method 'S::foo' during construction bypasses virtual dispatch}}
+    // CHECK: Call to pure virtual method 'S::bar' during construction has undefined behavior
+    // CHECK: cplusplus.PureVirtualCall
+    bar(); // expected-warning{{Call to pure virtual method 'S::bar' during construction has undefined behavior}}
+  }
+};

clang/test/Analysis/virtualcall.h

 namespace header {
   class Z {
   public:
     Z() {
-      foo(); // impure-warning {{Call to virtual function during construction}}
+      foo(); // impure-warning {{Call to virtual method 'Z::foo' during construction bypasses virtual dispatch}}
     }
     virtual int foo();
   };
 }

clang/test/Analysis/virtualcall.cpp

 // RUN: %clang_analyze_cc1 -analyzer-checker=core,optin.cplusplus.VirtualCall \
+// RUN:                    -analyzer-checker=debug.ExprInspection \
 // RUN:                    -std=c++11 -verify=expected,impure %s
 
 // RUN: %clang_analyze_cc1 -analyzer-checker=core,cplusplus.PureVirtualCall \
+// RUN:                    -analyzer-checker=debug.ExprInspection \
 // RUN:                    -std=c++11 -verify=expected -std=c++11 %s
 
 // RUN: %clang_analyze_cc1 -analyzer-checker=core,optin.cplusplus.VirtualCall \
 // RUN:                    -analyzer-config \
 // RUN:                        optin.cplusplus.VirtualCall:PureOnly=true \
+// RUN:                    -analyzer-checker=debug.ExprInspection \
 // RUN:                    -std=c++11 -verify=expected %s
 
 // RUN: %clang_analyze_cc1 -analyzer-checker=core,cplusplus.PureVirtualCall \
 // RUN:                    -analyzer-checker=optin.cplusplus.VirtualCall \
+// RUN:                    -analyzer-checker=debug.ExprInspection \
 // RUN:                    -std=c++11 -verify=expected,impure -std=c++11 %s
 
 // RUN: %clang_analyze_cc1 -analyzer-checker=core,cplusplus.PureVirtualCall \
 // RUN:                    -analyzer-checker=optin.cplusplus.VirtualCall \
 // RUN:                    -analyzer-config \
 // RUN:                        optin.cplusplus.VirtualCall:PureOnly=true \
+// RUN:                    -analyzer-checker=debug.ExprInspection \
 // RUN:                    -std=c++11 -verify=expected %s
 
 #include "virtualcall.h"
 
+void clang_analyzer_warnIfReached();
+
 class A {
 public:
   A();
 
   ~A(){};
 
   virtual int foo() = 0;
   virtual void bar() = 0;
   void f() {
-    foo(); // expected-warning{{Call to pure virtual function during construction}}
+    foo(); // expected-warning{{Call to pure virtual method 'A::foo' during construction has undefined behavior}}
+    clang_analyzer_warnIfReached(); // no-warning
   }
 };
 
-class B : public A {
+A::A() {
+  f();
+}
+
+class B {
 public:
   B() {
-    foo(); // impure-warning {{Call to virtual function during construction}}
+    foo(); // impure-warning {{Call to virtual method 'B::foo' during construction bypasses virtual dispatch}}
   }
   ~B();
 
   virtual int foo();
   virtual void bar() {
-    foo(); // impure-warning{{Call to virtual function during destruction}}
+    foo(); // impure-warning {{Call to virtual method 'B::foo' during destruction bypasses virtual dispatch}}
   }
 };
 
-A::A() {
-  f();
-}
-
 B::~B() {
   this->B::foo(); // no-warning
   this->B::bar();
-  this->foo(); // impure-warning {{Call to virtual function during destruction}}
+  this->foo(); // impure-warning {{Call to virtual method 'B::foo' during destruction bypasses virtual dispatch}}
 }
 
 class C : public B {
 public:
   C();
   ~C();
 
   virtual int foo();
   void f(int i);
 };
 
 C::C() {
-  f(foo()); // impure-warning {{Call to virtual function during construction}}
+  f(foo()); // impure-warning {{Call to virtual method 'C::foo' during construction bypasses virtual dispatch}}
 }
 
 class D : public B {
 public:
   D() {
     foo(); // no-warning
   }
   ~D() { bar(); }
void init() {
     if (initState)
       f(); // no warning
   }
 
   H(int i) {
     G g;
     g.foo();
     g.bar(); // no warning
-    f(); // impure-warning {{Call to virtual function during construction}}
+    f(); // impure-warning {{Call to virtual method 'H::f' during construction bypasses virtual dispatch}}
     H &h = *this;
-    h.f(); // impure-warning {{Call to virtual function during construction}}
+    h.f(); // impure-warning {{Call to virtual method 'H::f' during construction bypasses virtual dispatch}}
   }
 };
 
 class X {
 public:
   X() {
-    g(); // impure-warning {{Call to virtual function during construction}}
+    g(); // impure-warning {{Call to virtual method 'X::g' during construction bypasses virtual dispatch}}
   }
   X(int i) {
     if (i > 0) {
       X x(i - 1);
       x.g(); // no warning
     }
-    g(); // impure-warning {{Call to virtual function during construction}}
+    g(); // impure-warning {{Call to virtual method 'X::g' during construction bypasses virtual dispatch}}
   }
   virtual void g();
 };
 
 class M;
 class N {
 public:
   virtual void virtualMethod();
   void callFooOfM(M *);
 };
 class M {
 public:
   M() {
     N n;
     n.virtualMethod(); // no warning
     n.callFooOfM(this);
   }
   virtual void foo();
 };
 void N::callFooOfM(M *m) {
-  m->foo(); // impure-warning {{Call to virtual function during construction}}
+  m->foo(); // impure-warning {{Call to virtual method 'M::foo' during construction bypasses virtual dispatch}}
 }
 
 class Y {
 public:
   virtual void foobar();
   void fooY() {
     F f1;
-    foobar(); // impure-warning {{Call to virtual function during construction}}
+    foobar(); // impure-warning {{Call to virtual method 'Y::foobar' during construction bypasses virtual dispatch}}
   }
   Y() { fooY(); }
 };
 
 int main() {
   B b;
   C c;
   D d;