diff mbox

[C,C++] Implement -Wstatic-local

Message ID 51EEDDF2.9010001@redhat.com
State New
Headers show

Commit Message

Florian Weimer July 23, 2013, 7:48 p.m. UTC 
We sometimes deal with code bases which use static local variables to 
cut down frame size, for compatibility with legacy targets.  Obviously, 
this is bad for thread safety.  This new warning can be used to track 
down such cases once you suspect they exist.

Bootstrapped and regression-tested on x86_64-redhat-linux-gnu.  Okay for 
trunk?

Comments

Andrew Pinski July 23, 2013, 7:51 p.m. UTC | #1 
On Tue, Jul 23, 2013 at 12:48 PM, Florian Weimer <fweimer@redhat.com> wrote:
> We sometimes deal with code bases which use static local variables to cut
> down frame size, for compatibility with legacy targets.  Obviously, this is
> bad for thread safety.  This new warning can be used to track down such
> cases once you suspect they exist.

Hmm, since you mentioned bad for thread safety but then I see in your
patch you don't check to see if the variable is a thread local
variable.  Maybe you should not mention thread safety at all here or
change the code not to include TLS variables.

Thanks,
Andrew

>
> Bootstrapped and regression-tested on x86_64-redhat-linux-gnu.  Okay for
> trunk?
>
> --
> Florian Weimer / Red Hat Product Security Team
>
Florian Weimer July 23, 2013, 8:02 p.m. UTC | #2 
On 07/23/2013 09:51 PM, Andrew Pinski wrote:
> On Tue, Jul 23, 2013 at 12:48 PM, Florian Weimer <fweimer@redhat.com> wrote:
>> We sometimes deal with code bases which use static local variables to cut
>> down frame size, for compatibility with legacy targets.  Obviously, this is
>> bad for thread safety.  This new warning can be used to track down such
>> cases once you suspect they exist.
>
> Hmm, since you mentioned bad for thread safety but then I see in your
> patch you don't check to see if the variable is a thread local
> variable.  Maybe you should not mention thread safety at all here or
> change the code not to include TLS variables.

Good point.  What about this instead?

Warn for variables declared inside functions which are static, but not
declared const.  Such local variables can make functions not reentrant.
Gabriel Dos Reis July 24, 2013, 1:24 a.m. UTC | #3 
On Tue, Jul 23, 2013 at 3:02 PM, Florian Weimer <fweimer@redhat.com> wrote:
> On 07/23/2013 09:51 PM, Andrew Pinski wrote:
>>
>> On Tue, Jul 23, 2013 at 12:48 PM, Florian Weimer <fweimer@redhat.com>
>> wrote:
>>>
>>> We sometimes deal with code bases which use static local variables to cut
>>> down frame size, for compatibility with legacy targets.  Obviously, this
>>> is
>>> bad for thread safety.  This new warning can be used to track down such
>>> cases once you suspect they exist.
>>
>>
>> Hmm, since you mentioned bad for thread safety but then I see in your
>> patch you don't check to see if the variable is a thread local
>> variable.  Maybe you should not mention thread safety at all here or
>> change the code not to include TLS variables.
>
>
> Good point.  What about this instead?
>
> Warn for variables declared inside functions which are static, but not
> declared const.  Such local variables can make functions not reentrant.

Do you envision this to be useful for C++ too?

There are many bits here.  'const' in C++ does not necessary prevent
race conditions with the variable's type has to run non-atomic constructors,
or if the object has data members declared mutable, etc.

It used to be idiomatic in C++ to use local statics in order to enforce
initialization of "morally global" objects -- the function returns a pointer
or reference to the object.

I think the C++ ABI mandates that the implementation adds implicit locks
to enforce orderly initialization

-- Gaby
Florian Weimer July 24, 2013, 8:07 a.m. UTC | #4 
On 07/24/2013 03:24 AM, Gabriel Dos Reis wrote:

> Do you envision this to be useful for C++ too?

> There are many bits here.  'const' in C++ does not necessary prevent
> race conditions with the variable's type has to run non-atomic constructors,
> or if the object has data members declared mutable, etc.

