[clang-tidy] Check for dynamically initialized statics in headers.

[czhang]

Summary

Finds instances where variables with static storage are initialized dynamically in header files.

[lebedev.ri]

clang-tools-extra/clang-tidy/bugprone/DynamicStaticInitializersCheck.cpp

41

Most of the matching should be done here, not in check().

[Eugene.Zelenko]

clang-tools-extra/clang-tidy/bugprone/DynamicStaticInitializersCheck.cpp

38

Will be good idea to address FIXME or remove if it's irrelevant.

55

Will be good idea to address FIXME or remove if it's irrelevant.

56

Could it be const auto *?

clang-tools-extra/docs/clang-tidy/checks/bugprone-dynamic-static-initializers.rst

10

Will be good idea to provide example.

[czhang]

Addressed comments.

[czhang]

clang-tools-extra/clang-tidy/bugprone/DynamicStaticInitializersCheck.cpp

41

I believe that there is no way to use the AST matching language to express hasConstantDeclaration.

[lebedev.ri]

Hmm, but there already is clang's -Wglobal-constructors, that fires on some of these:
https://godbolt.org/z/rSnNuu
You are sure those extra warning this check produces ontop of -Wglobal-constructors are correct?
If so, maybe -Wglobal-constructors should be extended instead?

[czhang]

In D62829#1533345, @lebedev.ri wrote:

Hmm, but there already is clang's -Wglobal-constructors, that fires on some of these:
https://godbolt.org/z/rSnNuu
You are sure those extra warning this check produces ontop of -Wglobal-constructors are correct?
If so, maybe -Wglobal-constructors should be extended instead?

Yes, this change is for variables with static lifetimes that are not necessarily globally scoped. The name -global-constructors would be a misnomer. In addition, this check is intended purely for header files, for reasons documented in the .rst file. Although similar, I do not believe this check would make sense as an extension to -global-constructors.

[czhang]

ping

[aaron.ballman]

clang-tools-extra/clang-tidy/bugprone/DynamicStaticInitializersCheck.cpp

39

Do you want to restrict this matcher to only variable declarations that have initializers, or are you also intending for this check to cover cases like:

// At file scope.
struct S { S(); } s;

41

You can write a local AST matcher to hoist this functionality into the check() call, though. Some of it is already exposed for you, like hasInitializer(), isValueDependent(), and isConstexpr().

55

We have an AST matcher for this (isExpansionInSystemHeader()).

[czhang]

clang-tools-extra/clang-tidy/bugprone/DynamicStaticInitializersCheck.cpp

39

I think only variables with static storage are relevant to the stated goal of the checker.

55

Isn't this for system headers only, not just included 'user' headers?

[aaron.ballman]

clang-tools-extra/clang-tidy/bugprone/DynamicStaticInitializersCheck.cpp

39

Variables declared at file scope have static storage duration by default.

55

Ahh, good point! Still, this should be trivial to make a local AST matcher for.

[czhang]

Hoist constant initializer check and rebase

[czhang]

clang-tools-extra/clang-tidy/bugprone/DynamicStaticInitializersCheck.cpp

41

Perhaps not done in the most elegant way, but there is some amount of non-trivial side effecting caused by checkInitIsICE that makes me a little wary try to chain this more declaratively.

55

Seems like many of the google tidy checks choose to relegate this header checking (whence I copied this bit) in the checker, rather than match registration time. Not sure what the pros and cons are of hoisting, but I left it there to follow the convention of the other checks.

[aaron.ballman]

clang-tools-extra/docs/clang-tidy/checks/bugprone-dynamic-static-initializers.rst

10

Agreed, I'd like to see an example in the docs.

clang-tools-extra/test/clang-tidy/bugprone-dynamic-static-initializers.hpp

34–36

What problems can be caused here? Typically, dynamic init is only problematic when it happens before main() is executed (because of initialization order dependencies), but that doesn't apply to local statics.

[czhang]

clang-tools-extra/test/clang-tidy/bugprone-dynamic-static-initializers.hpp

34–36

Consider the case when synchronization is disabled for static initialization, and two threads call foo2 for the first time. It may be the case that they both try and initialize the static variable at the same time with different values (since the dynamic initializer may not be pure), creating a race condition.

[czhang]

Add example in documentation.

[aaron.ballman]

clang-tools-extra/test/clang-tidy/bugprone-dynamic-static-initializers.hpp

