[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

41

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

clang-tidy/cppcoreguidelines/InterfacesGlobalInitCheck.h

20

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

37

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

37

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.
+/// 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) {
+  auto IsStaticGlobal =
+      allOf(hasGlobalStorage(),
+            hasDeclContext(anyOf(translationUnitDecl(), // Global scope.
+                                 namespaceDecl(),       // Namespace scope.
+                                 recordDecl())),        // Class scope.
+            unless(isConstexpr()));
+  auto IsNotDefined = unless(isDefinition());
+  auto ReferencesStaticGlobalVar = declRefExpr(
+      hasDeclaration(varDecl(IsStaticGlobal, IsNotDefined).bind("referencee")));
+
+  Finder->addMatcher(
+      varDecl(IsStaticGlobal, isDefinition(),
+              hasInitializer(expr(hasDescendant(ReferencesStaticGlobalVar))))
+          .bind("var"),
+      this);
+}
+
+void InterfacesGlobalInitCheck::check(const MatchFinder::MatchResult &Result) {
+  const auto *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 *Referencee = Result.Nodes.getNodeAs<VarDecl>("referencee");
+  diag(Var->getLocation(),
+       "initializing static variable with non-const expression depending on "
+       "static variable '%0'.")
+      << Referencee->getName();
+}
+
+} // namespace cppcoreguidelines
+} // namespace tidy
+} // namespace clang

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
+

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 static variable with
+// non-const expression depending on static variable 'ExternGlobal'
+static int GlobalScopeBadInit2 = takesInt(ExternGlobal);
+// CHECK-MESSAGES: [[@LINE-1]]:12: warning: initializing static variable with
+// non-const expression depending on static variable 'ExternGlobal'
+static int GlobalScopeBadInit3 = takesIntPtr(&ExternGlobal);
+// CHECK-MESSAGES: [[@LINE-1]]:12: warning: initializing static variable with
+// non-const expression depending on static variable 'ExternGlobal'
+static int GlobalScopeBadInit4 = 3 * (ExternGlobal + 2);
+// CHECK-MESSAGES: [[@LINE-1]]:12: warning: initializing static variable with
+// non-const expression depending on static variable 'ExternGlobal'
+
+namespace ns {
+static int NamespaceScope = makesInt();
+static int NamespaceScopeBadInit = takesInt(ExternGlobal);
+// CHECK-MESSAGES: [[@LINE-1]]:12: warning: initializing static variable with
+// non-const expression depending on static variable 'ExternGlobal'
+
+struct A {
+  static int ClassScope;
+  static int ClassScopeBadInit;
+};
+
+int A::ClassScopeBadInit = takesInt(ExternGlobal);
+// CHECK-MESSAGES: [[@LINE-1]]:8: warning: initializing static variable with
+// non-const expression depending on static variable 'ExternGlobal'
+
+static int FromClassBadInit = takesInt(A::ClassScope);
+// CHECK-MESSAGES: [[@LINE-1]]:12: warning: initializing static variable with
+// non-const expression depending on static variable 'ClassScope'
+} // namspace ns
+
+void f() {
+  // This is fine, it's executed after dynamic initilization occurs.
+  static int G = takesInt(ExternGlobal);
+}
+
+// Defined global variables are fine:
+static int GlobalScope = makesInt();
+static int GlobalScopeBadInit = 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);