[analyzer] Improve VirtualCallChecker diagnostics and move to optin package.

[dcoughlin]

Summary

The VirtualCallChecker is in alpha because its interprocedural diagnostics represent the call path textually in the diagnostic message rather than with a path sensitive diagnostic.

This patch turns off the AST-based interprocedural analysis in the checker so that no call path is needed and improves with diagnostic text. With these changes, the checker is ready to be moved into the optin package.

Ultimately the right fix is to rewrite this checker to be path sensitive -- but there is still value in enabling the checker for intraprocedural analysis only. The interprocedural mode can be re-enabled with an -analyzer-config flag.

[NoQ]

LGTM!

test/Analysis/virtualcall.h

23 ↗

(On Diff #78263)

Hmm, line break handling is quite strange.

[zaks.anna]

Not sure if we should make pure vs not an option so that users could turn the checking off. Is there a way to suppress the warning?

lib/StaticAnalyzer/Checkers/VirtualCallChecker.cpp

210 ↗

(On Diff #78263)

Please, add an intra-procedual test case for pure in we don't have one already.

212 ↗

(On Diff #78263)

Can we detect if we are in a constructor or if we are in the destructor and make the error message more precise?

[dcoughlin]

• Update the diagnostic to be explicit about whether the issue occurs during construction or destruction
• Add test for pure, intraprocedural case (Sema also catches this).

[dcoughlin]

• Add a PureOnly analyzer-config option that, when set, will limit diagnostics to calls to only pure virtual functions.

This should have gone in with the prevision updated diff; my apologies for the noise.

[dcoughlin]

In D26768#607157, @zaks.anna wrote:

Not sure if we should make pure vs not an option so that users could turn the checking off. Is there a way to suppress the warning?

There is not a way to suppress this. I've added an analyzer-config option to limit diagnostics to pure-virtual calls only.

[zaks.anna]

Looks great!
Thank you.

[dcoughlin]

I evaluated this checker on three internal codebases that make large use of virtual functions.

Project 1: ~190,000 lines of C++. 16 alarms. I triaged all of them. There were 2 definite false positives (FPs) and 14 likely FPs.
Project 2: ~320,000 lines of C++. 116 alarms. I triaged 45. All likely FPs.
Project 3: ~23,000 lines of C++. 43 alarms. I triaged 13. 3 definite FPs and 10 likely FPs.

The definite false positives were cases where the programmer seemed aware of the semantics of virtual calls during construction/destruction and had each subclass explicitly call its own version of the virtual method in question. The likely false positives were cases where there was no subclass of the constructed class that overrode the method in question.

I think there is value in this checker: virtual calls in constructors are a definite code smell and are hard to get right. But I don't think we can turn it on by default given the sheer number of alarms. I think this needs to stay in alpha until we can reduce the number of false positives

[dcoughlin]

Keep the diagnostic improvements but remove the change to take the checker out of alpha.

[dcoughlin]

Move the checker to the optin package. The checker will be off by default but supported. Projects that are interested in adopting the rule of 'no virtual calls in constructors/destructors' can turn this checker on explicitly.

[malcolm.parsons]

In D26768#618651, @dcoughlin wrote:

The definite false positives were cases where the programmer seemed aware of the semantics of virtual calls during construction/destruction and had each subclass explicitly call its own version of the virtual method in question.

How is this avoiding the check for a qualifier on the call?

if (CME->getQualifier())
  callIsNonVirtual = true;

The likely false positives were cases where there was no subclass of the constructed class that overrode the method in question.

I'd like to be told about these so that the class can be marked final.

[dcoughlin]

In D26768#619222, @malcolm.parsons wrote:

In D26768#618651, @dcoughlin wrote:

The definite false positives were cases where the programmer seemed aware of the semantics of virtual calls during construction/destruction and had each subclass explicitly call its own version of the virtual method in question.

How is this avoiding the check for a qualifier on the call?

if (CME->getQualifier())
  callIsNonVirtual = true;

They didn't use the qualified version. Rather, each class had duplication of the same logic calling the virtual method its individual constructor/destructor. In the cases I examined either the virtual method didn't call super (most common pattern) or it did and was idempotent if called multiple times.

The likely false positives were cases where there was no subclass of the constructed class that overrode the method in question.

I'd like to be told about these so that the class can be marked final.

I agree, but this produces too many alarms to be on by default in the analyzer. I put the check in it in the 'optin' package so individual projects can decide whether they want to enforce this rule or not.

cfe/trunk/include/clang/StaticAnalyzer/Checkers/Checkers.td

 // Note: OptIn is *not* intended for checkers that are too noisy to be on by
 // default. Such checkers belong in the alpha package.
 def OptIn : Package<"optin">;
 
 def Nullability : Package<"nullability">;
 
 def Cplusplus : Package<"cplusplus">;
 def CplusplusAlpha : Package<"cplusplus">, InPackage<Alpha>, Hidden;
+def CplusplusOptIn : Package<"cplusplus">, InPackage<OptIn>;
 
 def Valist : Package<"valist">;
 def ValistAlpha : Package<"valist">, InPackage<Alpha>, Hidden;
 
 def DeadCode : Package<"deadcode">;
 def DeadCodeAlpha : Package<"deadcode">, InPackage<Alpha>, Hidden;
 
 def Performance : Package<"performance">, InPackage<OptIn>;
def NewDeleteLeaksChecker : Checker<"NewDeleteLeaks">,
   DescFile<"MallocChecker.cpp">;
 
 def CXXSelfAssignmentChecker : Checker<"SelfAssignment">,
   HelpText<"Checks C++ copy and move assignment operators for self assignment">,
   DescFile<"CXXSelfAssignmentChecker.cpp">;
 
 } // end: "cplusplus"
 
-let ParentPackage = CplusplusAlpha in {
+let ParentPackage = CplusplusOptIn in {
 
 def VirtualCallChecker : Checker<"VirtualCall">,
   HelpText<"Check virtual function calls during construction or destruction">,
   DescFile<"VirtualCallChecker.cpp">;
 
-} // end: "alpha.cplusplus"
+} // end: "optin.cplusplus"
 
 
 //===----------------------------------------------------------------------===//
 // Valist checkers.
 //===----------------------------------------------------------------------===//
 
 let ParentPackage = ValistAlpha in {
 

cfe/trunk/lib/StaticAnalyzer/Checkers/VirtualCallChecker.cpp

 
 namespace {
 
 class WalkAST : public StmtVisitor<WalkAST> {
   const CheckerBase *Checker;
   BugReporter &BR;
   AnalysisDeclContext *AC;
 
+  /// The root constructor or destructor whose callees are being analyzed.
+  const CXXMethodDecl *RootMethod = nullptr;
+
+  /// Whether the checker should walk into bodies of called functions.
+  /// Controlled by the "Interprocedural" analyzer-config option.
+  bool IsInterprocedural = false;
+
+  /// Whether the checker should only warn for calls to pure virtual functions
+  /// (which is undefined behavior) or for all virtual functions (which may
+  /// may result in unexpected behavior).
+  bool ReportPureOnly = false;
+
   typedef const CallExpr * WorkListUnit;
   typedef SmallVector<WorkListUnit, 20> DFSWorkList;
 
   /// A vector representing the worklist which has a chain of CallExprs.
   DFSWorkList WList;
 
   // PreVisited : A CallExpr to this FunctionDecl is in the worklist, but the
   // body has not been visited yet.
class WalkAST : public StmtVisitor<WalkAST> {
   llvm::DenseMap<const FunctionDecl *, Kind> VisitedFunctions;
 
   /// The CallExpr whose body is currently being visited.  This is used for
   /// generating bug reports.  This is null while visiting the body of a
   /// constructor or destructor.
   const CallExpr *visitingCallExpr;
 
 public:
-  WalkAST(const CheckerBase *checker, BugReporter &br,
-          AnalysisDeclContext *ac)
-      : Checker(checker), BR(br), AC(ac), visitingCallExpr(nullptr) {}
+  WalkAST(const CheckerBase *checker, BugReporter &br, AnalysisDeclContext *ac,
+          const CXXMethodDecl *rootMethod, bool isInterprocedural,
+          bool reportPureOnly)
+      : Checker(checker), BR(br), AC(ac), RootMethod(rootMethod),
+        IsInterprocedural(isInterprocedural), ReportPureOnly(reportPureOnly),
+        visitingCallExpr(nullptr) {
+    // Walking should always start from either a constructor or a destructor.
+    assert(isa<CXXConstructorDecl>(rootMethod) ||
+           isa<CXXDestructorDecl>(rootMethod));
+  }
 
   bool hasWork() const { return !WList.empty(); }
 
   /// This method adds a CallExpr to the worklist and marks the callee as
   /// being PreVisited.
   void Enqueue(WorkListUnit WLUnit) {
     const FunctionDecl *FD = WLUnit->getDirectCallee();
     if (!FD || !FD->getBody())

 void WalkAST::VisitChildren(Stmt *S) {
   for (Stmt *Child : S->children())
     if (Child)
       Visit(Child);
 }
 
 void WalkAST::VisitCallExpr(CallExpr *CE) {
   VisitChildren(CE);
+  if (IsInterprocedural)
     Enqueue(CE);
 }
 
 void WalkAST::VisitCXXMemberCallExpr(CallExpr *CE) {
   VisitChildren(CE);
   bool callIsNonVirtual = false;
 
   // Several situations to elide for checking.
   if (MemberExpr *CME = dyn_cast<MemberExpr>(CE->getCallee())) {
void WalkAST::VisitCXXMemberCallExpr(CallExpr *CE) {
   }
 
   // Get the callee.
   const CXXMethodDecl *MD = dyn_cast<CXXMethodDecl>(CE->getDirectCallee());
   if (MD && MD->isVirtual() && !callIsNonVirtual && !MD->hasAttr<FinalAttr>() &&
       !MD->getParent()->hasAttr<FinalAttr>())
     ReportVirtualCall(CE, MD->isPure());
 
+  if (IsInterprocedural)
     Enqueue(CE);
 }
 
 void WalkAST::ReportVirtualCall(const CallExpr *CE, bool isPure) {
+  if (ReportPureOnly && !isPure)
+    return;
+
   SmallString<100> buf;
   llvm::raw_svector_ostream os(buf);
 
+  // FIXME: The interprocedural diagnostic experience here is not good.
+  // Ultimately this checker should be re-written to be path sensitive.
+  // For now, only diagnose intraprocedurally, by default.
+  if (IsInterprocedural) {
     os << "Call Path : ";
     // Name of current visiting CallExpr.
     os << *CE->getDirectCallee();
 
-  // Name of the CallExpr whose body is current walking.
+    // Name of the CallExpr whose body is current being walked.
     if (visitingCallExpr)
       os << " <-- " << *visitingCallExpr->getDirectCallee();
     // Names of FunctionDecls in worklist with state PostVisited.
     for (SmallVectorImpl<const CallExpr *>::iterator I = WList.end(),
          E = WList.begin(); I != E; --I) {
       const FunctionDecl *FD = (*(I-1))->getDirectCallee();
       assert(FD);
       if (VisitedFunctions[FD] == PostVisited)
         os << " <-- " << *FD;
     }
 
+    os << "\n";
+  }
+
   PathDiagnosticLocation CELoc =
     PathDiagnosticLocation::createBegin(CE, BR.getSourceManager(), AC);
   SourceRange R = CE->getCallee()->getSourceRange();
 
-  if (isPure) {
-    os << "\n" <<  "Call pure virtual functions during construction or "
-       << "destruction may leads undefined behaviour";
-    BR.EmitBasicReport(AC->getDecl(), Checker,
-                       "Call pure virtual function during construction or "
-                       "Destruction",
-                       "Cplusplus", os.str(), CELoc, R);
-    return;
-  }
-  else {
-    os << "\n" << "Call virtual functions during construction or "
-       << "destruction will never go to a more derived class";
-    BR.EmitBasicReport(AC->getDecl(), Checker,
-                       "Call virtual function during construction or "
-                       "Destruction",
-                       "Cplusplus", os.str(), CELoc, R);
-    return;
-  }
+  os << "Call to ";
+  if (isPure)
+    os << "pure ";
+
+  os << "virtual function during ";
+
+  if (isa<CXXConstructorDecl>(RootMethod))
+    os << "construction ";
+  else
+    os << "destruction ";
+
+  if (isPure)
+    os << "has undefined behavior";
+  else
+    os << "will not dispatch to derived class";
+
+                     "Call to virtual function during construction or "
+                     "destruction",
+                     "C++ Object Lifecycle", os.str(), CELoc, R);
 }
 
 //===----------------------------------------------------------------------===//
 // VirtualCallChecker
 //===----------------------------------------------------------------------===//
 
 namespace {
 class VirtualCallChecker : public Checker<check::ASTDecl<CXXRecordDecl> > {
 public:
+  DefaultBool isInterprocedural;
+  DefaultBool isPureOnly;
+
   void checkASTDecl(const CXXRecordDecl *RD, AnalysisManager& mgr,
                     BugReporter &BR) const {
-    WalkAST walker(this, BR, mgr.getAnalysisDeclContext(RD));
+    AnalysisDeclContext *ADC = mgr.getAnalysisDeclContext(RD);
 
     // Check the constructors.
     for (const auto *I : RD->ctors()) {
       if (!I->isCopyOrMoveConstructor())
         if (Stmt *Body = I->getBody()) {
+          WalkAST walker(this, BR, ADC, I, isInterprocedural, isPureOnly);
           walker.Visit(Body);
           walker.Execute();
         }
     }
 
     // Check the destructor.
     if (CXXDestructorDecl *DD = RD->getDestructor())
       if (Stmt *Body = DD->getBody()) {
+        WalkAST walker(this, BR, ADC, DD, isInterprocedural, isPureOnly);
         walker.Visit(Body);
         walker.Execute();
       }
   }
 };
 }
 
 void ento::registerVirtualCallChecker(CheckerManager &mgr) {
-  mgr.registerChecker<VirtualCallChecker>();
+  VirtualCallChecker *checker = mgr.registerChecker<VirtualCallChecker>();
+  checker->isInterprocedural =
+      mgr.getAnalyzerOptions().getBooleanOption("Interprocedural", false,
+                                                checker);
+
+  checker->isPureOnly =
+      mgr.getAnalyzerOptions().getBooleanOption("PureOnly", false,
+                                                checker);
 }

cfe/trunk/test/Analysis/virtualcall.h

 }
 
 #else
 
 namespace header {
   class A {
   public:
     A() {
-      foo(); // expected-warning{{Call virtual functions during construction or destruction will never go to a more derived class}}
+      foo();
+#if !PUREONLY
+#if INTERPROCEDURAL
+          // expected-warning-re@-3 {{{{^}}Call Path : fooCall to virtual function during construction will not dispatch to derived class}}
+#else
+          // expected-warning-re@-5 {{{{^}}Call to virtual function during construction will not dispatch to derived class}}
+#endif
+#endif
+
     }
 
     virtual int foo();
   };
 }
 
 #endif

cfe/trunk/test/Analysis/virtualcall.cpp

-// RUN: %clang_cc1 -analyze -analyzer-checker=alpha.cplusplus.VirtualCall -analyzer-store region -verify -std=c++11 %s
+// RUN: %clang_cc1 -analyze -analyzer-checker=optin.cplusplus.VirtualCall -analyzer-store region -verify -std=c++11 %s
+// RUN: %clang_cc1 -analyze -analyzer-checker=optin.cplusplus.VirtualCall -analyzer-store region -analyzer-config optin.cplusplus.VirtualCall:Interprocedural=true -DINTERPROCEDURAL=1 -verify -std=c++11 %s
+// RUN: %clang_cc1 -analyze -analyzer-checker=optin.cplusplus.VirtualCall -analyzer-store region -analyzer-config optin.cplusplus.VirtualCall:PureOnly=true -DPUREONLY=1 -verify -std=c++11 %s
+
+/* When INTERPROCEDURAL is set, we expect diagnostics in all functions reachable
+   from a constructor or destructor. If it is not set, we expect diagnostics
+   only in the constructor or destructor.
+
+   When PUREONLY is set, we expect diagnostics only for calls to pure virtual
+   functions not to non-pure virtual functions.
+*/
 
 class A {
 public:
   A();
+  A(int i);
+
   ~A() {};
   
-  virtual int foo() = 0;
+  virtual int foo() = 0; // from Sema: expected-note {{'foo' declared here}}
   virtual void bar() = 0;
   void f() {
-    foo(); // expected-warning{{Call pure virtual functions during construction or destruction may leads undefined behaviour}}
+    foo();
+#if INTERPROCEDURAL
+        // expected-warning-re@-2 {{{{^}}Call Path : foo <-- fCall to pure virtual function during construction has undefined behavior}}
+#endif
   }
 };
 
 class B : public A {
 public:
   B() {
-    foo(); // expected-warning{{Call virtual functions during construction or destruction will never go to a more derived class}}
+    foo();
+#if !PUREONLY
+#if INTERPROCEDURAL
+        // expected-warning-re@-3 {{{{^}}Call Path : fooCall to virtual function during construction will not dispatch to derived class}}
+#else
+        // expected-warning-re@-5 {{{{^}}Call to virtual function during construction will not dispatch to derived class}}
+#endif
+#endif
+
   }
   ~B();
   
   virtual int foo();
-  virtual void bar() { foo(); }  // expected-warning{{Call virtual functions during construction or destruction will never go to a more derived class}}
+  virtual void bar() { foo(); }
+#if INTERPROCEDURAL
+      // expected-warning-re@-2 {{{{^}}Call Path : foo <-- barCall to virtual function during destruction will not dispatch to derived class}}
+#endif
 };
 
 A::A() {
   f();
 }
 
+A::A(int i) {
+  foo(); // From Sema: expected-warning {{call to pure virtual member function 'foo' has undefined behavior}}
+#if INTERPROCEDURAL
+      // expected-warning-re@-2 {{{{^}}Call Path : fooCall to pure virtual function during construction has undefined behavior}}
+#else
+      // expected-warning-re@-4 {{{{^}}Call to pure virtual function during construction has undefined behavior}}
+#endif
+}
+
 B::~B() {
   this->B::foo(); // no-warning
   this->B::bar();
-  this->foo(); // expected-warning{{Call virtual functions during construction or destruction will never go to a more derived class}}
+  this->foo();
+#if !PUREONLY
+#if INTERPROCEDURAL
+      // expected-warning-re@-3 {{{{^}}Call Path : fooCall to virtual function during destruction will not dispatch to derived class}}
+#else
+      // expected-warning-re@-5 {{{{^}}Call to virtual function during destruction will not dispatch to derived class}}
+#endif
+#endif
+
 }
 
 class C : public B {
 public:
   C();
   ~C();
   
   virtual int foo();
   void f(int i);
 };
 
 C::C() {
-  f(foo()); // expected-warning{{Call virtual functions during construction or destruction will never go to a more derived class}}
+  f(foo());
+#if !PUREONLY
+#if INTERPROCEDURAL
+      // expected-warning-re@-3 {{{{^}}Call Path : fooCall to virtual function during construction will not dispatch to derived class}}
+#else
+      // expected-warning-re@-5 {{{{^}}Call to virtual function during construction will not dispatch to derived class}}
+#endif
+#endif
 }
 
 class D : public B {
 public:
   D() {
     foo(); // no-warning
   }
   ~D() { bar(); }