From eec5191498f88e908540ed2103fc6b3337a0685c Mon Sep 17 00:00:00 2001 From: serge-sans-paille Date: Wed, 30 Oct 2024 11:05:25 +0000 Subject: [PATCH] Bug 1920717 - Add static checker for global variables with runtime initialisation r=glandium This patch both: 1. Provides a static checker to detect global variables which may not be initialized at compile-time 2. Verify that variables flagged as MOZ_RUNINIT are indeed initialized at runtime 3. In case of variables whose initialisation status varies based on macro definition or template parameters, just flag them as MOZ_GLOBINIT. Differential Revision: https://phabricator.services.mozilla.com/D223342 --- build/clang-plugin/Checks.inc | 1 + build/clang-plugin/ChecksIncludes.inc | 1 + build/clang-plugin/CustomAttributes.inc | 2 + build/clang-plugin/CustomMatchers.h | 36 +++++++++++ .../GlobalVariableInitializationChecker.cpp | 58 ++++++++++++++++++ .../GlobalVariableInitializationChecker.h | 18 ++++++ build/clang-plugin/moz.build | 1 + .../TestGlobalVariableInitialization.cpp | 59 +++++++++++++++++++ build/clang-plugin/tests/TestHeapClass.cpp | 4 +- .../tests/TestKungFuDeathGrip.cpp | 3 +- build/clang-plugin/tests/TestNoAutoType.cpp | 5 +- .../tests/TestNonMemMovableStd.cpp | 11 ++-- .../tests/TestNonTemporaryClass.cpp | 5 +- .../clang-plugin/tests/TestTemporaryClass.cpp | 4 +- build/clang-plugin/tests/moz.build | 1 + .../coding-style/coding_style_cpp.rst | 7 ++- widget/android/ImageDecoderSupport.cpp | 4 +- 17 files changed, 202 insertions(+), 18 deletions(-) create mode 100644 build/clang-plugin/GlobalVariableInitializationChecker.cpp create mode 100644 build/clang-plugin/GlobalVariableInitializationChecker.h create mode 100644 build/clang-plugin/tests/TestGlobalVariableInitialization.cpp diff --git a/build/clang-plugin/Checks.inc b/build/clang-plugin/Checks.inc index 97c77547ac64..9c899cb4c36e 100644 --- a/build/clang-plugin/Checks.inc +++ b/build/clang-plugin/Checks.inc @@ -10,6 +10,7 @@ CHECK(CanRunScriptChecker, "can-run-script") CHECK(DanglingOnTemporaryChecker, "dangling-on-temporary") CHECK(ExplicitImplicitChecker, "implicit-constructor") CHECK(ExplicitOperatorBoolChecker, "explicit-operator-bool") +CHECK(GlobalVariableInitializationChecker, "global-variable-init") CHECK(JSHandleRootedTypedefChecker, "js-handle-rooted-typedef") CHECK(KungFuDeathGripChecker, "kungfu-death-grip") CHECK(KnownLiveChecker, "known-live") diff --git a/build/clang-plugin/ChecksIncludes.inc b/build/clang-plugin/ChecksIncludes.inc index d029d6ea3038..5d71af62b8a0 100644 --- a/build/clang-plugin/ChecksIncludes.inc +++ b/build/clang-plugin/ChecksIncludes.inc @@ -15,6 +15,7 @@ #include "LoadLibraryUsageChecker.h" #include "FopenUsageChecker.h" #endif +#include "GlobalVariableInitializationChecker.h" #include "JSHandleRootedTypedefChecker.h" #include "KungFuDeathGripChecker.h" #include "KnownLiveChecker.h" diff --git a/build/clang-plugin/CustomAttributes.inc b/build/clang-plugin/CustomAttributes.inc index 883676e9c582..1b819597a317 100644 --- a/build/clang-plugin/CustomAttributes.inc +++ b/build/clang-plugin/CustomAttributes.inc @@ -2,6 +2,8 @@ ATTR(moz_allow_temporary) ATTR(moz_can_run_script) ATTR(moz_can_run_script_for_definition) ATTR(moz_can_run_script_boundary) +ATTR(moz_global_var) +ATTR(moz_generated) ATTR(moz_global_class) ATTR(moz_heap_allocator) ATTR(moz_heap_class) diff --git a/build/clang-plugin/CustomMatchers.h b/build/clang-plugin/CustomMatchers.h index 2ba1621dde94..a6a9fe043c07 100644 --- a/build/clang-plugin/CustomMatchers.h +++ b/build/clang-plugin/CustomMatchers.h @@ -16,6 +16,31 @@ namespace clang { namespace ast_matchers { +/// This matcher will match any function declaration that is declared as a heap +/// allocator. +AST_MATCHER(VarDecl, hasMozGlobalType) { + if(auto * TD = Node.getType().getTypePtr()->getAsTagDecl()) { + if(hasCustomAttribute(TD)) + return true; + } + return false; +} + +/// Match any variable declared with the MOZ_RUNINIT qualifier. +AST_MATCHER(VarDecl, isMozGlobal) { + return hasCustomAttribute(&Node); +} + +/// Match any variable declared with the MOZ_GLOBINIT qualifier. +AST_MATCHER(VarDecl, isMozGenerated) { + return hasCustomAttribute(&Node); +} + +/// Match any variable declared with the constinit qualifier. +AST_MATCHER(VarDecl, hasConstInitAttr) { + return Node.hasAttr(); +} + /// This matcher will match any function declaration that is declared as a heap /// allocator. AST_MATCHER(FunctionDecl, heapAllocator) { @@ -71,6 +96,17 @@ AST_MATCHER(DeclaratorDecl, isNotSpiderMonkey) { Path.find("XPC") == std::string::npos; } +/// This matcher will match any declaration with an initializer that's +/// considered as constant by the compiler. +AST_MATCHER(VarDecl, hasConstantInitializer) { + if(Node.hasInit()) + return Node.getInit()->isConstantInitializer( + Finder->getASTContext(), + Node.getType()->isReferenceType()); + else + return Node.getType().isTrivialType(Finder->getASTContext()); +} + /// This matcher will match temporary expressions. /// We need this matcher for compatibility with clang 3.* (clang 4 and above /// insert a MaterializeTemporaryExpr everywhere). diff --git a/build/clang-plugin/GlobalVariableInitializationChecker.cpp b/build/clang-plugin/GlobalVariableInitializationChecker.cpp new file mode 100644 index 000000000000..cae2c4addbf4 --- /dev/null +++ b/build/clang-plugin/GlobalVariableInitializationChecker.cpp @@ -0,0 +1,58 @@ +/* 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 "GlobalVariableInitializationChecker.h" +#include "CustomMatchers.h" + +void GlobalVariableInitializationChecker::registerMatchers(MatchFinder *AstMatcher) { + auto FirstPartyGlobalVariable = varDecl( + hasGlobalStorage(), + unless(hasDeclContext(functionDecl())), + allOf(isDefinition(), isFirstParty(), unless(isExpansionInSystemHeader())) + ); + + auto FirstPartyGlobalVariableWithRuntimeInit = varDecl(FirstPartyGlobalVariable, + allOf(anyOf(isConstexpr(), hasConstInitAttr(), hasConstantInitializer(), hasMozGlobalType()), isMozGlobal()), + unless(isMozGenerated()) + ); + + AstMatcher->addMatcher(FirstPartyGlobalVariableWithRuntimeInit.bind("flagged-constinit_global"), this); + + auto FirstPartyGlobalVariableWithoutAnnotation = varDecl( + FirstPartyGlobalVariable, + unless(anyOf(isConstexpr(), hasConstInitAttr(), isMozGlobal(), hasConstantInitializer(), hasMozGlobalType())) + ); + + AstMatcher->addMatcher(FirstPartyGlobalVariableWithoutAnnotation.bind("non-constinit_global"), this); +} + +void GlobalVariableInitializationChecker::check( + const MatchFinder::MatchResult &Result) { + if (const VarDecl *VD = + Result.Nodes.getNodeAs("flagged-constinit_global")) { + diag(VD->getBeginLoc(), "Global variable flagged as MOZ_RUNINIT but actually has constant initialisation. Consider flagging it as constexpr or MOZ_CONSTINIT instead.", + DiagnosticIDs::Error); + } + + if (const VarDecl *VD = + Result.Nodes.getNodeAs("non-constinit_global")) { + const SourceManager &SM = VD->getASTContext().getSourceManager(); + + SourceLocation Loc = VD->getLocation(); + + // Some macro from third party end up triggering warning, we can't fix that + // easily so ignore them. + if (SM.isMacroBodyExpansion(Loc)) + return; + + // FIXME: This captures some generated third party source, e.g. from + // google-protocol, that are generated by third-party generators but are not + // captured by `isFirstParty`, but may also catch some sources we own. + if (getFilename(SM, Loc).ends_with(".cc")) { + return; + } + diag(VD->getBeginLoc(), "Global variable has runtime initialisation, try to remove it, make it constexpr or MOZ_CONSTINIT if possible, or as a last resort flag it as MOZ_RUNINIT.", + DiagnosticIDs::Error); + } +} diff --git a/build/clang-plugin/GlobalVariableInitializationChecker.h b/build/clang-plugin/GlobalVariableInitializationChecker.h new file mode 100644 index 000000000000..9d335b0adee9 --- /dev/null +++ b/build/clang-plugin/GlobalVariableInitializationChecker.h @@ -0,0 +1,18 @@ +/* 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 GlobalVariableInitializationChecker_h__ +#define GlobalVariableInitializationChecker_h__ + +#include "plugin.h" + +class GlobalVariableInitializationChecker : public BaseCheck { +public: + GlobalVariableInitializationChecker(StringRef CheckName, ContextType *Context = nullptr) + : BaseCheck(CheckName, Context) {} + void registerMatchers(MatchFinder *AstMatcher) override; + void check(const MatchFinder::MatchResult &Result) override; +}; + +#endif diff --git a/build/clang-plugin/moz.build b/build/clang-plugin/moz.build index 43ec8aab787d..e6bc630f9675 100644 --- a/build/clang-plugin/moz.build +++ b/build/clang-plugin/moz.build @@ -18,6 +18,7 @@ HOST_SOURCES += [ "DiagnosticsMatcher.cpp", "ExplicitImplicitChecker.cpp", "ExplicitOperatorBoolChecker.cpp", + "GlobalVariableInitializationChecker.cpp", "JSHandleRootedTypedefChecker.cpp", "KnownLiveChecker.cpp", "KungFuDeathGripChecker.cpp", diff --git a/build/clang-plugin/tests/TestGlobalVariableInitialization.cpp b/build/clang-plugin/tests/TestGlobalVariableInitialization.cpp new file mode 100644 index 000000000000..322d6222b8aa --- /dev/null +++ b/build/clang-plugin/tests/TestGlobalVariableInitialization.cpp @@ -0,0 +1,59 @@ +#define MOZ_RUNINIT __attribute__((annotate("moz_global_var"))) +#define MOZ_CONSTINIT [[clang::require_constant_initialization]] +#define MOZ_GLOBAL_CLASS __attribute__((annotate("moz_global_class"))) + +// POD Type +struct POD { + int i, j, k; +}; + +POD g0; + +// constexpr constructor +struct ConstexprGlobal { + int i, j, k; + constexpr ConstexprGlobal() : i(0), j(1), k(2) {} +}; + +ConstexprGlobal g1; + +// Global with extern constructor +struct Global { + Global(); +}; + +Global g2; // expected-error {{Global variable has runtime initialisation, try to remove it, make it constexpr or MOZ_CONSTINIT if possible, or as a last resort flag it as MOZ_RUNINIT.}} + +// Global with extern constructor *but* marked MOZ_GLOBAL_CLASS +struct MOZ_GLOBAL_CLASS GlobalCls { + GlobalCls(); +}; + +GlobalCls g3; + +// Global with extern constructor *but* marked MOZ_RUNINIT +struct RuninitGlobal { + RuninitGlobal(); +}; + +MOZ_RUNINIT RuninitGlobal g4; + +// Global with constexpr constructor *but* marked MOZ_RUNINIT +struct InvalidRuninitGlobal { + constexpr InvalidRuninitGlobal() {} +}; + +MOZ_RUNINIT InvalidRuninitGlobal g5; // expected-error {{Global variable flagged as MOZ_RUNINIT but actually has constant initialisation. Consider flagging it as constexpr or MOZ_CONSTINIT instead.}} + +// Static variable with extern constructor +Global g6; // expected-error {{Global variable has runtime initialisation, try to remove it, make it constexpr or MOZ_CONSTINIT if possible, or as a last resort flag it as MOZ_RUNINIT.}} + +// Static variable with extern constructor within a function +void foo() { static Global g7; } + +// Global variable with extern constructor in a namespace +namespace bar {Global g8;} // expected-error {{Global variable has runtime initialisation, try to remove it, make it constexpr or MOZ_CONSTINIT if possible, or as a last resort flag it as MOZ_RUNINIT.}} + +// Static variable with extern constructor in a class +class foobar {static Global g9;}; +Global foobar::g9; // expected-error {{Global variable has runtime initialisation, try to remove it, make it constexpr or MOZ_CONSTINIT if possible, or as a last resort flag it as MOZ_RUNINIT.}} diff --git a/build/clang-plugin/tests/TestHeapClass.cpp b/build/clang-plugin/tests/TestHeapClass.cpp index 0cdd93925093..340445e15b21 100644 --- a/build/clang-plugin/tests/TestHeapClass.cpp +++ b/build/clang-plugin/tests/TestHeapClass.cpp @@ -4,8 +4,8 @@ #include struct MOZ_HEAP_CLASS Heap { - int i; - Heap() {} + int i = 0; + constexpr Heap() {} MOZ_IMPLICIT Heap(int a) {} Heap(int a, int b) {} void *operator new(size_t x) throw() { return 0; } diff --git a/build/clang-plugin/tests/TestKungFuDeathGrip.cpp b/build/clang-plugin/tests/TestKungFuDeathGrip.cpp index 02180158077f..7f45857759bb 100644 --- a/build/clang-plugin/tests/TestKungFuDeathGrip.cpp +++ b/build/clang-plugin/tests/TestKungFuDeathGrip.cpp @@ -1,6 +1,7 @@ #include #define MOZ_IMPLICIT __attribute__((annotate("moz_implicit"))) +#define MOZ_RUNINIT __attribute__((annotate("moz_global_var"))) template class already_AddRefed { @@ -139,4 +140,4 @@ void f(nsCOMPtr ignoredArgument, Type *param) { RefPtr kfdg_p4 = param; } -nsCOMPtr Type::someStaticCOMPtr(nullptr); +MOZ_RUNINIT nsCOMPtr Type::someStaticCOMPtr(nullptr); diff --git a/build/clang-plugin/tests/TestNoAutoType.cpp b/build/clang-plugin/tests/TestNoAutoType.cpp index 6c6e65f243ad..a582b37a9e11 100644 --- a/build/clang-plugin/tests/TestNoAutoType.cpp +++ b/build/clang-plugin/tests/TestNoAutoType.cpp @@ -1,4 +1,5 @@ #define MOZ_NON_AUTOABLE __attribute__((annotate("moz_non_autoable"))) +#define MOZ_RUNINIT __attribute__((annotate("moz_global_var"))) template struct MOZ_NON_AUTOABLE ExplicitTypeTemplate {}; @@ -35,7 +36,7 @@ void f() { } ExplicitType A; -auto B = A; // expected-error {{Cannot use auto to declare a variable of type 'ExplicitType'}} expected-note {{Please write out this type explicitly}} +MOZ_RUNINIT auto B = A; // expected-error {{Cannot use auto to declare a variable of type 'ExplicitType'}} expected-note {{Please write out this type explicitly}} NonExplicitType C; -auto D = C; +MOZ_RUNINIT auto D = C; diff --git a/build/clang-plugin/tests/TestNonMemMovableStd.cpp b/build/clang-plugin/tests/TestNonMemMovableStd.cpp index b380d0718680..18995caf2e22 100644 --- a/build/clang-plugin/tests/TestNonMemMovableStd.cpp +++ b/build/clang-plugin/tests/TestNonMemMovableStd.cpp @@ -1,4 +1,5 @@ #define MOZ_NEEDS_MEMMOVABLE_TYPE __attribute__((annotate("moz_needs_memmovable_type"))) +#define MOZ_RUNINIT __attribute__((annotate("moz_global_var"))) template class MOZ_NEEDS_MEMMOVABLE_TYPE Mover { T mForceInst; }; // expected-error-re 9 {{Cannot instantiate 'Mover<{{.*}}>' with non-memmovable template argument '{{.*}}'}} @@ -50,19 +51,19 @@ struct has_trivial_move { class HasString { std::string m; }; // expected-note {{'HasString' is a non-memmove()able type because member 'm' is a non-memmove()able type 'std::string' (aka 'basic_string')}} -static Mover bad; // expected-note-re {{instantiation of 'Mover{{ ?}}>' requested here}} -static Mover bad_mem; // expected-note {{instantiation of 'Mover' requested here}} +MOZ_RUNINIT static Mover bad; // expected-note-re {{instantiation of 'Mover{{ ?}}>' requested here}} +MOZ_RUNINIT static Mover bad_mem; // expected-note {{instantiation of 'Mover' requested here}} static Mover> good; -static Mover> not_good; // expected-note-re {{instantiation of 'Mover{{ ?}}>{{ ?}}>' requested here}} +MOZ_RUNINIT static Mover> not_good; // expected-note-re {{instantiation of 'Mover{{ ?}}>{{ ?}}>' requested here}} -static Mover nontrivial_dtor; // expected-note {{instantiation of 'Mover' requested here}} +MOZ_RUNINIT static Mover nontrivial_dtor; // expected-note {{instantiation of 'Mover' requested here}} static Mover nontrivial_copy; // expected-note {{instantiation of 'Mover' requested here}} static Mover nontrivial_move; // expected-note {{instantiation of 'Mover' requested here}} static Mover trivial_dtor; static Mover trivial_copy; static Mover trivial_move; -static Mover> pair_nontrivial_dtor; // expected-note {{instantiation of 'Mover>' requested here}} +MOZ_RUNINIT static Mover> pair_nontrivial_dtor; // expected-note {{instantiation of 'Mover>' requested here}} static Mover> pair_nontrivial_copy; // expected-note {{instantiation of 'Mover>' requested here}} static Mover> pair_nontrivial_move; // expected-note {{instantiation of 'Mover>' requested here}} static Mover> pair_trivial_dtor; diff --git a/build/clang-plugin/tests/TestNonTemporaryClass.cpp b/build/clang-plugin/tests/TestNonTemporaryClass.cpp index 682c8ad53074..656c33b3a85d 100644 --- a/build/clang-plugin/tests/TestNonTemporaryClass.cpp +++ b/build/clang-plugin/tests/TestNonTemporaryClass.cpp @@ -1,11 +1,12 @@ #define MOZ_NON_TEMPORARY_CLASS __attribute__((annotate("moz_non_temporary_class"))) #define MOZ_IMPLICIT __attribute__((annotate("moz_implicit"))) +#define MOZ_RUNINIT __attribute__((annotate("moz_global_var"))) #include struct MOZ_NON_TEMPORARY_CLASS NonTemporary { - int i; - NonTemporary() {} + int i = 0; + constexpr NonTemporary() {} MOZ_IMPLICIT NonTemporary(int a) {} NonTemporary(int a, int b) {} void *operator new(size_t x) throw() { return 0; } diff --git a/build/clang-plugin/tests/TestTemporaryClass.cpp b/build/clang-plugin/tests/TestTemporaryClass.cpp index f00acf0d7ae5..3d2402c26d23 100644 --- a/build/clang-plugin/tests/TestTemporaryClass.cpp +++ b/build/clang-plugin/tests/TestTemporaryClass.cpp @@ -4,8 +4,8 @@ #include struct MOZ_TEMPORARY_CLASS Temporary { - int i; - Temporary() {} + int i = 0; + constexpr Temporary() {} MOZ_IMPLICIT Temporary(int a) {} Temporary(int a, int b) {} void *operator new(size_t x) throw() { return 0; } diff --git a/build/clang-plugin/tests/moz.build b/build/clang-plugin/tests/moz.build index bcfa9f3580e2..b4fd9e0b7e97 100644 --- a/build/clang-plugin/tests/moz.build +++ b/build/clang-plugin/tests/moz.build @@ -15,6 +15,7 @@ SOURCES += [ "TestDanglingOnTemporary.cpp", "TestExplicitOperatorBool.cpp", "TestGlobalClass.cpp", + "TestGlobalVariableInitialization.cpp", "TestHeapClass.cpp", "TestInheritTypeAnnotationsFromTemplateArgs.cpp", "TestJSHandleRootedTypedef.cpp", diff --git a/docs/code-quality/coding-style/coding_style_cpp.rst b/docs/code-quality/coding-style/coding_style_cpp.rst index 06684e469c84..231c89327e69 100644 --- a/docs/code-quality/coding-style/coding_style_cpp.rst +++ b/docs/code-quality/coding-style/coding_style_cpp.rst @@ -394,11 +394,14 @@ C/C++ practices - One-argument constructors, that are not copy or move constructors, should generally be marked explicit. Exceptions should be annotated with ``MOZ_IMPLICIT``. -- Global variables with runtimle initialization should be avoided. Flagging +- Global variables with runtime initialization should be avoided. Flagging them as ``constexpr`` or ``MOZ_CONSTINIT`` is a good way to make sure the initialization happens at compile-time. If runtime initialization cannot be avoided, use the attribute ``MOZ_RUNINIT`` to identify those and tell the - linter to ignore that variable. + linter to ignore that variable. If a variable is flagged as ``MOZ_RUNINIT`` + while the linter detects it could be ``MOZ_CONSTINIT``, you get an error. In + case where the status of the global variable varies (e.g. depending on + template parameter), just flag it ``MOZ_GLOBINIT``. - Use ``char32_t`` as the return type or argument type of a method that returns or takes as argument a single Unicode scalar value. (Don't use UTF-32 strings, though.) diff --git a/widget/android/ImageDecoderSupport.cpp b/widget/android/ImageDecoderSupport.cpp index a642dd0a4db8..1b334aaec17b 100644 --- a/widget/android/ImageDecoderSupport.cpp +++ b/widget/android/ImageDecoderSupport.cpp @@ -25,8 +25,8 @@ namespace { class ImageCallbackHelper; MOZ_RUNINIT - HashSet, PointerHasher> - gDecodeRequests; +HashSet, PointerHasher> + gDecodeRequests; class ImageCallbackHelper : public imgIContainerCallback, public imgINotificationObserver {