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

[courbet]

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

[Eugene.Zelenko]

Please mention new check in docs/ReleaseNotes.rst.

[courbet]

In D18649#388337, @Eugene.Zelenko wrote:

Please mention new check in docs/ReleaseNotes.rst.

That should be in a different commit, right ? Release notes are in a different repo (forgive my ignorance, this is my first patch).

[alexfh]

In D18649#389294, @courbet wrote:

In D18649#388337, @Eugene.Zelenko wrote:

Please mention new check in docs/ReleaseNotes.rst.

That should be in a different commit, right ? Release notes are in a different repo (forgive my ignorance, this is my first patch).

Now they are in the same repo (clang-tools-extra, docs/ReleaseNotes.rst).

[courbet]

Updated release notes.

[alexfh]

Thank you for working on the new clang-tidy check!

We usually recommend authors to run their checks on a large code base to ensure it doesn't crash and doesn't generate obvious false positives. It would be nice, if you could provide a quick summary of such a run (total number of hits, number of what seems to be a false positive in a sample of ~100).

clang-tidy/cppcoreguidelines/InterfacesGlobalInitCheck.cpp

42

nit: No need for braces around single-line if/for/... bodies.

clang-tidy/cppcoreguidelines/InterfacesGlobalInitCheck.h

21

I'd leave the "For user-facing documentation, see ..." phrase that the add_new_check.py script adds.

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

38

These should be on a single line. I guess, your clang-format doesn't use -style=file by default and doesn't understand the local style configured for the test/ directory. Otherwise it wouldn't split the lines.

[etienneb]

some nits.

clang-tidy/cppcoreguidelines/InterfacesGlobalInitCheck.cpp

22

nit: const (and below)

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

43

nit: initilization -> initialization

[courbet]

Style fixes.

[courbet]

Looks like there's a race condition in phabricator.

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

38

Thanks for the hint. I was using -style=LLVM indeed.

[etienneb]

more nits, sorry didn't saw them first time.

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

33

nit: namspace -> namespace

44

nit: remove empty line here.

[courbet]

Constness + typos.

[alexfh]

clang-tidy/cppcoreguidelines/InterfacesGlobalInitCheck.cpp

47

nit: no trailing period needed in diagnostic messages

48

This might work even better without ->getName(). Not sure, but it makes sense to try.

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

8

What about indirect access of other globals, e.g. via function calls? We obviously can't analyze functions defined in a different translation unit, and I'm not sure we need to analyze even those defined in the same TU (since it may be imprecise and quite expensive), but maybe we should use some heuristic here (the most agressive would be to flag all initializers containing function calls, but that might be rather noisy).

[courbet]

In D18649#389363, @alexfh wrote:

Thank you for working on the new clang-tidy check!

We usually recommend authors to run their checks on a large code base to ensure it doesn't crash and doesn't generate obvious false positives. It would be nice, if you could provide a quick summary of such a run (total number of hits, number of what seems to be a false positive in a sample of ~100).

The tool generated 20k positives on our codebase. On a sample of 100, there are:

