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

[Phabricator via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL369568: [clang-tidy] Check for dynamically initialized 
statics in headers. (authored by yuanfang, committed by ).
Herald added a project: LLVM.

Changed prior to commit:
  https://reviews.llvm.org/D62829?vs=214275=216451#toc

Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D62829/new/

https://reviews.llvm.org/D62829

Files:
  clang-tools-extra/trunk/clang-tidy/bugprone/BugproneTidyModule.cpp
  clang-tools-extra/trunk/clang-tidy/bugprone/CMakeLists.txt
  clang-tools-extra/trunk/clang-tidy/bugprone/DynamicStaticInitializersCheck.cpp
  clang-tools-extra/trunk/clang-tidy/bugprone/DynamicStaticInitializersCheck.h
  clang-tools-extra/trunk/docs/ReleaseNotes.rst
  
clang-tools-extra/trunk/docs/clang-tidy/checks/bugprone-dynamic-static-initializers.rst
  clang-tools-extra/trunk/docs/clang-tidy/checks/list.rst
  
clang-tools-extra/trunk/test/clang-tidy/bugprone-dynamic-static-initializers.hpp

Index: clang-tools-extra/trunk/clang-tidy/bugprone/BugproneTidyModule.cpp
===
--- clang-tools-extra/trunk/clang-tidy/bugprone/BugproneTidyModule.cpp
+++ clang-tools-extra/trunk/clang-tidy/bugprone/BugproneTidyModule.cpp
@@ -16,6 +16,7 @@
 #include "BranchCloneCheck.h"
 #include "CopyConstructorInitCheck.h"
 #include "DanglingHandleCheck.h"
+#include "DynamicStaticInitializersCheck.h"
 #include "ExceptionEscapeCheck.h"
 #include "FoldInitTypeCheck.h"
 #include "ForwardDeclarationNamespaceCheck.h"
@@ -73,6 +74,8 @@
 "bugprone-copy-constructor-init");
 CheckFactories.registerCheck(
 "bugprone-dangling-handle");
+CheckFactories.registerCheck(
+"bugprone-dynamic-static-initializers");
 CheckFactories.registerCheck(
 "bugprone-exception-escape");
 CheckFactories.registerCheck(
Index: clang-tools-extra/trunk/clang-tidy/bugprone/DynamicStaticInitializersCheck.cpp
===
--- clang-tools-extra/trunk/clang-tidy/bugprone/DynamicStaticInitializersCheck.cpp
+++ clang-tools-extra/trunk/clang-tidy/bugprone/DynamicStaticInitializersCheck.cpp
@@ -0,0 +1,68 @@
+//===--- 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 {
+
+AST_MATCHER(clang::VarDecl, hasConstantDeclaration) {
+  const Expr *Init = Node.getInit();
+  if (Init && !Init->isValueDependent()) {
+if (Node.isConstexpr())
+  return true;
+return Node.checkInitIsICE();
+  }
+  return false;
+}
+
+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 ) {
+  Options.store(Opts, "HeaderFileExtensions", RawStringHeaderFileExtensions);
+}
+
+void DynamicStaticInitializersCheck::registerMatchers(MatchFinder *Finder) {
+  if (!getLangOpts().CPlusPlus || getLangOpts().ThreadsafeStatics)
+return;
+  Finder->addMatcher(
+  varDecl(hasGlobalStorage(), unless(hasConstantDeclaration())).bind("var"),
+  this);
+}
+
+void DynamicStaticInitializersCheck::check(const MatchFinder::MatchResult ) {
+  const auto *Var = Result.Nodes.getNodeAs("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.
+  diag(Loc, "static variable %0 may be dynamically initialized in this header file")
+<< Var;
+}
+
+} // namespace bugprone
+} // namespace tidy
+} // namespace clang
Index: clang-tools-extra/trunk/clang-tidy/bugprone/CMakeLists.txt
===
--- 

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

[Yuanfang Chen via Phabricator via cfe-commits]

ychen added a comment.

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


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D62829/new/

