Bug 602122 - Add a static analysis to find XPCOM classes with duplicate mRefCnt members; r=ehsan
authorMichael Layzell <michael@thelayzells.com>
Wed, 29 Jul 2015 22:01:58 -0400
changeset 287023 ea041b07a51b93ea54f50ffb68c65704e18eff57
parent 287022 96006ddf834d6708f933f62723511595b86edbf3
child 287024 ffd18bccd1ce140a71315e33ead24d7918a5f99f
push id5067
push userraliiev@mozilla.com
push dateMon, 21 Sep 2015 14:04:52 +0000
treeherdermozilla-beta@14221ffe5b2f [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersehsan
bugs602122
milestone42.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 602122 - Add a static analysis to find XPCOM classes with duplicate mRefCnt members; r=ehsan
build/clang-plugin/clang-plugin.cpp
build/clang-plugin/tests/TestNoDuplicateRefCntMember.cpp
build/clang-plugin/tests/moz.build
--- a/build/clang-plugin/clang-plugin.cpp
+++ b/build/clang-plugin/clang-plugin.cpp
@@ -91,16 +91,21 @@ private:
     virtual void run(const MatchFinder::MatchResult &Result);
   };
 
   class ExplicitOperatorBoolChecker : public MatchFinder::MatchCallback {
   public:
     virtual void run(const MatchFinder::MatchResult &Result);
   };
 
+  class NoDuplicateRefCntMemberChecker : public MatchFinder::MatchCallback {
+  public:
+    virtual void run(const MatchFinder::MatchResult &Result);
+  };
+
   class NeedsNoVTableTypeChecker : public MatchFinder::MatchCallback {
   public:
     virtual void run(const MatchFinder::MatchResult &Result);
   };
 
   class NonMemMovableChecker : public MatchFinder::MatchCallback {
   public:
     virtual void run(const MatchFinder::MatchResult &Result);
@@ -110,16 +115,17 @@ private:
   ScopeChecker globalClassChecker;
   NonHeapClassChecker nonheapClassChecker;
   ArithmeticArgChecker arithmeticArgChecker;
   TrivialCtorDtorChecker trivialCtorDtorChecker;
   NaNExprChecker nanExprChecker;
   NoAddRefReleaseOnReturnChecker noAddRefReleaseOnReturnChecker;
   RefCountedInsideLambdaChecker refCountedInsideLambdaChecker;
   ExplicitOperatorBoolChecker explicitOperatorBoolChecker;
+  NoDuplicateRefCntMemberChecker noDuplicateRefCntMemberChecker;
   NeedsNoVTableTypeChecker needsNoVTableTypeChecker;
   NonMemMovableChecker nonMemMovableChecker;
   MatchFinder astMatcher;
 };
 
 namespace {
 
 std::string getDeclarationNamespace(const Decl *decl) {
@@ -576,16 +582,58 @@ bool IsInSystemHeader(const ASTContext &
   auto &SourceManager = AC.getSourceManager();
   auto ExpansionLoc = SourceManager.getExpansionLoc(D.getLocStart());
   if (ExpansionLoc.isInvalid()) {
     return false;
   }
   return SourceManager.isInSystemHeader(ExpansionLoc);
 }
 
+const FieldDecl *getClassRefCntMember(const CXXRecordDecl *D) {
+  for (RecordDecl::field_iterator field = D->field_begin(), e = D->field_end();
+       field != e; ++field) {
+    if (field->getName() == "mRefCnt") {
+      return *field;
+    }
+  }
+  return 0;
+}
+
+const FieldDecl *getClassRefCntMember(QualType T) {
+  while (const ArrayType *arrTy = T->getAsArrayTypeUnsafe())
+    T = arrTy->getElementType();
+  CXXRecordDecl *clazz = T->getAsCXXRecordDecl();
+  return clazz ? getClassRefCntMember(clazz) : 0;
+}
+
+const FieldDecl *getBaseRefCntMember(QualType T);
+
+const FieldDecl *getBaseRefCntMember(const CXXRecordDecl *D) {
+  const FieldDecl *refCntMember = getClassRefCntMember(D);
+  if (refCntMember && isClassRefCounted(D)) {
+    return refCntMember;
+  }
+
+  for (CXXRecordDecl::base_class_const_iterator base = D->bases_begin(), e = D->bases_end();
+       base != e; ++base) {
+    refCntMember = getBaseRefCntMember(base->getType());
+    if (refCntMember) {
+      return refCntMember;
+    }
+  }
+  return 0;
+}
+
+const FieldDecl *getBaseRefCntMember(QualType T) {
+  while (const ArrayType *arrTy = T->getAsArrayTypeUnsafe())
+    T = arrTy->getElementType();
+  CXXRecordDecl *clazz = T->getAsCXXRecordDecl();
+  return clazz ? getBaseRefCntMember(clazz) : 0;
+}
+
 bool typeHasVTable(QualType T) {
   while (const ArrayType *arrTy = T->getAsArrayTypeUnsafe())
     T = arrTy->getElementType();
   CXXRecordDecl* offender = T->getAsCXXRecordDecl();
   return offender && offender->hasDefinition() && offender->isDynamicClass();
 }
 
 }
@@ -737,16 +785,20 @@ AST_POLYMORPHIC_MATCHER_P(equalsBoundNod
   };
   Visitor visitor(Node, ID, haveMatchingResult);
   bindings.visitMatches(&visitor);
   return haveMatchingResult;
 }
 
 #endif
 
