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

  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.

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               | 18 +++++++------
 gcc/testsuite/c-c++-common/Winit-self1.c | 32 ++++++++++++++++++++++++
 gcc/testsuite/c-c++-common/Winit-self2.c | 31 +++++++++++++++++++++++
 3 files changed, 74 insertions(+), 7 deletions(-)
 create mode 100644 gcc/testsuite/c-c++-common/Winit-self1.c
 create mode 100644 gcc/testsuite/c-c++-common/Winit-self2.c


base-commit: 600956c81c784f4a0cc9d10f6e03e01847afd961
  

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.

> 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               | 18 +++++++------
>   gcc/testsuite/c-c++-common/Winit-self1.c | 32 ++++++++++++++++++++++++
>   gcc/testsuite/c-c++-common/Winit-self2.c | 31 +++++++++++++++++++++++
>   3 files changed, 74 insertions(+), 7 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..2e011830846 100644
> --- a/gcc/c-family/c-gimplify.cc
> +++ b/gcc/c-family/c-gimplify.cc
> @@ -712,13 +712,17 @@ c_gimplify_expr (tree *expr_p, gimple_seq *pre_p ATTRIBUTE_UNUSED,
>         /* 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;
> +      {
> +	tree &decl = DECL_EXPR_DECL (*expr_p);
> +	if (VAR_P (decl)
> +	    && !DECL_EXTERNAL (decl)
> +	    && !TREE_STATIC (decl)
> +	    && (DECL_INITIAL (decl)
> +		&& tree_strip_nop_conversions (DECL_INITIAL (decl)) == decl)
> +	    && !warn_init_self)
> +	  suppress_warning (decl, OPT_Winit_self);
> +	break;
> +      }
>   
>       case PREINCREMENT_EXPR:
>       case PREDECREMENT_EXPR:
> 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..2a1a755fc71
> --- /dev/null
> +++ b/gcc/testsuite/c-c++-common/Winit-self1.c
> @@ -0,0 +1,32 @@
> +/* 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 ()
> +{
> +  /* ??? Do we want this warning in C++?  Probably not with -Wno-init-self.  */
> +  volatile int k = k; /* { dg-warning "used uninitialized" "" { target c++ } } */
> +  return k;
> +}
> +
> +int
> +fn4 ()
> +{
> +  const volatile int l = l; /* { dg-warning "used uninitialized" "" { target c++ } } */
> +  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: 600956c81c784f4a0cc9d10f6e03e01847afd961
  

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.

Marek
  

On Tue, 26 Jul 2022, 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?

As I said in the bug the bug is probably that we emit uninitialized
diagnostics for volatiles at all?  OTOH 'volatile' is recommended
for vars live around setjmp/longjmp and there diagnostics would be
welcome.  It's probably the difference between "compiler-hands-off"
and "hardware-controlled" :/

> 
> 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.
> 
> 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               | 18 +++++++------
>  gcc/testsuite/c-c++-common/Winit-self1.c | 32 ++++++++++++++++++++++++
>  gcc/testsuite/c-c++-common/Winit-self2.c | 31 +++++++++++++++++++++++
>  3 files changed, 74 insertions(+), 7 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..2e011830846 100644
> --- a/gcc/c-family/c-gimplify.cc
> +++ b/gcc/c-family/c-gimplify.cc
> @@ -712,13 +712,17 @@ c_gimplify_expr (tree *expr_p, gimple_seq *pre_p ATTRIBUTE_UNUSED,
>        /* 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;
> +      {
> +	tree &decl = DECL_EXPR_DECL (*expr_p);
> +	if (VAR_P (decl)
> +	    && !DECL_EXTERNAL (decl)
> +	    && !TREE_STATIC (decl)
> +	    && (DECL_INITIAL (decl)
> +		&& tree_strip_nop_conversions (DECL_INITIAL (decl)) == decl)
> +	    && !warn_init_self)
> +	  suppress_warning (decl, OPT_Winit_self);
> +	break;
> +      }
>  
>      case PREINCREMENT_EXPR:
>      case PREDECREMENT_EXPR:
> 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..2a1a755fc71
> --- /dev/null
> +++ b/gcc/testsuite/c-c++-common/Winit-self1.c
> @@ -0,0 +1,32 @@
> +/* 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 ()
> +{
> +  /* ??? Do we want this warning in C++?  Probably not with -Wno-init-self.  */
> +  volatile int k = k; /* { dg-warning "used uninitialized" "" { target c++ } } */
> +  return k;
> +}
> +
> +int
> +fn4 ()
> +{
> +  const volatile int l = l; /* { dg-warning "used uninitialized" "" { target c++ } } */
> +  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: 600956c81c784f4a0cc9d10f6e03e01847afd961
>
  

On Wed, Jul 27, 2022 at 06:41:09AM +0000, Richard Biener via Gcc-patches wrote:
> On Tue, 26 Jul 2022, 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?
> 
> As I said in the bug the bug is probably that we emit uninitialized
> diagnostics for volatiles at all?

Non-volatile const variables also have this problem, so I think we still
need this patch (or something like it).

> OTOH 'volatile' is recommended
> for vars live around setjmp/longjmp and there diagnostics would be
> welcome.  It's probably the difference between "compiler-hands-off"
> and "hardware-controlled" :/

Ah, you're right.  Then I guess it's better to leave the warning be,
(but we still have a discrepancy between C and C++).

Marek
  

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.

Jason