From 02f7ec8f63395f81c5318f1f5011ef894f261c53 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Emilio=20Cobos=20=C3=81lvarez?= Date: Tue, 6 Feb 2024 08:51:50 +0000 Subject: [PATCH] Bug 1878108 - Replace custom MOZ_LIFETIME_BOUND with built-in. r=nika,glandium The built-in version is better as it also allows annotating particular parameters (it not only applies to method declarations). Differential Revision: https://phabricator.services.mozilla.com/D200432 --- build/clang-plugin/Checks.inc | 1 - build/clang-plugin/ChecksIncludes.inc | 1 - build/clang-plugin/CustomAttributes.inc | 1 - build/clang-plugin/CustomMatchers.h | 5 - .../TemporaryLifetimeBoundChecker.cpp | 91 ------------- .../TemporaryLifetimeBoundChecker.h | 22 --- build/clang-plugin/moz.build | 1 - .../tests/TestTemporaryLifetimeBound.cpp | 126 ------------------ build/clang-plugin/tests/moz.build | 1 - mfbt/Attributes.h | 22 ++- xpcom/string/nsTLiteralString.h | 4 +- 11 files changed, 19 insertions(+), 256 deletions(-) delete mode 100644 build/clang-plugin/TemporaryLifetimeBoundChecker.cpp delete mode 100644 build/clang-plugin/TemporaryLifetimeBoundChecker.h delete mode 100644 build/clang-plugin/tests/TestTemporaryLifetimeBound.cpp diff --git a/build/clang-plugin/Checks.inc b/build/clang-plugin/Checks.inc index a62548e60dee..97c77547ac64 100644 --- a/build/clang-plugin/Checks.inc +++ b/build/clang-plugin/Checks.inc @@ -40,6 +40,5 @@ CHECK(RefCountedInsideLambdaChecker, "refcounted-inside-lambda") CHECK(RefCountedThisInsideConstructorChecker, "refcount-within-constructor") CHECK(ScopeChecker, "scope") CHECK(SprintfLiteralChecker, "sprintf-literal") -CHECK(TemporaryLifetimeBoundChecker, "temporary-lifetime-bound") CHECK(TrivialCtorDtorChecker, "trivial-constructor-destructor") CHECK(TrivialDtorChecker, "trivial-destructor") diff --git a/build/clang-plugin/ChecksIncludes.inc b/build/clang-plugin/ChecksIncludes.inc index 2127d21a017b..d029d6ea3038 100644 --- a/build/clang-plugin/ChecksIncludes.inc +++ b/build/clang-plugin/ChecksIncludes.inc @@ -41,6 +41,5 @@ #include "RefCountedThisInsideConstructorChecker.h" #include "ScopeChecker.h" #include "SprintfLiteralChecker.h" -#include "TemporaryLifetimeBoundChecker.h" #include "TrivialCtorDtorChecker.h" #include "TrivialDtorChecker.h" diff --git a/build/clang-plugin/CustomAttributes.inc b/build/clang-plugin/CustomAttributes.inc index 7ff21d499eeb..883676e9c582 100644 --- a/build/clang-plugin/CustomAttributes.inc +++ b/build/clang-plugin/CustomAttributes.inc @@ -27,6 +27,5 @@ ATTR(moz_required_base_method) ATTR(moz_stack_class) ATTR(moz_static_local_class) ATTR(moz_temporary_class) -ATTR(moz_lifetime_bound) ATTR(moz_trivial_ctor_dtor) ATTR(moz_trivial_dtor) diff --git a/build/clang-plugin/CustomMatchers.h b/build/clang-plugin/CustomMatchers.h index ece717e79918..97caf4d59cec 100644 --- a/build/clang-plugin/CustomMatchers.h +++ b/build/clang-plugin/CustomMatchers.h @@ -428,11 +428,6 @@ AST_MATCHER(FunctionDecl, isMozMustReturnFromCaller) { hasCustomAttribute(Decl); } -AST_MATCHER(FunctionDecl, isMozTemporaryLifetimeBound) { - const FunctionDecl *Decl = Node.getCanonicalDecl(); - return Decl && hasCustomAttribute(Decl); -} - /// This matcher will select default args which have nullptr as the value. AST_MATCHER(CXXDefaultArgExpr, isNullDefaultArg) { const Expr *Expr = Node.getExpr(); diff --git a/build/clang-plugin/TemporaryLifetimeBoundChecker.cpp b/build/clang-plugin/TemporaryLifetimeBoundChecker.cpp deleted file mode 100644 index dc66f62b0d3a..000000000000 --- a/build/clang-plugin/TemporaryLifetimeBoundChecker.cpp +++ /dev/null @@ -1,91 +0,0 @@ -/* 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 "TemporaryLifetimeBoundChecker.h" -#include "CustomMatchers.h" -#include "clang/Lex/Lexer.h" - -void TemporaryLifetimeBoundChecker::registerMatchers(MatchFinder *AstMatcher) { - // Look for a call to a MOZ_LIFETIME_BOUND member function - auto isTemporaryLifetimeBoundCall = - cxxMemberCallExpr( - onImplicitObjectArgument(anyOf(has(cxxTemporaryObjectExpr()), - has(materializeTemporaryExpr()))), - callee(functionDecl(isMozTemporaryLifetimeBound()))) - .bind("call"); - - // XXX This definitely does not catch everything relevant. In particular, the - // matching on conditionalOperator would need to be recursive. But it's a - // start. - auto hasTemporaryLifetimeBoundCall = - anyOf(isTemporaryLifetimeBoundCall, - conditionalOperator( - anyOf(hasFalseExpression(isTemporaryLifetimeBoundCall), - hasTrueExpression(isTemporaryLifetimeBoundCall)))); - - AstMatcher->addMatcher( - returnStmt(hasReturnValue( - allOf(exprWithCleanups().bind("expr-with-cleanups"), - ignoringParenCasts(hasTemporaryLifetimeBoundCall)))) - .bind("return-stmt"), - this); - - AstMatcher->addMatcher( - varDecl(hasType(references(cxxRecordDecl())), - hasInitializer( - allOf(exprWithCleanups(), - ignoringParenCasts(hasTemporaryLifetimeBoundCall)))) - .bind("var-decl"), - this); -} - -void TemporaryLifetimeBoundChecker::check( - const MatchFinder::MatchResult &Result) { - const auto *Call = Result.Nodes.getNodeAs("call"); - const auto *ReturnStatement = - Result.Nodes.getNodeAs("return-stmt"); - const auto *ReferenceVarDecl = Result.Nodes.getNodeAs("var-decl"); - - const char ErrorReturn[] = - "cannot return result of lifetime-bound function %0 on " - "temporary of type %1"; - - const char ErrorBindToReference[] = - "cannot bind result of lifetime-bound function %0 on " - "temporary of type %1 to reference, does not extend lifetime"; - - const char NoteCalledFunction[] = "member function declared here"; - - // We are either a return statement... - if (ReturnStatement) { - const auto *ExprWithCleanups = - Result.Nodes.getNodeAs("expr-with-cleanups"); - if (!ExprWithCleanups->isLValue()) { - return; - } - - const auto Range = ReturnStatement->getSourceRange(); - - diag(Range.getBegin(), ErrorReturn, DiagnosticIDs::Error) - << Range << Call->getMethodDecl() - << Call->getImplicitObjectArgument() - ->getType() - .withoutLocalFastQualifiers(); - } - - // ... or a variable declaration that declare a reference - if (ReferenceVarDecl) { - const auto Range = ReferenceVarDecl->getSourceRange(); - - diag(Range.getBegin(), ErrorBindToReference, DiagnosticIDs::Error) - << Range << Call->getMethodDecl() - << Call->getImplicitObjectArgument() - ->getType() - .withoutLocalFastQualifiers(); - } - - const auto *MethodDecl = Call->getMethodDecl(); - diag(MethodDecl->getCanonicalDecl()->getLocation(), NoteCalledFunction, - DiagnosticIDs::Note); -} diff --git a/build/clang-plugin/TemporaryLifetimeBoundChecker.h b/build/clang-plugin/TemporaryLifetimeBoundChecker.h deleted file mode 100644 index 712c0de9c013..000000000000 --- a/build/clang-plugin/TemporaryLifetimeBoundChecker.h +++ /dev/null @@ -1,22 +0,0 @@ -/* 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 TemporaryLifetimeBoundChecker_h__ -#define TemporaryLifetimeBoundChecker_h__ - -#include "plugin.h" - -class TemporaryLifetimeBoundChecker : public BaseCheck { -public: - TemporaryLifetimeBoundChecker(StringRef CheckName, - ContextType *Context = nullptr) - : BaseCheck(CheckName, Context) {} - - void registerMatchers(MatchFinder *AstMatcher) override; - void check(const MatchFinder::MatchResult &Result) override; - -private: -}; - -#endif diff --git a/build/clang-plugin/moz.build b/build/clang-plugin/moz.build index a0ac90706f04..c9c2c520db69 100644 --- a/build/clang-plugin/moz.build +++ b/build/clang-plugin/moz.build @@ -45,7 +45,6 @@ HOST_SOURCES += [ "RefCountedThisInsideConstructorChecker.cpp", "ScopeChecker.cpp", "SprintfLiteralChecker.cpp", - "TemporaryLifetimeBoundChecker.cpp", "TrivialCtorDtorChecker.cpp", "TrivialDtorChecker.cpp", "VariableUsageHelpers.cpp", diff --git a/build/clang-plugin/tests/TestTemporaryLifetimeBound.cpp b/build/clang-plugin/tests/TestTemporaryLifetimeBound.cpp deleted file mode 100644 index 0c51182cabd9..000000000000 --- a/build/clang-plugin/tests/TestTemporaryLifetimeBound.cpp +++ /dev/null @@ -1,126 +0,0 @@ -#define MOZ_LIFETIME_BOUND __attribute__((annotate("moz_lifetime_bound"))) - -struct Foo {}; - -struct Bar { - MOZ_LIFETIME_BOUND const Foo &AsFoo() const; // expected-note {{member function declared here}} expected-note {{member function declared here}} expected-note {{member function declared here}} expected-note {{member function declared here}} expected-note {{member function declared here}} - MOZ_LIFETIME_BOUND operator const Foo &() const; // expected-note {{member function declared here}} expected-note {{member function declared here}} expected-note {{member function declared here}} expected-note {{member function declared here}} expected-note {{member function declared here}} expected-note {{member function declared here}} -}; - -Bar MakeBar() { return Bar(); } - -Bar testReturnsInstance_Constructed() { return Bar(); } - -const Foo &testReturnsReference_Static() { - static constexpr auto bar = Bar{}; - return bar; -} - -/* TODO This is bad as well... but not related to a temporary. -const Foo& testReturnsReference_Local() { - constexpr auto bar = Bar{}; - return bar; -} -*/ - -const Foo &testReturnsReferenceToTemporaryViaLifetimeBound_Constructed() { - return Bar(); // expected-error {{cannot return result of lifetime-bound function 'operator const Foo &' on temporary of type 'Bar'}} -} - -const Foo &testReturnsReferenceToTemporaryViaLifetimeBound2_Constructed() { - return static_cast(Bar()); // expected-error {{cannot return result of lifetime-bound function 'operator const Foo &' on temporary of type 'Bar'}} -} - -const Foo &testReturnsReferenceToTemporaryViaLifetimeBound3_Constructed() { - return Bar().AsFoo(); // expected-error {{cannot return result of lifetime-bound function 'AsFoo' on temporary of type 'Bar'}} -} - -const Foo & -testReturnsReferenceToTemporaryViaLifetimeBound4_Constructed(bool aCond) { - static constexpr Foo foo; - return aCond ? foo : Bar().AsFoo(); // expected-error {{cannot return result of lifetime-bound function 'AsFoo' on temporary of type 'Bar'}} -} - -Foo testReturnsValueViaLifetimeBoundFunction_Constructed() { return Bar(); } - -Foo testReturnsValueViaLifetimeBoundFunction2_Constructed() { - return static_cast(Bar()); -} - -Foo testReturnsValueViaLifetimeBoundFunction3_Constructed() { - return Bar().AsFoo(); -} - -Bar testReturnInstance_Returned() { return MakeBar(); } - -const Foo &testReturnsReferenceToTemporaryViaLifetimeBound_Returned() { - return MakeBar(); // expected-error {{cannot return result of lifetime-bound function 'operator const Foo &' on temporary of type 'Bar'}} -} - -const Foo &testReturnsReferenceToTemporaryViaLifetimeBound2_Returned() { - return static_cast(MakeBar()); // expected-error {{cannot return result of lifetime-bound function 'operator const Foo &' on temporary of type 'Bar'}} -} - -const Foo &testReturnsReferenceToTemporaryViaLifetimeBound3_Returned() { - return MakeBar().AsFoo(); // expected-error {{cannot return result of lifetime-bound function 'AsFoo' on temporary of type 'Bar'}} -} - -Foo testReturnsValueViaLifetimeBoundFunction_Returned() { return MakeBar(); } - -Foo testReturnsValueViaLifetimeBoundFunction2_Returned() { - return static_cast(MakeBar()); -} - -Foo testReturnsValueViaLifetimeBoundFunction3_Returned() { - return MakeBar().AsFoo(); -} - -void testNoLifetimeExtension() { - const Foo &foo = Bar(); // expected-error {{cannot bind result of lifetime-bound function 'operator const Foo &' on temporary of type 'Bar' to reference, does not extend lifetime}} -} - -void testNoLifetimeExtension2() { - const auto &foo = static_cast(MakeBar()); // expected-error {{cannot bind result of lifetime-bound function 'operator const Foo &' on temporary of type 'Bar' to reference, does not extend lifetime}} -} - -void testNoLifetimeExtension3() { - const Foo &foo = Bar().AsFoo(); // expected-error {{cannot bind result of lifetime-bound function 'AsFoo' on temporary of type 'Bar' to reference, does not extend lifetime}} -} - -void testNoLifetimeExtension4(bool arg) { - const Foo foo; - const Foo &fooRef = arg ? foo : Bar().AsFoo(); // expected-error {{cannot bind result of lifetime-bound function 'AsFoo' on temporary of type 'Bar' to reference, does not extend lifetime}} -} - -// While this looks similar to testNoLifetimeExtension4, this is actually fine, -// as the coerced type of the conditional operator is `Foo` here rather than -// `const Foo&`, and thus an implicit copy of `Bar().AsFoo()` is created, whose -// lifetime is actually extended. -void testLifetimeExtension(bool arg) { - const Foo &foo = arg ? Foo() : Bar().AsFoo(); -} - -void testConvertToValue() { const Foo foo = Bar(); } - -Foo testReturnConvertToValue() { - return static_cast(Bar()); -} - -void FooFunc(const Foo &aFoo); - -// We want to allow binding to parameters of the target reference type though. -// This is the very reason the annotation is required, and the function cannot -// be restricted to lvalues. Lifetime is not an issue here, as the temporary's -// lifetime is until the end of the full expression anyway. - -void testBindToParameter() { - FooFunc(Bar()); - FooFunc(static_cast(Bar())); - FooFunc(Bar().AsFoo()); - FooFunc(MakeBar()); -} - -// This should be OK, because the return value isn't necessarily coming from the -// argument (and it should be OK for any type). -const Foo &RandomFunctionCall(const Foo &aFoo); -const Foo &testReturnFunctionCall() { return RandomFunctionCall(Bar()); } diff --git a/build/clang-plugin/tests/moz.build b/build/clang-plugin/tests/moz.build index b48a95131d04..45f416be4440 100644 --- a/build/clang-plugin/tests/moz.build +++ b/build/clang-plugin/tests/moz.build @@ -51,7 +51,6 @@ SOURCES += [ "TestStackClass.cpp", "TestStaticLocalClass.cpp", "TestTemporaryClass.cpp", - "TestTemporaryLifetimeBound.cpp", "TestTrivialCtorDtor.cpp", "TestTrivialDtor.cpp", ] diff --git a/mfbt/Attributes.h b/mfbt/Attributes.h index 5af718ebb122..efcfdd6ca1cb 100644 --- a/mfbt/Attributes.h +++ b/mfbt/Attributes.h @@ -446,6 +446,23 @@ # define MOZ_NO_STACK_PROTECTOR /* no support */ #endif +/** + * MOZ_LIFETIME_BOUND indicates that objects that are referred to by that + * parameter may also be referred to by the return value of the annotated + * function (or, for a parameter of a constructor, by the value of the + * constructed object). + * See: https://clang.llvm.org/docs/AttributeReference.html#lifetimebound + */ +#ifdef __has_cpp_attribute +# if __has_cpp_attribute(clang::lifetimebound) +# define MOZ_LIFETIME_BOUND [[clang::lifetimebound]] +# else +# define MOZ_LIFETIME_BOUND /* nothing */ +# endif +#else +# define MOZ_LIFETIME_BOUND /* nothing */ +#endif + #ifdef __cplusplus /** @@ -745,9 +762,6 @@ * MOZ_MAY_CALL_AFTER_MUST_RETURN: Applies to function or method declarations. * Calls to these methods may be made in functions after calls a * MOZ_MUST_RETURN_FROM_CALLER_IF_THIS_IS_ARG method. - * MOZ_LIFETIME_BOUND: Applies to method declarations. - * The result of calling these functions on temporaries may not be returned as - * a reference or bound to a reference variable. * MOZ_UNANNOTATED/MOZ_ANNOTATED: Applies to Mutexes/Monitors and variations on * them. MOZ_UNANNOTATED indicates that the Mutex/Monitor/etc hasn't been * examined and annotated using macros from mfbt/ThreadSafety -- @@ -838,7 +852,6 @@ __attribute__((annotate("moz_must_return_from_caller_if_this_is_arg"))) # define MOZ_MAY_CALL_AFTER_MUST_RETURN \ __attribute__((annotate("moz_may_call_after_must_return"))) -# define MOZ_LIFETIME_BOUND __attribute__((annotate("moz_lifetime_bound"))) # define MOZ_KNOWN_LIVE __attribute__((annotate("moz_known_live"))) # ifndef XGILL_PLUGIN # define MOZ_UNANNOTATED __attribute__((annotate("moz_unannotated"))) @@ -898,7 +911,6 @@ # define MOZ_REQUIRED_BASE_METHOD /* nothing */ # define MOZ_MUST_RETURN_FROM_CALLER_IF_THIS_IS_ARG /* nothing */ # define MOZ_MAY_CALL_AFTER_MUST_RETURN /* nothing */ -# define MOZ_LIFETIME_BOUND /* nothing */ # define MOZ_KNOWN_LIVE /* nothing */ # define MOZ_UNANNOTATED /* nothing */ # define MOZ_ANNOTATED /* nothing */ diff --git a/xpcom/string/nsTLiteralString.h b/xpcom/string/nsTLiteralString.h index b233183a73f1..38ffd32bdbdf 100644 --- a/xpcom/string/nsTLiteralString.h +++ b/xpcom/string/nsTLiteralString.h @@ -56,11 +56,11 @@ class nsTLiteralString : public mozilla::detail::nsTStringRepr { * Use sparingly. If possible, rewrite code to use const ns[C]String& * and the implicit cast will just work. */ - MOZ_LIFETIME_BOUND const nsTString& AsString() const { + const nsTString& AsString() const MOZ_LIFETIME_BOUND { return *reinterpret_cast*>(this); } - MOZ_LIFETIME_BOUND operator const nsTString&() const { return AsString(); } + operator const nsTString&() const MOZ_LIFETIME_BOUND { return AsString(); } template struct raw_type {