[PATCH v2] c-family: Honor -Wno-init-self for cv-qual vars [PR102633]

[Marek Polacek <polacek@redhat.com>]

On Sat, Aug 06, 2022 at 03:29:05PM -0700, Jason Merrill wrote:
> On 7/26/22 14:31, Marek Polacek wrote:
> > On Tue, Jul 26, 2022 at 04:24:18PM -0400, Jason Merrill wrote:
> > > On 7/26/22 15:03, Marek Polacek wrote:
> > > > Since r11-5188-g32934a4f45a721, we drop qualifiers during l-to-r
> > > > conversion by creating a NOP_EXPR.  For e.g.
> > > > 
> > > >     const int i = i;
> > > > 
> > > > that means that the DECL_INITIAL is '(int) i' and not 'i' anymore.
> > > > Consequently, we don't suppress_warning here:
> > > > 
> > > > 711     case DECL_EXPR:
> > > > 715       if (VAR_P (DECL_EXPR_DECL (*expr_p))
> > > > 716           && !DECL_EXTERNAL (DECL_EXPR_DECL (*expr_p))
> > > > 717           && !TREE_STATIC (DECL_EXPR_DECL (*expr_p))
> > > > 718           && (DECL_INITIAL (DECL_EXPR_DECL (*expr_p)) == DECL_EXPR_DECL (*expr_p))
> > > > 719           && !warn_init_self)
> > > > 720         suppress_warning (DECL_EXPR_DECL (*expr_p), OPT_Winit_self);
> > > > 
> > > > because of the check on line 718 -- (int) i is not i.  So -Wno-init-self
> > > > doesn't disable the warning as it's supposed to.
> > > > 
> > > > The following patch fixes it...except it doesn't, for volatile variables
> > > > in C++.  The problem is that for
> > > > 
> > > >     volatile int k = k;
> > > > 
> > > > we see that the initializer has TREE_SIDE_EFFECTS, so we perform dynamic
> > > > initialization.  So there's no DECL_INITIAL and the suppress_warning
> > > > call above is never done.  I suppose we could amend get_no_uninit_warning
> > > > to return true for volatile-qualified expressions.  I mean, can we ever
> > > > say for a fact that a volatile variable is uninitialized?
> > > > 
> > > > Bootstrapped/regtested on x86_64-pc-linux-gnu, ok for trunk?
> > > > 
> > > > 	PR middle-end/102633
> > > > 
> > > > gcc/c-family/ChangeLog:
> > > > 
> > > > 	* c-gimplify.cc (c_gimplify_expr): Strip NOPs of DECL_INITIAL.
> > > 
> > > I wonder if we want to handle this i = i case earlier, like in finish_decl.
> > 
> > I could, something like
> > 
> > @@ -5381,7 +5381,14 @@ finish_decl (tree decl, location_t init_loc, tree init,
> >       init = NULL_TREE;
> > 
> >     if (init)
> > -    store_init_value (init_loc, decl, init, origtype);
> > +    {
> > +      /* In the self-init case, undo the artificial NOP_EXPR we may have
> > +    added in convert_lvalue_to_rvalue so that c_gimplify_expr/DECL_EXPR
> > +    can perform suppress_warning.  */
> > +      if (TREE_CODE (init) == NOP_EXPR && TREE_OPERAND (init, 0) == decl)
> > +   init = TREE_OPERAND (init, 0);
> > +      store_init_value (init_loc, decl, init, origtype);
> > +    }
> > 
> > but then I'd have to do the same thing in cp_finish_decl because
> > decay_conversion also adds a NOP_EXPR for cv-qualified non-class prvalues.
> > Is that what we want?  To me that seems less clean than having c_gimplify_expr
> > see through NOP_EXPRs.
> 
> I was thinking of checking the form of the initializer before
> decay_conversion or anything else messes with it, and calling
> suppress_warning at that point instead of in c_gimplify_expr.

Aaah, okay.  Here's a patch that does it.  In the C FE it has to
happen really early.  Now both front ends behave the same wrt volatiles!

Bootstrapped/regtested on x86_64-pc-linux-gnu, ok for trunk?

-- >8 --
Since r11-5188-g32934a4f45a721, we drop qualifiers during l-to-r
conversion by creating a NOP_EXPR.  For e.g.

  const int i = i;

that means that the DECL_INITIAL is '(int) i' and not 'i' anymore.
Consequently, we don't suppress_warning here:

711     case DECL_EXPR:
715       if (VAR_P (DECL_EXPR_DECL (*expr_p))
716           && !DECL_EXTERNAL (DECL_EXPR_DECL (*expr_p))
717           && !TREE_STATIC (DECL_EXPR_DECL (*expr_p))
718           && (DECL_INITIAL (DECL_EXPR_DECL (*expr_p)) == DECL_EXPR_DECL (*expr_p))
719           && !warn_init_self)
720         suppress_warning (DECL_EXPR_DECL (*expr_p), OPT_Winit_self);

because of the check on line 718 -- (int) i is not i.  So -Wno-init-self
doesn't disable the warning as it's supposed to.

The following patch fixes it by moving the suppress_warning call from
c_gimplify_expr to the front ends, at points where we haven't created
the NOP_EXPR yet.

	PR middle-end/102633

gcc/c-family/ChangeLog:

	* c-gimplify.cc (c_gimplify_expr) <case DECL_EXPR>: Don't call
	suppress_warning here.

gcc/c/ChangeLog:

	* c-parser.cc (c_parser_initializer): Add new tree parameter.  Use it.
	Call suppress_warning.
	(c_parser_declaration_or_fndef): Pass d down to c_parser_initializer.
	(c_parser_omp_declare_reduction): Pass omp_priv down to
	c_parser_initializer.

gcc/cp/ChangeLog:

	* decl.cc (cp_finish_decl): Call suppress_warning.

gcc/testsuite/ChangeLog:

	* c-c++-common/Winit-self1.c: New test.
	* c-c++-common/Winit-self2.c: New test.
---
 gcc/c-family/c-gimplify.cc               | 12 ---------
 gcc/c/c-parser.cc                        | 19 ++++++++++++---
 gcc/cp/decl.cc                           |  8 ++++++
 gcc/testsuite/c-c++-common/Winit-self1.c | 31 ++++++++++++++++++++++++
 gcc/testsuite/c-c++-common/Winit-self2.c | 31 ++++++++++++++++++++++++
 5 files changed, 85 insertions(+), 16 deletions(-)
 create mode 100644 gcc/testsuite/c-c++-common/Winit-self1.c
 create mode 100644 gcc/testsuite/c-c++-common/Winit-self2.c

diff --git a/gcc/c-family/c-gimplify.cc b/gcc/c-family/c-gimplify.cc
index a6f26c9b0d3..039a4b93230 100644
--- a/gcc/c-family/c-gimplify.cc
+++ b/gcc/c-family/c-gimplify.cc
@@ -708,18 +708,6 @@ c_gimplify_expr (tree *expr_p, gimple_seq *pre_p ATTRIBUTE_UNUSED,
 	break;
       }
 
-    case DECL_EXPR:
-      /* This is handled mostly by gimplify.cc, but we have to deal with
-	 not warning about int x = x; as it is a GCC extension to turn off
-	 this warning but only if warn_init_self is zero.  */
-      if (VAR_P (DECL_EXPR_DECL (*expr_p))
-	  && !DECL_EXTERNAL (DECL_EXPR_DECL (*expr_p))
-	  && !TREE_STATIC (DECL_EXPR_DECL (*expr_p))
-	  && (DECL_INITIAL (DECL_EXPR_DECL (*expr_p)) == DECL_EXPR_DECL (*expr_p))
-	  && !warn_init_self)
-	suppress_warning (DECL_EXPR_DECL (*expr_p), OPT_Winit_self);
-      break;
-
     case PREINCREMENT_EXPR:
     case PREDECREMENT_EXPR:
     case POSTINCREMENT_EXPR:
diff --git a/gcc/c/c-parser.cc b/gcc/c/c-parser.cc
index 92049d1a101..6ed17ed9c48 100644
--- a/gcc/c/c-parser.cc
+++ b/gcc/c/c-parser.cc
@@ -1521,7 +1521,7 @@ static struct c_arg_info *c_parser_parms_list_declarator (c_parser *, tree,
 static struct c_parm *c_parser_parameter_declaration (c_parser *, tree, bool);
 static tree c_parser_simple_asm_expr (c_parser *);
 static tree c_parser_gnu_attributes (c_parser *);
-static struct c_expr c_parser_initializer (c_parser *);
+static struct c_expr c_parser_initializer (c_parser *, tree);
 static struct c_expr c_parser_braced_init (c_parser *, tree, bool,
 					   struct obstack *);
 static void c_parser_initelt (c_parser *, struct obstack *);
@@ -2286,7 +2286,7 @@ c_parser_declaration_or_fndef (c_parser *parser, bool fndef_ok,
 		  int flag_sanitize_save = flag_sanitize;
 		  if (TREE_CODE (d) == PARM_DECL)
 		    flag_sanitize = 0;
-		  init = c_parser_initializer (parser);
+		  init = c_parser_initializer (parser, d);
 		  flag_sanitize = flag_sanitize_save;
 		  finish_init ();
 		}
@@ -5211,11 +5211,13 @@ c_parser_type_name (c_parser *parser, bool alignas_ok)
    Any expression without commas is accepted in the syntax for the
    constant-expressions, with non-constant expressions rejected later.
 
+   DECL is the declaration we're parsing this initializer for.
+
    This function is only used for top-level initializers; for nested
    ones, see c_parser_initval.  */
 
 static struct c_expr
-c_parser_initializer (c_parser *parser)
+c_parser_initializer (c_parser *parser, tree decl)
 {
   if (c_parser_next_token_is (parser, CPP_OPEN_BRACE))
     return c_parser_braced_init (parser, NULL_TREE, false, NULL);
@@ -5224,6 +5226,15 @@ c_parser_initializer (c_parser *parser)
       struct c_expr ret;
       location_t loc = c_parser_peek_token (parser)->location;
       ret = c_parser_expr_no_commas (parser, NULL);
+      /* This is handled mostly by gimplify.cc, but we have to deal with
+	 not warning about int x = x; as it is a GCC extension to turn off
+	 this warning but only if warn_init_self is zero.  */
+      if (VAR_P (decl)
+	  && !DECL_EXTERNAL (decl)
+	  && !TREE_STATIC (decl)
+	  && ret.value == decl
+	  && !warn_init_self)
+	suppress_warning (decl, OPT_Winit_self);
       if (TREE_CODE (ret.value) != STRING_CST
 	  && TREE_CODE (ret.value) != COMPOUND_LITERAL_EXPR)
 	ret = convert_lvalue_to_rvalue (loc, ret, true, true);
@@ -22576,7 +22587,7 @@ c_parser_omp_declare_reduction (c_parser *parser, enum pragma_context context)
 		  location_t loc = c_parser_peek_token (parser)->location;
 		  rich_location richloc (line_table, loc);
 		  start_init (omp_priv, NULL_TREE, 0, &richloc);
-		  struct c_expr init = c_parser_initializer (parser);
+		  struct c_expr init = c_parser_initializer (parser, omp_priv);
 		  finish_init ();
 		  finish_decl (omp_priv, loc, init.value,
 		      	       init.original_type, NULL_TREE);
diff --git a/gcc/cp/decl.cc b/gcc/cp/decl.cc
index 70ad681467e..ff56fddba54 100644
--- a/gcc/cp/decl.cc
+++ b/gcc/cp/decl.cc
@@ -8253,6 +8253,14 @@ cp_finish_decl (tree decl, tree init, bool init_const_expr_p,
 	      && !TYPE_REF_P (type))
 	    TREE_CONSTANT (decl) = 1;
 	}
+      /* This is handled mostly by gimplify.cc, but we have to deal with
+	 not warning about int x = x; as it is a GCC extension to turn off
+	 this warning but only if warn_init_self is zero.  */
+      if (!DECL_EXTERNAL (decl)
+	  && !TREE_STATIC (decl)
+	  && decl == tree_strip_any_location_wrapper (init)
+	  && !warn_init_self)
+	suppress_warning (decl, OPT_Winit_self);
     }
 
   if (flag_openmp
diff --git a/gcc/testsuite/c-c++-common/Winit-self1.c b/gcc/testsuite/c-c++-common/Winit-self1.c
new file mode 100644
index 00000000000..740b83b5e9f
--- /dev/null
+++ b/gcc/testsuite/c-c++-common/Winit-self1.c
@@ -0,0 +1,31 @@
+/* PR middle-end/102633 */
+/* { dg-do compile } */
+/* { dg-options "-Wuninitialized -Wno-init-self" } */
+
+int
+fn1 (void)
+{
+  int i = i;
+  return i;
+}
+
+int
+fn2 ()
+{
+  const int j = j;
+  return j;
+}
+
+int
+fn3 ()
+{
+  volatile int k = k;
+  return k;
+}
+
+int
+fn4 ()
+{
+  const volatile int l = l;
+  return l;
+}
diff --git a/gcc/testsuite/c-c++-common/Winit-self2.c b/gcc/testsuite/c-c++-common/Winit-self2.c
new file mode 100644
index 00000000000..13aa9efdf26
--- /dev/null
+++ b/gcc/testsuite/c-c++-common/Winit-self2.c
@@ -0,0 +1,31 @@
+/* PR middle-end/102633 */
+/* { dg-do compile } */
+/* { dg-options "-Wuninitialized -Winit-self" } */
+
+int
+fn1 (void)
+{
+  int i = i; /* { dg-warning "used uninitialized" } */
+  return i;
+}
+
+int
+fn2 ()
+{
+  const int j = j; /* { dg-warning "used uninitialized" } */
+  return j;
+}
+
+int
+fn3 ()
+{
+  volatile int k = k; /* { dg-warning "used uninitialized" } */
+  return k;
+}
+
+int
+fn4 ()
+{
+  const volatile int l = l; /* { dg-warning "used uninitialized" } */
+  return l;
+}

base-commit: 21c7aab09805d0c8c7695c8a69c8715d673a739a
-- 
2.37.1