diff options
Diffstat (limited to 'build/clang-plugin/alpha')
-rw-r--r-- | build/clang-plugin/alpha/AlphaChecks.inc | 10 | ||||
-rw-r--r-- | build/clang-plugin/alpha/AlphaIncludes.inc | 2 | ||||
-rw-r--r-- | build/clang-plugin/alpha/NonStdMoveChecker.cpp | 115 | ||||
-rw-r--r-- | build/clang-plugin/alpha/NonStdMoveChecker.h | 29 | ||||
-rw-r--r-- | build/clang-plugin/alpha/TempRefPtrChecker.cpp | 57 | ||||
-rw-r--r-- | build/clang-plugin/alpha/TempRefPtrChecker.h | 21 | ||||
-rw-r--r-- | build/clang-plugin/alpha/sources.mozbuild | 11 | ||||
-rw-r--r-- | build/clang-plugin/alpha/tests/TestNonStdMove.cpp | 125 | ||||
-rw-r--r-- | build/clang-plugin/alpha/tests/TestTempRefPtr.cpp | 52 | ||||
-rw-r--r-- | build/clang-plugin/alpha/tests/sources.mozbuild | 11 |
10 files changed, 433 insertions, 0 deletions
diff --git a/build/clang-plugin/alpha/AlphaChecks.inc b/build/clang-plugin/alpha/AlphaChecks.inc new file mode 100644 index 0000000000..4e0d050edd --- /dev/null +++ b/build/clang-plugin/alpha/AlphaChecks.inc @@ -0,0 +1,10 @@ +/* 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/. */ + +// The list of checker classes that are compatible with clang-tidy and are considered +// to be in alpha stage development. + +// CHECK(AlphaChecker, "alpha-checker") +CHECK(NonStdMoveChecker, "non-std-move") +CHECK(TempRefPtrChecker, "performance-temp-refptr") diff --git a/build/clang-plugin/alpha/AlphaIncludes.inc b/build/clang-plugin/alpha/AlphaIncludes.inc new file mode 100644 index 0000000000..376c35b878 --- /dev/null +++ b/build/clang-plugin/alpha/AlphaIncludes.inc @@ -0,0 +1,2 @@ +#include "NonStdMoveChecker.h" +#include "TempRefPtrChecker.h" diff --git a/build/clang-plugin/alpha/NonStdMoveChecker.cpp b/build/clang-plugin/alpha/NonStdMoveChecker.cpp new file mode 100644 index 0000000000..e9ede00cea --- /dev/null +++ b/build/clang-plugin/alpha/NonStdMoveChecker.cpp @@ -0,0 +1,115 @@ +/* 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 "NonStdMoveChecker.h" +#include "CustomMatchers.h" +#include "clang/Lex/Lexer.h" + +constexpr const char *kConstructExpr = "construct"; +constexpr const char *kOperatorCallExpr = "operator-call"; +constexpr const char *kSourceExpr = "source-expr"; +constexpr const char *kMaterializeExpr = "materialize-expr"; + +void NonStdMoveChecker::registerMatchers(MatchFinder *AstMatcher) { + + // Assignment through forget + AstMatcher->addMatcher( + cxxOperatorCallExpr( + hasOverloadedOperatorName("="), + hasAnyArgument(materializeTemporaryExpr( + has(cxxBindTemporaryExpr(has(cxxMemberCallExpr( + has(memberExpr(member(hasName("forget")))), + on(expr().bind(kSourceExpr))))))) + .bind(kMaterializeExpr))) + .bind(kOperatorCallExpr), + this); + + // Construction through forget + + AstMatcher->addMatcher( + cxxConstructExpr(has(materializeTemporaryExpr( + has(cxxBindTemporaryExpr(has(cxxMemberCallExpr( + has(memberExpr(member(hasName("forget")))), + on(expr().bind(kSourceExpr))))))) + .bind(kMaterializeExpr))) + .bind(kConstructExpr), + this); +} + +#if CLANG_VERSION_FULL >= 1600 +std::optional<FixItHint> +#else +Optional<FixItHint> +#endif +NonStdMoveChecker::makeFixItHint(const MatchFinder::MatchResult &Result, + const Expr *const TargetExpr) { + const auto *MaterializeExpr = Result.Nodes.getNodeAs<Expr>(kMaterializeExpr); + + // TODO: In principle, we should check here if TargetExpr if + // assignable/constructible from std::move(SourceExpr). Not sure how to do + // this. Currently, we only filter out the case where the targetTypeTemplate + // is already_AddRefed, where this is known to fail. + + const auto *targetTypeTemplate = getNonTemplateSpecializedCXXRecordDecl( + TargetExpr->getType().getCanonicalType()); + const auto *sourceTypeTemplate = getNonTemplateSpecializedCXXRecordDecl( + MaterializeExpr->getType().getCanonicalType()); + + if (targetTypeTemplate && sourceTypeTemplate) { + // TODO is there a better way to check this than by name? otherwise, the + // names probably are necessarily unique in the scope + if (targetTypeTemplate->getName() == sourceTypeTemplate->getName() && + targetTypeTemplate->getName() == "already_AddRefed") { + return {}; + } + } + + const auto *SourceExpr = Result.Nodes.getNodeAs<Expr>(kSourceExpr); + + const auto sourceText = Lexer::getSourceText( + CharSourceRange::getTokenRange(SourceExpr->getSourceRange()), + Result.Context->getSourceManager(), Result.Context->getLangOpts()); + + return FixItHint::CreateReplacement(MaterializeExpr->getSourceRange(), + ("std::move(" + sourceText + ")").str()); +} + +void NonStdMoveChecker::check(const MatchFinder::MatchResult &Result) { + // TODO: Include source and target type name in messages. + + const auto *OCE = + Result.Nodes.getNodeAs<CXXOperatorCallExpr>(kOperatorCallExpr); + + if (OCE) { + const auto *refPtrDecl = + dyn_cast<const CXXRecordDecl>(OCE->getCalleeDecl()->getDeclContext()); + + const auto XFixItHint = makeFixItHint(Result, OCE); + // TODO: produce diagnostic but no FixItHint in this case? + if (XFixItHint) { + diag(OCE->getBeginLoc(), "non-standard move assignment to %0 obscures " + "move, use std::move instead") + << refPtrDecl << *XFixItHint; + } + } + + const auto *CoE = Result.Nodes.getNodeAs<CXXConstructExpr>(kConstructExpr); + + if (CoE) { + const auto *refPtrDecl = + dyn_cast<const CXXRecordDecl>(CoE->getConstructor()->getDeclContext()); + + const auto XFixItHint = makeFixItHint(Result, CoE); + // TODO: produce diagnostic but no FixItHint in this case? + if (XFixItHint) { + diag(CoE->getBeginLoc(), "non-standard move construction of %0 obscures " + "move, use std::move instead") + << refPtrDecl << *XFixItHint; + } + } + + // TODO: What about swap calls immediately after default-construction? These + // can also be replaced by move-construction, but this may require + // control-flow analysis. +} diff --git a/build/clang-plugin/alpha/NonStdMoveChecker.h b/build/clang-plugin/alpha/NonStdMoveChecker.h new file mode 100644 index 0000000000..bb0565cdf0 --- /dev/null +++ b/build/clang-plugin/alpha/NonStdMoveChecker.h @@ -0,0 +1,29 @@ +/* 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 NonStdMoveChecker_h__ +#define NonStdMoveChecker_h__ + +#include "plugin.h" + +class NonStdMoveChecker final : public BaseCheck { +public: + NonStdMoveChecker(StringRef CheckName, ContextType *Context = nullptr) + : BaseCheck(CheckName, Context) {} + void registerMatchers(MatchFinder *AstMatcher) override; + void check(const MatchFinder::MatchResult &Result) override; + +private: + CompilerInstance *CI; + + static +#if CLANG_VERSION_FULL >= 1600 + std::optional<FixItHint> +#else + Optional<FixItHint> +#endif + makeFixItHint(const MatchFinder::MatchResult &Result, const Expr *TargetExpr); +}; + +#endif diff --git a/build/clang-plugin/alpha/TempRefPtrChecker.cpp b/build/clang-plugin/alpha/TempRefPtrChecker.cpp new file mode 100644 index 0000000000..0a4d078368 --- /dev/null +++ b/build/clang-plugin/alpha/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; + } + } +} diff --git a/build/clang-plugin/alpha/TempRefPtrChecker.h b/build/clang-plugin/alpha/TempRefPtrChecker.h new file mode 100644 index 0000000000..ebed50c3a0 --- /dev/null +++ b/build/clang-plugin/alpha/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 diff --git a/build/clang-plugin/alpha/sources.mozbuild b/build/clang-plugin/alpha/sources.mozbuild new file mode 100644 index 0000000000..616078639f --- /dev/null +++ b/build/clang-plugin/alpha/sources.mozbuild @@ -0,0 +1,11 @@ +# -*- Mode: python; indent-tabs-mode: nil; tab-width: 40 -*- +# vim: set filetype=python: +# 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/. + +HOST_SOURCES += [ + # 'AlphaChecker.cpp', + "NonStdMoveChecker.cpp", + 'TempRefPtrChecker.cpp', +] diff --git a/build/clang-plugin/alpha/tests/TestNonStdMove.cpp b/build/clang-plugin/alpha/tests/TestNonStdMove.cpp new file mode 100644 index 0000000000..379f9655dd --- /dev/null +++ b/build/clang-plugin/alpha/tests/TestNonStdMove.cpp @@ -0,0 +1,125 @@ +#include <mozilla/RefPtr.h> + +// we can't include nsCOMPtr.h here, so let's redefine a basic version +template<typename T> +struct nsCOMPtr { + nsCOMPtr() = default; + + template<typename U> + MOZ_IMPLICIT nsCOMPtr(already_AddRefed<U>&& aSrc); + + template<typename U> + nsCOMPtr& operator=(already_AddRefed<U>&& aSrc); +}; + + +using namespace mozilla; + +struct RefCountedBase { + void AddRef(); + void Release(); + + void method_test(); +}; + +struct RefCountedDerived : RefCountedBase {}; + +struct RefCountedBaseHolder { + RefPtr<RefCountedBase> GetRefCountedBase() const { + return mRefCountedBase; + } + +private: + RefPtr<RefCountedBase> mRefCountedBase = MakeRefPtr<RefCountedBase>(); +}; + + +void test_assign_same_type() { + RefPtr<RefCountedBase> a = MakeRefPtr<RefCountedBase>(); + RefPtr<RefCountedBase> b; + + b = a.forget(); // expected-warning {{non-standard move assignment}} +} + +void test_assign_implicit_cast() { + RefPtr<RefCountedDerived> a = MakeRefPtr<RefCountedDerived>(); + RefPtr<RefCountedBase> b; + + b = a.forget(); // expected-warning {{non-standard move assignment}} +} + +void test_assign_different_template() { + RefPtr<RefCountedDerived> a = MakeRefPtr<RefCountedDerived>(); + nsCOMPtr<RefCountedBase> b; + + b = a.forget(); // expected-warning {{non-standard move assignment}} +} + +void test_construct_different_template() { + RefPtr<RefCountedDerived> a = MakeRefPtr<RefCountedDerived>(); + nsCOMPtr<RefCountedBase> b = a.forget(); // expected-warning {{non-standard move construction}} +} + +void test_assign_already_addrefed() { + RefPtr<RefCountedDerived> a = MakeRefPtr<RefCountedDerived>(); + already_AddRefed<RefCountedDerived> b; + + b = a.forget(); +} + +void test_construct_already_addrefed() { + RefPtr<RefCountedDerived> a = MakeRefPtr<RefCountedDerived>(); + already_AddRefed<RefCountedDerived> b = a.forget(); +} + +void test_construct_same_type() { + RefPtr<RefCountedBase> a = MakeRefPtr<RefCountedBase>(); + RefPtr<RefCountedBase> b = a.forget(); // expected-warning {{non-standard move construction}} +} + +void test_construct_implicit_cast() { + RefPtr<RefCountedDerived> a = MakeRefPtr<RefCountedDerived>(); + RefPtr<RefCountedBase> b = a.forget(); // expected-warning {{non-standard move construction}} +} + +void test_construct_brace_same_type() { + RefPtr<RefCountedBase> a = MakeRefPtr<RefCountedBase>(); + auto b = RefPtr<RefCountedBase>{a.forget()}; // expected-warning {{non-standard move construction}} +} + +void test_construct_brace_implicit_cast() { + RefPtr<RefCountedDerived> a = MakeRefPtr<RefCountedDerived>(); + auto b = RefPtr<RefCountedBase>{a.forget()}; // expected-warning {{non-standard move construction}} +} + +void test_construct_function_style_same_type() { + RefPtr<RefCountedBase> a = MakeRefPtr<RefCountedBase>(); + auto b = RefPtr<RefCountedBase>(a.forget()); // expected-warning {{non-standard move construction}} +} + +void test_construct_function_style_implicit_cast() { + RefPtr<RefCountedDerived> a = MakeRefPtr<RefCountedDerived>(); + auto b = RefPtr<RefCountedBase>(a.forget()); // expected-warning {{non-standard move construction}} +} + +void test_construct_result_type() { + RefPtr<RefCountedBase> a = MakeRefPtr<RefCountedBase>(); + already_AddRefed<RefCountedBase> b = a.forget(); +} + +void test_construct_implicitly_cast_result_type() { + RefPtr<RefCountedDerived> a = MakeRefPtr<RefCountedDerived>(); + already_AddRefed<RefCountedBase> b = a.forget(); +} + +void foo(already_AddRefed<RefCountedBase>&& aArg); + +void test_call_with_result_type() { + RefPtr<RefCountedBase> a = MakeRefPtr<RefCountedBase>(); + foo(a.forget()); +} + +void test_call_with_implicitly_cast_result_type() { + RefPtr<RefCountedDerived> a = MakeRefPtr<RefCountedDerived>(); + foo(a.forget()); +} diff --git a/build/clang-plugin/alpha/tests/TestTempRefPtr.cpp b/build/clang-plugin/alpha/tests/TestTempRefPtr.cpp new file mode 100644 index 0000000000..51f756b8e6 --- /dev/null +++ b/build/clang-plugin/alpha/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())); +} diff --git a/build/clang-plugin/alpha/tests/sources.mozbuild b/build/clang-plugin/alpha/tests/sources.mozbuild new file mode 100644 index 0000000000..b72cacd253 --- /dev/null +++ b/build/clang-plugin/alpha/tests/sources.mozbuild @@ -0,0 +1,11 @@ +# -*- Mode: python; indent-tabs-mode: nil; tab-width: 40 -*- +# vim: set filetype=python: +# 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/. + +SOURCES += [ + # 'AlphaTest.cpp', + "TestNonStdMove.cpp", + 'TestTempRefPtr.cpp', +] |