[PATCH v2] PR71482: Add -Wglobal-constructors
This adds a new warning, -Wglobal-constructors, that warns whenever a
decl requires a global constructor or destructor. Global destructors are
required whenever a decl with thread_local or global storage is declared
with a type with a nontrivial destructor. Global constructors are required
whenever a declaration's initializer is a non-trivial, non-constant
initializtion.
This warning mirrors the Clang option -Wglobal-constructors, which warns
on the same thing. -Wglobal-constructors was present in Apple's GCC and
later made its way into Clang.
Bootstrapped and regression-tested on x86-64 linux, new tests passing.
gcc/ChangeLog:
2019-05-28 Sean Gillespie
PR c++/71482
* doc/invoke.texi: Add new flag -Wglobal-constructors.
gcc/c-family/ChangeLog:
2019-05-28 Sean Gillespie
PR c++/71482
* c.opt: Add new flag -Wglobal-constructors.
gcc/cp/ChangeLog:
2019-05-28 Sean Gillespie
PR c++/71482
* decl.c (expand_static_init): Warn if a thread local or static decl
requires a non-trivial constructor or destructor.
gcc/testsuite/ChangeLog:
2019-05-28 Sean Gillespie
PR c++/71482
* g++.dg/warn/global-constructors-1.C: New test.
* g++.dg/warn/global-constructors-2.C: New test.
---
gcc/c-family/c.opt| 4 ++
gcc/cp/decl.c | 22 ++--
gcc/doc/invoke.texi | 35
.../g++.dg/warn/global-constructors-1.C | 53 +++
.../g++.dg/warn/global-constructors-2.C | 49 +
5 files changed, 159 insertions(+), 4 deletions(-)
create mode 100644 gcc/testsuite/g++.dg/warn/global-constructors-1.C
create mode 100644 gcc/testsuite/g++.dg/warn/global-constructors-2.C
diff --git a/gcc/c-family/c.opt b/gcc/c-family/c.opt
index 046d489f7eb..21f12d4f7b2 100644
--- a/gcc/c-family/c.opt
+++ b/gcc/c-family/c.opt
@@ -613,6 +613,10 @@ Wformat-truncation=
C ObjC C++ LTO ObjC++ Joined RejectNegative UInteger Var(warn_format_trunc)
Warning LangEnabledBy(C ObjC C++ LTO ObjC++,Wformat=, warn_format >= 1, 0)
IntegerRange(0, 2)
Warn about calls to snprintf and similar functions that truncate output.
+Wglobal-constructors
+C++ ObjC++ Var(warn_global_constructors) Warning
+Warn about objects with static storage duration that require dynamic
initialization or have nontrivial destructors.
+
Wif-not-aligned
C ObjC C++ ObjC++ Var(warn_if_not_aligned) Init(1) Warning
Warn when the field in a struct is not aligned.
diff --git a/gcc/cp/decl.c b/gcc/cp/decl.c
index 19d14a6a5e9..8dc366aa0c6 100644
--- a/gcc/cp/decl.c
+++ b/gcc/cp/decl.c
@@ -8324,10 +8324,10 @@ expand_static_init (tree decl, tree init)
return;
}
+ location_t dloc = DECL_SOURCE_LOCATION (decl);
if (CP_DECL_THREAD_LOCAL_P (decl) && DECL_GNU_TLS_P (decl)
&& !DECL_FUNCTION_SCOPE_P (decl))
{
- location_t dloc = DECL_SOURCE_LOCATION (decl);
if (init)
error_at (dloc, "non-local variable %qD declared %<__thread%> "
"needs dynamic initialization", decl);
@@ -8467,10 +8467,24 @@ expand_static_init (tree decl, tree init)
finish_then_clause (if_stmt);
finish_if_stmt (if_stmt);
}
- else if (CP_DECL_THREAD_LOCAL_P (decl))
-tls_aggregates = tree_cons (init, decl, tls_aggregates);
else
-static_aggregates = tree_cons (init, decl, static_aggregates);
+{
+ if (CP_DECL_THREAD_LOCAL_P (decl))
+tls_aggregates = tree_cons (init, decl, tls_aggregates);
+ else
+static_aggregates = tree_cons (init, decl, static_aggregates);
+
+ if (TYPE_HAS_NONTRIVIAL_DESTRUCTOR (TREE_TYPE (decl)))
+{
+ warning_at (dloc, OPT_Wglobal_constructors,
+"declaration requires a global destructor");
+ return;
+}
+
+ if (DECL_NONTRIVIALLY_INITIALIZED_P (decl) && !decl_constant_var_p
(decl))
+warning_at (dloc, OPT_Wglobal_constructors,
+ "declaration requires a global constructor");
+}
}
diff --git a/gcc/doc/invoke.texi b/gcc/doc/invoke.texi
index 4964cc41ba3..77d324584ec 100644
--- a/gcc/doc/invoke.texi
+++ b/gcc/doc/invoke.texi
@@ -312,6 +312,7 @@ Objective-C and Objective-C++ Dialects}.
-Wformat-security -Wformat-signedness -Wformat-truncation=@var{n} @gol
-Wformat-y2k -Wframe-address @gol
-Wframe-larger-than=@var{byte-size} -Wno-free-nonheap-object @gol
+-Wglobal-constructors @gol
-Wjump-misses-init @gol
-Whsa -Wif-not-aligned @gol
-Wignored-qualifiers -Wignored-attributes -Wincompatible-pointer-types @gol
@@ -6509,6 +6510,40 @@ to @option{-Wframe-larger-than=}@samp{SIZE_MAX} or
larger.
Do not warn when attempting to free an object that was not allocated
on the heap.
+@item -Wglobal-constructors @r{(C++ and Objective-C++ only)}
+@opindex Wglobal-constructors
+@opindex Wno-global-constructors
+War
Re: [PATCH] PR71482: Add -Wglobal-constructors
On Fri, May 31, 2019 at 9:30 AM Martin Sebor wrote:
>
> On 5/31/19 10:12 AM, Sean Gillespie wrote:
> > On Thu, May 30, 2019 at 4:28 PM Martin Sebor wrote:
> >>
> >> On 5/28/19 3:30 PM, Sean Gillespie wrote:
> >>> This adds a new warning, -Wglobal-constructors, that warns whenever a
> >>> decl requires a global constructor or destructor. This new warning fires
> >>> whenever a thread_local or global variable is declared whose type has a
> >>> non-trivial constructor or destructor. When faced with both a constructor
> >>> and a destructor, the error message mentions the destructor and is only
> >>> fired once.
> >>>
> >>> This warning mirrors the Clang option -Wglobal-constructors, which warns
> >>> on the same thing. -Wglobal-constructors was present in Apple's GCC and
> >>> later made its way into Clang.
> >>>
> >>> Bootstrapped and regression-tested on x86-64 linux, new tests passing.
> >>
> >> I can't tell from the Clang online manual:
> >>
> >> Is the warning meant to trigger just for globals, or for all
> >> objects with static storage duration regardless of scope (i.e.,
> >> including namespace-scope objects and static locals and static
> >> class members)?
> >>
> >> "Requires a constructor to initialize" doesn't seem very clear
> >> to me. Is the warning intended to trigger for objects that
> >> require dynamic initialization? If so, then what about dynamic
> >> intialization of objects of trivial types, such as this:
> >>
> >> static int i = std::string ("foo").length ();
> >>
> >> or even
> >>
> >> static int j = strlen (getenv ("TMP"));
> >>
> >> If these aren't meant to be diagnosed then the description should
> >> make it clear (the first one involves a ctor, the second one does
> >> not). But I would think that if the goal is to find sources of
> >> dynamic initialization then diagnosing the above would be useful.
> >> If so, the description should make it clear and tests verifying
> >> that it works should be added.
> >>
> >> Martin
> >
> > The answer to both of those is yes. Clang warns on both of these with
> > -Wglobal-constructors because the goal of -Wglobal-constructors is to
> > catch and warn against potential life-before-main behavior.
> >
> > Based on Marek's feedback I'm about to push another patch that also
> > warns on your above two examples and adds tests for them. The idea
> > is that any C++-level dynamically initialized globals get warned. Constexpr
> > constructors or things that otherwise can be statically initialized
> > are not warned.
>
> That makes sense.
>
> >
> > Regarding the doc description, how about something like this:
> >
> > "Warns whenever an object with static storage duration requires
> > dynamic initialiation,
> > through global constructors or otherwise."
>
> This makes it clearer -- i.e, global constructors refers to the ELF
> concept rather than to the C++ one. What does the otherwise part
> refer to? Some non-ELF mechanism?
Ah, I was thinking "otherwise" to mean the C++ sense of constructor,
and not the ELF one. I realize now that it's clearer without "for
otherwise", since then "global constructors" would refer to just the
ELF concept.
>
> >> PS Dynamic initialization can be optimized into static
> >> initialization, even when it involves a user-defined constructor.
> >> If the purpose of the warning is to find objects that are
> >> dynamically initialized in the sense of the C++ language then
> >> implementing it in the front-end is sufficient. But if the goal
> >> is to detect constructors that will actually run at runtime (i.e.,
> >> have a startup cost and may have dependencies on one another) then
> >> it needs to be implemented much later.
> >>
> >
> > Clang sticks to the C++ language definition of dynamic initialization for
> > the
> > purposes of this warning and I think we should as well. Without runtime
> > checks
> > this is a best-effort warning, but it is capable of catching a bunch
> > of the common
> > ways that you can get constructors running on startup, which is the goal.
>
> It might be worth mentioning that in the manual, perhaps along with
> an example showing an instance of the warning that will most likely
> be a true positive vs a false positive. E.g., something like this:
>
>const char* const tmp = getenv ("TMP");
>
> vs
>
>const int n = strlen ("123"); // initialized statically
>
Will do.
Sean
Re: [PATCH] PR71482: Add -Wglobal-constructors
On Thu, May 30, 2019 at 4:28 PM Martin Sebor wrote:
>
> On 5/28/19 3:30 PM, Sean Gillespie wrote:
> > This adds a new warning, -Wglobal-constructors, that warns whenever a
> > decl requires a global constructor or destructor. This new warning fires
> > whenever a thread_local or global variable is declared whose type has a
> > non-trivial constructor or destructor. When faced with both a constructor
> > and a destructor, the error message mentions the destructor and is only
> > fired once.
> >
> > This warning mirrors the Clang option -Wglobal-constructors, which warns
> > on the same thing. -Wglobal-constructors was present in Apple's GCC and
> > later made its way into Clang.
> >
> > Bootstrapped and regression-tested on x86-64 linux, new tests passing.
>
> I can't tell from the Clang online manual:
>
> Is the warning meant to trigger just for globals, or for all
> objects with static storage duration regardless of scope (i.e.,
> including namespace-scope objects and static locals and static
> class members)?
>
> "Requires a constructor to initialize" doesn't seem very clear
> to me. Is the warning intended to trigger for objects that
> require dynamic initialization? If so, then what about dynamic
> intialization of objects of trivial types, such as this:
>
>static int i = std::string ("foo").length ();
>
> or even
>
>static int j = strlen (getenv ("TMP"));
>
> If these aren't meant to be diagnosed then the description should
> make it clear (the first one involves a ctor, the second one does
> not). But I would think that if the goal is to find sources of
> dynamic initialization then diagnosing the above would be useful.
> If so, the description should make it clear and tests verifying
> that it works should be added.
>
> Martin
The answer to both of those is yes. Clang warns on both of these with
-Wglobal-constructors because the goal of -Wglobal-constructors is to
catch and warn against potential life-before-main behavior.
Based on Marek's feedback I'm about to push another patch that also
warns on your above two examples and adds tests for them. The idea
is that any C++-level dynamically initialized globals get warned. Constexpr
constructors or things that otherwise can be statically initialized
are not warned.
Regarding the doc description, how about something like this:
"Warns whenever an object with static storage duration requires
dynamic initialiation,
through global constructors or otherwise."
>
> PS Dynamic initialization can be optimized into static
> initialization, even when it involves a user-defined constructor.
> If the purpose of the warning is to find objects that are
> dynamically initialized in the sense of the C++ language then
> implementing it in the front-end is sufficient. But if the goal
> is to detect constructors that will actually run at runtime (i.e.,
> have a startup cost and may have dependencies on one another) then
> it needs to be implemented much later.
>
Clang sticks to the C++ language definition of dynamic initialization for the
purposes of this warning and I think we should as well. Without runtime checks
this is a best-effort warning, but it is capable of catching a bunch
of the common
ways that you can get constructors running on startup, which is the goal.
Sean Gillespie
Re: [PATCH] PR71482: Add -Wglobal-constructors
Thank you for the review!
On Thu, May 30, 2019 at 12:38 PM Marek Polacek wrote:
>
> Thanks for the patch! Do you have a copyright assignment on file? Unsure
> if this patch is small enough not to need it.
>
Also unsure, though I assumed that this patch was small enough to not need one.
I'll go ahead and do it regardless.
> > gcc/ChangeLog:
> >
> > 2019-05-28 Sean Gillespie
> >
> > * doc/invoke.texi: Add new flag -Wglobal-constructors.
> >
> > gcc/c-family/ChangeLog:
> >
> > 2019-05-28 Sean Gillespie
> >
> > * c.opt: Add new flag -Wglobal-constructors.
> >
> > gcc/cp/ChangeLog:
> >
> > 2019-05-28 Sean Gillespie
> >
> > * decl.c (expand_static_init): Warn if a thread local or static decl
> > requires a non-trivial constructor or destructor.
> >
> > gcc/testsuite/ChangeLog:
> >
> > 2019-05-28 Sean Gillespie
> >
> > * g++.dg/warn/global-constructors-1.C: New test.
> > * g++.dg/warn/global-constructors-2.C: New test.
>
> These are fine but please also mention "PR c++/71482" in them.
>
Will fix.
> > ---
> > gcc/c-family/c.opt| 4 +++
> > gcc/cp/decl.c | 17 ++---
> > gcc/doc/invoke.texi | 7 ++
> > .../g++.dg/warn/global-constructors-1.C | 25 +++
> > .../g++.dg/warn/global-constructors-2.C | 23 +
> > 5 files changed, 73 insertions(+), 3 deletions(-)
> > create mode 100644 gcc/testsuite/g++.dg/warn/global-constructors-1.C
> > create mode 100644 gcc/testsuite/g++.dg/warn/global-constructors-2.C
> >
> > diff --git a/gcc/c-family/c.opt b/gcc/c-family/c.opt
> > index 046d489f7eb..cf62625ca42 100644
> > --- a/gcc/c-family/c.opt
> > +++ b/gcc/c-family/c.opt
> > @@ -613,6 +613,10 @@ Wformat-truncation=
> > C ObjC C++ LTO ObjC++ Joined RejectNegative UInteger
> > Var(warn_format_trunc) Warning LangEnabledBy(C ObjC C++ LTO
> > ObjC++,Wformat=, warn_format >= 1, 0) IntegerRange(0, 2)
> > Warn about calls to snprintf and similar functions that truncate output.
> >
> > +Wglobal-constructors
> > +C++ ObjC++ Var(warn_global_constructors) Warning
> > +Warn when a global declaration requires a constructor to initialize.
> > +
>
> Ok, I agree this shouldn't be turned on by -Wall or -Wextra.
Yeah - the way I use this with clang is to forbid global constructors
with -Werror=global-constructors and -Wglobal-constructors. Since
this is a legitimate language feature I don't think it should be
included in either of those catch-all warning sets.
>
> > diff --git a/gcc/cp/decl.c b/gcc/cp/decl.c
> > index 19d14a6a5e9..c1d66195bd7 100644
> > --- a/gcc/cp/decl.c
> > +++ b/gcc/cp/decl.c
> > @@ -8467,10 +8467,21 @@ expand_static_init (tree decl, tree init)
> >finish_then_clause (if_stmt);
> >finish_if_stmt (if_stmt);
> > }
> > - else if (CP_DECL_THREAD_LOCAL_P (decl))
> > -tls_aggregates = tree_cons (init, decl, tls_aggregates);
> >else
> > -static_aggregates = tree_cons (init, decl, static_aggregates);
> > + {
> > +if (CP_DECL_THREAD_LOCAL_P (decl))
> > + tls_aggregates = tree_cons (init, decl, tls_aggregates);
> > +else
> > + static_aggregates = tree_cons (init, decl, static_aggregates);
> > +
> > +if (warn_global_constructors)
>
> This check is not wrong but you can probably drop it. We use it if the
> warning
> is expensive, but this doesn't seem to be the case.
>
Will fix.
> > + {
> > + if (TYPE_HAS_NONTRIVIAL_DESTRUCTOR (TREE_TYPE (decl)))
> > +warning(OPT_Wglobal_constructors, "declaration requires a global
> > destructor");
> > + else
> > +warning(OPT_Wglobal_constructors, "declaration requires a global
> > constructor");
> > + }
> > + }
>
> The formatting a bit wrong, please use two spaces to indent. There should be
> a space before a (. And the lines should not exceed 80 characters.
>
> I think it would be better to use warning_at instead of warning. As the
> location, you can use 'dloc' defined earlier in the function if you move
> it out of the block.
>
Will fix.
>
> It seems this patch will also warn for
>
> struct A {
> A() = default;
> };
>
> A a;
>
> which I think we don't want?
>
> I also think you want to add a test using a constexpr constructor.
>
> Marek
You're completely right. I have a modified version of this patch that
expands the test coverage to include these cases while also
handling the general problem of constant and non-constant
initializers. I'll submit it shortly.
Sean Gillespie
[PATCH] PR71482: Add -Wglobal-constructors
This adds a new warning, -Wglobal-constructors, that warns whenever a
decl requires a global constructor or destructor. This new warning fires
whenever a thread_local or global variable is declared whose type has a
non-trivial constructor or destructor. When faced with both a constructor
and a destructor, the error message mentions the destructor and is only
fired once.
This warning mirrors the Clang option -Wglobal-constructors, which warns
on the same thing. -Wglobal-constructors was present in Apple's GCC and
later made its way into Clang.
Bootstrapped and regression-tested on x86-64 linux, new tests passing.
gcc/ChangeLog:
2019-05-28 Sean Gillespie
* doc/invoke.texi: Add new flag -Wglobal-constructors.
gcc/c-family/ChangeLog:
2019-05-28 Sean Gillespie
* c.opt: Add new flag -Wglobal-constructors.
gcc/cp/ChangeLog:
2019-05-28 Sean Gillespie
* decl.c (expand_static_init): Warn if a thread local or static decl
requires a non-trivial constructor or destructor.
gcc/testsuite/ChangeLog:
2019-05-28 Sean Gillespie
* g++.dg/warn/global-constructors-1.C: New test.
* g++.dg/warn/global-constructors-2.C: New test.
---
gcc/c-family/c.opt| 4 +++
gcc/cp/decl.c | 17 ++---
gcc/doc/invoke.texi | 7 ++
.../g++.dg/warn/global-constructors-1.C | 25 +++
.../g++.dg/warn/global-constructors-2.C | 23 +
5 files changed, 73 insertions(+), 3 deletions(-)
create mode 100644 gcc/testsuite/g++.dg/warn/global-constructors-1.C
create mode 100644 gcc/testsuite/g++.dg/warn/global-constructors-2.C
diff --git a/gcc/c-family/c.opt b/gcc/c-family/c.opt
index 046d489f7eb..cf62625ca42 100644
--- a/gcc/c-family/c.opt
+++ b/gcc/c-family/c.opt
@@ -613,6 +613,10 @@ Wformat-truncation=
C ObjC C++ LTO ObjC++ Joined RejectNegative UInteger Var(warn_format_trunc)
Warning LangEnabledBy(C ObjC C++ LTO ObjC++,Wformat=, warn_format >= 1, 0)
IntegerRange(0, 2)
Warn about calls to snprintf and similar functions that truncate output.
+Wglobal-constructors
+C++ ObjC++ Var(warn_global_constructors) Warning
+Warn when a global declaration requires a constructor to initialize.
+
Wif-not-aligned
C ObjC C++ ObjC++ Var(warn_if_not_aligned) Init(1) Warning
Warn when the field in a struct is not aligned.
diff --git a/gcc/cp/decl.c b/gcc/cp/decl.c
index 19d14a6a5e9..c1d66195bd7 100644
--- a/gcc/cp/decl.c
+++ b/gcc/cp/decl.c
@@ -8467,10 +8467,21 @@ expand_static_init (tree decl, tree init)
finish_then_clause (if_stmt);
finish_if_stmt (if_stmt);
}
- else if (CP_DECL_THREAD_LOCAL_P (decl))
-tls_aggregates = tree_cons (init, decl, tls_aggregates);
else
-static_aggregates = tree_cons (init, decl, static_aggregates);
+ {
+if (CP_DECL_THREAD_LOCAL_P (decl))
+ tls_aggregates = tree_cons (init, decl, tls_aggregates);
+else
+ static_aggregates = tree_cons (init, decl, static_aggregates);
+
+if (warn_global_constructors)
+ {
+ if (TYPE_HAS_NONTRIVIAL_DESTRUCTOR (TREE_TYPE (decl)))
+warning(OPT_Wglobal_constructors, "declaration requires a global
destructor");
+ else
+warning(OPT_Wglobal_constructors, "declaration requires a global
constructor");
+ }
+ }
}
diff --git a/gcc/doc/invoke.texi b/gcc/doc/invoke.texi
index 4964cc41ba3..a9b2e47a302 100644
--- a/gcc/doc/invoke.texi
+++ b/gcc/doc/invoke.texi
@@ -312,6 +312,7 @@ Objective-C and Objective-C++ Dialects}.
-Wformat-security -Wformat-signedness -Wformat-truncation=@var{n} @gol
-Wformat-y2k -Wframe-address @gol
-Wframe-larger-than=@var{byte-size} -Wno-free-nonheap-object @gol
+-Wglobal-constructors @gol
-Wjump-misses-init @gol
-Whsa -Wif-not-aligned @gol
-Wignored-qualifiers -Wignored-attributes -Wincompatible-pointer-types @gol
@@ -6509,6 +6510,12 @@ to @option{-Wframe-larger-than=}@samp{SIZE_MAX} or
larger.
Do not warn when attempting to free an object that was not allocated
on the heap.
+@item -Wglobal-constructors @r{(C++ and Objective-C++ only)}
+@opindex Wglobal-constructors
+@opindex Wno-global-constructors
+Warn if the compiler detects a global declaration that requires
+a constructor to initialize.
+
@item -Wstack-usage=@var{byte-size}
@opindex Wstack-usage
@opindex Wno-stack-usage
diff --git a/gcc/testsuite/g++.dg/warn/global-constructors-1.C
b/gcc/testsuite/g++.dg/warn/global-constructors-1.C
new file mode 100644
index 000..5a6869e9dcd
--- /dev/null
+++ b/gcc/testsuite/g++.dg/warn/global-constructors-1.C
@@ -0,0 +1,25 @@
+// { dg-do compile }
+// { dg-options "-Wglobal-constructors" }
+
+struct with_ctor {
+int x;
+with_ctor() : x(42) {}
+};
+
+struct with_dtor {
+~with_dtor() {}
+};
+
+struct with_both {
+int x;
+with_both() : x(42) {}
+~with_both() {}
+};
+
+with_ctor global_var; /* { dg-warni
