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

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.

The patch is a bit bigger because of documentation (which I'll be happy
to improve based on your suggestions) and testsuite churn, but the main
bit is the following added test in warn_uninit function:

  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;
    }

The reason why I also test DECL_HAS_DEBUG_EXPR_P is
gfortran.dg/pr39666-2.f90 - without it the test silences a warning about
a decl representing the return value which is an artificial scalar
variable (probably all the way from the front-end).  We can of course
not care about not warning for it but then I don't know how to call and
document the new option :-)  Generally, if someone can think of a better
test to identify SRA DECLs, I'll be happy to change that.  We might put
a bit to identify SRA decls in the decl tree, but I tend to think that
is not a good use of the few remaining bits there.

What do you think, is something along these lines a good idea?

Thanks,

Martin



2019-11-08  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.

	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/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 ++++++++-
 7 files changed, 81 insertions(+), 6 deletions(-)
 create mode 100644 gcc/testsuite/g++.dg/warn/pr80635.C

On Fri, Nov 8, 2019 at 1:41 PM Martin Jambor <mjambor@suse.cz> 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.
>
> The patch is a bit bigger because of documentation (which I'll be happy
> to improve based on your suggestions) and testsuite churn, but the main
> bit is the following added test in warn_uninit function:
>
>   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;
>     }

I wonder if this works in practice since the whole point of SRA is to
have things copy propagated out (so the 't' with the SSA_NAME_VAR
doesn't prevail or gets commoned with sth else).  Or is this about
the default defs SRA creates itself to actually emit uninit warnings?

> The reason why I also test DECL_HAS_DEBUG_EXPR_P is
> gfortran.dg/pr39666-2.f90 - without it the test silences a warning about
> a decl representing the return value which is an artificial scalar
> variable (probably all the way from the front-end).  We can of course
> not care about not warning for it but then I don't know how to call and
> document the new option :-)  Generally, if someone can think of a better
> test to identify SRA DECLs, I'll be happy to change that.  We might put
> a bit to identify SRA decls in the decl tree, but I tend to think that
> is not a good use of the few remaining bits there.
>
> What do you think, is something along these lines a good idea?
>
> Thanks,
>
> Martin
>
>
>
> 2019-11-08  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.
>
>         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/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 ++++++++-
>  7 files changed, 81 insertions(+), 6 deletions(-)
>  create mode 100644 gcc/testsuite/g++.dg/warn/pr80635.C
>
> diff --git a/gcc/common.opt b/gcc/common.opt
> index cc279f411d7..03769299df8 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 faa7fa95a0e..dbc3219b770 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
> @@ -4498,6 +4499,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
> @@ -5690,10 +5692,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/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 {
> --
> 2.23.0
>

Hi,


On Mon, Nov 11 2019, Richard Biener wrote:
> On Fri, Nov 8, 2019 at 1:41 PM Martin Jambor <mjambor@suse.cz> 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.
>>
>> The patch is a bit bigger because of documentation (which I'll be happy
>> to improve based on your suggestions) and testsuite churn, but the main
>> bit is the following added test in warn_uninit function:
>>
>>   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;
>>     }
>
> I wonder if this works in practice since the whole point of SRA is to
> have things copy propagated out (so the 't' with the SSA_NAME_VAR
> doesn't prevail or gets commoned with sth else).  Or is this about
> the default defs SRA creates itself to actually emit uninit warnings?

Yes, not warning for those default defs is the idea.  After all, only
when we warn about those DECLs (to which those default defs belong) the
user gets a warning that a bit which they cannot easily initialize on
their own is maybe uninitialized:

pr80635.C:12:13: warning: ‘c1.A::i’ may be used uninitialized in this function [-Wmaybe-uninitialized]

Martin

On 11/8/19 5:41 AM, 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.
> 
> The patch is a bit bigger because of documentation (which I'll be happy
> to improve based on your suggestions) and testsuite churn, but the main
> bit is the following added test in warn_uninit function:
> 
>    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;
>      }
> 
> The reason why I also test DECL_HAS_DEBUG_EXPR_P is
> gfortran.dg/pr39666-2.f90 - without it the test silences a warning about
> a decl representing the return value which is an artificial scalar
> variable (probably all the way from the front-end).  We can of course
> not care about not warning for it but then I don't know how to call and
> document the new option :-)  Generally, if someone can think of a better
> test to identify SRA DECLs, I'll be happy to change that.  We might put
> a bit to identify SRA decls in the decl tree, but I tend to think that
> is not a good use of the few remaining bits there.
> 
> What do you think, is something along these lines a good idea?
> 
> Thanks,
> 
> Martin
> 
> 
> 
> 2019-11-08  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.
> 
> 	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/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 ++++++++-
>   7 files changed, 81 insertions(+), 6 deletions(-)
>   create mode 100644 gcc/testsuite/g++.dg/warn/pr80635.C
> 
> diff --git a/gcc/common.opt b/gcc/common.opt
> index cc279f411d7..03769299df8 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 faa7fa95a0e..dbc3219b770 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
> @@ -4498,6 +4499,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
> @@ -5690,10 +5692,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}.
> +

