Bug 1018486 - Part 10: Add an analysis to reject the kungFuDeathGrip pattern on function results and member variables, r=ehsan
authorMichael Layzell <michael@thelayzells.com>
Fri, 15 Jul 2016 10:43:35 -0400
changeset 313024 d2d69b3328ffdc44ebfedc1f7ff29d96f0b48139
parent 313023 59495d40a3c1270d59fafa99127be3c6e23a3300
child 313025 2356dac937b763c3b4918a0f82d65741c1a28905
push id20479
push userkwierso@gmail.com
push dateThu, 08 Sep 2016 01:08:46 +0000
treeherderfx-team@fb7c6b034329 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersehsan
bugs1018486
milestone51.0a1
Bug 1018486 - Part 10: Add an analysis to reject the kungFuDeathGrip pattern on function results and member variables, r=ehsan MozReview-Commit-ID: K8rehjAxIA6
build/clang-plugin/clang-plugin.cpp
build/clang-plugin/tests/TestKungFuDeathGrip.cpp
build/clang-plugin/tests/moz.build
--- a/build/clang-plugin/clang-plugin.cpp
+++ b/build/clang-plugin/clang-plugin.cpp
@@ -153,32 +153,38 @@ private:
     virtual void run(const MatchFinder::MatchResult &Result);
   };
 
   class AssertAssignmentChecker : public MatchFinder::MatchCallback {
   public:
     virtual void run(const MatchFinder::MatchResult &Result);
   };
 
+  class KungFuDeathGripChecker : public MatchFinder::MatchCallback {
+  public:
+    virtual void run(const MatchFinder::MatchResult &Result);
+  };
+
   ScopeChecker Scope;
   ArithmeticArgChecker ArithmeticArg;
   TrivialCtorDtorChecker TrivialCtorDtor;
   NaNExprChecker NaNExpr;
   NoAddRefReleaseOnReturnChecker NoAddRefReleaseOnReturn;
   RefCountedInsideLambdaChecker RefCountedInsideLambda;
   ExplicitOperatorBoolChecker ExplicitOperatorBool;
   NoDuplicateRefCntMemberChecker NoDuplicateRefCntMember;
   NeedsNoVTableTypeChecker NeedsNoVTableType;
   NonMemMovableTemplateArgChecker NonMemMovableTemplateArg;
   NonMemMovableMemberChecker NonMemMovableMember;
   ExplicitImplicitChecker ExplicitImplicit;
   NoAutoTypeChecker NoAutoType;
   NoExplicitMoveConstructorChecker NoExplicitMoveConstructor;
   RefCountedCopyConstructorChecker RefCountedCopyConstructor;
   AssertAssignmentChecker AssertAttribution;
+  KungFuDeathGripChecker KungFuDeathGrip;
   MatchFinder AstMatcher;
 };
 
 namespace {
 
 std::string getDeclarationNamespace(const Decl *Declaration) {
   const DeclContext *DC =
       Declaration->getDeclContext()->getEnclosingNamespaceContext();
@@ -309,16 +315,54 @@ bool isIgnoredExprForMustUse(const Expr 
 
   return false;
 }
 
 template<typename T>
 StringRef getNameChecked(const T& D) {
   return D->getIdentifier() ? D->getName() : "";
 }
+
+bool typeIsRefPtr(QualType Q) {
+  CXXRecordDecl *D = Q->getAsCXXRecordDecl();
+  if (!D || !D->getIdentifier()) {
+    return false;
+  }
+
+  StringRef name = D->getName();
+  if (name == "RefPtr" || name == "nsCOMPtr") {
+    return true;
+  }
+  return false;
+}
+
+// The method defined in clang for ignoring implicit nodes doesn't work with
+// some AST trees. To get around this, we define our own implementation of
+// IgnoreImplicit.
+const Stmt *IgnoreImplicit(const Stmt *s) {
+  while (true) {
+    if (auto *ewc = dyn_cast<ExprWithCleanups>(s)) {
+      s = ewc->getSubExpr();
+    } else if (auto *mte = dyn_cast<MaterializeTemporaryExpr>(s)) {
+      s = mte->GetTemporaryExpr();
+    } else if (auto *bte = dyn_cast<CXXBindTemporaryExpr>(s)) {
+      s = bte->getSubExpr();
+    } else if (auto *ice = dyn_cast<ImplicitCastExpr>(s)) {
+      s = ice->getSubExpr();
+    } else {
+      break;
+    }
+  }
+
+  return s;
+}
+
+const Expr *IgnoreImplicit(const Expr *e) {
+  return cast<Expr>(IgnoreImplicit(static_cast<const Stmt *>(e)));
+}
 }
 
 class CustomTypeAnnotation {
   enum ReasonKind {
     RK_None,
     RK_Direct,
     RK_ArrayElement,
     RK_BaseClass,
@@ -816,16 +860,19 @@ AST_MATCHER(CallExpr, isAssertAssignment
       && Method->getDeclName().isIdentifier()
       && Method->getName() == AssertName;
 }
 
 AST_MATCHER(CXXRecordDecl, isLambdaDecl) {
   return Node.isLambda();
 }
 
+AST_MATCHER(QualType, isRefPtr) {
+  return typeIsRefPtr(Node);
+}
 }
 }
 
 namespace {
 
 void CustomTypeAnnotation::dumpAnnotationReason(DiagnosticsEngine &Diag,
                                                 QualType T,
                                                 SourceLocation Loc) {
@@ -1140,16 +1187,19 @@ DiagnosticsMatcher::DiagnosticsMatcher()
           hasDeclaration(cxxConstructorDecl(isCompilerProvidedCopyConstructor(),
                                             ofClass(hasRefCntMember()))))
           .bind("node"),
       &RefCountedCopyConstructor);
 
   AstMatcher.addMatcher(
       callExpr(isAssertAssignmentTestFunc()).bind("funcCall"),
       &AssertAttribution);
+
+  AstMatcher.addMatcher(varDecl(hasType(isRefPtr())).bind("decl"),
+                        &KungFuDeathGrip);
 }
 
 // These enum variants determine whether an allocation has occured in the code.
 enum AllocationVariety {
   AV_None,
   AV_Global,
   AV_Automatic,
   AV_Temporary,
@@ -1688,16 +1738,107 @@ void DiagnosticsMatcher::AssertAssignmen
       DiagnosticIDs::Error, "Forbidden assignment in assert expression");
   const CallExpr *FuncCall = Result.Nodes.getNodeAs<CallExpr>("funcCall");
 
   if (FuncCall && hasSideEffectAssignment(FuncCall)) {
     Diag.Report(FuncCall->getLocStart(), AssignInsteadOfComp);
   }
 }
 
