Bug 1609638 - Add analysis identifying dereferences of temporary RefPtr objects. r=andi

Differential Revision: https://phabricator.services.mozilla.com/D60167

--HG--
extra : moz-landing-system : lando

build/clang-plugin/Checks.inc

@@ -33,5 +33,6 @@ CHECK(RefCountedCopyConstructorChecker, "refcounted-copy-constructor")
 CHECK(RefCountedInsideLambdaChecker, "refcounted-inside-lambda")
 CHECK(ScopeChecker, "scope")
 CHECK(SprintfLiteralChecker, "sprintf-literal")
+CHECK(TempRefPtrChecker, "performance-temp-refptr")
 CHECK(TrivialCtorDtorChecker, "trivial-constructor-destructor")
 CHECK(TrivialDtorChecker, "trivial-destructor")

build/clang-plugin/ChecksIncludes.inc

@@ -34,6 +34,6 @@
 #include "RefCountedInsideLambdaChecker.h"
 #include "ScopeChecker.h"
 #include "SprintfLiteralChecker.h"
+#include "TempRefPtrChecker.h"
 #include "TrivialCtorDtorChecker.h"
 #include "TrivialDtorChecker.h"
-

build/clang-plugin/CustomMatchers.h

@@ -306,6 +306,18 @@ AST_MATCHER(QualType, isSmartPtrToRefCounted) {
   return D && hasCustomAttribute<moz_is_smartptr_to_refcounted>(D);
 }
 