Let me ask a question.  Suppose I have code like this:

   struct S { char a[4], b[4]; };

   void* g (void)
   {
     struct S *p = malloc (sizeof *p);
     strcpy (p->a + 1, p->b + 1);
     return p;
   }

(I include the offsets only because they make an interesting
difference in the internal representation.  My question is
the same even without them.)

With this new warning, would the appropriate diagnostic to
issue be -Wmaybe-uninitialized-aggregates or -Wuninitialized?

The description makes it sound like the former but I'm not
sure that's what I would want, either as an implementer of
the uninitialized strcpy warning (I plan to add one) or as
its user.

On the other hand, if the answer is the latter (or that it
depends) then introducing an option for it would seem like
exposing an interface to an internal detail (limitation)
with unspecified effects.

Martin

PS In the first sentence of the description, the phrase should
read "enables the same warning as", not "like".


>   @item -Wunknown-pragmas
>   @opindex Wunknown-pragmas
>   @opindex Wno-unknown-pragmas
> 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 {
>

Hi,

On Mon, Nov 11 2019, Martin Sebor wrote:
> On 11/8/19 5:41 AM, 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.
>> 
>> The patch is a bit bigger because of documentation (which I'll be happy
>> to improve based on your suggestions) and testsuite churn, but the main
>> bit is the following added test in warn_uninit function:
>> 
>>    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;
>>      }
>> 
>> The reason why I also test DECL_HAS_DEBUG_EXPR_P is
>> gfortran.dg/pr39666-2.f90 - without it the test silences a warning about
>> a decl representing the return value which is an artificial scalar
>> variable (probably all the way from the front-end).  We can of course
>> not care about not warning for it but then I don't know how to call and
>> document the new option :-)  Generally, if someone can think of a better
>> test to identify SRA DECLs, I'll be happy to change that.  We might put
>> a bit to identify SRA decls in the decl tree, but I tend to think that
>> is not a good use of the few remaining bits there.
>> 
>> What do you think, is something along these lines a good idea?
>> 
>> Thanks,
>> 
>> Martin
>> 
>> 
>> 
>> 2019-11-08  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.
>> 
>> 	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/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 ++++++++-
>>   7 files changed, 81 insertions(+), 6 deletions(-)
>>   create mode 100644 gcc/testsuite/g++.dg/warn/pr80635.C
>> 
>> diff --git a/gcc/common.opt b/gcc/common.opt
>> index cc279f411d7..03769299df8 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 faa7fa95a0e..dbc3219b770 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
>> @@ -4498,6 +4499,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
>> @@ -5690,10 +5692,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}.
>> +
>
> Let me ask a question.  Suppose I have code like this:
>
>    struct S { char a[4], b[4]; };
>
>    void* g (void)
>    {
>      struct S *p = malloc (sizeof *p);
>      strcpy (p->a + 1, p->b + 1);
>      return p;
>    }
>
> (I include the offsets only because they make an interesting
> difference in the internal representation.  My question is
> the same even without them.)
>
> With this new warning, would the appropriate diagnostic to
> issue be -Wmaybe-uninitialized-aggregates or -Wuninitialized?

The patch should not change the behavior of -Wuninitialized so that if
an uninitialized value use is detected on a spot which is always
executed, a warning should be emitted regardless if the underpinning
DECL is a result of SRA or not - the user should fix their code, not
silence a spurious warning because it is not spurious.

It's the tricky maybe-uninitialized cases where I wanted to mitigate a
common source of false positives which are difficult to silence.