https://reviews.llvm.org/D62829



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman accepted this revision.
aaron.ballman added a comment.
This revision is now accepted and ready to land.

LGTM!




Comment at: 
clang-tools-extra/test/clang-tidy/bugprone-dynamic-static-initializers.hpp:1
+// RUN: %check_clang_tidy %s bugprone-dynamic-static-initializers %t
+

alexfh wrote:
> aaron.ballman wrote:
> > 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?
> > 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.
Awesome, thank you for weighing in, @alexfh!


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D62829/new/

https://reviews.llvm.org/D62829



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Alexander Kornienko via Phabricator via cfe-commits]

alexfh added inline comments.



Comment at: 
clang-tools-extra/test/clang-tidy/bugprone-dynamic-static-initializers.hpp:1
+// RUN: %check_clang_tidy %s bugprone-dynamic-static-initializers %t
+

aaron.ballman wrote:
> 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?
> 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.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D62829/new/

https://reviews.llvm.org/D62829



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Charles Zhang via Phabricator via cfe-commits]

czhang updated this revision to Diff 214275.
czhang added a comment.

Fix test and formatting.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D62829/new/

https://reviews.llvm.org/D62829

Files:
  clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp
  clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt
  clang-tools-extra/clang-tidy/bugprone/DynamicStaticInitializersCheck.cpp
  clang-tools-extra/clang-tidy/bugprone/DynamicStaticInitializersCheck.h
  clang-tools-extra/docs/ReleaseNotes.rst
  
clang-tools-extra/docs/clang-tidy/checks/bugprone-dynamic-static-initializers.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  clang-tools-extra/test/clang-tidy/bugprone-dynamic-static-initializers.hpp

Index: clang-tools-extra/test/clang-tidy/bugprone-dynamic-static-initializers.hpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/bugprone-dynamic-static-initializers.hpp
@@ -0,0 +1,44 @@
+// RUN: %check_clang_tidy %s bugprone-dynamic-static-initializers %t -- -- -fno-threadsafe-statics
+
+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;
+}
Index: clang-tools-extra/docs/clang-tidy/checks/list.rst
===
--- clang-tools-extra/docs/clang-tidy/checks/list.rst
+++ clang-tools-extra/docs/clang-tidy/checks/list.rst
@@ -43,6 +43,7 @@
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
Index: clang-tools-extra/docs/clang-tidy/checks/bugprone-dynamic-static-initializers.rst
===
--- /dev/null
+++ clang-tools-extra/docs/clang-tidy/checks/bugprone-dynamic-static-initializers.rst
@@ -0,0 +1,27 @@
+.. 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 (e.g. by ``-fno-threadsafe-statics``), even if a particular project
+takes the necessary precautions to prevent race conditions during
+initialization by providing their own synchronization, header files included from other projects may
+not. Therefore, such a check is helpful for ensuring that disabling
+compiler generated synchronization for static variable initialization will not cause
+problems.
+
+Consider the following code:
+
+-- code-block:: c
+
+int foo() {
+  static int k = bar();
+  return k;
+}
+
+When synchronization of static initialization is disabled, if two threads both call `foo` for the first time, there is the possibility that `k` will be double initialized, creating a race condition.
Index: clang-tools-extra/docs/ReleaseNotes.rst
===
--- clang-tools-extra/docs/ReleaseNotes.rst
+++ clang-tools-extra/docs/ReleaseNotes.rst
@@ -111,6 +111,12 @@
 
   This checks ensures that ``pipe2()`` is called with the O_CLOEXEC flag.
 
+- New :doc:`bugprone-dynamic-static-initializers
+  ` check.
+
+  Finds instances where variables with static storage are initialized
+  dynamically in header files.
+
 - New :doc:`bugprone-unhandled-self-assignment
   ` check.
 
