Optionally check base classes for refcounting issues

Update the Chromium style checker plugin so that it can
scan the entire public class hierarchy for ref-counting
problems. This is to detect situations where a RefCounted type
implements some Delegate-type class and the Delegate may be
directly deleted, even if the RefCounted type may not be.

Also removed unused flag -skip-refcounted-dtors while here

BUG=123295
TEST=tools/clang/plugins/tests/test.sh

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=180077

[Ryan Sleevi, 2012-05-20 08:29:14]

Nico: Mind taking a look? I know Elliot is listed in OWNERS, but I thought you
might be more familiar with this.

Also, let me know if you think the notes are too verbose. While they tend to be
redundant for something like code in base/ or net/, by the time you get to
content/ or chrome/, where multiple inheritance abides, I found it helps.

It'll be some time before this is ready to be turned on - besides there being a
lot of errors, these tend to be a bit trickier in nature to sort out - and to
determine if real bugs are lurking.

[Ryan Sleevi, 2012-05-31 23:12:20]

ping

[Nico, 2012-05-31 23:13:39]

Why does this require a new plugin?

[Ryan Sleevi, 2012-05-31 23:18:44]

On 2012/05/31 23:13:39, Nico wrote:
> Why does this require a new plugin?

It doesn't, but it seemed more appropriate to split it into two, since one is
checking purely style issues, while the other is checking for potential bugs.

[Nico, 2012-05-31 23:25:31]

On 2012/05/31 23:18:44, Ryan Sleevi wrote:
> On 2012/05/31 23:13:39, Nico wrote:
> > Why does this require a new plugin?
> 
> It doesn't, but it seemed more appropriate to split it into two, since one is
> checking purely style issues, while the other is checking for potential bugs.

All the checks in the plugin should be for potential bugs. (for the
out-of-lining, .o bloat bugs.) Pure style issues don't belong in the plugin.

It makes sense to move this to a new file if it's getting big, but having a
second plugin seems unnecessary (requires a second pass over the ast etc)

[Ryan Sleevi, 2012-05-31 23:30:45]

> It makes sense to move this to a new file if it's getting big, but having a
> second plugin seems unnecessary (requires a second pass over the ast etc)

Really? Eww. I (perhaps naively) assumed that it'd iterate the AST just once,
calling each plugin. I guess if the AST is mutable, multi-pass might be
necessary, but still, surprising...

[Ryan Sleevi, 2012-07-08 23:44:50]

Nico: Mind taking another stab?

Ignore the .submodules issue, it's yet another point of pain involving this
whole git/svn transition.

[Ryan Sleevi, 2012-07-20 00:23:43]

ping

[Nico, 2012-07-21 18:36:36]

Looks pretty good as far as I can tell.

http://codereview.chromium.org/10407057/diff/11001/tools/clang/plugins/FindBa...
File tools/clang/plugins/FindBadConstructs.cpp (left):

http://codereview.chromium.org/10407057/diff/11001/tools/clang/plugins/FindBa...
tools/clang/plugins/FindBadConstructs.cpp:144: }
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?

http://codereview.chromium.org/10407057/diff/11001/tools/clang/plugins/FindBa...
File tools/clang/plugins/FindBadConstructs.cpp (right):

http://codereview.chromium.org/10407057/diff/11001/tools/clang/plugins/FindBa...
tools/clang/plugins/FindBadConstructs.cpp:31: static const char
kNoExplicitDtor[] =
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.)

http://codereview.chromium.org/10407057/diff/11001/tools/clang/plugins/FindBa...
tools/clang/plugins/FindBadConstructs.cpp:42: "[chromium-style] Public
destructor declared here";
nice :-)