• 8 instances of the same exact code structure that's just wrong: const string var = FLAGS_some_flag + "some_sufix";
• 8 false positives.
• 84 possible issues. (interestingly 6 of these are from premature use of variations of "extern char* empty_string;"

The false positives fall into 3 categories:

1. 3 variations of:

extern int i;
static const int* pi = &i;  // diag

// Then pi is dereferenced later, once i is intialized.
Public example of this: https://github.com/python-git/python/blob/py3k/Objects/dictobject.c#L2027

2. 3 variations of:

// .h
class A {
  static const int i = 42;
};
// .cc
int A::i;  // diag

3. 2 variations of:

// .h
class A {
static int i;
static int j;
};
// .cc
int A::i = 0;
int A::j = i;  // diag

[courbet]

cosmetics + more documentation

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

8

The guidelines actually recommend flagging calls non-constexpr functions, but this would give too many positives. I've added a note.

[alexfh]

In D18649#390862, @courbet wrote:

In D18649#389363, @alexfh wrote:

Thank you for working on the new clang-tidy check!

We usually recommend authors to run their checks on a large code base to ensure it doesn't crash and doesn't generate obvious false positives. It would be nice, if you could provide a quick summary of such a run (total number of hits, number of what seems to be a false positive in a sample of ~100).

The tool generated 20k positives on our codebase. On a sample of 100, there are:

• 8 instances of the same exact code structure that's just wrong: const string var = FLAGS_some_flag + "some_sufix";
• 8 false positives.
• 84 possible issues. (interestingly 6 of these are from premature use of variations of "extern char* empty_string;"

The false positives fall into 3 categories:

1. 3 variations of:

extern int i;
static const int* pi = &i;  // diag

Should we warn at all when only an address of a global variable is used?

// Then pi is dereferenced later, once i is intialized.
Public example of this: https://github.com/python-git/python/blob/py3k/Objects/dictobject.c#L2027

2. 3 variations of:

// .h
class A {
  static const int i = 42;
};
// .cc
int A::i;  // diag

Looks like we have all information to fix this kind of a false positive.

3. 2 variations of:

// .h
class A {
static int i;
static int j;
};
// .cc
int A::i = 0;
int A::j = i;  // diag

ditto

[courbet]

In D18649#391001, @alexfh wrote:

In D18649#390862, @courbet wrote:

In D18649#389363, @alexfh wrote:

Thank you for working on the new clang-tidy check!

We usually recommend authors to run their checks on a large code base to ensure it doesn't crash and doesn't generate obvious false positives. It would be nice, if you could provide a quick summary of such a run (total number of hits, number of what seems to be a false positive in a sample of ~100).

The tool generated 20k positives on our codebase. On a sample of 100, there are:

• 8 instances of the same exact code structure that's just wrong: const string var = FLAGS_some_flag + "some_sufix";
• 8 false positives.
• 84 possible issues. (interestingly 6 of these are from premature use of variations of "extern char* empty_string;"

The false positives fall into 3 categories:

1. 3 variations of:

extern int i;
static const int* pi = &i;  // diag

Should we warn at all when only an address of a global variable is used?

We could, but it would allow stuff like:

extern const int i;
const char c = *(const char*)&i;

No strong opinion here.

// Then pi is dereferenced later, once i is intialized.
Public example of this: https://github.com/python-git/python/blob/py3k/Objects/dictobject.c#L2027

2. 3 variations of:

// .h
class A {
  static const int i = 42;
};
// .cc
int A::i;  // diag

Looks like we have all information to fix this kind of a false positive.

3. 2 variations of:

// .h
class A {
static int i;
static int j;
};
// .cc
int A::i = 0;
int A::j = i;  // diag

ditto

It turns out (2) and (3) are just variations of the same - (2) was just:

// .h
class A {
  static const int i = 0;
  static const int j = i;
};
// .cc
int A::i;
int A::j;

Which has *exactly* the same semantics as (3).

I could not find a way to go from a decl to its (possible) definition. I was thinking of matching all global definitions that are not declarations and then making sure that the current matches are not referring to one of those definitions (in syntactic order because of [3.6.2.3]). Does this sound like a reasonable idea ?

[alexfh]

In D18649#391112, @courbet wrote:

In D18649#391001, @alexfh wrote:

In D18649#390862, @courbet wrote:

In D18649#389363, @alexfh wrote:

Thank you for working on the new clang-tidy check!

We usually recommend authors to run their checks on a large code base to ensure it doesn't crash and doesn't generate obvious false positives. It would be nice, if you could provide a quick summary of such a run (total number of hits, number of what seems to be a false positive in a sample of ~100).

The tool generated 20k positives on our codebase. On a sample of 100, there are:

• 8 instances of the same exact code structure that's just wrong: const string var = FLAGS_some_flag + "some_sufix";
• 8 false positives.
• 84 possible issues. (interestingly 6 of these are from premature use of variations of "extern char* empty_string;"

The false positives fall into 3 categories:

1. 3 variations of:

extern int i;
static const int* pi = &i;  // diag

Should we warn at all when only an address of a global variable is used?

We could, but it would allow stuff like:

extern const int i;
const char c = *(const char*)&i;

No strong opinion here.

// Then pi is dereferenced later, once i is intialized.
Public example of this: https://github.com/python-git/python/blob/py3k/Objects/dictobject.c#L2027

2. 3 variations of:

// .h
class A {
  static const int i = 42;
};
// .cc
int A::i;  // diag

Looks like we have all information to fix this kind of a false positive.

3. 2 variations of:

// .h
class A {
static int i;
static int j;
};
// .cc
int A::i = 0;
int A::j = i;  // diag

ditto

It turns out (2) and (3) are just variations of the same - (2) was just:

// .h
class A {
  static const int i = 0;
  static const int j = i;
};
// .cc
int A::i;
int A::j;

Which has *exactly* the same semantics as (3).

I could not find a way to go from a decl to its (possible) definition.

VarDecl is also Redeclarable<VarDecl>, which has various ways to iterate over all related declarations.

I was thinking of matching all global definitions that are not declarations and then making sure that the current matches are not referring to one of those definitions (in syntactic order because of [3.6.2.3]). Does this sound like a reasonable idea ?

[courbet]

Fix 5 of the 8 false positives. Add tests.

[courbet]

Update naming to be constistent with the standard: use "non-local" instead of "global" (or "static" in my case, as I started from classes and the name stuck).

[aaron.ballman]

clang-tidy/cppcoreguidelines/InterfacesGlobalInitCheck.cpp

28

This can be moved into ReferencesUndefinedGlobalVar

[courbet]

cosmetics

[alexfh]

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

8

What happens if you add:

extern int ExternGlobal;
extern int ExternGlobal;
int ExternGlobal = 123;

?

[courbet]

Add unit test for multiple declaration then definition.

[courbet]

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

8

It does not trigger (as expected). I've added a test.

[alexfh]

Looks good! Thank you for the new check!

Do you need me to submit the patch for you?

[alexfh]

(if you do, please rebase the patch on top of HEAD)

[courbet]

Rebase on HEAD for submission.

[courbet]

That'd be great. Thanks all for the review.

clang-tidy/cppcoreguidelines/CMakeLists.txt

 set(LLVM_LINK_COMPONENTS support)
 
 add_clang_library(clangTidyCppCoreGuidelinesModule
   CppCoreGuidelinesTidyModule.cpp
+  InterfacesGlobalInitCheck.cpp
   ProBoundsArrayToPointerDecayCheck.cpp
   ProBoundsConstantArrayIndexCheck.cpp
   ProBoundsPointerArithmeticCheck.cpp
   ProTypeConstCastCheck.cpp
   ProTypeCstyleCastCheck.cpp
   ProTypeMemberInitCheck.cpp
   ProTypeReinterpretCastCheck.cpp
   ProTypeStaticCastDowncastCheck.cpp

clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp

 //===--- CppCoreGuidelinesModule.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 "../ClangTidy.h"
 #include "../ClangTidyModule.h"
 #include "../ClangTidyModuleRegistry.h"
 #include "../misc/AssignOperatorSignatureCheck.h"
+#include "InterfacesGlobalInitCheck.h"
 #include "ProBoundsArrayToPointerDecayCheck.h"
 #include "ProBoundsConstantArrayIndexCheck.h"
 #include "ProBoundsPointerArithmeticCheck.h"
 #include "ProTypeConstCastCheck.h"
 #include "ProTypeCstyleCastCheck.h"
 #include "ProTypeMemberInitCheck.h"
 #include "ProTypeReinterpretCastCheck.h"
 #include "ProTypeStaticCastDowncastCheck.h"
 #include "ProTypeUnionAccessCheck.h"
 #include "ProTypeVarargCheck.h"
 
 namespace clang {
 namespace tidy {
 namespace cppcoreguidelines {
 
 /// A module containing checks of the C++ Core Guidelines
 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>(
         "cppcoreguidelines-pro-bounds-constant-array-index");
     CheckFactories.registerCheck<ProBoundsPointerArithmeticCheck>(
         "cppcoreguidelines-pro-bounds-pointer-arithmetic");
     CheckFactories.registerCheck<ProTypeConstCastCheck>(
         "cppcoreguidelines-pro-type-const-cast");

clang-tidy/cppcoreguidelines/InterfacesGlobalInitCheck.h

+//===--- 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-tidy/cppcoreguidelines/InterfacesGlobalInitCheck.cpp

+//===--- 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

docs/ReleaseNotes.rst

- New `readability-redundant-control-flow
   Looks for procedures (functions returning no value) with ``return`` statements
   at the end of the function.  Such `return` statements are redundant.
 
 - New `readability-redundant-string-init
   <http://clang.llvm.org/extra/clang-tidy/checks/readability-redundant-string-init.html>`_ check
 
   Finds unnecessary string initializations.
 
+- 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.
 
   Crash when running with the `-fdelayed-template-parsing` flag.
 
 Clang-tidy changes from 3.7 to 3.8
 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

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

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

docs/clang-tidy/checks/list.rst

.. toctree::
    cert-env33-c
    cert-err52-cpp
    cert-err58-cpp
    cert-err60-cpp
    cert-err61-cpp (redirects to misc-throw-by-value-catch-by-reference) <cert-err61-cpp>
    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
    cppcoreguidelines-pro-type-const-cast
    cppcoreguidelines-pro-type-cstyle-cast
    cppcoreguidelines-pro-type-member-init
    cppcoreguidelines-pro-type-reinterpret-cast
    cppcoreguidelines-pro-type-static-cast-downcast

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

+// 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;
+