Optionally check base classes for refcounting issues

tools/clang/plugins/FindBadConstructs.cpp

Index: tools/clang/plugins/FindBadConstructs.cpp
diff --git a/tools/clang/plugins/FindBadConstructs.cpp b/tools/clang/plugins/FindBadConstructs.cpp
index 99d22270af36962103b233f502858f43a588f6b2..4a16db4ec4c09d9e57551e609fe031bfe030f1f0 100644
--- a/tools/clang/plugins/FindBadConstructs.cpp
+++ b/tools/clang/plugins/FindBadConstructs.cpp
@@ -29,6 +29,19 @@ using namespace clang;
 
 namespace {
 
+const char kNoExplicitDtor[] =
+    "[chromium-style] Classes that are ref-counted should have explicit "
+    "destructors that are declared protected or private.";
+const char kPublicDtor[] =
+    "[chromium-style] Classes that are ref-counted should not have "
+    "public destructors.";
+const char kNoteInheritance[] =
+    "[chromium-style] %0 inherits from %1 here";
+const char kNoteImplicitDtor[] =
+    "[chromium-style] No explicit destructor for %0 defined";
+const char kNotePublicDtor[] =
+    "[chromium-style] Public destructor declared here";
+
 bool TypeHasNonTrivialDtor(const Type* type) {
   if (const CXXRecordDecl* cxx_r = type->getPointeeCXXRecordDecl())
     return cxx_r->hasTrivialDestructor();
@@ -37,7 +50,8 @@ bool TypeHasNonTrivialDtor(const Type* type) {
 }
 
 // Returns the underlying Type for |type| by expanding typedefs and removing
-// any namespace qualifiers.
+// any namespace qualifiers. This is similar to desugaring, except that for
+// ElaboratedTypes, desugar will unwrap too much.
 const Type* UnwrapType(const Type* type) {
   if (const ElaboratedType* elaborated = dyn_cast<ElaboratedType>(type))
     return UnwrapType(elaborated->getNamedType().getTypePtr());
@@ -50,11 +64,26 @@ const Type* UnwrapType(const Type* type) {
 class FindBadConstructsConsumer : public ChromeClassTester {
  public:
   FindBadConstructsConsumer(CompilerInstance& instance,
+                            bool check_base_classes,
                             bool check_refcounted_dtors,
                             bool check_virtuals_in_implementations)
       : ChromeClassTester(instance),
+        check_base_classes_(check_base_classes),
         check_refcounted_dtors_(check_refcounted_dtors),
         check_virtuals_in_implementations_(check_virtuals_in_implementations) {
+    // Register warning/error messages.
+    diag_no_explicit_dtor_ = diagnostic().getCustomDiagID(
+        getErrorLevel(), kNoExplicitDtor);
+    diag_public_dtor_ = diagnostic().getCustomDiagID(
+        getErrorLevel(), kPublicDtor);
+
+    // Registers notes to make it easier to interpret warnings.
+    diag_note_inheritance_ = diagnostic().getCustomDiagID(
+        DiagnosticsEngine::Note, kNoteInheritance);
+    diag_note_implicit_dtor_ = diagnostic().getCustomDiagID(
+        DiagnosticsEngine::Note, kNoteImplicitDtor);
+    diag_note_public_dtor_ = diagnostic().getCustomDiagID(
+        DiagnosticsEngine::Note, kNotePublicDtor);
   }
 
   virtual void CheckChromeClass(SourceLocation record_location,
@@ -80,8 +109,22 @@ class FindBadConstructsConsumer : public ChromeClassTester {
   }
 
  private:
-  bool check_refcounted_dtors_;
+  // The type of problematic ref-counting pattern that was encountered.
+  enum RefcountIssue {
+    None,
+    ImplicitDestructor,
+    PublicDestructor
+  };
+
+  bool check_base_classes_;
   bool check_virtuals_in_implementations_;
+  bool check_refcounted_dtors_;
+
+  unsigned diag_no_explicit_dtor_;
+  unsigned diag_public_dtor_;
+  unsigned diag_note_inheritance_;
+  unsigned diag_note_implicit_dtor_;
+  unsigned diag_note_public_dtor_;
 
   // Prints errors if the constructor/destructor weight is too heavy.
   void CheckCtorDtorWeight(SourceLocation record_location,
@@ -330,9 +373,38 @@ class FindBadConstructsConsumer : public ChromeClassTester {
     }
   }
 
+  // Check |record| for issues that are problematic for ref-counted types.
+  // Note that |record| may not be a ref-counted type, but a base class for
+  // a type that is.
+  // If there are issues, update |loc| with the SourceLocation of the issue
+  // and returns appropriately, or returns None if there are no issues.
+  static RefcountIssue CheckRecordForRefcountIssue(
+      const CXXRecordDecl* record,
+      SourceLocation &loc) {
+    if (!record->hasUserDeclaredDestructor()) {
+      loc = record->getLocation();
+      return ImplicitDestructor;
+    }
+
+    if (CXXDestructorDecl* dtor = record->getDestructor()) {
+      if (dtor->getAccess() == AS_public) {
+        loc = dtor->getInnerLocStart();
+        return PublicDestructor;
+      }
+    }
+
+    return None;
+  }
+
+  // Adds either a warning or error, based on the current handling of
+  // -Werror.
+  DiagnosticsEngine::Level getErrorLevel() {
+    return diagnostic().getWarningsAsErrors() ?
+        DiagnosticsEngine::Error : DiagnosticsEngine::Warning;
+  }
+
   // Returns true if |base| specifies one of the Chromium reference counted
-  // classes (base::RefCounted / base::RefCountedThreadSafe). |user_data| is
-  // ignored.
+  // classes (base::RefCounted / base::RefCountedThreadSafe).
   static bool IsRefCountedCallback(const CXXBaseSpecifier* base,
                                    CXXBasePath& path,
                                    void* user_data) {
@@ -360,33 +432,128 @@ class FindBadConstructsConsumer : public ChromeClassTester {
         return true;
       }
     }
+
     return false;
   }
 
-  // Prints errors if the destructor of a RefCounted class is public.
+  // Returns true if |base| specifies a class that has a public destructor,
+  // either explicitly or implicitly.
+  static bool HasPublicDtorCallback(const CXXBaseSpecifier* base,
+                                    CXXBasePath& path,
+                                    void* user_data) {
+    // Only examine paths that have public inheritance, as they are the
+    // only ones which will result in the destructor potentially being
+    // exposed. This check is largely redundant, as Chromium code should be
+    // exclusively using public inheritance.
+    if (path.Access != AS_public)
+      return false;
+
+    CXXRecordDecl* record = dyn_cast<CXXRecordDecl>(
+        base->getType()->getAs<RecordType>()->getDecl());
+    SourceLocation unused;
+    return None != CheckRecordForRefcountIssue(record, unused);
+  }
+
+  // Outputs a C++ inheritance chain as a diagnostic aid.
+  void PrintInheritanceChain(const CXXBasePath& path) {
+    for (CXXBasePath::const_iterator it = path.begin(); it != path.end();
+         ++it) {
+      diagnostic().Report(it->Base->getLocStart(), diag_note_inheritance_)
+          << it->Class << it->Base->getType();
+    }
+  }
+
+  // Check |record| to determine if it has any problematic refcounting
+  // issues and, if so, print them as warnings/errors based on the current
+  // value of getErrorLevel().
+  //
+  // If |record| is a C++ class, and if it inherits from one of the Chromium
+  // ref-counting classes (base::RefCounted / base::RefCountedThreadSafe),
+  // ensure that there are no public destructors in the class hierarchy. This
+  // is to guard against accidentally stack-allocating a RefCounted class or
+  // sticking it in a non-ref-counted container (like scoped_ptr<>).
   void CheckRefCountedDtors(SourceLocation record_location,
                             CXXRecordDecl* record) {
     // Skip anonymous structs.
     if (record->getIdentifier() == NULL)
       return;
 
-    CXXBasePaths paths;
+    // Determine if the current type is even ref-counted.
+    CXXBasePaths refcounted_path;
     if (!record->lookupInBases(
-            &FindBadConstructsConsumer::IsRefCountedCallback, this, paths)) {
+            &FindBadConstructsConsumer::IsRefCountedCallback, this,
+            refcounted_path)) {
       return;  // Class does not derive from a ref-counted base class.
     }
 
-    if (!record->hasUserDeclaredDestructor()) {
-      emitWarning(
-          record_location,
-          "Classes that are ref-counted should have explicit "
-          "destructors that are protected or private.");
-    } else if (CXXDestructorDecl* dtor = record->getDestructor()) {
-      if (dtor->getAccess() == AS_public) {
-        emitWarning(
-            dtor->getInnerLocStart(),
-            "Classes that are ref-counted should not have "
-            "public destructors.");
+    // Easy check: Check to see if the current type is problematic.
+    SourceLocation loc;
+    RefcountIssue issue = CheckRecordForRefcountIssue(record, loc);
+    if (issue == ImplicitDestructor) {
+      diagnostic().Report(loc, diag_no_explicit_dtor_);
+      PrintInheritanceChain(refcounted_path.front());
+      return;
+    }
+    if (issue == PublicDestructor) {
+      diagnostic().Report(loc, diag_public_dtor_);
+      PrintInheritanceChain(refcounted_path.front());
+      return;
+    }
+
+    // Long check: Check all possible base classes for problematic
+    // destructors. This checks for situations involving multiple
+    // inheritance, where the ref-counted class may be implementing an
+    // interface that has a public or implicit destructor.
+    //
+    // struct SomeInterface {
+    //   virtual void DoFoo();
+    // };
+    //
+    // struct RefCountedInterface
+    //    : public base::RefCounted<RefCountedInterface>,
+    //      public SomeInterface {
+    //  private:
+    //   friend class base::Refcounted<RefCountedInterface>;
+    //   virtual ~RefCountedInterface() {}
+    // };
+    //
+    // While RefCountedInterface is "safe", in that its destructor is
+    // private, it's possible to do the following "unsafe" code:
+    //   scoped_refptr<RefCountedInterface> some_class(
+    //       new RefCountedInterface);
+    //   // Calls SomeInterface::~SomeInterface(), which is unsafe.
+    //   delete static_cast<SomeInterface*>(some_class.get());
+    if (!check_base_classes_)
+      return;
+
+    // Find all public destructors. This will record the class hierarchy
+    // that leads to the public destructor in |dtor_paths|.
+    CXXBasePaths dtor_paths;
+    if (!record->lookupInBases(
+            &FindBadConstructsConsumer::HasPublicDtorCallback, this,
+            dtor_paths)) {
+      return;
+    }
+
+    for (CXXBasePaths::const_paths_iterator it = dtor_paths.begin();
+         it != dtor_paths.end(); ++it) {
+      // The record with the problem will always be the last record
+      // in the path, since it is the record that stopped the search.
+      const CXXRecordDecl* problem_record = dyn_cast<CXXRecordDecl>(
+          it->back().Base->getType()->getAs<RecordType>()->getDecl());
+
+      issue = CheckRecordForRefcountIssue(problem_record, loc);
+
+      if (issue == ImplicitDestructor) {
+        diagnostic().Report(record_location, diag_no_explicit_dtor_);
+        PrintInheritanceChain(refcounted_path.front());
+        diagnostic().Report(loc, diag_note_implicit_dtor_) << problem_record;
+        PrintInheritanceChain(*it);
+      } else if (issue == PublicDestructor) {
+        diagnostic().Report(record_location, diag_public_dtor_);
+        PrintInheritanceChain(refcounted_path.front());
+        diagnostic().Report(loc, diag_note_public_dtor_);
+        PrintInheritanceChain(*it);
       }
     }
   }
@@ -395,7 +562,8 @@ class FindBadConstructsConsumer : public ChromeClassTester {
 class FindBadConstructsAction : public PluginASTAction {
  public:
   FindBadConstructsAction()
-      : check_refcounted_dtors_(true),
+      : check_base_classes_(false),
+        check_refcounted_dtors_(true),
         check_virtuals_in_implementations_(true) {
   }
 
@@ -404,7 +572,8 @@ class FindBadConstructsAction : public PluginASTAction {
   virtual ASTConsumer* CreateASTConsumer(CompilerInstance& instance,
                                          llvm::StringRef ref) {
     return new FindBadConstructsConsumer(
-        instance, check_refcounted_dtors_, check_virtuals_in_implementations_);
+        instance, check_base_classes_, check_refcounted_dtors_,
+        check_virtuals_in_implementations_);
   }
 
   virtual bool ParseArgs(const CompilerInstance& instance,
@@ -416,6 +585,8 @@ class FindBadConstructsAction : public PluginASTAction {
         check_refcounted_dtors_ = false;
       } else if (args[i] == "skip-virtuals-in-implementations") {
         check_virtuals_in_implementations_ = false;
+      } else if (args[i] == "check-base-classes") {
+        check_base_classes_ = true;
       } else {
         parsed = false;
         llvm::errs() << "Unknown clang plugin argument: " << args[i] << "\n";
@@ -426,6 +597,7 @@ class FindBadConstructsAction : public PluginASTAction {
   }
 
  private:
+  bool check_base_classes_;
   bool check_refcounted_dtors_;
   bool check_virtuals_in_implementations_;
 };