Only warn for maybe-uninitialized SRAed bits in -Wextra (PR 80635)

Hi,

On Fri, Nov 08 2019, Martin Jambor wrote:
> Hi,
>
> this patch is an attempt to implement my idea from a previous thread
> about moving -Wmaybe-uninitialized to -Wextra:
>
> https://gcc.gnu.org/ml/gcc-patches/2019-02/msg00220.html
>
> Specifically, it attempts to split -Wmaybe-uninitialized into those that
> are about SRA DECLs and those which are not, and move to -Wextra only
> the former ones.  The main idea is that false -Wmaybe-uninitialized
> warings about values that are scalar in user's code are easy to silence
> by initializing them to zero or something, as opposed to bits of
> aggregates such as a value in std::optional which are not.  Therefore,
> the warnings about user-scalars can remain in -Wall but warnings about
> SRAed pieces should be moved to -Wextra.
>

I have not seen any opposition to the idea in principle and so I'd like
to propose it as a patch (as opposed to an RFC) while this stage 1
lasts.

I have also re-based the patch and tried to bootstrap it... and it
failed because of PR 91825.  That bug was fixed by adding

#pragma GCC diagnostic warning "-Wmaybe-uninitialized"

to expmed.c by Jason in r277864 and I of course had to change that to:

#pragma GCC diagnostic warning "-Wmaybe-uninitialized-aggregates"

This episode only reinforced my opinion that maybe-uninitialized
warnings for aggregates are nasty - if even we have to work around them
they should not be part of -Wall.  I would not oppose moving the whole
-Wmaybe-uninitialized as it is now to -Wextra but if we want it to stay
for scalars, I think we need something like the below, which is a
version that did survive a round of bootstrap and testing.

Thanks,

Martin


2019-11-15  Martin Jambor  <mjambor@suse.cz>

	* common.opt (Wmaybe-uninitialized-aggregates): New.
	* tree-ssa-uninit.c (gate_warn_uninitialized): Also run if
	warn_maybe_uninitialized_aggregates is set.
	(warn_uninit): Warn for artificial DECLs only if
	warn_maybe_uninitialized_aggregates is set.
	* doc/invoke.texi (Warning Options): Add
	-Wmaybe-uninitialized-aggregates to the list.
	(-Wextra): Likewise.
	(-Wmaybe-uninitialized): Document that it only works on scalars.
	(-Wmaybe-uninitialized-aggregates): Document.
	* expmed.c: Change GCC diagnostic warning to
	-Wmaybe-uninitialized-aggregates

	testsuite/
	* gcc.dg/pr45083.c: Add Wmaybe-uninitialized-aggregates to options.
	* gcc.dg/ubsan/pr81981.c: Likewise.
	* gfortran.dg/pr25923.f90: Likewise.
	* g++.dg/warn/pr80635.C: New.
---
 gcc/common.opt                        |  4 +++
 gcc/doc/invoke.texi                   | 18 +++++++++--
 gcc/expmed.c                          |  2 +-
 gcc/testsuite/g++.dg/warn/pr80635.C   | 45 +++++++++++++++++++++++++++
 gcc/testsuite/gcc.dg/pr45083.c        |  2 +-
 gcc/testsuite/gcc.dg/ubsan/pr81981.c  |  2 +-
 gcc/testsuite/gfortran.dg/pr25923.f90 |  2 +-
 gcc/tree-ssa-uninit.c                 | 14 ++++++++-
 8 files changed, 82 insertions(+), 7 deletions(-)
 create mode 100644 gcc/testsuite/g++.dg/warn/pr80635.C

On 11/15/19 4:20 PM, Martin Jambor wrote:
> Hi,
> 
> On Fri, Nov 08 2019, Martin Jambor wrote:
>> Hi,
>>
>> this patch is an attempt to implement my idea from a previous thread
>> about moving -Wmaybe-uninitialized to -Wextra:
>>
>> https://gcc.gnu.org/ml/gcc-patches/2019-02/msg00220.html
>>
>> Specifically, it attempts to split -Wmaybe-uninitialized into those that
>> are about SRA DECLs and those which are not, and move to -Wextra only
>> the former ones.  The main idea is that false -Wmaybe-uninitialized
>> warings about values that are scalar in user's code are easy to silence
>> by initializing them to zero or something, as opposed to bits of
>> aggregates such as a value in std::optional which are not.  Therefore,
>> the warnings about user-scalars can remain in -Wall but warnings about
>> SRAed pieces should be moved to -Wextra.
>>
> 
> I have not seen any opposition to the idea in principle and so I'd like
> to propose it as a patch (as opposed to an RFC) while this stage 1
> lasts.