As far as the strcpy example is concerned, ideally it would be emitted
as part of both -Wuninitialized and -Wmaybe-uninitialized-aggregates
depending on whether it is a maybe warning or not, but not with only
-Wmaybe-uninitialized.

>
> The description makes it sound like the former but I'm not
> sure that's what I would want, either as an implementer of
> the uninitialized strcpy warning (I plan to add one) or as
> its user.

What are the problems for the user?  I think that the distinction
between maybe uninitialized and always uninitialized is genuinely useful
one.  And as an implementor of a new similar warning, don't you need to
distinguish between them even now?

>
> On the other hand, if the answer is the latter (or that it
> depends) then introducing an option for it would seem like
> exposing an interface to an internal detail (limitation)
> with unspecified effects.

I agree that describing the new option would be tricky and I am opened
to suggestions for improvement/overhaul of that.  On the other hand, the
options already depend on internal details and limitations because they
only work on stuff that GCC can track.  E.g. they stop working as soon
as a variable - even a scalar one - happens to be addressable, for
example if we do less inlining.  Likewise if SRA suddenly decided not to
scalarize something the warning for that bit would be gone too.

> PS In the first sentence of the description, the phrase should
> read "enables the same warning as", not "like".

OK, thanks,

Martin

On 11/11/19 10:29 AM, Martin Jambor wrote:
> Hi,
> 
> On Mon, Nov 11 2019, Martin Sebor wrote:
>> On 11/8/19 5:41 AM, 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.
>>>
>>> The patch is a bit bigger because of documentation (which I'll be happy
>>> to improve based on your suggestions) and testsuite churn, but the main
>>> bit is the following added test in warn_uninit function:
>>>
>>>     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;
>>>       }
>>>
>>> The reason why I also test DECL_HAS_DEBUG_EXPR_P is
>>> gfortran.dg/pr39666-2.f90 - without it the test silences a warning about
>>> a decl representing the return value which is an artificial scalar
>>> variable (probably all the way from the front-end).  We can of course
>>> not care about not warning for it but then I don't know how to call and
>>> document the new option :-)  Generally, if someone can think of a better
>>> test to identify SRA DECLs, I'll be happy to change that.  We might put
>>> a bit to identify SRA decls in the decl tree, but I tend to think that
>>> is not a good use of the few remaining bits there.
>>>
>>> What do you think, is something along these lines a good idea?
>>>
>>> Thanks,
>>>
>>> Martin
>>>
>>>
>>>
>>> 2019-11-08  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.
>>>
>>> 	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/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 ++++++++-
>>>    7 files changed, 81 insertions(+), 6 deletions(-)
>>>    create mode 100644 gcc/testsuite/g++.dg/warn/pr80635.C
>>>
>>> diff --git a/gcc/common.opt b/gcc/common.opt
>>> index cc279f411d7..03769299df8 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 faa7fa95a0e..dbc3219b770 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
>>> @@ -4498,6 +4499,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
>>> @@ -5690,10 +5692,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}.
>>> +
>>
>> Let me ask a question.  Suppose I have code like this:
>>
>>     struct S { char a[4], b[4]; };
>>
>>     void* g (void)
>>     {
>>       struct S *p = malloc (sizeof *p);
>>       strcpy (p->a + 1, p->b + 1);
>>       return p;
>>     }
>>
>> (I include the offsets only because they make an interesting
>> difference in the internal representation.  My question is
>> the same even without them.)
>>
>> With this new warning, would the appropriate diagnostic to
>> issue be -Wmaybe-uninitialized-aggregates or -Wuninitialized?
> 
> The patch should not change the behavior of -Wuninitialized so that if
> an uninitialized value use is detected on a spot which is always
> executed, a warning should be emitted regardless if the underpinning
> DECL is a result of SRA or not - the user should fix their code, not
> silence a spurious warning because it is not spurious.
> 
> It's the tricky maybe-uninitialized cases where I wanted to mitigate a
> common source of false positives which are difficult to silence.

Understood.

> 
> As far as the strcpy example is concerned, ideally it would be emitted
> as part of both -Wuninitialized and -Wmaybe-uninitialized-aggregates
> depending on whether it is a maybe warning or not, but not with only
> -Wmaybe-uninitialized.

I see.  This makes sense for the simple example above.