34–36

Consider the case when synchronization is disabled for static initialization

This is a compiler bug, though: http://eel.is/c++draft/stmt.dcl#4.sentence-3

[czhang]

clang-tools-extra/test/clang-tidy/bugprone-dynamic-static-initializers.hpp

34–36

Sorry, I guess I didn't make it clear enough in the rst documentation file, but this check is for those who explicitly enable the -fno-threadsafe-statics flag because they provide their own synchronization. Then they would like to check if the headers they didn't write may possibly run into this issue when compiling with this flag.

[aaron.ballman]

clang-tools-extra/test/clang-tidy/bugprone-dynamic-static-initializers.hpp

34–36

Ah! Thank you for the explanation. In that case, this behavior makes more sense, but I think you should only warn if the user has enabled that feature flag rather than always warning.

[czhang]

clang-tools-extra/test/clang-tidy/bugprone-dynamic-static-initializers.hpp

34–36

I haven't been able to find much documentation on how to actually make a tidy check run against a feature flag. Is it possible to do this? I would think that said users would manually run this check on their header files.

[lebedev.ri]

clang-tools-extra/test/clang-tidy/bugprone-dynamic-static-initializers.hpp

34–36

Sorry, I guess I didn't make it clear enough in the rst documentation file, but this check is for those who explicitly enable the -fno-threadsafe-statics flag because they provide their own synchronization.

I too want to see this explicitly spelled out in documentation.

Then they would like to check if the headers they didn't write may possibly run into this issue when compiling with this flag.

I'm very much not a fan of this solution.
Are you sure that is not exposed in LangOptions, e.g. as ThreadsafeStatics?

[czhang]

Be more explicit about -fno-threadsafe-statics

clang-tools-extra/test/clang-tidy/bugprone-dynamic-static-initializers.hpp

34–36

I'm very much not a fan of this solution.
Are you sure that is not exposed in LangOptions, e.g. as ThreadsafeStatics?

Can you clarify what you mean? Are you not a fan of users disabling synchronization and providing their own? Or are you agreeing with Aaron that we should only enable this check with ThreadsafeStatics is enabled?

Either way, I have put a check against LangOpts for this now.

[lebedev.ri]

I think this last update clarified the concerns i raised in last comment.
But, did you mean to update the tests? They should have broke now that you require !getLangOpts().ThreadsafeStatics.

clang-tools-extra/docs/clang-tidy/checks/bugprone-dynamic-static-initializers.rst

12

escape it:

``-fno-threadsafe-statics``

[aaron.ballman]

clang-tools-extra/clang-tidy/bugprone/DynamicStaticInitializersCheck.cpp

48

Why is this check disabled for C++? I would expect dynamic init of a static in a C++ header file would be flagged by this check.

clang-tools-extra/test/clang-tidy/bugprone-dynamic-static-initializers.hpp

2

I'm a bit confused.

1. Why is this file a .hpp if the check is currently supposed to be C-only?
2. Why are there no #include directives in the test if the check is supposed to only work on header files?

As best I can tell, this is a moral equivalent to doing: clang -x c whatever.h, and so this should be a compilation unit and not a header file. @alexfh, do you have thoughts there?

[czhang]

clang-tools-extra/clang-tidy/bugprone/DynamicStaticInitializersCheck.cpp

48

I'm confused now. If the language is not C++, we do an early return; that is, the check is run if we are on C++. Perhaps the early return is too confusing?

[lebedev.ri]

clang-tools-extra/clang-tidy/bugprone/DynamicStaticInitializersCheck.cpp

48

Then the question is opposite, is this meaningless for C?

[czhang]

clang-tools-extra/clang-tidy/bugprone/DynamicStaticInitializersCheck.cpp

48

C can only initialize statics with constants, right?

[aaron.ballman]

clang-tools-extra/clang-tidy/bugprone/DynamicStaticInitializersCheck.cpp

48

Oye, my eyes saw one thing and my brain saw another. :-P You're right, @czhang, this code is correct as-is (C only initializes statics with a constant).

[czhang]

Fix test and formatting.

[alexfh]

clang-tools-extra/test/clang-tidy/bugprone-dynamic-static-initializers.hpp

2

Why is this file a .hpp if the check is currently supposed to be C-only?

This seems to have been cleared in a different comment: the check is C++-only.

