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 43d23c2f67f5c9089b9145d2d745056e0203d249..0bd9480c2b88bf2937ffb3fc4ed9ccc6e09b913c 100644
--- a/tools/clang/plugins/FindBadConstructs.cpp
+++ b/tools/clang/plugins/FindBadConstructs.cpp
@@ -28,6 +28,19 @@ using namespace clang;
 
 namespace {
 
+static const char kNoExplicitDtor[] =

[Nico, 2012/07/21 18:36:37]

const has implicit internal linkage, no need for static (besides, this is in an
unnamed namespace).

(const char* bla = ".." doesn't have implicit linkage because that only marks
the character data const, not the pointer itself. but you're doing const char
bla[] here.)

+    "[chromium-style] Classes that are ref-counted should have explicit "
+    "destructors that are declared protected or private.";
+static const char kPublicDtor[] =
+    "[chromium-style] Classes that are ref-counted should not have "
+    "public destructors.";
+static const char kNoteInheritance[] =
+    "[chromium-style] %0 inherits from %1 here";
+static const char kNoteImplicitDtor[] =
+    "[chromium-style] No explicit destructor for %0 defined";
+static const char kNotePublicDtor[] =
+    "[chromium-style] Public destructor declared here";

[Nico, 2012/07/21 18:36:37]

nice :-)