> 
>>
>> The description makes it sound like the former but I'm not
>> sure that's what I would want, either as an implementer of
>> the uninitialized strcpy warning (I plan to add one) or as
>> its user.
> 
> What are the problems for the user?  I think that the distinction
> between maybe uninitialized and always uninitialized is genuinely useful
> one.  And as an implementor of a new similar warning, don't you need to
> distinguish between them even now?

Yes, the distinction between the "maybe" and "definitely" kinds
of warnings is useful and (IMO) clear, and not my concern. (Sorry,
I think my example may not have been as helpful as I wanted it to
be.)

To be useful, the I think the distinction between -Wmaybe-
uninitialized and -Wmaybe-uninitialized-aggregates will need
to be made also very clear.  But I'm not sure that it will be
possible.  In the strcpy example, the GIMPLE looks like this:

   _1 = &MEM <char[4]> [(void *)p_5 + 5B];
   _2 = &MEM <char[4]> [(void *)p_5 + 1B];
   strcpy (_2, _1);

i.e., it's clear the accesses are to distinct parts of the same
object, but it's not not necessarily as clear whether they are
to distinct subobjects of the same aggregate.

It becomes even less clear if one or both offsets are non-constant
(but in a known range), or when only parts of the larger objects
are initialized (e.g., just some members, or some initial bytes
of a chunk of memory allocated by malloc as often happens with
string functions).  It's also unclear when the access involves
a PHI node some of whose operands are aggregates and the others
are not.

I haven't had time to think about it very hard but it seems that
the answer in each of these cases might come down to a judgment
call.  That will blur the distinction between the two and make
the new warning option less useful.

>> On the other hand, if the answer is the latter (or that it
>> depends) then introducing an option for it would seem like
>> exposing an interface to an internal detail (limitation)
>> with unspecified effects.
> 
> I agree that describing the new option would be tricky and I am opened
> to suggestions for improvement/overhaul of that.  On the other hand, the
> options already depend on internal details and limitations because they
> only work on stuff that GCC can track.  E.g. they stop working as soon
> as a variable - even a scalar one - happens to be addressable, for
> example if we do less inlining.  Likewise if SRA suddenly decided not to
> scalarize something the warning for that bit would be gone too.

Sure, all late warnings are limited by what GCC tracks and how
how how well.  What I meant is that I'm not sure it's a good
idea to introduce a distinction between uninitialized objects
and their subobjects only because GCC happens to have an easier
time avoiding false positives in one than in the other.
Especially if it's uncertain that it will be possible to make
the difference clear in all cases, or that the balance won't
swing in the other direction in the future.

My general concern is that if the new option is documented to
control reads of uninitialized aggregates and users learn to
disable it because it suppresses common false positives, they
will also disable all the true positives in warnings that may
not be susceptible to the same false positive problem, either
present or future.

Martin

> 
>> PS In the first sentence of the description, the phrase should
>> read "enables the same warning as", not "like".
> 
> OK, thanks,
> 
> Martin
>

Hi,

On Mon, Nov 11 2019, Martin Sebor wrote:
> On 11/11/19 10:29 AM, Martin Jambor wrote:
>> On Mon, Nov 11 2019, Martin Sebor wrote:
>>> On 11/8/19 5:41 AM, 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.
>>>>

...

>>>> +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}.
>>>> +
>>>
>>> Let me ask a question.  Suppose I have code like this:
>>>
>>>     struct S { char a[4], b[4]; };
>>>
>>>     void* g (void)
>>>     {
>>>       struct S *p = malloc (sizeof *p);
>>>       strcpy (p->a + 1, p->b + 1);
>>>       return p;
>>>     }
>>>
>>> (I include the offsets only because they make an interesting
>>> difference in the internal representation.  My question is
>>> the same even without them.)
>>>
>>> With this new warning, would the appropriate diagnostic to
>>> issue be -Wmaybe-uninitialized-aggregates or -Wuninitialized?
>> 
>> The patch should not change the behavior of -Wuninitialized so that if
>> an uninitialized value use is detected on a spot which is always
>> executed, a warning should be emitted regardless if the underpinning
>> DECL is a result of SRA or not - the user should fix their code, not
>> silence a spurious warning because it is not spurious.
>> 
>> It's the tricky maybe-uninitialized cases where I wanted to mitigate a
>> common source of false positives which are difficult to silence.
>
> Understood.
>
>> 
>> As far as the strcpy example is concerned, ideally it would be emitted
>> as part of both -Wuninitialized and -Wmaybe-uninitialized-aggregates
>> depending on whether it is a maybe warning or not, but not with only
>> -Wmaybe-uninitialized.
>
> I see.  This makes sense for the simple example above.
>
>> 
>>>
>>> The description makes it sound like the former but I'm not
>>> sure that's what I would want, either as an implementer of
>>> the uninitialized strcpy warning (I plan to add one) or as
>>> its user.
>> 
>> What are the problems for the user?  I think that the distinction
>> between maybe uninitialized and always uninitialized is genuinely useful
>> one.  And as an implementor of a new similar warning, don't you need to
>> distinguish between them even now?
>
> Yes, the distinction between the "maybe" and "definitely" kinds
> of warnings is useful and (IMO) clear, and not my concern. (Sorry,
> I think my example may not have been as helpful as I wanted it to
> be.)
>
> To be useful, the I think the distinction between -Wmaybe-
> uninitialized and -Wmaybe-uninitialized-aggregates will need
> to be made also very clear.  But I'm not sure that it will be
> possible.  In the strcpy example, the GIMPLE looks like this:
>
>    _1 = &MEM <char[4]> [(void *)p_5 + 5B];
>    _2 = &MEM <char[4]> [(void *)p_5 + 1B];
>    strcpy (_2, _1);
>
> i.e., it's clear the accesses are to distinct parts of the same
> object, but it's not not necessarily as clear whether they are
> to distinct subobjects of the same aggregate.

In my simple world, anything that is not an outright scalar is an
aggregate (well, naked complex numbers and vectors are perhaps in the
grey area).  I would say that once you look at MEM_REF, it is an
aggregate.

>
> It becomes even less clear if one or both offsets are non-constant
> (but in a known range), or when only parts of the larger objects
> are initialized (e.g., just some members, or some initial bytes
> of a chunk of memory allocated by malloc as often happens with
> string functions).  It's also unclear when the access involves
> a PHI node some of whose operands are aggregates and the others
> are not.
>
> I haven't had time to think about it very hard but it seems that
> the answer in each of these cases might come down to a judgment
> call.  That will blur the distinction between the two and make
> the new warning option less useful.
>
>>> On the other hand, if the answer is the latter (or that it
>>> depends) then introducing an option for it would seem like
>>> exposing an interface to an internal detail (limitation)
>>> with unspecified effects.
>> 
>> I agree that describing the new option would be tricky and I am opened
>> to suggestions for improvement/overhaul of that.  On the other hand, the
>> options already depend on internal details and limitations because they
>> only work on stuff that GCC can track.  E.g. they stop working as soon
>> as a variable - even a scalar one - happens to be addressable, for
>> example if we do less inlining.  Likewise if SRA suddenly decided not to
>> scalarize something the warning for that bit would be gone too.
>
> Sure, all late warnings are limited by what GCC tracks and how
> how how well.  What I meant is that I'm not sure it's a good
> idea to introduce a distinction between uninitialized objects
> and their subobjects only because GCC happens to have an easier
> time avoiding false positives in one than in the other.

No, no, no!  I do not want to introduce the split of
-Wmaybe-uninitialized into -Wmaybe-uninitialized and
-Wmaybe-uninitialized-aggregates because the latter has more false
positives.  I don't think it does, I can't see a reason why it would (in
its current form).  I want to split them because the former can be
easily silenced by initializing the scalar variable when the user
defines it while the latter cannot.  That's why the former should be in
-Wall and the latter only in -Wextra.

> Especially if it's uncertain that it will be possible to make
> the difference clear in all cases, or that the balance won't
> swing in the other direction in the future.

I understand this concern but see above my note about MEM_REF basically
always implying some aggregateness.  The aim is to distinguish between
easily silenceable cases from not so easily silenceable ones.
Describing that to the user would be tricky, but then making the
distinction IMHO not that much.

> My general concern is that if the new option is documented to
> control reads of uninitialized aggregates and users learn to
> disable it because it suppresses common false positives, they
> will also disable all the true positives in warnings that may
> not be susceptible to the same false positive problem, either
> present or future.

Well, -Wno-error=maybe-uninitialized was suggested as the best
workaround in PR 80635 and doing that is not so far away from outright
-Wno-maybe-uninitialized.  This patch is actually an attempt to persuade
people not to do either.

Martin