+void DiagnosticsMatcher::KungFuDeathGripChecker::run(
+    const MatchFinder::MatchResult &Result) {
+  DiagnosticsEngine &Diag = Result.Context->getDiagnostics();
+  unsigned ErrorID = Diag.getDiagnosticIDs()->getCustomDiagID(
+      DiagnosticIDs::Error,
+      "Unused \"kungFuDeathGrip\" %0 objects constructed from %1 are prohibited");
+
+  unsigned NoteID = Diag.getDiagnosticIDs()->getCustomDiagID(
+      DiagnosticIDs::Note,
+      "Please switch all accesses to this %0 to go through '%1', or explicitly pass '%1' to `mozilla::Unused`");
+
+  const VarDecl *D = Result.Nodes.getNodeAs<VarDecl>("decl");
+  if (D->isReferenced() || !D->hasLocalStorage() || !D->hasInit()) {
+    return;
+  }
+
+  // Not interested in parameters.
+  if (isa<ImplicitParamDecl>(D) || isa<ParmVarDecl>(D)) {
+    return;
+  }
+
+  const Expr *E = IgnoreImplicit(D->getInit());
+  const CXXConstructExpr *CE = dyn_cast<CXXConstructExpr>(E);
+  if (CE && CE->getNumArgs() == 0) {
+    // We don't report an error when we construct and don't use a nsCOMPtr /
+    // nsRefPtr with no arguments. We don't report it because the error is not
+    // related to the current check. In the future it may be reported through a
+    // more generic mechanism.
+    return;
+  }
+
+  // We don't want to look at the single argument conversion constructors
+  // which are inbetween the declaration and the actual object which we are
+  // assigning into the nsCOMPtr/RefPtr. To do this, we repeatedly
+  // IgnoreImplicit, then look at the expression. If it is one of these
+  // conversion constructors, we ignore it and continue to dig.
+  while ((CE = dyn_cast<CXXConstructExpr>(E)) && CE->getNumArgs() == 1) {
+    E = IgnoreImplicit(CE->getArg(0));
+  }
+
+  // We allow taking a kungFuDeathGrip of `this` because it cannot change
+  // beneath us, so calling directly through `this` is OK. This is the same
+  // for local variable declarations.
+  //
+  // We also don't complain about unused RefPtrs which are constructed from
+  // the return value of a new expression, as these are required in order to
+  // immediately destroy the value created (which was presumably created for
+  // its side effects), and are not used as a death grip.
+  if (isa<CXXThisExpr>(E) || isa<DeclRefExpr>(E) || isa<CXXNewExpr>(E)) {
+    return;
+  }
+
+  // These types are assigned into nsCOMPtr and RefPtr for their side effects,
+  // and not as a kungFuDeathGrip. We don't want to consider RefPtr and nsCOMPtr
+  // types which are initialized with these types as errors.
+  const TagDecl *TD = E->getType()->getAsTagDecl();
+  if (TD && TD->getIdentifier()) {
+    static const char *IgnoreTypes[] = {
+      "already_AddRefed",
+      "nsGetServiceByCID",
+      "nsGetServiceByCIDWithError",
+      "nsGetServiceByContractID",
+      "nsGetServiceByContractIDWithError",
+      "nsCreateInstanceByCID",
+      "nsCreateInstanceByContractID",
+      "nsCreateInstanceFromFactory",
+    };
+
+    for (uint32_t i = 0; i < sizeof(IgnoreTypes) / sizeof(IgnoreTypes[0]); ++i) {
+      if (TD->getName() == IgnoreTypes[i]) {
+        return;
+      }
+    }
+  }
+
+  // Report the error
+  const char *ErrThing;
+  const char *NoteThing;
+  if (isa<MemberExpr>(E)) {
+    ErrThing  = "members";
+    NoteThing = "member";
+  } else {
+    ErrThing = "temporary values";
+    NoteThing = "value";
+  }
+
+  // We cannot provide the note if we don't have an initializer
+  Diag.Report(D->getLocStart(), ErrorID) << D->getType() << ErrThing;
+  Diag.Report(E->getLocStart(), NoteID) << NoteThing << getNameChecked(D);
+}
+
 class MozCheckAction : public PluginASTAction {
 public:
   ASTConsumerPtr CreateASTConsumer(CompilerInstance &CI,
                                    StringRef FileName) override {
 #if CLANG_VERSION_FULL >= 306
     std::unique_ptr<MozChecker> Checker(llvm::make_unique<MozChecker>(CI));
     ASTConsumerPtr Other(Checker->getOtherConsumer());
 
new file mode 100644
--- /dev/null
+++ b/build/clang-plugin/tests/TestKungFuDeathGrip.cpp
@@ -0,0 +1,107 @@
+#define MOZ_IMPLICIT __attribute__((annotate("moz_implicit")))
+
+template <typename T>
+class already_AddRefed {
+public:
+  already_AddRefed();
+  T* mPtr;
+};
+
+template <typename T>
+class RefPtr {
+public:
+  RefPtr();
+  MOZ_IMPLICIT RefPtr(T* aIn);
+  MOZ_IMPLICIT RefPtr(already_AddRefed<T> aIn);
+  ~RefPtr();
+  T* mPtr;
+};
+
+template <typename T>
+class nsCOMPtr {
+public:
+  nsCOMPtr();
+  MOZ_IMPLICIT nsCOMPtr(T* aIn);
+  MOZ_IMPLICIT nsCOMPtr(already_AddRefed<T> aIn);
+  ~nsCOMPtr();
+  T* mPtr;
+};
+
+class Type {
+public:
+  static nsCOMPtr<Type> someStaticCOMPtr;
+
+  void f(nsCOMPtr<Type> ignoredArgument, Type *param) {
+    nsCOMPtr<Type> never_referenced;
+    nsCOMPtr<Type> kfdg_t1(this);
+    nsCOMPtr<Type> kfdg_t2 = this;
+
+    nsCOMPtr<Type> kfdg_m1(p); // expected-error {{Unused "kungFuDeathGrip" 'nsCOMPtr<Type>' objects constructed from members are prohibited}} expected-note {{Please switch all accesses to this member to go through 'kfdg_m1', or explicitly pass 'kfdg_m1' to `mozilla::Unused`}}
+    nsCOMPtr<Type> kfdg_m2 = p; // expected-error {{Unused "kungFuDeathGrip" 'nsCOMPtr<Type>' objects constructed from members are prohibited}} expected-note {{Please switch all accesses to this member to go through 'kfdg_m2', or explicitly pass 'kfdg_m2' to `mozilla::Unused`}}
+    nsCOMPtr<Type> kfdg_m3(p);
+    kfdg_m3.mPtr->f(nullptr, nullptr);
+    nsCOMPtr<Type> kfdg_m4 = p;
+    kfdg_m4.mPtr->f(nullptr, nullptr);
+
+    nsCOMPtr<Type> kfdg_a1((already_AddRefed<Type>()));
+    nsCOMPtr<Type> kfdg_a2 = already_AddRefed<Type>();
+
+    nsCOMPtr<Type> kfdg_p1(param);
+    nsCOMPtr<Type> kfdg_p2 = param;
+
+
+    RefPtr<Type> never_referenced2;
+    RefPtr<Type> kfdg_t3(this);
+    RefPtr<Type> kfdg_t4 = this;
+
+    RefPtr<Type> kfdg_m5(p); // expected-error {{Unused "kungFuDeathGrip" 'RefPtr<Type>' objects constructed from members are prohibited}} expected-note {{Please switch all accesses to this member to go through 'kfdg_m5', or explicitly pass 'kfdg_m5' to `mozilla::Unused`}}
+    RefPtr<Type> kfdg_m6 = p; // expected-error {{Unused "kungFuDeathGrip" 'RefPtr<Type>' objects constructed from members are prohibited}} expected-note {{Please switch all accesses to this member to go through 'kfdg_m6', or explicitly pass 'kfdg_m6' to `mozilla::Unused`}}
+    RefPtr<Type> kfdg_m7(p);
+    kfdg_m7.mPtr->f(nullptr, nullptr);
+    RefPtr<Type> kfdg_m8 = p;
+    kfdg_m8.mPtr->f(nullptr, nullptr);
+
+    RefPtr<Type> kfdg_a3((already_AddRefed<Type>()));
+    RefPtr<Type> kfdg_a4 = already_AddRefed<Type>();
+
+    RefPtr<Type> kfdg_p3(param);
+    RefPtr<Type> kfdg_p4 = param;
+  }
+
+  Type *p;
+};
+
+void f(nsCOMPtr<Type> ignoredArgument, Type *param) {
+  nsCOMPtr<Type> never_referenced;
+  Type t;
+  // Type *p = nullptr;
+  nsCOMPtr<Type> kfdg_m1(t.p); // expected-error {{Unused "kungFuDeathGrip" 'nsCOMPtr<Type>' objects constructed from members are prohibited}} expected-note {{Please switch all accesses to this member to go through 'kfdg_m1', or explicitly pass 'kfdg_m1' to `mozilla::Unused`}}
+  nsCOMPtr<Type> kfdg_m2 = t.p; // expected-error {{Unused "kungFuDeathGrip" 'nsCOMPtr<Type>' objects constructed from members are prohibited}} expected-note {{Please switch all accesses to this member to go through 'kfdg_m2', or explicitly pass 'kfdg_m2' to `mozilla::Unused`}}
+  nsCOMPtr<Type> kfdg_m3(t.p);
+  kfdg_m3.mPtr->f(nullptr, nullptr);
+  nsCOMPtr<Type> kfdg_m4 = t.p;
+  kfdg_m4.mPtr->f(nullptr, nullptr);
+
+  nsCOMPtr<Type> kfdg_a1((already_AddRefed<Type>()));
+  nsCOMPtr<Type> kfdg_a2 = already_AddRefed<Type>();
+
+  nsCOMPtr<Type> kfdg_p1(param);
+  nsCOMPtr<Type> kfdg_p2 = param;
+
+
+  RefPtr<Type> never_referenced2;
+  RefPtr<Type> kfdg_m5(t.p); // expected-error {{Unused "kungFuDeathGrip" 'RefPtr<Type>' objects constructed from members are prohibited}} expected-note {{Please switch all accesses to this member to go through 'kfdg_m5', or explicitly pass 'kfdg_m5' to `mozilla::Unused`}}
+  RefPtr<Type> kfdg_m6 = t.p; // expected-error {{Unused "kungFuDeathGrip" 'RefPtr<Type>' objects constructed from members are prohibited}} expected-note {{Please switch all accesses to this member to go through 'kfdg_m6', or explicitly pass 'kfdg_m6' to `mozilla::Unused`}}
+  RefPtr<Type> kfdg_m7(t.p);
+  kfdg_m7.mPtr->f(nullptr, nullptr);
+  RefPtr<Type> kfdg_m8 = t.p;
+  kfdg_m8.mPtr->f(nullptr, nullptr);
+
+  RefPtr<Type> kfdg_a3((already_AddRefed<Type>()));
+  RefPtr<Type> kfdg_a4 = already_AddRefed<Type>();
+
+  RefPtr<Type> kfdg_p3(param);
+  RefPtr<Type> kfdg_p4 = param;
+}
+
+nsCOMPtr<Type> Type::someStaticCOMPtr(nullptr);
--- a/build/clang-plugin/tests/moz.build
+++ b/build/clang-plugin/tests/moz.build
@@ -10,16 +10,17 @@ Library('clang-plugin-tests')
 SOURCES += [
     'TestAssertWithAssignment.cpp',
     'TestBadImplicitConversionCtor.cpp',
     'TestCustomHeap.cpp',
     'TestExplicitOperatorBool.cpp',
     'TestGlobalClass.cpp',
     'TestHeapClass.cpp',
     'TestInheritTypeAnnotationsFromTemplateArgs.cpp',
+    'TestKungFuDeathGrip.cpp',
     'TestMultipleAnnotations.cpp',
     'TestMustOverride.cpp',
     'TestMustUse.cpp',
     'TestNANTestingExpr.cpp',
     'TestNANTestingExprC.c',
     'TestNeedsNoVTableType.cpp',
     'TestNoAddRefReleaseOnReturn.cpp',
     'TestNoArithmeticExprInArgument.cpp',