+AST_MATCHER(ClassTemplateSpecializationDecl, isSmartPtrToRefCountedDecl) {
+  auto *D = dyn_cast_or_null<CXXRecordDecl>(Node.getSpecializedTemplate()->getTemplatedDecl());
+  if (!D) {
+    return false;
+  }
+
+  D = D->getCanonicalDecl();
+
+  return D && hasCustomAttribute<moz_is_smartptr_to_refcounted>(D);
+}
+
+
 AST_MATCHER(CXXRecordDecl, hasBaseClasses) {
   const CXXRecordDecl *Decl = Node.getCanonicalDecl();
 

build/clang-plugin/TempRefPtrChecker.cpp

@@ -0,0 +1,57 @@
+/* This Source Code Form is subject to the terms of the Mozilla Public
+ * License, v. 2.0. If a copy of the MPL was not distributed with this
+ * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
+
+#include "TempRefPtrChecker.h"
+#include "CustomMatchers.h"
+
+constexpr const char *kCallExpr = "call-expr";
+constexpr const char *kOperatorCallExpr = "operator-call";
+
+void TempRefPtrChecker::registerMatchers(MatchFinder *AstMatcher) {
+  AstMatcher->addMatcher(
+      cxxOperatorCallExpr(
+          hasOverloadedOperatorName("->"),
+          hasAnyArgument(implicitCastExpr(
+              hasSourceExpression(materializeTemporaryExpr(anyOf(
+                  hasDescendant(callExpr().bind(kCallExpr)), anything()))))),
+          callee(hasDeclContext(classTemplateSpecializationDecl(
+              isSmartPtrToRefCountedDecl(),
+              // ignore any calls on temporary RefPtr<MozPromise<T>>,
+              // since these typically need to be locally ref-counted,
+              // e.g. in Then chains where the promise might be resolved
+              // concurrently
+              unless(hasTemplateArgument(
+                  0, refersToType(hasDeclaration(
+                         cxxRecordDecl(hasName("mozilla::MozPromise"))))))))))
+          .bind(kOperatorCallExpr),
+      this);
+}
+
+void TempRefPtrChecker::check(const MatchFinder::MatchResult &Result) {
+  const auto *OCE =
+      Result.Nodes.getNodeAs<CXXOperatorCallExpr>(kOperatorCallExpr);
+
+  const auto *refPtrDecl =
+      dyn_cast<const CXXRecordDecl>(OCE->getCalleeDecl()->getDeclContext());
+
+  diag(OCE->getOperatorLoc(),
+       "performance issue: temporary %0 is only dereferenced here once which "
+       "involves short-lived AddRef/Release calls")
+      << refPtrDecl;
+
+  const auto *InnerCE = Result.Nodes.getNodeAs<CallExpr>(kCallExpr);
+  if (InnerCE) {
+    const auto functionName =
+        InnerCE->getCalleeDecl()->getAsFunction()->getQualifiedNameAsString();
+
+    if (functionName != "mozilla::MakeRefPtr") {
+      diag(
+          OCE->getOperatorLoc(),
+          "consider changing function %0 to return a raw reference instead (be "
+          "sure that the pointee is held alive by someone else though!)",
+          DiagnosticIDs::Note)
+          << functionName;
+    }
+  }
+}

build/clang-plugin/TempRefPtrChecker.h

@@ -0,0 +1,21 @@
+/* This Source Code Form is subject to the terms of the Mozilla Public
+ * License, v. 2.0. If a copy of the MPL was not distributed with this
+ * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
+
+#ifndef TempRefPtrChecker_h__
+#define TempRefPtrChecker_h__
+
+#include "plugin.h"
+
+class TempRefPtrChecker final : public BaseCheck {
+public:
+  TempRefPtrChecker(StringRef CheckName, ContextType *Context = nullptr)
+      : BaseCheck(CheckName, Context) {}
+  void registerMatchers(MatchFinder *AstMatcher) override;
+  void check(const MatchFinder::MatchResult &Result) override;
+
+private:
+  CompilerInstance *CI;
+};
+
+#endif

build/clang-plugin/moz.build

@@ -39,6 +39,7 @@ HOST_SOURCES += [
     'RefCountedInsideLambdaChecker.cpp',
     'ScopeChecker.cpp',
     'SprintfLiteralChecker.cpp',
+    'TempRefPtrChecker.cpp',
     'TrivialCtorDtorChecker.cpp',
     'TrivialDtorChecker.cpp',
     'VariableUsageHelpers.cpp',

build/clang-plugin/tests/TestCanRunScript.cpp

@@ -375,7 +375,7 @@ struct DisallowConstNonRefPtrMemberArgs {
 };
 
 MOZ_CAN_RUN_SCRIPT void test_temporary_1() {
-  RefPtr<RefCountedBase>(new RefCountedBase())->method_test();
+  RefPtr<RefCountedBase>(new RefCountedBase())->method_test(); // expected-warning {{performance issue: temporary 'RefPtr<RefCountedBase>' is only dereferenced here once which involves short-lived AddRef/Release calls}}
 }
 
 MOZ_CAN_RUN_SCRIPT void test_temporary_2() {

build/clang-plugin/tests/TestTempRefPtr.cpp

@@ -0,0 +1,52 @@
+#include <mozilla/RefPtr.h>
+
+using namespace mozilla;
+
+struct RefCountedBase {
+  void AddRef();
+  void Release();
+
+  void method_test();
+};
+
+struct RefCountedBaseHolder {
+  RefPtr<RefCountedBase> GetRefCountedBase() const {
+    return mRefCountedBase;
+  }
+
+private:
+  RefPtr<RefCountedBase> mRefCountedBase = MakeRefPtr<RefCountedBase>();
+};
+
+
+void test_arrow_temporary_new_refptr_function_style_cast() {
+  RefPtr<RefCountedBase>(new RefCountedBase())->method_test(); // expected-warning {{performance issue: temporary 'RefPtr<RefCountedBase>' is only dereferenced here once which involves short-lived AddRef/Release calls}}
+}
+
+void test_arrow_temporary_new_refptr_brace() {
+  RefPtr<RefCountedBase>{new RefCountedBase()}->method_test(); // expected-warning {{performance issue: temporary 'RefPtr<RefCountedBase>' is only dereferenced here once which involves short-lived AddRef/Release calls}}
+}
+
+void test_arrow_temporary_new_c_style_cast() {
+  ((RefPtr<RefCountedBase>)(new RefCountedBase()))->method_test(); // expected-warning {{performance issue: temporary 'RefPtr<RefCountedBase>' is only dereferenced here once which involves short-lived AddRef/Release calls}}
+}
+
+void test_arrow_temporary_new_static_cast() {
+  static_cast<RefPtr<RefCountedBase>>(new RefCountedBase())->method_test(); // expected-warning {{performance issue: temporary 'RefPtr<RefCountedBase>' is only dereferenced here once which involves short-lived AddRef/Release calls}}
+}
+
+void test_arrow_temporary_new_refptr_makerefptr() {
+  MakeRefPtr<RefCountedBase>()->method_test(); // expected-warning {{performance issue: temporary 'RefPtr<RefCountedBase>' is only dereferenced here once which involves short-lived AddRef/Release calls}}
+}
+
+void test_arrow_temporary_get_refptr_from_member_function() {
+  const RefCountedBaseHolder holder;
+  holder.GetRefCountedBase()->method_test(); // expected-warning {{performance issue: temporary 'RefPtr<RefCountedBase>' is only dereferenced here once which involves short-lived AddRef/Release calls}} expected-note {{consider changing function RefCountedBaseHolder::GetRefCountedBase to return a raw reference instead}}
+}
+
+void test_ref(RefCountedBase &aRefCountedBase);
+
+void test_star_temporary_new_refptr_function_style_cast() {
+  // TODO: Should we warn about operator* as well?
+  test_ref(*RefPtr<RefCountedBase>(new RefCountedBase()));
+}

build/clang-plugin/tests/moz.build

@@ -45,6 +45,7 @@ SOURCES += [
     'TestStackClass.cpp',
     'TestStaticLocalClass.cpp',
     'TestTemporaryClass.cpp',
+    'TestTempRefPtr.cpp',
     'TestTrivialCtorDtor.cpp',
     'TestTrivialDtor.cpp',
 ]