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

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

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

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

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

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

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

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

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

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

[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: