On Fri, Nov 8, 2019 at 1:41 PM Martin Jambor <mjam...@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 <mjam...@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
>