Index: clang-tools-extra/clang-tidy/bugprone/DynamicStaticInitializersCheck.h
===
--- /dev/null
+++ 

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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: 
clang-tools-extra/clang-tidy/bugprone/DynamicStaticInitializersCheck.cpp:47
+void DynamicStaticInitializersCheck::registerMatchers(MatchFinder *Finder) {
+  if (!getLangOpts().CPlusPlus || getLangOpts().ThreadsafeStatics)
+return;

czhang wrote:
> lebedev.ri wrote:
> > czhang wrote:
> > > aaron.ballman wrote:
> > > > 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.
> > > 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?
> > Then the question is opposite, is this meaningless for C?
> C can only initialize statics with constants, right?
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).


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D62829/new/

https://reviews.llvm.org/D62829



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Charles Zhang via Phabricator via cfe-commits]

czhang marked an inline comment as done.
czhang added inline comments.



Comment at: 
clang-tools-extra/clang-tidy/bugprone/DynamicStaticInitializersCheck.cpp:47
+void DynamicStaticInitializersCheck::registerMatchers(MatchFinder *Finder) {
+  if (!getLangOpts().CPlusPlus || getLangOpts().ThreadsafeStatics)
+return;

lebedev.ri wrote:
> czhang wrote:
> > aaron.ballman wrote:
> > > 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.
> > 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?
> Then the question is opposite, is this meaningless for C?
C can only initialize statics with constants, right?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D62829/new/

https://reviews.llvm.org/D62829



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added inline comments.



Comment at: 
clang-tools-extra/clang-tidy/bugprone/DynamicStaticInitializersCheck.cpp:47
+void DynamicStaticInitializersCheck::registerMatchers(MatchFinder *Finder) {
+  if (!getLangOpts().CPlusPlus || getLangOpts().ThreadsafeStatics)
+return;

czhang wrote:
> aaron.ballman wrote:
> > 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.
> 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?
Then the question is opposite, is this meaningless for C?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D62829/new/

https://reviews.llvm.org/D62829



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Charles Zhang via Phabricator via cfe-commits]

czhang marked an inline comment as done.
czhang added inline comments.



Comment at: 
clang-tools-extra/clang-tidy/bugprone/DynamicStaticInitializersCheck.cpp:47
+void DynamicStaticInitializersCheck::registerMatchers(MatchFinder *Finder) {
+  if (!getLangOpts().CPlusPlus || getLangOpts().ThreadsafeStatics)
+return;

aaron.ballman wrote:
> 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.
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?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D62829/new/

https://reviews.llvm.org/D62829



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: 
clang-tools-extra/clang-tidy/bugprone/DynamicStaticInitializersCheck.cpp:47
+void DynamicStaticInitializersCheck::registerMatchers(MatchFinder *Finder) {
+  if (!getLangOpts().CPlusPlus || getLangOpts().ThreadsafeStatics)
+return;

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.



Comment at: 
clang-tools-extra/test/clang-tidy/bugprone-dynamic-static-initializers.hpp:1
+// RUN: %check_clang_tidy %s bugprone-dynamic-static-initializers %t
+

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?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D62829/new/

https://reviews.llvm.org/D62829



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added a comment.

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




Comment at: 
clang-tools-extra/docs/clang-tidy/checks/bugprone-dynamic-static-initializers.rst:11
+when disabling compiler generated synchronization instructions for
+static variables initialized at runtime (e.g. by -fno-threadsafe-statics), 
even if a particular project
+takes the necessary precautions to prevent race conditions during

escape it:
```
``-fno-threadsafe-statics``
```


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D62829/new/

https://reviews.llvm.org/D62829



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Charles Zhang via Phabricator via cfe-commits]

czhang added inline comments.



Comment at: 
clang-tools-extra/test/clang-tidy/bugprone-dynamic-static-initializers.hpp:33-35
+  // 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.

lebedev.ri wrote:
> czhang wrote:
> > aaron.ballman wrote:
> > > czhang wrote:
> > > > aaron.ballman wrote:
> > > > > czhang wrote:
> > > > > > aaron.ballman wrote:
> > > > > > > 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.
> > > > > > 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.
> > > > > > 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
> > > > 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.
> > > 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.
> > 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.
> > 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`?
> 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. 


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D62829/new/

https://reviews.llvm.org/D62829



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Charles Zhang via Phabricator via cfe-commits]

czhang updated this revision to Diff 213102.
czhang marked an inline comment as done.
czhang added a comment.

Be more explicit about -fno-threadsafe-statics


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D62829/new/

https://reviews.llvm.org/D62829

Files:
  clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp
  clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt
  clang-tools-extra/clang-tidy/bugprone/DynamicStaticInitializersCheck.cpp
  clang-tools-extra/clang-tidy/bugprone/DynamicStaticInitializersCheck.h
  clang-tools-extra/docs/ReleaseNotes.rst
  
clang-tools-extra/docs/clang-tidy/checks/bugprone-dynamic-static-initializers.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  clang-tools-extra/test/clang-tidy/bugprone-dynamic-static-initializers.hpp

Index: clang-tools-extra/test/clang-tidy/bugprone-dynamic-static-initializers.hpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/bugprone-dynamic-static-initializers.hpp
@@ -0,0 +1,44 @@
+// 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;
+}
Index: clang-tools-extra/docs/clang-tidy/checks/list.rst
===
--- clang-tools-extra/docs/clang-tidy/checks/list.rst
+++ clang-tools-extra/docs/clang-tidy/checks/list.rst
@@ -43,6 +43,7 @@
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
Index: clang-tools-extra/docs/clang-tidy/checks/bugprone-dynamic-static-initializers.rst
===
--- /dev/null
+++ clang-tools-extra/docs/clang-tidy/checks/bugprone-dynamic-static-initializers.rst
@@ -0,0 +1,27 @@
+.. 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 (e.g. by -fno-threadsafe-statics), even if a particular project
+takes the necessary precautions to prevent race conditions during
+initialization by providing their own synchronization, header files included from other projects may
+not. Therefore, such a check is helpful for ensuring that disabling
+compiler generated synchronization for static variable initialization will not cause
+problems.
+
+Consider the following code:
+
+-- code-block:: c
+
+int foo() {
+  static int k = bar();
+  return k;
+}
+
+When synchronization of static initialization is disabled, if two threads both call `foo` for the first time, there is the possibility that `k` will be double initialized, creating a race condition.
Index: clang-tools-extra/docs/ReleaseNotes.rst
===
--- clang-tools-extra/docs/ReleaseNotes.rst
+++ clang-tools-extra/docs/ReleaseNotes.rst
@@ -111,6 +111,12 @@
 
   This checks ensures that ``pipe2()`` is called with the O_CLOEXEC flag.
 
+- New :doc:`bugprone-dynamic-static-initializers
+  ` check.
+
+  Finds instances where variables with static storage are initialized
+  dynamically in header files.
+
 - New :doc:`bugprone-unhandled-self-assignment
   ` check.
 
Index: clang-tools-extra/clang-tidy/bugprone/DynamicStaticInitializersCheck.h

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

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added inline comments.



Comment at: 
clang-tools-extra/test/clang-tidy/bugprone-dynamic-static-initializers.hpp:33-35
+  // 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.

czhang wrote:
> aaron.ballman wrote:
> > czhang wrote:
> > > aaron.ballman wrote:
> > > > czhang wrote:
> > > > > aaron.ballman wrote:
> > > > > > 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.
> > > > > 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.
> > > > > 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
> > > 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.
> > 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.
> 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.
> 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`?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D62829/new/

https://reviews.llvm.org/D62829



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Charles Zhang via Phabricator via cfe-commits]

czhang marked 2 inline comments as done.
czhang added inline comments.



Comment at: 
clang-tools-extra/test/clang-tidy/bugprone-dynamic-static-initializers.hpp:33-35
+  // 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.

aaron.ballman wrote:
> czhang wrote:
> > aaron.ballman wrote:
> > > czhang wrote:
> > > > aaron.ballman wrote:
> > > > > 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.
> > > > 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.
> > > > 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
> > 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.
> 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.
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.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D62829/new/

https://reviews.llvm.org/D62829



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: 
clang-tools-extra/test/clang-tidy/bugprone-dynamic-static-initializers.hpp:33-35
+  // 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.

czhang wrote:
> aaron.ballman wrote:
> > czhang wrote:
> > > aaron.ballman wrote:
> > > > 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.
> > > 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.
> > > 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
> 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.
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.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D62829/new/

https://reviews.llvm.org/D62829



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Charles Zhang via Phabricator via cfe-commits]

czhang marked 2 inline comments as done.
czhang added inline comments.



Comment at: 
clang-tools-extra/test/clang-tidy/bugprone-dynamic-static-initializers.hpp:33-35
+  // 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.

aaron.ballman wrote:
> czhang wrote:
> > aaron.ballman wrote:
> > > 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.
> > 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.
> > 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
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.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D62829/new/

https://reviews.llvm.org/D62829



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: 
clang-tools-extra/test/clang-tidy/bugprone-dynamic-static-initializers.hpp:33-35
+  // 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.

czhang wrote:
> aaron.ballman wrote:
> > 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.
> 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.
> 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


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D62829/new/

https://reviews.llvm.org/D62829



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Charles Zhang via Phabricator via cfe-commits]

czhang updated this revision to Diff 212890.
czhang marked an inline comment as done.
czhang added a comment.

Add example in documentation.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D62829/new/

https://reviews.llvm.org/D62829

Files:
  clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp
  clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt
  clang-tools-extra/clang-tidy/bugprone/DynamicStaticInitializersCheck.cpp
  clang-tools-extra/clang-tidy/bugprone/DynamicStaticInitializersCheck.h
  clang-tools-extra/docs/ReleaseNotes.rst
  
clang-tools-extra/docs/clang-tidy/checks/bugprone-dynamic-static-initializers.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  clang-tools-extra/test/clang-tidy/bugprone-dynamic-static-initializers.hpp

