This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
Re: [RFC] Only warn for maybe-uninitialized SRAed bits in -Wextra (PR 80635)
- From: Martin Sebor <msebor at gmail dot com>
- To: Martin Jambor <mjambor at suse dot cz>, GCC Patches <gcc-patches at gcc dot gnu dot org>
- Cc: Jeff Law <law at redhat dot com>
- Date: Mon, 11 Nov 2019 09:21:01 -0700
- Subject: Re: [RFC] Only warn for maybe-uninitialized SRAed bits in -Wextra (PR 80635)
- References: <ri6imnuleur.fsf@suse.cz>
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 {