I think the warning will mean the same thing for PODs without mutable 
fields as in C.  It's true that a const object is not necessarily 
physically const (i.e., in .rodata or .data.rel.ro), but I don't think 
this is a problem.

My main concern here is C, but I think it's useful to have parity 
between the two front ends if possible.

> I think the C++ ABI mandates that the implementation adds implicit locks
> to enforce orderly initialization

Yes, that's how we implement it.
diff mbox

Patch

gcc/ChangeLog:

2013-07-23  Florian Weimer  <fweimer@redhat.com>

	* doc/invoke.texi (Warning Options): Document -Wstatic-local.

c-family/ChangeLog:

2013-07-23  Florian Weimer  <fweimer@redhat.com>

	* c.opt (Wstatic-local): Add option -Wstatic-local.

c/ChangeLog:

2013-07-23  Florian Weimer  <fweimer@redhat.com>

	* c-decl.c (pushdecl): Implement -Wstatic-local.

cp/ChangeLog:

2013-07-23  Florian Weimer  <fweimer@redhat.com>

	* decl.c (cp_finish_decl): Implement -Wstatic-local.

testsuite/ChangeLog:

2013-07-23  Florian Weimer  <fweimer@redhat.com>

	* c-c++-common/Wstatic-local.c: New.
	* g++.dg/warn/Wstatic-local-1.C: New.
	* g++.dg/warn/Wstatic-local-2.C: New.

diff --git a/gcc/c-family/c.opt b/gcc/c-family/c.opt
index 9690a08..7b9f3a2 100644
--- a/gcc/c-family/c.opt
+++ b/gcc/c-family/c.opt
@@ -745,6 +745,10 @@  Wmaybe-uninitialized
 C ObjC C++ ObjC++ LangEnabledBy(C ObjC C++ ObjC++,Wall)
 ;
 
+Wstatic-local
+C ObjC C++ ObjC++ Warning Var(warn_static_local)
+Warn about static local variables
+
 Wunknown-pragmas
 C ObjC C++ ObjC++ Warning Var(warn_unknown_pragmas) LangEnabledBy(C ObjC C++ ObjC++,Wall, 1, 0)
 Warn about unrecognized pragmas
diff --git a/gcc/c/c-decl.c b/gcc/c/c-decl.c
index f7ae648..9c718b5 100644
--- a/gcc/c/c-decl.c
+++ b/gcc/c/c-decl.c
@@ -2608,6 +2608,14 @@  pushdecl (tree x)
 	  || DECL_INITIAL (x) || !DECL_EXTERNAL (x)))
     DECL_CONTEXT (x) = current_function_decl;
 