I still don't think carving out a subset of -Wmaybe-uninitialized
and letting it be suppressed by a new option is a good solution.
It means that any new warnings issued under this option will be
suppressed whether or not they're also subject to the same problems
as PR 80635.  I mentioned warnings for accesses by string functions
as an example, but it includes all kinds of others, such as:

   int f (int i)
   {
     int a[2];
     a[0] = 0;
     return a[i];   // should trigger a "maybe" kind of a warning
   }

It also means that any existing -Wmaybe-uninitialized warnings
issued for aggregates will need to be "converted" and issued
under the new option (otherwise they would be bugs).

The problem in 80635 is with std::optional where the warning
triggers because the member is intentionally initialized only
conditionally and used also only conditionally.  The condition
is the invariant between the use and initialization.  But GCC
doesn't know about the invariant, and -Wmaybe-uninitialized is
designed to trigger whenever it cannot prove that this kind of
an invariant exists or holds.

This problem has nothing to with the structure of objects per
se.  It only exists because there is no mechanism to tell GCC
about the invariant.  I think the right solution is to let
programmers either express that invariant somehow, or exempt
the object (and only it) from the uninitialized checking.

Martin

PS I don't disagree that -Wmaybe-uninitialized fits better under
-Wextra than under -Wall, but I see that as orthogonal to reports
like 80635.

> 
> I have also re-based the patch and tried to bootstrap it... and it
> failed because of PR 91825.  That bug was fixed by adding
> 
> #pragma GCC diagnostic warning "-Wmaybe-uninitialized"
> 
> to expmed.c by Jason in r277864 and I of course had to change that to:
> 
> #pragma GCC diagnostic warning "-Wmaybe-uninitialized-aggregates"
> 
> This episode only reinforced my opinion that maybe-uninitialized
> warnings for aggregates are nasty - if even we have to work around them
> they should not be part of -Wall.  I would not oppose moving the whole
> -Wmaybe-uninitialized as it is now to -Wextra but if we want it to stay
> for scalars, I think we need something like the below, which is a
> version that did survive a round of bootstrap and testing.
> 
> Thanks,
> 
> Martin
> 
> 
> 2019-11-15  Martin Jambor  <mjambor@suse.cz>
> 
> 	* common.opt (Wmaybe-uninitialized-aggregates): New.
> 	* tree-ssa-uninit.c (gate_warn_uninitialized): Also run if
> 	warn_maybe_uninitialized_aggregates is set.
> 	(warn_uninit): Warn for artificial DECLs only if
> 	warn_maybe_uninitialized_aggregates is set.
> 	* doc/invoke.texi (Warning Options): Add
> 	-Wmaybe-uninitialized-aggregates to the list.
> 	(-Wextra): Likewise.
> 	(-Wmaybe-uninitialized): Document that it only works on scalars.
> 	(-Wmaybe-uninitialized-aggregates): Document.
> 	* expmed.c: Change GCC diagnostic warning to
> 	-Wmaybe-uninitialized-aggregates
> 
> 	testsuite/
> 	* gcc.dg/pr45083.c: Add Wmaybe-uninitialized-aggregates to options.
> 	* gcc.dg/ubsan/pr81981.c: Likewise.
> 	* gfortran.dg/pr25923.f90: Likewise.
> 	* g++.dg/warn/pr80635.C: New.
> ---
>   gcc/common.opt                        |  4 +++
>   gcc/doc/invoke.texi                   | 18 +++++++++--
>   gcc/expmed.c                          |  2 +-
>   gcc/testsuite/g++.dg/warn/pr80635.C   | 45 +++++++++++++++++++++++++++
>   gcc/testsuite/gcc.dg/pr45083.c        |  2 +-
>   gcc/testsuite/gcc.dg/ubsan/pr81981.c  |  2 +-
>   gcc/testsuite/gfortran.dg/pr25923.f90 |  2 +-
>   gcc/tree-ssa-uninit.c                 | 14 ++++++++-
>   8 files changed, 82 insertions(+), 7 deletions(-)
>   create mode 100644 gcc/testsuite/g++.dg/warn/pr80635.C
> 
> diff --git a/gcc/common.opt b/gcc/common.opt
> index 12c0083964e..212e55f88c7 100644
> --- a/gcc/common.opt
> +++ b/gcc/common.opt
> @@ -783,6 +783,10 @@ Wmaybe-uninitialized
>   Common Var(warn_maybe_uninitialized) Warning EnabledBy(Wuninitialized)
>   Warn about maybe uninitialized automatic variables.
>   
> +Wmaybe-uninitialized-aggregates
> +Common Var(warn_maybe_uninitialized_aggregates) Warning EnabledBy(Wextra)
> +Warn about maybe uninitialized automatic parts of aggregates.
> +
>   Wunreachable-code
>   Common Ignore Warning
>   Does nothing. Preserved for backward compatibility.
> diff --git a/gcc/doc/invoke.texi b/gcc/doc/invoke.texi
> index 00eb7e77808..60612271eed 100644
> --- a/gcc/doc/invoke.texi
> +++ b/gcc/doc/invoke.texi
> @@ -328,7 +328,8 @@ Objective-C and Objective-C++ Dialects}.
>   -Wzero-length-bounds @gol
>   -Winvalid-pch  -Wlarger-than=@var{byte-size} @gol
>   -Wlogical-op  -Wlogical-not-parentheses  -Wlong-long @gol
> --Wmain  -Wmaybe-uninitialized  -Wmemset-elt-size  -Wmemset-transposed-args @gol
> +-Wmain  -Wmaybe-uninitialized -Wmaybe-uninitialized-aggregates @gol
> +-Wmemset-elt-size  -Wmemset-transposed-args @gol
>   -Wmisleading-indentation  -Wmissing-attributes  -Wmissing-braces @gol
>   -Wmissing-field-initializers  -Wmissing-format-attribute @gol
>   -Wmissing-include-dirs  -Wmissing-noreturn  -Wmissing-profile @gol
> @@ -4503,6 +4504,7 @@ name is still supported, but the newer name is more descriptive.)
>   -Wempty-body  @gol
>   -Wignored-qualifiers @gol
>   -Wimplicit-fallthrough=3 @gol
> +-Wmaybe-uninitialized-aggregates @gol
>   -Wmissing-field-initializers  @gol
>   -Wmissing-parameter-type @r{(C only)}  @gol
>   -Wold-style-declaration @r{(C only)}  @gol
> @@ -5695,10 +5697,22 @@ in fact be called at the place that would cause a problem.
>   
>   Some spurious warnings can be avoided if you declare all the functions
>   you use that never return as @code{noreturn}.  @xref{Function
> -Attributes}.
> +Attributes}.  This option only warns about scalar variables, use
> +@option{-Wmaybe-uninitialized-aggregates} to enable the warnings on parts of
> +aggregates which the compiler can track.
>   
>   This warning is enabled by @option{-Wall} or @option{-Wextra}.
>   
> +@item -Wmaybe-uninitialized-aggregates
> +@opindex Wmaybe-uninitialized-aggregates
> +@opindex Wmaybe-uninitialized-aggregates
> +
> +This option enables the same warning like @option{-Wmaybe-uninitialized} but
> +for parts of aggregates (i.g.@: structures, arrays or unions) that GCC
> +optimizers can track.  These warnings are only possible in optimizing
> +compilation, because otherwise GCC does not keep track of the state of
> +variables.  This warning is enabled by @option{-Wextra}.
> +
>   @item -Wunknown-pragmas
>   @opindex Wunknown-pragmas
>   @opindex Wno-unknown-pragmas
> diff --git a/gcc/expmed.c b/gcc/expmed.c
> index ff8554b1562..8f23da6b2d5 100644
> --- a/gcc/expmed.c
> +++ b/gcc/expmed.c
> @@ -19,7 +19,7 @@ along with GCC; see the file COPYING3.  If not see
>   <http://www.gnu.org/licenses/>.  */
>   
>   /* Work around tree-optimization/91825.  */
> -#pragma GCC diagnostic warning "-Wmaybe-uninitialized"
> +#pragma GCC diagnostic warning "-Wmaybe-uninitialized-aggregates"
>   
>   #include "config.h"
>   #include "system.h"
> diff --git a/gcc/testsuite/g++.dg/warn/pr80635.C b/gcc/testsuite/g++.dg/warn/pr80635.C
> new file mode 100644
> index 00000000000..eaf6b34a29d
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/warn/pr80635.C
> @@ -0,0 +1,45 @@
> +// PR C++/80635
> +// { dg-options "-std=c++11 -Wuninitialized -O" }
> +
> +void* operator new (__SIZE_TYPE__, void *p) { return p; }
> +
> +int f ();
> +int x;
> +
> +struct A {
> +  int i;
> +  A (): i (f ()) { }  // { dg-bogus "uninitialized" }
> +  ~A () { x = i; }
> +};
> +
> +struct B {
> +  B ();
> +  ~B ();
> +};
> +
> +
> +template <class T>
> +struct C {
> +  C (): t (), b () { }
> +  ~C () { if (b) t.~T (); }
> +
> +  void f () {
> +    new (&t) T ();
> +    b = true;
> +  }
> +
> +  union {
> +    T t;
> +    char x[sizeof (T)];
> +  };
> +  bool b;
> +};
> +
> +void g ()
> +{
> +  C<A> c1;
> +  C<B> c2;
> +
> +  c1.f ();
> +  c2.f ();
> +}
> diff --git a/gcc/testsuite/gcc.dg/pr45083.c b/gcc/testsuite/gcc.dg/pr45083.c
> index c9a4dbfe191..bce7d0bd7a6 100644
> --- a/gcc/testsuite/gcc.dg/pr45083.c
> +++ b/gcc/testsuite/gcc.dg/pr45083.c
> @@ -1,6 +1,6 @@
>   /* PR tree-optimization/45083 */
>   /* { dg-do compile } */
> -/* { dg-options "-O2 -Wuninitialized" } */
> +/* { dg-options "-O2 -Wuninitialized -Wmaybe-uninitialized-aggregates" } */
>   
>   struct S { char *a; unsigned b; unsigned c; };
>   extern int foo (const char *);
> diff --git a/gcc/testsuite/gcc.dg/ubsan/pr81981.c b/gcc/testsuite/gcc.dg/ubsan/pr81981.c
> index b2636d4c934..6a381f6b180 100644
> --- a/gcc/testsuite/gcc.dg/ubsan/pr81981.c
> +++ b/gcc/testsuite/gcc.dg/ubsan/pr81981.c
> @@ -1,6 +1,6 @@
>   /* PR sanitizer/81981 */
>   /* { dg-do compile } */
> -/* { dg-options "-O2 -Wmaybe-uninitialized -fsanitize=undefined -ffat-lto-objects" } */
> +/* { dg-options "-O2 -Wmaybe-uninitialized -Wmaybe-uninitialized-aggregates -fsanitize=undefined -ffat-lto-objects" } */
>   
>   int v;
>   
> diff --git a/gcc/testsuite/gfortran.dg/pr25923.f90 b/gcc/testsuite/gfortran.dg/pr25923.f90
> index 3283ba21f32..2ddc3b92485 100644
> --- a/gcc/testsuite/gfortran.dg/pr25923.f90
> +++ b/gcc/testsuite/gfortran.dg/pr25923.f90
> @@ -1,5 +1,5 @@
>   ! { dg-do compile }
> -! { dg-options "-O -Wuninitialized" }
> +! { dg-options "-O -Wuninitialized -Wmaybe-uninitialized-aggregates" }
>   
>   module foo
>   implicit none
> diff --git a/gcc/tree-ssa-uninit.c b/gcc/tree-ssa-uninit.c
> index fe8f8f0bc28..362af73e5d2 100644
> --- a/gcc/tree-ssa-uninit.c
> +++ b/gcc/tree-ssa-uninit.c
> @@ -134,6 +134,16 @@ warn_uninit (enum opt_code wc, tree t, tree expr, tree var,
>       return;
>     if (!has_undefined_value_p (t))
>       return;
> +  if (wc == OPT_Wmaybe_uninitialized
> +      && SSA_NAME_VAR (t)
> +      && DECL_ARTIFICIAL (SSA_NAME_VAR (t))
> +      && DECL_HAS_DEBUG_EXPR_P (SSA_NAME_VAR (t)))
> +    {
> +      if (warn_maybe_uninitialized_aggregates)
> +	wc = OPT_Wmaybe_uninitialized_aggregates;
> +      else
> +	return;
> +    }
>   
>     /* Anonymous SSA_NAMEs shouldn't be uninitialized, but ssa_undefined_value_p
>        can return true if the def stmt of anonymous SSA_NAME is COMPLEX_EXPR
> @@ -2622,7 +2632,9 @@ warn_uninitialized_phi (gphi *phi, vec<gphi *> *worklist,
>   static bool
>   gate_warn_uninitialized (void)
>   {
> -  return warn_uninitialized || warn_maybe_uninitialized;
> +  return (warn_uninitialized
> +	  || warn_maybe_uninitialized
> +	  || warn_maybe_uninitialized_aggregates);
>   }
>   
>   namespace {
>