+
 bool TypeHasNonTrivialDtor(const Type* type) {
   if (const CXXRecordDecl* cxx_r = type->getCXXRecordDeclForPointerType())
     return cxx_r->hasTrivialDestructor();
@@ -36,7 +49,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());
@@ -49,11 +63,26 @@ const Type* UnwrapType(const Type* type) {
 class FindBadConstructsConsumer : public ChromeClassTester {
  public:
   FindBadConstructsConsumer(CompilerInstance& instance,
+                            bool check_virtuals_in_implementations,
                             bool check_refcounted_dtors,
-                            bool check_virtuals_in_implementations)
+                            bool check_base_classes)
       : ChromeClassTester(instance),
+        check_virtuals_in_implementations_(check_virtuals_in_implementations),
         check_refcounted_dtors_(check_refcounted_dtors),
-        check_virtuals_in_implementations_(check_virtuals_in_implementations) {
+        check_base_classes_(check_base_classes) {
+    // 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,
@@ -79,69 +108,22 @@ class FindBadConstructsConsumer : public ChromeClassTester {
   }
 
  private:
-  bool check_refcounted_dtors_;
-  bool check_virtuals_in_implementations_;
-
-  // Returns true if |base| specifies one of the Chromium reference counted
-  // classes (base::RefCounted / base::RefCountedThreadSafe). |user_data| is
-  // ignored.
-  static bool IsRefCountedCallback(const CXXBaseSpecifier* base,
-                                   CXXBasePath& path,
-                                   void* user_data) {
-    FindBadConstructsConsumer* self =
-        static_cast<FindBadConstructsConsumer*>(user_data);
-
-    const TemplateSpecializationType* base_type =
-        dyn_cast<TemplateSpecializationType>(
-            UnwrapType(base->getType().getTypePtr()));
-    if (!base_type) {
-      // Base-most definition is not a template, so this cannot derive from
-      // base::RefCounted. However, it may still be possible to use with a
-      // scoped_refptr<> and support ref-counting, so this is not a perfect
-      // guarantee of safety.
-      return false;
-    }
-
-    TemplateName name = base_type->getTemplateName();
-    if (TemplateDecl* decl = name.getAsTemplateDecl()) {
-      std::string base_name = decl->getNameAsString();
-
-      // Check for both base::RefCounted and base::RefCountedThreadSafe.
-      if (base_name.compare(0, 10, "RefCounted") == 0 &&
-          self->GetNamespace(decl) == "base") {
-        return true;
-      }
-    }
-    return false;
-  }
+  // The type of problematic ref-counting pattern that was encountered.
+  enum RefcountIssue {
+    None,
+    ImplicitDestructor,
+    PublicDestructor
+  };
 
-  // Prints errors if the destructor of a RefCounted class is public.
-  void CheckRefCountedDtors(SourceLocation record_location,
-                            CXXRecordDecl* record) {
-    // Skip anonymous structs.
-    if (record->getIdentifier() == NULL)
-      return;
-
-    CXXBasePaths paths;
-    if (!record->lookupInBases(
-            &FindBadConstructsConsumer::IsRefCountedCallback, this, paths)) {
-      return;  // Class does not derive from a ref-counted base class.
-    }
+  bool check_virtuals_in_implementations_;
+  bool check_refcounted_dtors_;
+  bool check_base_classes_;
 
-    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.");
-      }
-    }
-  }

[Nico, 2012/07/21 18:36:37]

This is a bit harder to review since it moved around in addition to being
changed. Can you TBR a change that does nothing but move this down and rebase
this CL on top of that?

+  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,
@@ -389,19 +371,208 @@ 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() {

[Nico, 2012/07/21 18:36:37]

Do you need this? If you always emit a warning, doesn't clang's normal diag
level mapping machinery convert that to an error already when building with
-Werror?

[Nico, 2013/01/31 22:52:20]

This question still stands.

[Ryan Sleevi, 2013/01/31 23:09:30]

This matches ChromeClassTester::emitWarning()

Clang has much more complicated logic for mapping warnings as errors, which
includes opting in to make sure the diagnostic/diagnostic group WILL map a
warning to an error (since not all groups do). This logic exists as long as
we've had our plugin, so my presumption was that it was done so with reason.

[Ryan Sleevi, 2013/02/01 00:19:36]

Ah, sorry, brain job -
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Basic/DiagnosticIDs.cpp?ann...

Diag's that are user mapped use explicitly the warning level specified by the
user - they do not get mapped according to -Werror, as explained in that.

So yeah, still necessary.

+    return diagnostic().getWarningsAsErrors() ?
+        DiagnosticsEngine::Error : DiagnosticsEngine::Warning;
+  }
+
+  // Returns true if |base| specifies one of the Chromium reference counted
+  // classes (base::RefCounted / base::RefCountedThreadSafe).
+  static bool IsRefCountedCallback(const CXXBaseSpecifier* base,
+                                   CXXBasePath& path,
+                                   void* user_data) {
+    FindBadConstructsConsumer* self =
+        static_cast<FindBadConstructsConsumer*>(user_data);
+
+    const TemplateSpecializationType* base_type =
+        dyn_cast<TemplateSpecializationType>(
+            UnwrapType(base->getType().getTypePtr()));
+
+    if (!base_type) {
+      // Base-most definition is not a template, so this cannot derive from
+      // base::RefCounted. However, it may still be possible to use with a
+      // scoped_refptr<> and support ref-counting, so this is not a perfect
+      // guarantee of safety.
+      return false;
+    }
+
+    TemplateName name = base_type->getTemplateName();
+    if (TemplateDecl* decl = name.getAsTemplateDecl()) {
+      std::string base_name = decl->getNameAsString();
+
+      // Check for both base::RefCounted and base::RefCountedThreadSafe.
+      if (base_name.compare(0, 10, "RefCounted") == 0 &&
+          self->GetNamespace(decl) == "base") {
+        return true;
+      }
+    }
+
+    return false;
+  }
+
+  // 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();

[Nico, 2012/07/21 18:36:37]

This might get pretty wordy. Maybe elide some levels in the middle?

+    }
+  }
+
+  // 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;
+
+    // Determine if the current type is even ref-counted.
+    CXXBasePaths refcounted_path;
+    if (!record->lookupInBases(
+            &FindBadConstructsConsumer::IsRefCountedCallback, this,
+            refcounted_path)) {
+      return;  // Class does not derive from a ref-counted base class.
+    }
+
+    // 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);
+      }
+    }
+  }
+
 };
 
 class FindBadConstructsAction : public PluginASTAction {
  public:
   FindBadConstructsAction()
-      : check_refcounted_dtors_(true),
-        check_virtuals_in_implementations_(true) {
+      : check_virtuals_in_implementations_(true),
+        check_refcounted_dtors_(true),
+        check_base_classes_(false) {
   }
 
  protected:
   ASTConsumer* CreateASTConsumer(CompilerInstance &CI, llvm::StringRef ref) {
     return new FindBadConstructsConsumer(
-        CI, check_refcounted_dtors_, check_virtuals_in_implementations_);
+        CI, check_virtuals_in_implementations_, check_refcounted_dtors_,
+        check_base_classes_);
   }
 
   bool ParseArgs(const CompilerInstance &CI,
@@ -413,6 +584,8 @@ class FindBadConstructsAction : public PluginASTAction {
         check_refcounted_dtors_ = false;
       } else if (args[i] == "skip-virtuals-in-implementations") {
         check_virtuals_in_implementations_ = false;

[Nico, 2013/01/31 22:52:20]

Are the two flags above still needed? (I think you added those too?)

[Ryan Sleevi, 2013/01/31 23:09:30]

I will remove --skip-refcounted-dtors, since that's been fixed on the Chrome
side and stuck successfully.

skip-virtuals-in-implementations is http://crbug.com/115047 , although I've been
playing with libtooling to see if there's a way to automate this conversion (and
reformat the lines). But yes, still needed.

+      } else if (args[i] == "check-base-classes") {
+        check_base_classes_ = true;

[Nico, 2013/01/31 22:52:20]

Should there be a link to a tracking bug for getting the code base free of
violations here? So that I don't have to ask the above question about this flag
too :-)

[Ryan Sleevi, 2013/01/31 23:09:30]

http://crbug.com/123295

       } else {
         parsed = false;
         llvm::errs() << "Unknown argument: " << args[i] << "\n";
@@ -423,8 +596,9 @@ class FindBadConstructsAction : public PluginASTAction {
   }
 
  private:
-  bool check_refcounted_dtors_;
   bool check_virtuals_in_implementations_;

[Nico, 2012/07/21 18:36:37]

Why move this up? it seemed to be alphabetically ordered.

+  bool check_refcounted_dtors_;
+  bool check_base_classes_;
 };
 
 }  // namespace