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 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?
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
Martin
Re: [PATCH] PR71482: Add -Wglobal-constructors
On Fri, May 31, 2019 at 06:25:03PM +0200, Jakub Jelinek wrote:
> On Fri, May 31, 2019 at 12:16:58PM -0400, Marek Polacek wrote:
> > > The warning is not meant to diagnose these. But I do agree that the
> > > documentation for the new warning should be improved.
> >
> > I was partially wrong: if i/j are at file scope, then the warning triggers.
> > But if i/j are in a function, then it doesn't warn.
>
> That is correct, because the block scope statics aren't actually initialized
> from a global constructor, but with a guard variable whenever a function is
> called the first time. So on
> int bar ();
> struct S { S (); ~S (); };
>
> void
> foo ()
> {
> static int a = bar ();
> static S s;
> }
> this warning shouldn't warn at all. It might be useful to have another
> warning for these, but it should be a different warning.
Right, agreed -- and the patch already does the right thing.
Marek
Re: [PATCH] PR71482: Add -Wglobal-constructors
On Fri, May 31, 2019 at 12:16:58PM -0400, Marek Polacek wrote:
> > The warning is not meant to diagnose these. But I do agree that the
> > documentation for the new warning should be improved.
>
> I was partially wrong: if i/j are at file scope, then the warning triggers.
> But if i/j are in a function, then it doesn't warn.
That is correct, because the block scope statics aren't actually initialized
from a global constructor, but with a guard variable whenever a function is
called the first time. So on
int bar ();
struct S { S (); ~S (); };
void
foo ()
{
static int a = bar ();
static S s;
}
this warning shouldn't warn at all. It might be useful to have another
warning for these, but it should be a different warning.
Jakub
Re: [PATCH] PR71482: Add -Wglobal-constructors
On Fri, May 31, 2019 at 11:50:47AM -0400, Marek Polacek wrote:
> On Thu, May 30, 2019 at 05:28:47PM -0600, 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"));
>
> The warning is not meant to diagnose these. But I do agree that the
> documentation for the new warning should be improved.
I was partially wrong: if i/j are at file scope, then the warning triggers.
But if i/j are in a function, then it doesn't warn.
My apologies.
Marek
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
On Thu, May 30, 2019 at 05:28:47PM -0600, 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"));
The warning is not meant to diagnose these. But I do agree that the
documentation for the new warning should be improved.
> 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
>
> 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
My sense is that this is what we're doing here. So the patch seems reasonable
to me. I suspect we'll have to tweak the warning a little bit, but overall it
looks fine to me. As always, there's room to expand and improve, but the
warning looks useful to me as-is.
Marek
Re: [PATCH] PR71482: Add -Wglobal-constructors
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
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.
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
---
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
Re: [PATCH] PR71482: Add -Wglobal-constructors
On Tue, May 28, 2019 at 09:30:56PM +, 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.
Thanks for the patch! Do you have a copyright assignment on file? Unsure
if this patch is small enough not to need it.
> 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.
> ---
> 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.
> 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.
> + {
> + 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.
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
[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-warning "declaration requires a global