Index: clang-tools-extra/test/clang-tidy/bugprone-dynamic-static-initializers.hpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/bugprone-dynamic-static-initializers.hpp
@@ -0,0 +1,44 @@
+// 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;
+}
Index: clang-tools-extra/docs/clang-tidy/checks/list.rst
===
--- clang-tools-extra/docs/clang-tidy/checks/list.rst
+++ clang-tools-extra/docs/clang-tidy/checks/list.rst
@@ -43,6 +43,7 @@
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
Index: clang-tools-extra/docs/clang-tidy/checks/bugprone-dynamic-static-initializers.rst
===
--- /dev/null
+++ clang-tools-extra/docs/clang-tidy/checks/bugprone-dynamic-static-initializers.rst
@@ -0,0 +1,27 @@
+.. 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.
+
+Consider the following code:
+
+-- code-block:: c
+
+int foo() {
+  static int k = bar();
+  return k;
+}
+
+When synchronization of static initialization is disabled, if two threads both call `foo` for the first time, there is the possibility that `k` will be double initialized, creating a race condition.
Index: clang-tools-extra/docs/ReleaseNotes.rst
===
--- clang-tools-extra/docs/ReleaseNotes.rst
+++ clang-tools-extra/docs/ReleaseNotes.rst
@@ -111,6 +111,12 @@
 
   This checks ensures that ``pipe2()`` is called with the O_CLOEXEC flag.
 
+- New :doc:`bugprone-dynamic-static-initializers
+  ` check.
+
+  Finds instances where variables with static storage are initialized
+  dynamically in header files.
+
 - New :doc:`bugprone-unhandled-self-assignment
   ` check.
 
Index: clang-tools-extra/clang-tidy/bugprone/DynamicStaticInitializersCheck.h
===
--- /dev/null
+++ clang-tools-extra/clang-tidy/bugprone/DynamicStaticInitializersCheck.h
@@ -0,0 +1,43 

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

[Charles Zhang via Phabricator via cfe-commits]

czhang marked 3 inline comments as done.
czhang added inline comments.



Comment at: 
clang-tools-extra/test/clang-tidy/bugprone-dynamic-static-initializers.hpp:33-35
+  // 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.

aaron.ballman wrote:
> 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.
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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D62829/new/