Why are there no #include directives in the test if the check is supposed to only work on header files?

Since the check is using a list of extensions to figure out, if a file is a header, it's not overly important to actually add a separate file that includes this one. And there's a certain benefit in convenience of not doing so (it's nice when CHECK-... and RUN: directives just work). At least one more check - misc-definitions-in-headers - does it the same way.

[aaron.ballman]

LGTM!

clang-tools-extra/test/clang-tidy/bugprone-dynamic-static-initializers.hpp

2

Awesome, thank you for weighing in, @alexfh!

[ychen]

Since Charlie has completed his internship before LGTM, I'll commit this on his behalf.

clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp

 #include "BranchCloneCheck.h"
 #include "CopyConstructorInitCheck.h"
 #include "DanglingHandleCheck.h"
+#include "DynamicStaticInitializersCheck.h"
 #include "ExceptionEscapeCheck.h"
 #include "FoldInitTypeCheck.h"
 #include "ForwardDeclarationNamespaceCheck.h"

         "bugprone-copy-constructor-init");
     CheckFactories.registerCheck<DanglingHandleCheck>(
         "bugprone-dangling-handle");
+    CheckFactories.registerCheck<DynamicStaticInitializersCheck>(
+        "bugprone-dynamic-static-initializers");
     CheckFactories.registerCheck<ExceptionEscapeCheck>(
         "bugprone-exception-escape");
     CheckFactories.registerCheck<FoldInitTypeCheck>(

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

   BugproneTidyModule.cpp
   CopyConstructorInitCheck.cpp
   DanglingHandleCheck.cpp
+  DynamicStaticInitializersCheck.cpp
   ExceptionEscapeCheck.cpp
   FoldInitTypeCheck.cpp
   ForwardDeclarationNamespaceCheck.cpp

clang-tools-extra/clang-tidy/bugprone/DynamicStaticInitializersCheck.h

+//===--- DynamicStaticInitializersCheck.h - clang-tidy ----------*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_DYNAMIC_STATIC_INITIALIZERS_CHECK_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_DYNAMIC_STATIC_INITIALIZERS_CHECK_H
+
+#include "../ClangTidyCheck.h"
+#include "../utils/HeaderFileExtensionsUtils.h"
+
+namespace clang {
+namespace tidy {
+namespace bugprone {
+
+/// Finds dynamically initialized static variables in header files.
+///
+/// The check supports these options:
+///   - `HeaderFileExtensions`: a comma-separated list of filename extensions of
+///     header files (The filename extensions should not contain "." prefix).
+///     "h,hh,hpp,hxx" by default.
+///     For extension-less header files, using an empty string or leaving an
+///     empty string between "," if there are other filename extensions.
+class DynamicStaticInitializersCheck : public ClangTidyCheck {
+public:
+  DynamicStaticInitializersCheck(StringRef Name, ClangTidyContext *Context);
+  void storeOptions(ClangTidyOptions::OptionMap &Opts) override;
+  void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+  void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+
+private:
+  const std::string RawStringHeaderFileExtensions;
+  utils::HeaderFileExtensionsSet HeaderFileExtensions;
+};
+
+} // namespace bugprone
+} // namespace tidy
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_DYNAMIC_STATIC_INITIALIZERS_CHECK_H

clang-tools-extra/clang-tidy/bugprone/DynamicStaticInitializersCheck.cpp

+//===--- DynamicStaticInitializersCheck.cpp - clang-tidy ------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#include "DynamicStaticInitializersCheck.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+
+using namespace clang::ast_matchers;
+
+namespace clang {
+namespace tidy {
+namespace bugprone {
+
+DynamicStaticInitializersCheck::DynamicStaticInitializersCheck(StringRef Name,
+                                                               ClangTidyContext *Context)
+    : ClangTidyCheck(Name, Context),
+      RawStringHeaderFileExtensions(Options.getLocalOrGlobal(
+        "HeaderFileExtensions", utils::defaultHeaderFileExtensions())) {
+  if (!utils::parseHeaderFileExtensions(RawStringHeaderFileExtensions,
+                                        HeaderFileExtensions, ',')) {
+    llvm::errs() << "Invalid header file extension: "
+                 << RawStringHeaderFileExtensions << "\n";
+  }
+}
+
+void DynamicStaticInitializersCheck::storeOptions(
+    ClangTidyOptions::OptionMap &Opts) {
+  Options.store(Opts, "HeaderFileExtensions", RawStringHeaderFileExtensions);
+}
+
+void DynamicStaticInitializersCheck::registerMatchers(MatchFinder *Finder) {
+  if (!getLangOpts().CPlusPlus)
+    return;
+  Finder->addMatcher(varDecl(hasGlobalStorage()).bind("var"), this);
+}
+
+static bool hasConstantDeclaration(const VarDecl *Var) {
+  const Expr *Init = Var->getInit();
+  if (Init && !Init->isValueDependent()) {
+    if (Var->isConstexpr())
+      return true;
+    return Var->checkInitIsICE();
+  }
+  return false;
+}
+
+void DynamicStaticInitializersCheck::check(const MatchFinder::MatchResult &Result) {
+  const auto *Var = Result.Nodes.getNodeAs<VarDecl>("var");
+  SourceLocation Loc = Var->getLocation();
+  if (!Loc.isValid() || !utils::isPresumedLocInHeaderFile(Loc, *Result.SourceManager,
+                                                          HeaderFileExtensions))
+    return;
+  // If the initializer is a constant expression, then the compiler
+  // doesn't have to dynamically initialize it.
+  if (!hasConstantDeclaration(Var))
+    diag(Loc, "static variable %0 may be dynamically initialized in this header file")
+      << Var;
+}
+
+} // namespace bugprone
+} // namespace tidy
+} // namespace clang

clang-tools-extra/docs/ReleaseNotes.rst

   Finds and fixes ``absl::Time`` subtraction expressions to do subtraction
   in the Time domain instead of the numeric domain.
 
+- New :doc:`bugprone-dynamic-static-initializers
+  <clang-tidy/checks/bugprone-dynamic-static-initializers>` check.
+
+  Finds instances where variables with static storage are initialized
+  dynamically in header files.
+
 - New :doc:`bugprone-unhandled-self-assignment
   <clang-tidy/checks/bugprone-unhandled-self-assignment>` check.
 

clang-tools-extra/docs/clang-tidy/checks/bugprone-dynamic-static-initializers.rst

+.. title:: clang-tidy - bugprone-dynamic-static-initializers
+
+bugprone-dynamic-static-initializers
+====================================
+
+Finds instances of static variables that are dynamically initialized
+in header files.
+
+This can pose problems in certain multithreaded contexts. For example,
+when disabling compiler generated synchronization instructions for
+static variables initialized at runtime, even if a particular project
+takes the necessary precautions to prevent race conditions during
+initialization, header files included from other projects may
+not. Therefore, such a check is helpful for ensuring that disabling
+synchronization for static variable initialization will not cause
+problems.

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

    bugprone-branch-clone
    bugprone-copy-constructor-init
    bugprone-dangling-handle
+   bugprone-dynamic-static-initializers
    bugprone-exception-escape
    bugprone-fold-init-type
    bugprone-forward-declaration-namespace

clang-tools-extra/test/clang-tidy/bugprone-dynamic-static-initializers.hpp

+// RUN: %check_clang_tidy %s bugprone-dynamic-static-initializers %t
+
+int fact(int n) {
+  return (n == 0) ? 1 : n * fact(n - 1);
+}
+
+int static_thing = fact(5);
+// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: static variable 'static_thing' may be dynamically initialized in this header file [bugprone-dynamic-static-initializers]
+
+int sample() {
+    int x;
+    return x;
+}
+
+int dynamic_thing = sample();
+// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: static variable 'dynamic_thing' may be dynamically initialized in this header file [bugprone-dynamic-static-initializers]
+
+int not_so_bad = 12 + 4942; // no warning
+
+extern int bar();
+
+int foo() {
+  static int k = bar();
+  // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: static variable 'k' may be dynamically initialized in this header file [bugprone-dynamic-static-initializers]
+  return k;
+}
+
+int bar2() {
+  return 7;
+}
+
+int foo2() {
+  // This may work fine when optimization is enabled because bar() can
+  // be turned into a constant 7.  But without optimization, it can
+  // cause problems. Therefore, we must err on the side of conservatism.
+  static int x = bar2();
+  // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: static variable 'x' may be dynamically initialized in this header file [bugprone-dynamic-static-initializers]
+  return x;
+}
+
+int foo3() {
+  static int p = 7 + 83; // no warning
+  return p;
+}