[clang-tidy] cppcoreguidelines-interfaces-global-init

Summary:
This check flags initializers of globals that access extern objects, and therefore can lead to order-of-initialization problems (this recommandation is part of CPP core guidelines).
Note that this only checks half of the guideline for now (it does not enforce using constexpr functions).

Reviewers: aaron.ballman, alexfh

Subscribers: aaron.ballman, etienneb, Eugene.Zelenko, cfe-commits

Patch by Clement Courbet!

Differential Revision: http://reviews.llvm.org/D18649

llvm-svn: 265774

clang-tools-extra/clang-tidy/cppcoreguidelines/CMakeLists.txt

@@ -2,6 +2,7 @@ set(LLVM_LINK_COMPONENTS support)
 
 add_clang_library(clangTidyCppCoreGuidelinesModule
   CppCoreGuidelinesTidyModule.cpp
+  InterfacesGlobalInitCheck.cpp
   ProBoundsArrayToPointerDecayCheck.cpp
   ProBoundsConstantArrayIndexCheck.cpp
   ProBoundsPointerArithmeticCheck.cpp

clang-tools-extra/clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp

@@ -11,6 +11,7 @@
 #include "../ClangTidyModule.h"
 #include "../ClangTidyModuleRegistry.h"
 #include "../misc/AssignOperatorSignatureCheck.h"
+#include "InterfacesGlobalInitCheck.h"
 #include "ProBoundsArrayToPointerDecayCheck.h"
 #include "ProBoundsConstantArrayIndexCheck.h"
 #include "ProBoundsPointerArithmeticCheck.h"
@@ -30,6 +31,8 @@ namespace cppcoreguidelines {
 class CppCoreGuidelinesModule : public ClangTidyModule {
 public:
   void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override {
+    CheckFactories.registerCheck<InterfacesGlobalInitCheck>(
+        "cppcoreguidelines-interfaces-global-init");
     CheckFactories.registerCheck<ProBoundsArrayToPointerDecayCheck>(
         "cppcoreguidelines-pro-bounds-array-to-pointer-decay");
     CheckFactories.registerCheck<ProBoundsConstantArrayIndexCheck>(

clang-tools-extra/clang-tidy/cppcoreguidelines/InterfacesGlobalInitCheck.cpp

@@ -0,0 +1,59 @@
+//===--- InterfacesGlobalInitCheck.cpp - clang-tidy------------------------===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#include "InterfacesGlobalInitCheck.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+
+using namespace clang::ast_matchers;
+
+namespace clang {
+namespace tidy {
+namespace cppcoreguidelines {
+
+void InterfacesGlobalInitCheck::registerMatchers(MatchFinder *Finder) {
+  const auto IsGlobal =
+      allOf(hasGlobalStorage(),
+            hasDeclContext(anyOf(translationUnitDecl(), // Global scope.
+                                 namespaceDecl(),       // Namespace scope.
+                                 recordDecl())),        // Class scope.
+            unless(isConstexpr()));
+
+  const auto ReferencesUndefinedGlobalVar = declRefExpr(hasDeclaration(
+      varDecl(IsGlobal, unless(isDefinition())).bind("referencee")));
+
+  Finder->addMatcher(
+      varDecl(IsGlobal, isDefinition(),
+              hasInitializer(expr(hasDescendant(ReferencesUndefinedGlobalVar))))
+          .bind("var"),
+      this);
+}
+
+void InterfacesGlobalInitCheck::check(const MatchFinder::MatchResult &Result) {
+  const auto *const Var = Result.Nodes.getNodeAs<VarDecl>("var");
+  // For now assume that people who write macros know what they're doing.
+  if (Var->getLocation().isMacroID())
+    return;
+  const auto *const Referencee = Result.Nodes.getNodeAs<VarDecl>("referencee");
+  // If the variable has been defined, we're good.
+  const auto *const ReferenceeDef = Referencee->getDefinition();
+  if (ReferenceeDef != nullptr &&
+      Result.SourceManager->isBeforeInTranslationUnit(
+          ReferenceeDef->getLocation(), Var->getLocation())) {
+    return;
+  }
+  diag(Var->getLocation(),
+       "initializing non-local variable with non-const expression depending on "
+       "uninitialized non-local variable %0")
+      << Referencee;
+}
+
+} // namespace cppcoreguidelines
+} // namespace tidy
+} // namespace clang

clang-tools-extra/clang-tidy/cppcoreguidelines/InterfacesGlobalInitCheck.h

@@ -0,0 +1,35 @@
+//===--- InterfacesGlobalInitCheck.h - clang-tidy----------------*- C++ -*-===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_CPPCOREGUIDELINES_INTERFACES_GLOBAL_INIT_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_CPPCOREGUIDELINES_INTERFACES_GLOBAL_INIT_H
+
+#include "../ClangTidy.h"
+
+namespace clang {
+namespace tidy {
+namespace cppcoreguidelines {
+
+/// Flags possible initialization order issues of static variables.
+///
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/cppcoreguidelines-interfaces-global-init.html
+class InterfacesGlobalInitCheck : public ClangTidyCheck {
+public:
+  InterfacesGlobalInitCheck(StringRef Name, ClangTidyContext *Context)
+      : ClangTidyCheck(Name, Context) {}
+  void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+  void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+};
+
+} // namespace cppcoreguidelines
+} // namespace tidy
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_CPPCOREGUIDELINES_INTERFACES_GLOBAL_INIT_H

clang-tools-extra/docs/ReleaseNotes.rst

@@ -164,6 +164,13 @@ identified.  The improvements since the 3.8 release include:
 
   Finds static function and variable definitions in anonymous namespace.
 
+- New `cppcoreguidelines-interfaces-global-init
+  <http://clang.llvm.org/extra/clang-tidy/checks/cppcoreguidelines-interfaces-global-init.html>`_ check
+
+  Flags initializers of globals that access extern objects, and therefore can
+  lead to order-of-initialization problems.
+
+
 Fixed bugs:
 
 - Crash when running on compile database with relative source files paths.

clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines-interfaces-global-init.rst

@@ -0,0 +1,14 @@
+.. title:: clang-tidy - cppcoreguidelines-interfaces-global-init
+
+cppcoreguidelines-interfaces-global-init
+========================================
+
+This check flags initializers of globals that access extern objects,
+and therefore can lead to order-of-initialization problems.
+
+This rule is part of the "Interfaces" profile of the C++ Core Guidelines, see
+https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md#Ri-global-init
+
+Note that currently this does not flag calls to non-constexpr functions, and
+therefore globals could still be accessed from functions themselves.
+

clang-tools-extra/docs/clang-tidy/checks/list.rst

@@ -16,6 +16,7 @@ Clang-Tidy Checks
    cert-fio38-c (redirects to misc-non-copyable-objects) <cert-fio38-c>
    cert-flp30-c
    cert-oop11-cpp (redirects to misc-move-constructor-init) <cert-oop11-cpp>
+   cppcoreguidelines-interfaces-global-init
    cppcoreguidelines-pro-bounds-array-to-pointer-decay
    cppcoreguidelines-pro-bounds-constant-array-index
    cppcoreguidelines-pro-bounds-pointer-arithmetic

clang-tools-extra/test/clang-tidy/cppcoreguidelines-interfaces-global-init.cpp

@@ -0,0 +1,84 @@
+// RUN: %check_clang_tidy %s cppcoreguidelines-interfaces-global-init %t
+
+constexpr int makesInt() { return 3; }
+constexpr int takesInt(int i) { return i + 1; }
+constexpr int takesIntPtr(int *i) { return *i; }
+
+extern int ExternGlobal;
+static int GlobalScopeBadInit1 = ExternGlobal;
+// CHECK-MESSAGES: [[@LINE-1]]:12: warning: initializing non-local variable with non-const expression depending on uninitialized non-local variable 'ExternGlobal'
+static int GlobalScopeBadInit2 = takesInt(ExternGlobal);
+// CHECK-MESSAGES: [[@LINE-1]]:12: warning: initializing non-local variable with non-const expression depending on uninitialized non-local variable 'ExternGlobal'
+static int GlobalScopeBadInit3 = takesIntPtr(&ExternGlobal);
+// CHECK-MESSAGES: [[@LINE-1]]:12: warning: initializing non-local variable with non-const expression depending on uninitialized non-local variable 'ExternGlobal'
+static int GlobalScopeBadInit4 = 3 * (ExternGlobal + 2);
+// CHECK-MESSAGES: [[@LINE-1]]:12: warning: initializing non-local variable with non-const expression depending on uninitialized non-local variable 'ExternGlobal'
+
+namespace ns {
+static int NamespaceScope = makesInt();
+static int NamespaceScopeBadInit = takesInt(ExternGlobal);
+// CHECK-MESSAGES: [[@LINE-1]]:12: warning: initializing non-local variable with non-const expression depending on uninitialized non-local variable 'ExternGlobal'
+
+struct A {
+  static int ClassScope;
+  static int ClassScopeBadInit;
+};
+
+int A::ClassScopeBadInit = takesInt(ExternGlobal);
+// CHECK-MESSAGES: [[@LINE-1]]:8: warning: initializing non-local variable with non-const expression depending on uninitialized non-local variable 'ExternGlobal'
+
+static int FromClassBadInit = takesInt(A::ClassScope);
+// CHECK-MESSAGES: [[@LINE-1]]:12: warning: initializing non-local variable with non-const expression depending on uninitialized non-local variable 'ClassScope'
+} // namespace ns
+
+// "const int B::I;" is fine, it just ODR-defines B::I. See [9.4.3] Static
+// members [class.static]. However the ODR-definitions are not in the right
+// order (C::I after C::J, see [3.6.2.3]).
+class B1 {
+  static const int I = 0;
+  static const int J = I;
+};
+const int B1::J;
+// CHECK-MESSAGES: [[@LINE-1]]:15: warning: initializing non-local variable with non-const expression depending on uninitialized non-local variable 'I'
+const int B1::I;
+
+void f() {
+  // This is fine, it's executed after dynamic initialization occurs.
+  static int G = takesInt(ExternGlobal);
+}
+
+// Declaration then definition then usage is fine.
+extern int ExternGlobal2;
+extern int ExternGlobal2;
+int ExternGlobal2 = 123;
+static int GlobalScopeGoodInit1 = ExternGlobal2;
+
+
+// Defined global variables are fine:
+static int GlobalScope = makesInt();
+static int GlobalScopeGoodInit2 = takesInt(GlobalScope);
+static int GlobalScope2 = takesInt(ns::NamespaceScope);
+// Enums are fine.
+enum Enum { kEnumValue = 1 };
+static int GlobalScopeFromEnum = takesInt(kEnumValue);
+
+// Leave constexprs alone.
+extern constexpr int GlobalScopeConstexpr = makesInt();
+static constexpr int GlobalScopeConstexprOk =
+    takesInt(GlobalScopeConstexpr);
+
+// This is a pretty common instance: People are usually not using constexpr, but
+// this is what they should write:
+static constexpr const char kValue[] = "value";
+constexpr int Fingerprint(const char *value) { return 0; }
+static int kFingerprint = Fingerprint(kValue);
+
+// This is fine because the ODR-definitions are in the right order (C::J after
+// C::I).
+class B2 {
+  static const int I = 0;
+  static const int J = I;
+};
+const int B2::I;
+const int B2::J;
+