https://reviews.llvm.org/D62829



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: 
clang-tools-extra/docs/clang-tidy/checks/bugprone-dynamic-static-initializers.rst:9
+
+This can pose problems in certain multithreaded contexts.

Eugene.Zelenko wrote:
> Will be good idea to provide example.
Agreed, I'd like to see an example in the docs.



Comment at: 
clang-tools-extra/test/clang-tidy/bugprone-dynamic-static-initializers.hpp:33-35
+  // 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.

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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D62829/new/

https://reviews.llvm.org/D62829



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Charles Zhang via Phabricator via cfe-commits]

czhang marked 4 inline comments as done.
czhang added inline comments.



Comment at: 
clang-tools-extra/clang-tidy/bugprone/DynamicStaticInitializersCheck.cpp:40
+return;
+  Finder->addMatcher(varDecl().bind("var"), this);
+}

aaron.ballman wrote:
> czhang wrote:
> > lebedev.ri wrote:
> > > Most of the matching should be done here, not in `check()`.
> > I believe that there is no way to use the AST matching language to express 
> > hasConstantDeclaration.
> 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()`.
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.



Comment at: 
clang-tools-extra/clang-tidy/bugprone/DynamicStaticInitializersCheck.cpp:55
+  SourceLocation Loc = Var->getLocation();
+  if (!Loc.isValid() || !utils::isPresumedLocInHeaderFile(Loc, 
*Result.SourceManager,
+  
HeaderFileExtensions))

aaron.ballman wrote:
> czhang wrote:
> > aaron.ballman wrote:
> > > We have an AST matcher for this (`isExpansionInSystemHeader()`).
> > Isn't this for system headers only, not just included 'user' headers?
> Ahh, good point! Still, this should be trivial to make a local AST matcher 
> for.
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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D62829/new/

https://reviews.llvm.org/D62829



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Charles Zhang via Phabricator via cfe-commits]

czhang updated this revision to Diff 209071.
czhang added a comment.

Hoist constant initializer check and rebase


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D62829/new/

https://reviews.llvm.org/D62829

Files:
  clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp
  clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt
  clang-tools-extra/clang-tidy/bugprone/DynamicStaticInitializersCheck.cpp
  clang-tools-extra/clang-tidy/bugprone/DynamicStaticInitializersCheck.h
  clang-tools-extra/docs/ReleaseNotes.rst
  
clang-tools-extra/docs/clang-tidy/checks/bugprone-dynamic-static-initializers.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  clang-tools-extra/test/clang-tidy/bugprone-dynamic-static-initializers.hpp

Index: clang-tools-extra/test/clang-tidy/bugprone-dynamic-static-initializers.hpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/bugprone-dynamic-static-initializers.hpp
@@ -0,0 +1,44 @@
+// 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;
+}
Index: clang-tools-extra/docs/clang-tidy/checks/list.rst
===
--- clang-tools-extra/docs/clang-tidy/checks/list.rst
+++ clang-tools-extra/docs/clang-tidy/checks/list.rst
@@ -43,6 +43,7 @@
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
Index: clang-tools-extra/docs/clang-tidy/checks/bugprone-dynamic-static-initializers.rst
===
--- /dev/null
+++ clang-tools-extra/docs/clang-tidy/checks/bugprone-dynamic-static-initializers.rst
@@ -0,0 +1,16 @@
+.. 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.
Index: clang-tools-extra/docs/ReleaseNotes.rst
===
--- clang-tools-extra/docs/ReleaseNotes.rst
+++ clang-tools-extra/docs/ReleaseNotes.rst
@@ -111,6 +111,12 @@
 
   This checks ensures that ``pipe2()`` is called with the O_CLOEXEC flag.
 
+- New :doc:`bugprone-dynamic-static-initializers
+  ` check.
+
+  Finds instances where variables with static storage are initialized
+  dynamically in header files.
+
 - New :doc:`bugprone-unhandled-self-assignment
   ` check.
 
Index: clang-tools-extra/clang-tidy/bugprone/DynamicStaticInitializersCheck.h
===
--- /dev/null
+++ clang-tools-extra/clang-tidy/bugprone/DynamicStaticInitializersCheck.h
@@ -0,0 +1,43 @@
+//===--- 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
+//

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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: 
clang-tools-extra/clang-tidy/bugprone/DynamicStaticInitializersCheck.cpp:39
+return;
+  Finder->addMatcher(varDecl(hasGlobalStorage()).bind("var"), this);
+}

czhang wrote:
> aaron.ballman wrote:
> > 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;
> > ```
> I think only variables with static storage are relevant to the stated goal of 
> the checker.
Variables declared at file scope have static storage duration by default.