+AST_MATCHER(CXXRecordDecl, hasRefCntMember) {
+  return isClassRefCounted(&Node) && getClassRefCntMember(&Node);
+}
+
 AST_MATCHER(QualType, hasVTable) {
   return typeHasVTable(Node);
 }
 
 AST_MATCHER(CXXRecordDecl, hasNeedsNoVTableTypeAttr) {
   return MozChecker::hasCustomAnnotation(&Node, "moz_needs_no_vtable_type");
 }
 
@@ -979,16 +1031,20 @@ DiagnosticsMatcher::DiagnosticsMatcher()
 
   // Older clang versions such as the ones used on the infra recognize these
   // conversions as 'operator _Bool', but newer clang versions recognize these
   // as 'operator bool'.
   astMatcher.addMatcher(methodDecl(anyOf(hasName("operator bool"),
                                          hasName("operator _Bool"))).bind("node"),
     &explicitOperatorBoolChecker);
 
+  astMatcher.addMatcher(recordDecl(allOf(decl().bind("decl"),
+                                         hasRefCntMember())),
+                        &noDuplicateRefCntMemberChecker);
+
   astMatcher.addMatcher(classTemplateSpecializationDecl(
              allOf(hasAnyTemplateArgument(refersToType(hasVTable())),
                    hasNeedsNoVTableTypeAttr())).bind("node"),
      &needsNoVTableTypeChecker);
 
   // Handle non-mem-movable template specializations
   astMatcher.addMatcher(classTemplateSpecializationDecl(
       allOf(needsMemMovable(),
@@ -1172,16 +1228,42 @@ void DiagnosticsMatcher::ExplicitOperato
       !MozChecker::hasCustomAnnotation(method, "moz_implicit") &&
       !IsInSystemHeader(method->getASTContext(), *method) &&
       isInterestingDeclForImplicitConversion(method)) {
     Diag.Report(method->getLocStart(), errorID) << clazz;
     Diag.Report(method->getLocStart(), noteID) << "'operator bool'";
   }
 }
 
+void DiagnosticsMatcher::NoDuplicateRefCntMemberChecker::run(
+    const MatchFinder::MatchResult &Result) {
+  DiagnosticsEngine &Diag = Result.Context->getDiagnostics();
+  unsigned warningID = Diag.getDiagnosticIDs()->getCustomDiagID(
+      DiagnosticIDs::Error, "Refcounted record %0 has multiple mRefCnt members");
+  unsigned note1ID = Diag.getDiagnosticIDs()->getCustomDiagID(
+      DiagnosticIDs::Note, "Superclass %0 also has an mRefCnt member");
+  unsigned note2ID = Diag.getDiagnosticIDs()->getCustomDiagID(
+      DiagnosticIDs::Note, "Consider using the _INHERITED macros for AddRef and Release here");
+
+  const CXXRecordDecl *decl = Result.Nodes.getNodeAs<CXXRecordDecl>("decl");
+  const FieldDecl *refCntMember = getClassRefCntMember(decl);
+  assert(refCntMember && "The matcher checked to make sure we have a refCntMember");
+
+  // Check every superclass for whether it has a base with a refcnt member, and warn for those which do
+  for (CXXRecordDecl::base_class_const_iterator base = decl->bases_begin(), e = decl->bases_end();
+       base != e; ++base) {
+    const FieldDecl *baseRefCntMember = getBaseRefCntMember(base->getType());
+    if (baseRefCntMember) {
+      Diag.Report(decl->getLocStart(), warningID) << decl;
+      Diag.Report(baseRefCntMember->getLocStart(), note1ID) << baseRefCntMember->getParent();
+      Diag.Report(refCntMember->getLocStart(), note2ID);
+    }
+  }
+}
+
 void DiagnosticsMatcher::NeedsNoVTableTypeChecker::run(
     const MatchFinder::MatchResult &Result) {
   DiagnosticsEngine &Diag = Result.Context->getDiagnostics();
   unsigned errorID = Diag.getDiagnosticIDs()->getCustomDiagID(
       DiagnosticIDs::Error, "%0 cannot be instantiated because %1 has a VTable");
   unsigned noteID = Diag.getDiagnosticIDs()->getCustomDiagID(
       DiagnosticIDs::Note, "bad instantiation of %0 requested here");
 
new file mode 100644
--- /dev/null
+++ b/build/clang-plugin/tests/TestNoDuplicateRefCntMember.cpp
@@ -0,0 +1,38 @@
+class C1 {};
+
+class RC1 {
+public:
+  virtual void AddRef();
+  virtual void Release();
+
+private:
+  int mRefCnt; // expected-note 2 {{Superclass 'RC1' also has an mRefCnt member}}
+};
+
+class RC2 : public RC1 { // expected-error {{Refcounted record 'RC2' has multiple mRefCnt members}}
+public:
+  virtual void AddRef();
+  virtual void Release();
+
+private:
+  int mRefCnt; // expected-note {{Consider using the _INHERITED macros for AddRef and Release here}}
+};
+
+class C2 : public RC1 {};
+
+class RC3 : public RC1 {};
+
+class RC4 : public RC3, public C2 {};
+
+class RC5 : public RC1 {};
+
+class RC6 : public C1, public RC5 { // expected-error {{Refcounted record 'RC6' has multiple mRefCnt members}}
+public:
+  virtual void AddRef();
+  virtual void Release();
+
+private:
+  int mRefCnt; // expected-note {{Consider using the _INHERITED macros for AddRef and Release here}}
+};
+
+class Predecl;
--- a/build/clang-plugin/tests/moz.build
+++ b/build/clang-plugin/tests/moz.build
@@ -12,16 +12,17 @@ SOURCES += [
     'TestMultipleAnnotations.cpp',
     'TestMustOverride.cpp',
     'TestMustUse.cpp',
     'TestNANTestingExpr.cpp',
     'TestNANTestingExprC.c',
     'TestNeedsNoVTableType.cpp',
     'TestNoAddRefReleaseOnReturn.cpp',
     'TestNoArithmeticExprInArgument.cpp',
+    'TestNoDuplicateRefCntMember.cpp',
     'TestNonHeapClass.cpp',
     'TestNonMemMovable.cpp',
     'TestNoRefcountedInsideLambdas.cpp',
     'TestStackClass.cpp',
     'TestTrivialCtorDtor.cpp',
 ]
 
 DISABLE_STL_WRAPPING = True