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

@@ -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")

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"

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)

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<moz_global_class>(TD))
+      return true;
+  }
+  return false;
+}
+
+/// Match any variable declared with the MOZ_RUNINIT qualifier.
+AST_MATCHER(VarDecl, isMozGlobal) {
+  return hasCustomAttribute<moz_global_var>(&Node);
+}
+
+/// Match any variable declared with the MOZ_GLOBINIT qualifier.
+AST_MATCHER(VarDecl, isMozGenerated) {
+  return hasCustomAttribute<moz_generated>(&Node);
+}
+
+/// Match any variable declared with the constinit qualifier.
+AST_MATCHER(VarDecl, hasConstInitAttr) {
+  return Node.hasAttr<ConstInitAttr>();
+}
+
 /// 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).

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<VarDecl>("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<VarDecl>("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);
+  }
+}

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

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",

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.}}

build/clang-plugin/tests/TestHeapClass.cpp

@@ -4,8 +4,8 @@
 #include <stddef.h>
 
 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; }

build/clang-plugin/tests/TestKungFuDeathGrip.cpp

@@ -1,6 +1,7 @@
 #include <utility>
 
 #define MOZ_IMPLICIT __attribute__((annotate("moz_implicit")))
+#define MOZ_RUNINIT  __attribute__((annotate("moz_global_var")))
 
 template <typename T>
 class already_AddRefed {
@@ -139,4 +140,4 @@ void f(nsCOMPtr<Type> ignoredArgument, Type *param) {
   RefPtr<Type> kfdg_p4 = param;
 }
 
-nsCOMPtr<Type> Type::someStaticCOMPtr(nullptr);
+MOZ_RUNINIT nsCOMPtr<Type> Type::someStaticCOMPtr(nullptr);

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<class T>
 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;

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 T>
 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<char>')}}
 
-static Mover<std::string> bad; // expected-note-re {{instantiation of 'Mover<std::basic_string<char>{{ ?}}>' requested here}}
-static Mover<HasString> bad_mem; // expected-note {{instantiation of 'Mover<HasString>' requested here}}
+MOZ_RUNINIT  static Mover<std::string> bad; // expected-note-re {{instantiation of 'Mover<std::basic_string<char>{{ ?}}>' requested here}}
+MOZ_RUNINIT  static Mover<HasString> bad_mem; // expected-note {{instantiation of 'Mover<HasString>' requested here}}
 static Mover<std::pair<bool, int>> good;
-static Mover<std::pair<bool, std::string>> not_good; // expected-note-re {{instantiation of 'Mover<std::pair<bool, std::basic_string<char>{{ ?}}>{{ ?}}>' requested here}}
+MOZ_RUNINIT  static Mover<std::pair<bool, std::string>> not_good; // expected-note-re {{instantiation of 'Mover<std::pair<bool, std::basic_string<char>{{ ?}}>{{ ?}}>' requested here}}
 
-static Mover<std::has_nontrivial_dtor> nontrivial_dtor; // expected-note {{instantiation of 'Mover<std::has_nontrivial_dtor>' requested here}}
+MOZ_RUNINIT  static Mover<std::has_nontrivial_dtor> nontrivial_dtor; // expected-note {{instantiation of 'Mover<std::has_nontrivial_dtor>' requested here}}
 static Mover<std::has_nontrivial_copy> nontrivial_copy; // expected-note {{instantiation of 'Mover<std::has_nontrivial_copy>' requested here}}
 static Mover<std::has_nontrivial_move> nontrivial_move; // expected-note {{instantiation of 'Mover<std::has_nontrivial_move>' requested here}}
 static Mover<std::has_trivial_dtor> trivial_dtor;
 static Mover<std::has_trivial_copy> trivial_copy;
 static Mover<std::has_trivial_move> trivial_move;
 
-static Mover<std::pair<bool, std::has_nontrivial_dtor>> pair_nontrivial_dtor; // expected-note {{instantiation of 'Mover<std::pair<bool, std::has_nontrivial_dtor>>' requested here}}
+MOZ_RUNINIT  static Mover<std::pair<bool, std::has_nontrivial_dtor>> pair_nontrivial_dtor; // expected-note {{instantiation of 'Mover<std::pair<bool, std::has_nontrivial_dtor>>' requested here}}
 static Mover<std::pair<bool, std::has_nontrivial_copy>> pair_nontrivial_copy; // expected-note {{instantiation of 'Mover<std::pair<bool, std::has_nontrivial_copy>>' requested here}}
 static Mover<std::pair<bool, std::has_nontrivial_move>> pair_nontrivial_move; // expected-note {{instantiation of 'Mover<std::pair<bool, std::has_nontrivial_move>>' requested here}}
 static Mover<std::pair<bool, std::has_trivial_dtor>> pair_trivial_dtor;

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 <stddef.h>
 
 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; }

build/clang-plugin/tests/TestTemporaryClass.cpp

@@ -4,8 +4,8 @@
 #include <stddef.h>
 
 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; }

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",

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.)

widget/android/ImageDecoderSupport.cpp

@@ -25,8 +25,8 @@ namespace {
 class ImageCallbackHelper;
 
 MOZ_RUNINIT
-    HashSet<RefPtr<ImageCallbackHelper>, PointerHasher<ImageCallbackHelper*>>
-        gDecodeRequests;
+HashSet<RefPtr<ImageCallbackHelper>, PointerHasher<ImageCallbackHelper*>>
+    gDecodeRequests;
 
 class ImageCallbackHelper : public imgIContainerCallback,
                             public imgINotificationObserver {