Comment at: 
clang-tools-extra/clang-tidy/bugprone/DynamicStaticInitializersCheck.cpp:55
+  SourceLocation Loc = Var->getLocation();
+  if (!Loc.isValid() || !utils::isPresumedLocInHeaderFile(Loc, 
*Result.SourceManager,
+  
HeaderFileExtensions))

czhang wrote:
> aaron.ballman wrote:
> > We have an AST matcher for this (`isExpansionInSystemHeader()`).
> Isn't this for system headers only, not just included 'user' headers?
Ahh, good point! Still, this should be trivial to make a local AST matcher for.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D62829/new/

https://reviews.llvm.org/D62829



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Charles Zhang via Phabricator via cfe-commits]

czhang marked an inline comment as done.
czhang added inline comments.



Comment at: 
clang-tools-extra/clang-tidy/bugprone/DynamicStaticInitializersCheck.cpp:39
+return;
+  Finder->addMatcher(varDecl(hasGlobalStorage()).bind("var"), this);
+}

aaron.ballman wrote:
> 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;
> ```
I think only variables with static storage are relevant to the stated goal of 
the checker.



Comment at: 
clang-tools-extra/clang-tidy/bugprone/DynamicStaticInitializersCheck.cpp:55
+  SourceLocation Loc = Var->getLocation();
+  if (!Loc.isValid() || !utils::isPresumedLocInHeaderFile(Loc, 
*Result.SourceManager,
+  
HeaderFileExtensions))

aaron.ballman wrote:
> We have an AST matcher for this (`isExpansionInSystemHeader()`).
Isn't this for system headers only, not just included 'user' headers?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D62829/new/

https://reviews.llvm.org/D62829



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: 
clang-tools-extra/clang-tidy/bugprone/DynamicStaticInitializersCheck.cpp:40
+return;
+  Finder->addMatcher(varDecl().bind("var"), this);
+}

czhang wrote:
> lebedev.ri wrote:
> > Most of the matching should be done here, not in `check()`.
> I believe that there is no way to use the AST matching language to express 
> hasConstantDeclaration.
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()`.