+  /* Optionally warn about non-const static variables declared inside
+     functions. */
+  if (current_function_decl && warn_static_local
+      && TREE_CODE (x) == VAR_DECL && TREE_STATIC (x)
+      && !TREE_READONLY (x))
+    warning_at (DECL_SOURCE_LOCATION (x), OPT_Wstatic_local,
+		"local variable declared static, but not const");
+
   /* Anonymous decls are just inserted in the scope.  */
   if (!name)
     {
diff --git a/gcc/cp/decl.c b/gcc/cp/decl.c
index 6fe4fed..1cd3726 100644
--- a/gcc/cp/decl.c
+++ b/gcc/cp/decl.c
@@ -6387,6 +6387,14 @@  cp_finish_decl (tree decl, tree init, bool init_const_expr_p,
 	   non local class. Record the types it uses
 	   so that we can decide later to emit debug info for them.  */
 	record_types_used_by_current_var_decl (decl);
+
+      /* Optionally warn about static local variables which are
+	 not const. */
+      if (warn_static_local && TREE_STATIC (decl)
+	  && DECL_FUNCTION_SCOPE_P (decl)
+	  && !TREE_READONLY (decl) && TREE_CODE (type) != REFERENCE_TYPE)
+	warning_at (DECL_SOURCE_LOCATION (decl), OPT_Wstatic_local,
+		    "local variable declared static, but not const");
     }
   else if (TREE_CODE (decl) == FIELD_DECL
 	   && TYPE_FOR_JAVA (type) && MAYBE_CLASS_TYPE_P (type))
diff --git a/gcc/doc/invoke.texi b/gcc/doc/invoke.texi
index 3df4662..df94214 100644
--- a/gcc/doc/invoke.texi
+++ b/gcc/doc/invoke.texi
@@ -4393,6 +4393,13 @@  programs.
 Warn for variables that might be changed by @samp{longjmp} or
 @samp{vfork}.  This warning is also enabled by @option{-Wextra}.
 
+@item -Wstatic-local
+@opindex Wstatic-local
+@opindex Wno-static-local
+Warn for variables declared inside functions which are static, but not
+declared const.  These local variables represent potential thread
+safety hazards.
+
 @item -Wconditionally-supported @r{(C++ and Objective-C++ only)}
 @opindex Wconditionally-supported
 @opindex Wno-conditionally-supported
diff --git a/gcc/testsuite/c-c++-common/Wstatic-local.c b/gcc/testsuite/c-c++-common/Wstatic-local.c
new file mode 100644
index 0000000..a900569
--- /dev/null
+++ b/gcc/testsuite/c-c++-common/Wstatic-local.c
@@ -0,0 +1,26 @@ 
+/* Test for -Wstatic-local */
+
+/* { dg-do compile } */
+/* { dg-options "-Wstatic-local" } */
+
+int a;
+static int b;
+const int c = 1;
+static const int d = 2;
+
+void
+g (void)
+{
+  static int v; /* { dg-warning "local variable declared static" } */
+  static const int v1 = 3;
+  static int *v2 = &v; /* { dg-warning "local variable declared static" } */
+  static const int *v3 = &v1; /* { dg-warning "local variable declared static" } */
+  static int *const v4 = &v;
+  static const char *w1 = "string"; /* { dg-warning "local variable declared static" } */
+  static const char *const w2 = "string";
+  static const char *x[] = {"string", 0};  /* { dg-warning "local variable declared static" } */
+  static const char *const x1[] = {"string", 0};
+
+  int z1;
+  const int z2 = 1;
+}
diff --git a/gcc/testsuite/g++.dg/warn/Wstatic-local-1.C b/gcc/testsuite/g++.dg/warn/Wstatic-local-1.C
new file mode 100644
index 0000000..bf6b965
--- /dev/null
+++ b/gcc/testsuite/g++.dg/warn/Wstatic-local-1.C
@@ -0,0 +1,27 @@ 
+// Test -Wstatic-local.  Also see c-c++-common/Wstatic-local.c.
+
+// { dg-do compile }
+// { dg-options "-Wstatic-local" }
+
+template <class T> void
+g1()
+{
+  static T a = T(); // { dg-warning "local variable declared static" }
+  static const T b = a;
+}
+
+template <class T> void
+g2()
+{
+  static T a = T();
+  static const T b = a;
+}
+
+void
+h()
+{
+  g1<int>();
+  g2<const int>();
+  static int a = 5; // { dg-warning "local variable declared static" }
+  static int &b = a;
+}
diff --git a/gcc/testsuite/g++.dg/warn/Wstatic-local-2.C b/gcc/testsuite/g++.dg/warn/Wstatic-local-2.C
new file mode 100644
index 0000000..6f8b2c7
--- /dev/null
+++ b/gcc/testsuite/g++.dg/warn/Wstatic-local-2.C
@@ -0,0 +1,28 @@ 
+// Test -Wstatic-local.  Also see c-c++-common/Wstatic-local.c.
+
+// { dg-do compile }
+// { dg-options "-std=c++11 -Wstatic-local" }
+
+static constexpr int a = 1;
+
+template <class T> void
+g1()
+{
+  static constexpr T a = T();
+  static constexpr T b = a;
+}
+
+template <class T> void
+g2()
+{
+  static constexpr T a = T();
+  static constexpr T b = a;
+}
+
+void
+h()
+{
+  g1<int>();
+  static constexpr int a = 5;
+  static int b = a; // { dg-warning "local variable declared static" }
+}