http://codereview.chromium.org/10407057/diff/11001/tools/clang/plugins/FindBa...
tools/clang/plugins/FindBadConstructs.cpp:400: DiagnosticsEngine::Level
getErrorLevel() {
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?

http://codereview.chromium.org/10407057/diff/11001/tools/clang/plugins/FindBa...
tools/clang/plugins/FindBadConstructs.cpp:462: << it->Class <<
it->Base->getType();
This might get pretty wordy. Maybe elide some levels in the middle?

http://codereview.chromium.org/10407057/diff/11001/tools/clang/plugins/FindBa...
tools/clang/plugins/FindBadConstructs.cpp:599: bool
check_virtuals_in_implementations_;
Why move this up? it seemed to be alphabetically ordered.

http://codereview.chromium.org/10407057/diff/11001/tools/clang/plugins/tests/...
File tools/clang/plugins/tests/test.sh (right):

http://codereview.chromium.org/10407057/diff/11001/tools/clang/plugins/tests/...
tools/clang/plugins/tests/test.sh:28: -Xclang -add-plugin -Xclang
find-bad-constructs ${1} 2>&1)"
Why this change?

[Ryan Sleevi, 2013-01-26 07:33:47]

Nico: Care to take another pass now?

Note I've updated the unittests with the ability for per-test flags, since I
need this flag to be off by default while I fix even more regressions.


And yes, the warnings are wordy, but I've found that by the time these errors
are being generated in chrome/, there's often five classes of subtle
implementation.

I experimented with a less verbose warning, just warning that
"Classes that are ref-counted should not have public destructors"
[path to most derive class]
Declared RefCounted here
[path to the base class that inherits from base::RefCounted]
Public destructor declared here
[path to the base class with a public destructor]

But I found that tended to hide the necessary details of understanding how X
inherited from A and how X inherited from D in order to refactor.

In an ideal world, you'd never see these warnings :)

[Ryan Sleevi, 2013-01-31 02:14:34]

ping

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

Still looks pretty good.

https://codereview.chromium.org/10407057/diff/11001/tools/clang/plugins/FindB...
File tools/clang/plugins/FindBadConstructs.cpp (right):

https://codereview.chromium.org/10407057/diff/11001/tools/clang/plugins/FindB...
tools/clang/plugins/FindBadConstructs.cpp:400: DiagnosticsEngine::Level
getErrorLevel() {
On 2012/07/21 18:36:37, Nico wrote:
> 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?

This question still stands.

https://codereview.chromium.org/10407057/diff/11001/tools/clang/plugins/FindB...
tools/clang/plugins/FindBadConstructs.cpp:586:
check_virtuals_in_implementations_ = false;
Are the two flags above still needed? (I think you added those too?)

https://codereview.chromium.org/10407057/diff/11001/tools/clang/plugins/FindB...
tools/clang/plugins/FindBadConstructs.cpp:588: check_base_classes_ = true;
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]

https://codereview.chromium.org/10407057/diff/11001/tools/clang/plugins/FindB...
File tools/clang/plugins/FindBadConstructs.cpp (right):

https://codereview.chromium.org/10407057/diff/11001/tools/clang/plugins/FindB...
tools/clang/plugins/FindBadConstructs.cpp:400: DiagnosticsEngine::Level
getErrorLevel() {
On 2013/01/31 22:52:20, Nico wrote:
> On 2012/07/21 18:36:37, Nico wrote:
> > 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?
> 
> This question still stands.

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.

https://codereview.chromium.org/10407057/diff/11001/tools/clang/plugins/FindB...
tools/clang/plugins/FindBadConstructs.cpp:586:
check_virtuals_in_implementations_ = false;
On 2013/01/31 22:52:20, Nico wrote:
> Are the two flags above still needed? (I think you added those too?)

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.

https://codereview.chromium.org/10407057/diff/11001/tools/clang/plugins/FindB...
tools/clang/plugins/FindBadConstructs.cpp:588: check_base_classes_ = true;
On 2013/01/31 22:52:20, Nico wrote:
> 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 :-)

http://crbug.com/123295

[Nico, 2013-01-31 23:11:44]

On 2013/01/31 23:09:30, Ryan Sleevi wrote:
>
https://codereview.chromium.org/10407057/diff/11001/tools/clang/plugins/FindB...
> File tools/clang/plugins/FindBadConstructs.cpp (right):
> 
>
https://codereview.chromium.org/10407057/diff/11001/tools/clang/plugins/FindB...
> tools/clang/plugins/FindBadConstructs.cpp:400: DiagnosticsEngine::Level
> getErrorLevel() {
> On 2013/01/31 22:52:20, Nico wrote:
> > On 2012/07/21 18:36:37, Nico wrote:
> > > 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?
> > 
> > This question still stands.
> 
> 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.

Can you check if the plugin correctly produces errors if you just always warn
with DiagnosticsEngine::Warning?

> 
>
https://codereview.chromium.org/10407057/diff/11001/tools/clang/plugins/FindB...
> tools/clang/plugins/FindBadConstructs.cpp:586:
> check_virtuals_in_implementations_ = false;
> On 2013/01/31 22:52:20, Nico wrote:
> > Are the two flags above still needed? (I think you added those too?)
> 
> 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.
> 
>
https://codereview.chromium.org/10407057/diff/11001/tools/clang/plugins/FindB...
> tools/clang/plugins/FindBadConstructs.cpp:588: check_base_classes_ = true;
> On 2013/01/31 22:52:20, Nico wrote:
> > 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 :-)
> 
> http://crbug.com/123295

Cool, can you add these two links as comments in the corresponding if blocks?

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

https://codereview.chromium.org/10407057/diff/11001/tools/clang/plugins/FindB...
File tools/clang/plugins/FindBadConstructs.cpp (right):

https://codereview.chromium.org/10407057/diff/11001/tools/clang/plugins/FindB...
tools/clang/plugins/FindBadConstructs.cpp:400: DiagnosticsEngine::Level
getErrorLevel() {
On 2013/01/31 23:09:30, Ryan Sleevi wrote:
> On 2013/01/31 22:52:20, Nico wrote:
> > On 2012/07/21 18:36:37, Nico wrote:
> > > 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?
> > 
> > This question still stands.
> 
> 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.

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.

[Nico, 2013-02-01 00:24:32]

lgtm, thanks for checking. Maybe that's worth a comment somewhere.

https://codereview.chromium.org/10407057/diff/33001/tools/clang/plugins/FindB...
File tools/clang/plugins/FindBadConstructs.cpp (right):

https://codereview.chromium.org/10407057/diff/33001/tools/clang/plugins/FindB...
tools/clang/plugins/FindBadConstructs.cpp:584: if (args[i] ==
"skip-refcounted-dtors") {
Add "Also removed unused flag -skip-refcounted-dtors while here" to CL
description.

[commit-bot: I haz the power, 2013-02-01 01:12:35]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rsleevi@chromium.org/10407057/36001

[commit-bot: I haz the power, 2013-02-01 01:12:37]

Failed to apply patch for tools/clang/plugins/FindBadConstructs.cpp:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file tools/clang/plugins/FindBadConstructs.cpp
  Hunk #4 FAILED at 102.
  Hunk #5 succeeded at 110 with fuzz 2 (offset -247 lines).
  Hunk #6 succeeded at 169 (offset -247 lines).
  1 out of 10 hunks FAILED -- saving rejects to file
tools/clang/plugins/FindBadConstructs.cpp.rej

Patch:       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..59d4d6e3d80785ea7eab814634cd2cc8e3b52357
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,24 @@ const Type* UnwrapType(const Type* type) {
 class FindBadConstructsConsumer : public ChromeClassTester {
  public:
   FindBadConstructsConsumer(CompilerInstance& instance,
-                            bool check_refcounted_dtors,
+                            bool check_base_classes,
                             bool check_virtuals_in_implementations)
       : ChromeClassTester(instance),
-        check_refcounted_dtors_(check_refcounted_dtors),
+        check_base_classes_(check_base_classes),
         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,
@@ -75,14 +102,26 @@ class FindBadConstructsConsumer : public ChromeClassTester
{
       CheckVirtualMethods(record_location, record, warn_on_inline_bodies);
     }
 
-    if (check_refcounted_dtors_)
-      CheckRefCountedDtors(record_location, record);
+    CheckRefCountedDtors(record_location, record);
   }
 
  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_;
 
+  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,
                            CXXRecordDecl* record) {
@@ -330,9 +369,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 +428,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>,
+    //     …
(message too large)

[commit-bot: I haz the power, 2013-02-01 01:30:32]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rsleevi@chromium.org/10407057/39001

[commit-bot: I haz the power, 2013-02-01 05:09:16]

Change committed as 180077