Comment at: 
clang-tools-extra/clang-tidy/bugprone/DynamicStaticInitializersCheck.cpp:39
+return;
+  Finder->addMatcher(varDecl(hasGlobalStorage()).bind("var"), this);
+}

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;
```



Comment at: 
clang-tools-extra/clang-tidy/bugprone/DynamicStaticInitializersCheck.cpp:55
+  SourceLocation Loc = Var->getLocation();
+  if (!Loc.isValid() || !utils::isPresumedLocInHeaderFile(Loc, 
*Result.SourceManager,
+  
HeaderFileExtensions))

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


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D62829/new/

https://reviews.llvm.org/D62829



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Charles Zhang via Phabricator via cfe-commits]

czhang added a comment.

ping


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D62829/new/

https://reviews.llvm.org/D62829



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Charles Zhang via Phabricator via cfe-commits]

czhang added a comment.

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.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D62829/new/

https://reviews.llvm.org/D62829



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added a comment.

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?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D62829/new/

https://reviews.llvm.org/D62829



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Charles Zhang via Phabricator via cfe-commits]

czhang marked an inline comment as done.
czhang added inline comments.



Comment at: 
clang-tools-extra/clang-tidy/bugprone/DynamicStaticInitializersCheck.cpp:40
+return;
+  Finder->addMatcher(varDecl().bind("var"), this);
+}

lebedev.ri wrote:
> Most of the matching should be done here, not in `check()`.
I believe that there is no way to use the AST matching language to express 
hasConstantDeclaration.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D62829/new/

https://reviews.llvm.org/D62829



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Charles Zhang via Phabricator via cfe-commits]

czhang updated this revision to Diff 203462.
czhang marked an inline comment as done.
czhang added a comment.

Addressed comments.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D62829/new/

https://reviews.llvm.org/D62829

Files:
  clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp
  clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt
  clang-tools-extra/clang-tidy/bugprone/DynamicStaticInitializersCheck.cpp
  clang-tools-extra/clang-tidy/bugprone/DynamicStaticInitializersCheck.h
  clang-tools-extra/docs/ReleaseNotes.rst
  
clang-tools-extra/docs/clang-tidy/checks/bugprone-dynamic-static-initializers.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  clang-tools-extra/test/clang-tidy/bugprone-dynamic-static-initializers.hpp

Index: clang-tools-extra/test/clang-tidy/bugprone-dynamic-static-initializers.hpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/bugprone-dynamic-static-initializers.hpp
@@ -0,0 +1,44 @@
+// 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;
+}
Index: clang-tools-extra/docs/clang-tidy/checks/list.rst
===
--- clang-tools-extra/docs/clang-tidy/checks/list.rst
+++ clang-tools-extra/docs/clang-tidy/checks/list.rst
@@ -41,6 +41,7 @@
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
Index: clang-tools-extra/docs/clang-tidy/checks/bugprone-dynamic-static-initializers.rst
===
--- /dev/null
+++ clang-tools-extra/docs/clang-tidy/checks/bugprone-dynamic-static-initializers.rst
@@ -0,0 +1,16 @@
+.. 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.
Index: clang-tools-extra/docs/ReleaseNotes.rst
===
--- clang-tools-extra/docs/ReleaseNotes.rst
+++ clang-tools-extra/docs/ReleaseNotes.rst
@@ -101,6 +101,12 @@
   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
+  ` check.
+
+  Finds instances where variables with static storage are initialized
+  dynamically in header files.
+
 - New :doc:`bugprone-unhandled-self-assignment
   ` check.
 
Index: clang-tools-extra/clang-tidy/bugprone/DynamicStaticInitializersCheck.h
===
--- /dev/null
+++ clang-tools-extra/clang-tidy/bugprone/DynamicStaticInitializersCheck.h
@@ -0,0 +1,43 @@
+//===--- 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: 

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

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added inline comments.



Comment at: 
clang-tools-extra/clang-tidy/bugprone/DynamicStaticInitializersCheck.cpp:37
+void DynamicStaticInitializersCheck::registerMatchers(MatchFinder *Finder) {
+  // FIXME: Add matchers.
+  if (!getLangOpts().CPlusPlus)

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



Comment at: 
clang-tools-extra/clang-tidy/bugprone/DynamicStaticInitializersCheck.cpp:54
+void DynamicStaticInitializersCheck::check(const MatchFinder::MatchResult 
) {
+  // FIXME: Add callback implementation.
+  auto *Var = Result.Nodes.getNodeAs("var");

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



Comment at: 
clang-tools-extra/clang-tidy/bugprone/DynamicStaticInitializersCheck.cpp:55
+  // FIXME: Add callback implementation.
+  auto *Var = Result.Nodes.getNodeAs("var");
+  SourceLocation Loc = Var->getLocation();

Could it be const auto *?



Comment at: 
clang-tools-extra/docs/clang-tidy/checks/bugprone-dynamic-static-initializers.rst:9
+
+This can pose problems in certain multithreaded contexts.

Will be good idea to provide example.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D62829/new/

https://reviews.llvm.org/D62829



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added inline comments.



Comment at: 
clang-tools-extra/clang-tidy/bugprone/DynamicStaticInitializersCheck.cpp:40
+return;
+  Finder->addMatcher(varDecl().bind("var"), this);
+}

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


Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D62829/new/

https://reviews.llvm.org/D62829



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits