[PATCH] PR71482: Add -Wglobal-constructors

Marek Polacek polacek@redhat.com
Thu May 30 19:44:00 GMT 2019


On Tue, May 28, 2019 at 09:30:56PM +0000, 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  <sean@swgillespie.me>
> 
> 	* doc/invoke.texi: Add new flag -Wglobal-constructors.
> 
> gcc/c-family/ChangeLog:
> 
> 2019-05-28  Sean Gillespie  <sean@swgillespie.me>
> 
> 	* c.opt: Add new flag -Wglobal-constructors.
> 
> gcc/cp/ChangeLog:
> 
> 2019-05-28  Sean Gillespie  <sean@swgillespie.me>
> 
> 	* 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  <sean@swgillespie.me>
> 
> 	* 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



More information about the Gcc-patches mailing list