[RFC][Patch][middle-end/PR102359]Not add initialization for READONLY variables with -ftrivial-auto-var-init

[RFC][Patch][middle-end/PR102359]Not add initialization for READONLY variables with -ftrivial-auto-var-init

[Qing Zhao]

Hi,

PR102359 (ICE gimplification failed since  r12-3433-ga25e0b5e6ac8a77a)
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=102359

Is due to -ftrivial-auto-var-init adding initialization for READONLY variable “this” in the following routine: (t.cpp.005t.original)

=======

;; Function A::foo()::<lambda()> (null)
;; enabled by -tree-original

{
  const struct A * const this [value-expr: &__closure->__this];
    const struct A * const this [value-expr: &__closure->__this];
  return <retval> = (double) ((const struct A *) this)->a;
}
=======

However, in the above routine, “this” is NOT marked as READONLY, but its value-expr "&__closure->__this” is marked as READONLY. 

There are two major issues:

1. In the routine “is_var_need_auto_init”, we should exclude “decl” that is marked as READONLY;
2. In the C++ FE, “this” should be marked as READONLY.

The idea solution will be:

1. Fix “is_var_need_auto_init” to exclude TREE_READONLY (decl);
2. Fix C++ FE to mark “this” as TREE_READONLY (decl)==true;

Not sure whether it’s hard for C++ FE to fix the 2nd issue or not? 

In the case it’s not a quick fix in C++FE, I proposed the following fix in middle end:

Let me know your comments or suggestions on this.

Thanks a lot for the help.

Qing

==============================
From 0a5982cd61bc4610655d3df00ae8d2fbcb3c8e9b Mon Sep 17 00:00:00 2001
From: Qing Zhao <qing.zhao@oracle.com>
Date: Wed, 29 Sep 2021 20:49:59 +0000
Subject: [PATCH] Fix PR102359

---
 gcc/gimplify.c                  | 15 +++++++++++++++
 gcc/testsuite/g++.dg/pr102359.C | 13 +++++++++++++
 2 files changed, 28 insertions(+)
 create mode 100644 gcc/testsuite/g++.dg/pr102359.C

diff --git a/gcc/gimplify.c b/gcc/gimplify.c
index 1067113b1639..a2587869b35d 100644
--- a/gcc/gimplify.c
+++ b/gcc/gimplify.c
@@ -1819,12 +1819,27 @@ gimple_add_padding_init_for_auto_var (tree decl, bool is_vla,
   gimplify_seq_add_stmt (seq_p, call);
 }
 
+/* Return true if the DECL is READONLY.
+   This is to workaround a C++ FE bug that only mark the value_expr of "this"
+   as readonly but does not mark "this" as readonly.
+   C++ FE should fix this issue before replacing this routine with
+   TREE_READONLY (decl).  */
+
+static bool
+is_decl_readonly (tree decl)
+{
+  return (TREE_READONLY (decl) 
+	   || (DECL_HAS_VALUE_EXPR_P (decl) 
+		 && TREE_READONLY (DECL_VALUE_EXPR (decl))));
+}
+
 /* Return true if the DECL need to be automaticly initialized by the
    compiler.  */
 static bool
 is_var_need_auto_init (tree decl)
 {
   if (auto_var_p (decl)
+      && !is_decl_readonly (decl)
       && (flag_auto_var_init > AUTO_INIT_UNINITIALIZED)
       && (!lookup_attribute ("uninitialized", DECL_ATTRIBUTES (decl)))
       && !is_empty_type (TREE_TYPE (decl)))
diff --git a/gcc/testsuite/g++.dg/pr102359.C b/gcc/testsuite/g++.dg/pr102359.C
new file mode 100644
index 000000000000..da643cde7bed
--- /dev/null
+++ b/gcc/testsuite/g++.dg/pr102359.C
@@ -0,0 +1,13 @@
+/* PR middle-end/102359 ICE gimplification failed since
+   r12-3433-ga25e0b5e6ac8a77a.  */
+/* { dg-do compile } */
+/* { dg-options "-ftrivial-auto-var-init=zero" } */
+/* { dg-require-effective-target c++17 } */
+
+struct A {
+  double a = 111;
+  auto foo() {
+    return [*this] { return a; };
+  }
+};
+int X = A{}.foo()();
-- 
2.27.0

Re: [RFC][Patch][middle-end/PR102359]Not add initialization for READONLY variables with -ftrivial-auto-var-init

[Jason Merrill]

On 9/29/21 17:30, Qing Zhao wrote:
> Hi,
> 
> PR102359 (ICE gimplification failed since  r12-3433-ga25e0b5e6ac8a77a)
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=102359
> 
> Is due to -ftrivial-auto-var-init adding initialization for READONLY variable “this” in the following routine: (t.cpp.005t.original)
> 
> =======
> 
> ;; Function A::foo()::<lambda()> (null)
> ;; enabled by -tree-original
> 
> {
>    const struct A * const this [value-expr: &__closure->__this];
>      const struct A * const this [value-expr: &__closure->__this];
>    return <retval> = (double) ((const struct A *) this)->a;
> }
> =======
> 
> However, in the above routine, “this” is NOT marked as READONLY, but its value-expr "&__closure->__this” is marked as READONLY.
> 
> There are two major issues:
> 
> 1. In the routine “is_var_need_auto_init”, we should exclude “decl” that is marked as READONLY;
> 2. In the C++ FE, “this” should be marked as READONLY.
> 
> The idea solution will be:
> 
> 1. Fix “is_var_need_auto_init” to exclude TREE_READONLY (decl);
> 2. Fix C++ FE to mark “this” as TREE_READONLY (decl)==true;
> 
> Not sure whether it’s hard for C++ FE to fix the 2nd issue or not?
> 
> In the case it’s not a quick fix in C++FE, I proposed the following fix in middle end:
> 
> Let me know your comments or suggestions on this.
> 
> Thanks a lot for the help.

I'd think is_var_need_auto_init should be false for any variable with 
DECL_HAS_VALUE_EXPR_P, as they aren't really variables, just ways of 
naming objects that are initialized elsewhere.

> Qing
> 
> ==============================
>  From 0a5982cd61bc4610655d3df00ae8d2fbcb3c8e9b Mon Sep 17 00:00:00 2001
> From: Qing Zhao <qing.zhao@oracle.com>
> Date: Wed, 29 Sep 2021 20:49:59 +0000
> Subject: [PATCH] Fix PR102359
> 
> ---
>   gcc/gimplify.c                  | 15 +++++++++++++++
>   gcc/testsuite/g++.dg/pr102359.C | 13 +++++++++++++
>   2 files changed, 28 insertions(+)
>   create mode 100644 gcc/testsuite/g++.dg/pr102359.C
> 
> diff --git a/gcc/gimplify.c b/gcc/gimplify.c
> index 1067113b1639..a2587869b35d 100644
> --- a/gcc/gimplify.c
> +++ b/gcc/gimplify.c
> @@ -1819,12 +1819,27 @@ gimple_add_padding_init_for_auto_var (tree decl, bool is_vla,
>     gimplify_seq_add_stmt (seq_p, call);
>   }
>   
> +/* Return true if the DECL is READONLY.
> +   This is to workaround a C++ FE bug that only mark the value_expr of "this"
> +   as readonly but does not mark "this" as readonly.
> +   C++ FE should fix this issue before replacing this routine with
> +   TREE_READONLY (decl).  */
> +
> +static bool
> +is_decl_readonly (tree decl)
> +{
> +  return (TREE_READONLY (decl)
> +	   || (DECL_HAS_VALUE_EXPR_P (decl)
> +		 && TREE_READONLY (DECL_VALUE_EXPR (decl))));
> +}
> +
>   /* Return true if the DECL need to be automaticly initialized by the
>      compiler.  */
>   static bool
>   is_var_need_auto_init (tree decl)
>   {
>     if (auto_var_p (decl)
> +      && !is_decl_readonly (decl)
>         && (flag_auto_var_init > AUTO_INIT_UNINITIALIZED)
>         && (!lookup_attribute ("uninitialized", DECL_ATTRIBUTES (decl)))
>         && !is_empty_type (TREE_TYPE (decl)))
> diff --git a/gcc/testsuite/g++.dg/pr102359.C b/gcc/testsuite/g++.dg/pr102359.C
> new file mode 100644
> index 000000000000..da643cde7bed
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/pr102359.C
> @@ -0,0 +1,13 @@
> +/* PR middle-end/102359 ICE gimplification failed since
> +   r12-3433-ga25e0b5e6ac8a77a.  */
> +/* { dg-do compile } */
> +/* { dg-options "-ftrivial-auto-var-init=zero" } */
> +/* { dg-require-effective-target c++17 } */
> +
> +struct A {
> +  double a = 111;
> +  auto foo() {
> +    return [*this] { return a; };
> +  }
> +};
> +int X = A{}.foo()();
> 

Re: [RFC][Patch][middle-end/PR102359]Not add initialization for READONLY variables with -ftrivial-auto-var-init

[Richard Biener]

On Thu, 30 Sep 2021, Jason Merrill wrote:

> On 9/29/21 17:30, Qing Zhao wrote:
> > Hi,
> > 
> > PR102359 (ICE gimplification failed since  r12-3433-ga25e0b5e6ac8a77a)
> > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=102359
> > 
> > Is due to -ftrivial-auto-var-init adding initialization for READONLY
> > variable “this” in the following routine: (t.cpp.005t.original)
> > 
> > =======
> > 
> > ;; Function A::foo()::<lambda()> (null)
> > ;; enabled by -tree-original
> > 
> > {
> >    const struct A * const this [value-expr: &__closure->__this];
> >      const struct A * const this [value-expr: &__closure->__this];
> >    return <retval> = (double) ((const struct A *) this)->a;
> > }
> > =======
> > 
> > However, in the above routine, “this” is NOT marked as READONLY, but its
> > value-expr "&__closure->__this” is marked as READONLY.
> > 
> > There are two major issues:
> > 
> > 1. In the routine “is_var_need_auto_init”, we should exclude “decl” that is
> > marked as READONLY;
> > 2. In the C++ FE, “this” should be marked as READONLY.
> > 
> > The idea solution will be:
> > 
> > 1. Fix “is_var_need_auto_init” to exclude TREE_READONLY (decl);
> > 2. Fix C++ FE to mark “this” as TREE_READONLY (decl)==true;
> > 
> > Not sure whether it’s hard for C++ FE to fix the 2nd issue or not?
> > 
> > In the case it’s not a quick fix in C++FE, I proposed the following fix in
> > middle end:
> > 
> > Let me know your comments or suggestions on this.
> > 
> > Thanks a lot for the help.
> 
> I'd think is_var_need_auto_init should be false for any variable with
> DECL_HAS_VALUE_EXPR_P, as they aren't really variables, just ways of naming
> objects that are initialized elsewhere.

IIRC handing variables with DECL_HAS_VALUE_EXPR_P is necessary to
auto-init VLAs, otherwise I tend to agree - would we handle those
when we see a DECL_EXPR then?

> 
> > Qing
> > 
> > ==============================
> > From 0a5982cd61bc4610655d3df00ae8d2fbcb3c8e9b Mon Sep 17 00:00:00 2001
> > From: Qing Zhao <qing.zhao@oracle.com>
> > Date: Wed, 29 Sep 2021 20:49:59 +0000
> > Subject: [PATCH] Fix PR102359
> > 
> > ---
> >   gcc/gimplify.c                  | 15 +++++++++++++++
> >   gcc/testsuite/g++.dg/pr102359.C | 13 +++++++++++++
> >   2 files changed, 28 insertions(+)
> >   create mode 100644 gcc/testsuite/g++.dg/pr102359.C
> > 
> > diff --git a/gcc/gimplify.c b/gcc/gimplify.c
> > index 1067113b1639..a2587869b35d 100644
> > --- a/gcc/gimplify.c
> > +++ b/gcc/gimplify.c
> > @@ -1819,12 +1819,27 @@ gimple_add_padding_init_for_auto_var (tree decl,
> > bool is_vla,
> >     gimplify_seq_add_stmt (seq_p, call);
> >   }
> >   
> > +/* Return true if the DECL is READONLY.
> > +   This is to workaround a C++ FE bug that only mark the value_expr of
> > "this"
> > +   as readonly but does not mark "this" as readonly.
> > +   C++ FE should fix this issue before replacing this routine with
> > +   TREE_READONLY (decl).  */
> > +
> > +static bool
> > +is_decl_readonly (tree decl)
> > +{
> > +  return (TREE_READONLY (decl)
> > +	   || (DECL_HAS_VALUE_EXPR_P (decl)
> > +		 && TREE_READONLY (DECL_VALUE_EXPR (decl))));
> > +}
> > +
> >   /* Return true if the DECL need to be automaticly initialized by the
> >      compiler.  */
> >   static bool
> >   is_var_need_auto_init (tree decl)
> >   {
> >     if (auto_var_p (decl)
> > +      && !is_decl_readonly (decl)
> >         && (flag_auto_var_init > AUTO_INIT_UNINITIALIZED)
> >         && (!lookup_attribute ("uninitialized", DECL_ATTRIBUTES (decl)))
> >         && !is_empty_type (TREE_TYPE (decl)))
> > diff --git a/gcc/testsuite/g++.dg/pr102359.C
> > b/gcc/testsuite/g++.dg/pr102359.C
> > new file mode 100644
> > index 000000000000..da643cde7bed
> > --- /dev/null
> > +++ b/gcc/testsuite/g++.dg/pr102359.C
> > @@ -0,0 +1,13 @@
> > +/* PR middle-end/102359 ICE gimplification failed since
> > +   r12-3433-ga25e0b5e6ac8a77a.  */
> > +/* { dg-do compile } */
> > +/* { dg-options "-ftrivial-auto-var-init=zero" } */
> > +/* { dg-require-effective-target c++17 } */
> > +
> > +struct A {
> > +  double a = 111;
> > +  auto foo() {
> > +    return [*this] { return a; };
> > +  }
> > +};
> > +int X = A{}.foo()();
> > 
> 
> 

-- 
Richard Biener <rguenther@suse.de>
SUSE Software Solutions Germany GmbH, Maxfeldstrasse 5, 90409 Nuernberg,
Germany; GF: Felix Imendörffer; HRB 36809 (AG Nuernberg)

Re: [RFC][Patch][middle-end/PR102359]Not add initialization for READONLY variables with -ftrivial-auto-var-init

[Qing Zhao]

> On Sep 30, 2021, at 1:54 AM, Richard Biener <rguenther@suse.de> wrote:
> 
> On Thu, 30 Sep 2021, Jason Merrill wrote:
> 
>> On 9/29/21 17:30, Qing Zhao wrote:
>>> Hi,
>>> 
>>> PR102359 (ICE gimplification failed since  r12-3433-ga25e0b5e6ac8a77a)
>>> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=102359
>>> 
>>> Is due to -ftrivial-auto-var-init adding initialization for READONLY
>>> variable “this” in the following routine: (t.cpp.005t.original)
>>> 
>>> =======
>>> 
>>> ;; Function A::foo()::<lambda()> (null)
>>> ;; enabled by -tree-original
>>> 
>>> {
>>>   const struct A * const this [value-expr: &__closure->__this];
>>>     const struct A * const this [value-expr: &__closure->__this];
>>>   return <retval> = (double) ((const struct A *) this)->a;
>>> }
>>> =======
>>> 
>>> However, in the above routine, “this” is NOT marked as READONLY, but its
>>> value-expr "&__closure->__this” is marked as READONLY.
>>> 
>>> There are two major issues:
>>> 
>>> 1. In the routine “is_var_need_auto_init”, we should exclude “decl” that is
>>> marked as READONLY;
>>> 2. In the C++ FE, “this” should be marked as READONLY.
>>> 
>>> The idea solution will be:
>>> 
>>> 1. Fix “is_var_need_auto_init” to exclude TREE_READONLY (decl);
>>> 2. Fix C++ FE to mark “this” as TREE_READONLY (decl)==true;
>>> 
>>> Not sure whether it’s hard for C++ FE to fix the 2nd issue or not?
>>> 
>>> In the case it’s not a quick fix in C++FE, I proposed the following fix in
>>> middle end:
>>> 
>>> Let me know your comments or suggestions on this.
>>> 
>>> Thanks a lot for the help.
>> 
>> I'd think is_var_need_auto_init should be false for any variable with
>> DECL_HAS_VALUE_EXPR_P, as they aren't really variables, just ways of naming
>> objects that are initialized elsewhere.
> 
> IIRC handing variables with DECL_HAS_VALUE_EXPR_P is necessary to
> auto-init VLAs,

Yes, that’s correct.  
i.e, when adding call to .DEFFERED_INIT to auto variables, DECL for a VLA already has a DECL_VALUE_EXPR.

> otherwise I tend to agree - would we handle those
> when we see a DECL_EXPR then?

You mean, for VLA DECL? YES, we added a call to .DEFFERED_INIT for VLA DECL right now.

Qing
> 
>> 
>>> Qing
>>> 
>>> ==============================
>>> From 0a5982cd61bc4610655d3df00ae8d2fbcb3c8e9b Mon Sep 17 00:00:00 2001
>>> From: Qing Zhao <qing.zhao@oracle.com>
>>> Date: Wed, 29 Sep 2021 20:49:59 +0000
>>> Subject: [PATCH] Fix PR102359
>>> 
>>> ---
>>>  gcc/gimplify.c                  | 15 +++++++++++++++
>>>  gcc/testsuite/g++.dg/pr102359.C | 13 +++++++++++++
>>>  2 files changed, 28 insertions(+)
>>>  create mode 100644 gcc/testsuite/g++.dg/pr102359.C
>>> 
>>> diff --git a/gcc/gimplify.c b/gcc/gimplify.c
>>> index 1067113b1639..a2587869b35d 100644
>>> --- a/gcc/gimplify.c
>>> +++ b/gcc/gimplify.c
>>> @@ -1819,12 +1819,27 @@ gimple_add_padding_init_for_auto_var (tree decl,
>>> bool is_vla,
>>>    gimplify_seq_add_stmt (seq_p, call);
>>>  }
>>> 
>>> +/* Return true if the DECL is READONLY.
>>> +   This is to workaround a C++ FE bug that only mark the value_expr of
>>> "this"
>>> +   as readonly but does not mark "this" as readonly.
>>> +   C++ FE should fix this issue before replacing this routine with
>>> +   TREE_READONLY (decl).  */
>>> +
>>> +static bool
>>> +is_decl_readonly (tree decl)
>>> +{
>>> +  return (TREE_READONLY (decl)
>>> +	   || (DECL_HAS_VALUE_EXPR_P (decl)
>>> +		 && TREE_READONLY (DECL_VALUE_EXPR (decl))));
>>> +}
>>> +
>>>  /* Return true if the DECL need to be automaticly initialized by the
>>>     compiler.  */
>>>  static bool
>>>  is_var_need_auto_init (tree decl)
>>>  {
>>>    if (auto_var_p (decl)
>>> +      && !is_decl_readonly (decl)
>>>        && (flag_auto_var_init > AUTO_INIT_UNINITIALIZED)
>>>        && (!lookup_attribute ("uninitialized", DECL_ATTRIBUTES (decl)))
>>>        && !is_empty_type (TREE_TYPE (decl)))
>>> diff --git a/gcc/testsuite/g++.dg/pr102359.C
>>> b/gcc/testsuite/g++.dg/pr102359.C
>>> new file mode 100644
>>> index 000000000000..da643cde7bed
>>> --- /dev/null
>>> +++ b/gcc/testsuite/g++.dg/pr102359.C
>>> @@ -0,0 +1,13 @@
>>> +/* PR middle-end/102359 ICE gimplification failed since
>>> +   r12-3433-ga25e0b5e6ac8a77a.  */
>>> +/* { dg-do compile } */
>>> +/* { dg-options "-ftrivial-auto-var-init=zero" } */
>>> +/* { dg-require-effective-target c++17 } */
>>> +
>>> +struct A {
>>> +  double a = 111;
>>> +  auto foo() {
>>> +    return [*this] { return a; };
>>> +  }
>>> +};
>>> +int X = A{}.foo()();
>>> 
>> 
>> 
> 
> -- 
> Richard Biener <rguenther@suse.de>
> SUSE Software Solutions Germany GmbH, Maxfeldstrasse 5, 90409 Nuernberg,
> Germany; GF: Felix Imendörffer; HRB 36809 (AG Nuernberg)

Re: [RFC][Patch][middle-end/PR102359]Not add initialization for READONLY variables with -ftrivial-auto-var-init

[Qing Zhao]

> On Sep 30, 2021, at 1:54 AM, Richard Biener <rguenther@suse.de> wrote:
> 
> On Thu, 30 Sep 2021, Jason Merrill wrote:
> 
>> On 9/29/21 17:30, Qing Zhao wrote:
>>> Hi,
>>> 
>>> PR102359 (ICE gimplification failed since  r12-3433-ga25e0b5e6ac8a77a)
>>> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=102359
>>> 
>>> Is due to -ftrivial-auto-var-init adding initialization for READONLY
>>> variable “this” in the following routine: (t.cpp.005t.original)
>>> 
>>> =======
>>> 
>>> ;; Function A::foo()::<lambda()> (null)
>>> ;; enabled by -tree-original
>>> 
>>> {
>>>  const struct A * const this [value-expr: &__closure->__this];
>>>    const struct A * const this [value-expr: &__closure->__this];
>>>  return <retval> = (double) ((const struct A *) this)->a;
>>> }
>>> =======
>>> 
>>> However, in the above routine, “this” is NOT marked as READONLY, but its
>>> value-expr "&__closure->__this” is marked as READONLY.
>>> 
>>> There are two major issues:
>>> 
>>> 1. In the routine “is_var_need_auto_init”, we should exclude “decl” that is
>>> marked as READONLY;
>>> 2. In the C++ FE, “this” should be marked as READONLY.
>>> 
>>> The idea solution will be:
>>> 
>>> 1. Fix “is_var_need_auto_init” to exclude TREE_READONLY (decl);
>>> 2. Fix C++ FE to mark “this” as TREE_READONLY (decl)==true;
>>> 
>>> Not sure whether it’s hard for C++ FE to fix the 2nd issue or not?
>>> 
>>> In the case it’s not a quick fix in C++FE, I proposed the following fix in
>>> middle end:
>>> 
>>> Let me know your comments or suggestions on this.
>>> 
>>> Thanks a lot for the help.
>> 
>> I'd think is_var_need_auto_init should be false for any variable with
>> DECL_HAS_VALUE_EXPR_P, as they aren't really variables, just ways of naming
>> objects that are initialized elsewhere.
> 
> IIRC handing variables with DECL_HAS_VALUE_EXPR_P is necessary to
> auto-init VLAs, otherwise I tend to agree - would we handle those
> when we see a DECL_EXPR then?

The current implementation is:


gimplify_decl_expr:


For each DECL_EXPR “decl”

   If (VAR_P (decl) && !DECL_EXTERNAL (decl))
     {
	if (is_vla (decl))
          gimplify_vla_decl (decl, …);      /* existing handling: create a VALUE_EXPR for this vla decl*/

	…
	if (has_explicit_init (decl))
	  {
	   …;     /* existing handling.  */
	  }
	else if (is_var_need_auto_init (decl))      /*. New code. */
	  {
	    gimple_add_init_for_auto_var (….);   /*  new code.  */
	    ...
	  }
     }


Since the “DECL_VALUE_EXPR (decl)” is NOT a DECL_EXPR, it will not be scanned and added initialization. 

if we do not add initialization for a decl that has DECL_VALUE_EXPR, then the “DECL_VALUE_EXPR (decl)” will not be added an initialization either.  We will miss adding initializations for these decls. 

So, I think that the current implementation is correct. 

And if C++ FE will not mark “this” as READONLY, only mark DECL_VALUE_EXPR(this) as READONLY, the proposed fix is correct too.

Let me know your opinion on this.

Thanks.

Qing

> 
>> 
>>> Qing
>>> 
>>> ==============================
>>> From 0a5982cd61bc4610655d3df00ae8d2fbcb3c8e9b Mon Sep 17 00:00:00 2001
>>> From: Qing Zhao <qing.zhao@oracle.com>
>>> Date: Wed, 29 Sep 2021 20:49:59 +0000
>>> Subject: [PATCH] Fix PR102359
>>> 
>>> ---
>>> gcc/gimplify.c                  | 15 +++++++++++++++
>>> gcc/testsuite/g++.dg/pr102359.C | 13 +++++++++++++
>>> 2 files changed, 28 insertions(+)
>>> create mode 100644 gcc/testsuite/g++.dg/pr102359.C
>>> 
>>> diff --git a/gcc/gimplify.c b/gcc/gimplify.c
>>> index 1067113b1639..a2587869b35d 100644
>>> --- a/gcc/gimplify.c
>>> +++ b/gcc/gimplify.c
>>> @@ -1819,12 +1819,27 @@ gimple_add_padding_init_for_auto_var (tree decl,
>>> bool is_vla,
>>>   gimplify_seq_add_stmt (seq_p, call);
>>> }
>>> 
>>> +/* Return true if the DECL is READONLY.
>>> +   This is to workaround a C++ FE bug that only mark the value_expr of
>>> "this"
>>> +   as readonly but does not mark "this" as readonly.
>>> +   C++ FE should fix this issue before replacing this routine with
>>> +   TREE_READONLY (decl).  */
>>> +
>>> +static bool
>>> +is_decl_readonly (tree decl)
>>> +{
>>> +  return (TREE_READONLY (decl)
>>> +	   || (DECL_HAS_VALUE_EXPR_P (decl)
>>> +		 && TREE_READONLY (DECL_VALUE_EXPR (decl))));
>>> +}
>>> +
>>> /* Return true if the DECL need to be automaticly initialized by the
>>>    compiler.  */
>>> static bool
>>> is_var_need_auto_init (tree decl)
>>> {
>>>   if (auto_var_p (decl)
>>> +      && !is_decl_readonly (decl)
>>>       && (flag_auto_var_init > AUTO_INIT_UNINITIALIZED)
>>>       && (!lookup_attribute ("uninitialized", DECL_ATTRIBUTES (decl)))
>>>       && !is_empty_type (TREE_TYPE (decl)))
>>> diff --git a/gcc/testsuite/g++.dg/pr102359.C
>>> b/gcc/testsuite/g++.dg/pr102359.C
>>> new file mode 100644
>>> index 000000000000..da643cde7bed
>>> --- /dev/null
>>> +++ b/gcc/testsuite/g++.dg/pr102359.C
>>> @@ -0,0 +1,13 @@
>>> +/* PR middle-end/102359 ICE gimplification failed since
>>> +   r12-3433-ga25e0b5e6ac8a77a.  */
>>> +/* { dg-do compile } */
>>> +/* { dg-options "-ftrivial-auto-var-init=zero" } */
>>> +/* { dg-require-effective-target c++17 } */
>>> +
>>> +struct A {
>>> +  double a = 111;
>>> +  auto foo() {
>>> +    return [*this] { return a; };
>>> +  }
>>> +};
>>> +int X = A{}.foo()();
>>> 
>> 
>> 
> 
> -- 
> Richard Biener <rguenther@suse.de>
> SUSE Software Solutions Germany GmbH, Maxfeldstrasse 5, 90409 Nuernberg,
> Germany; GF: Felix Imendörffer; HRB 36809 (AG Nuernberg)

Re: [RFC][Patch][middle-end/PR102359]Not add initialization for READONLY variables with -ftrivial-auto-var-init

[Qing Zhao]

On Sep 30, 2021, at 1:54 AM, Richard Biener <rguenther@suse.de> wrote:

On Thu, 30 Sep 2021, Jason Merrill wrote:

On 9/29/21 17:30, Qing Zhao wrote:
Hi,

PR102359 (ICE gimplification failed since  r12-3433-ga25e0b5e6ac8a77a)
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=102359

Is due to -ftrivial-auto-var-init adding initialization for READONLY
variable “this” in the following routine: (t.cpp.005t.original)

=======

;; Function A::foo()::<lambda()> (null)
;; enabled by -tree-original

{
 const struct A * const this [value-expr: &__closure->__this];
   const struct A * const this [value-expr: &__closure->__this];
 return <retval> = (double) ((const struct A *) this)->a;
}
=======

However, in the above routine, “this” is NOT marked as READONLY, but its
value-expr "&__closure->__this” is marked as READONLY.

There are two major issues:

1. In the routine “is_var_need_auto_init”, we should exclude “decl” that is
marked as READONLY;
2. In the C++ FE, “this” should be marked as READONLY.

The idea solution will be:

1. Fix “is_var_need_auto_init” to exclude TREE_READONLY (decl);
2. Fix C++ FE to mark “this” as TREE_READONLY (decl)==true;

Not sure whether it’s hard for C++ FE to fix the 2nd issue or not?

In the case it’s not a quick fix in C++FE, I proposed the following fix in
middle end:

Let me know your comments or suggestions on this.

Thanks a lot for the help.

I'd think is_var_need_auto_init should be false for any variable with
DECL_HAS_VALUE_EXPR_P, as they aren't really variables, just ways of naming
objects that are initialized elsewhere.

IIRC handing variables with DECL_HAS_VALUE_EXPR_P is necessary to
auto-init VLAs, otherwise I tend to agree - would we handle those
when we see a DECL_EXPR then?

The current implementation is:


gimplify_decl_expr:


For each DECL_EXPR “decl”

   If (VAR_P (decl) && !DECL_EXTERNAL (decl))
     {
if (is_vla (decl))
          gimplify_vla_decl (decl, …);      /* existing handling: create a VALUE_EXPR for this vla decl*/

…
if (has_explicit_init (decl))
 {
  …;     /* existing handling.  */
 }
else if (is_var_need_auto_init (decl))      /*. New code. */
 {
   gimple_add_init_for_auto_var (….);   /*  new code.  */
   ...
 }
     }


Since the “DECL_VALUE_EXPR (decl)” is NOT a DECL_EXPR, it will not be scanned and added initialization.

if we do not add initialization for a decl that has DECL_VALUE_EXPR, then the “DECL_VALUE_EXPR (decl)” will not be added an initialization either.  We will miss adding initializations for these decls.

So, I think that the current implementation is correct.

And if C++ FE will not mark “this” as READONLY, only mark DECL_VALUE_EXPR(this) as READONLY, the proposed fix is correct too.

Let me know your opinion on this.

Thanks.

Qing



Qing

==============================
From 0a5982cd61bc4610655d3df00ae8d2fbcb3c8e9b Mon Sep 17 00:00:00 2001
From: Qing Zhao <qing.zhao@oracle.com>
Date: Wed, 29 Sep 2021 20:49:59 +0000
Subject: [PATCH] Fix PR102359

---
gcc/gimplify.c                  | 15 +++++++++++++++
gcc/testsuite/g++.dg/pr102359.C | 13 +++++++++++++
2 files changed, 28 insertions(+)
create mode 100644 gcc/testsuite/g++.dg/pr102359.C

diff --git a/gcc/gimplify.c b/gcc/gimplify.c
index 1067113b1639..a2587869b35d 100644
--- a/gcc/gimplify.c
+++ b/gcc/gimplify.c
@@ -1819,12 +1819,27 @@ gimple_add_padding_init_for_auto_var (tree decl,
bool is_vla,
  gimplify_seq_add_stmt (seq_p, call);
}

+/* Return true if the DECL is READONLY.
+   This is to workaround a C++ FE bug that only mark the value_expr of
"this"
+   as readonly but does not mark "this" as readonly.
+   C++ FE should fix this issue before replacing this routine with
+   TREE_READONLY (decl).  */
+
+static bool
+is_decl_readonly (tree decl)
+{
+  return (TREE_READONLY (decl)
+   || (DECL_HAS_VALUE_EXPR_P (decl)
+ && TREE_READONLY (DECL_VALUE_EXPR (decl))));
+}
+
/* Return true if the DECL need to be automaticly initialized by the
   compiler.  */
static bool
is_var_need_auto_init (tree decl)
{
  if (auto_var_p (decl)
+      && !is_decl_readonly (decl)
      && (flag_auto_var_init > AUTO_INIT_UNINITIALIZED)
      && (!lookup_attribute ("uninitialized", DECL_ATTRIBUTES (decl)))
      && !is_empty_type (TREE_TYPE (decl)))
diff --git a/gcc/testsuite/g++.dg/pr102359.C
b/gcc/testsuite/g++.dg/pr102359.C
new file mode 100644
index 000000000000..da643cde7bed
--- /dev/null
+++ b/gcc/testsuite/g++.dg/pr102359.C
@@ -0,0 +1,13 @@
+/* PR middle-end/102359 ICE gimplification failed since
+   r12-3433-ga25e0b5e6ac8a77a.  */
+/* { dg-do compile } */
+/* { dg-options "-ftrivial-auto-var-init=zero" } */
+/* { dg-require-effective-target c++17 } */
+
+struct A {
+  double a = 111;
+  auto foo() {
+    return [*this] { return a; };
+  }
+};
+int X = A{}.foo()();




--
Richard Biener <rguenther@suse.de>
SUSE Software Solutions Germany GmbH, Maxfeldstrasse 5, 90409 Nuernberg,
Germany; GF: Felix Imendörffer; HRB 36809 (AG Nuernberg)

Re: [RFC][Patch][middle-end/PR102359]Not add initialization for READONLY variables with -ftrivial-auto-var-init

[Jason Merrill]

On 9/30/21 11:42, Qing Zhao wrote:
> 
> 
>> On Sep 30, 2021, at 1:54 AM, Richard Biener <rguenther@suse.de> wrote:
>>
>> On Thu, 30 Sep 2021, Jason Merrill wrote:
>>
>>> On 9/29/21 17:30, Qing Zhao wrote:
>>>> Hi,
>>>>
>>>> PR102359 (ICE gimplification failed since  r12-3433-ga25e0b5e6ac8a77a)
>>>> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=102359
>>>>
>>>> Is due to -ftrivial-auto-var-init adding initialization for READONLY
>>>> variable “this” in the following routine: (t.cpp.005t.original)
>>>>
>>>> =======
>>>>
>>>> ;; Function A::foo()::<lambda()> (null)
>>>> ;; enabled by -tree-original
>>>>
>>>> {
>>>>   const struct A * const this [value-expr: &__closure->__this];
>>>>     const struct A * const this [value-expr: &__closure->__this];
>>>>   return <retval> = (double) ((const struct A *) this)->a;
>>>> }
>>>> =======
>>>>
>>>> However, in the above routine, “this” is NOT marked as READONLY, but its
>>>> value-expr "&__closure->__this” is marked as READONLY.
>>>>
>>>> There are two major issues:
>>>>
>>>> 1. In the routine “is_var_need_auto_init”, we should exclude “decl” that is
>>>> marked as READONLY;
>>>> 2. In the C++ FE, “this” should be marked as READONLY.
>>>>
>>>> The idea solution will be:
>>>>
>>>> 1. Fix “is_var_need_auto_init” to exclude TREE_READONLY (decl);
>>>> 2. Fix C++ FE to mark “this” as TREE_READONLY (decl)==true;
>>>>
>>>> Not sure whether it’s hard for C++ FE to fix the 2nd issue or not?
>>>>
>>>> In the case it’s not a quick fix in C++FE, I proposed the following fix in
>>>> middle end:
>>>>
>>>> Let me know your comments or suggestions on this.
>>>>
>>>> Thanks a lot for the help.
>>>
>>> I'd think is_var_need_auto_init should be false for any variable with
>>> DECL_HAS_VALUE_EXPR_P, as they aren't really variables, just ways of naming
>>> objects that are initialized elsewhere.
>>
>> IIRC handing variables with DECL_HAS_VALUE_EXPR_P is necessary to
>> auto-init VLAs, otherwise I tend to agree - would we handle those
>> when we see a DECL_EXPR then?
> 
> The current implementation is:
> 
> 
> gimplify_decl_expr:
> 
> 
> For each DECL_EXPR “decl”
> 
>     If (VAR_P (decl) && !DECL_EXTERNAL (decl))
>       {
> 	if (is_vla (decl))
>            gimplify_vla_decl (decl, …);      /* existing handling: create a VALUE_EXPR for this vla decl*/
> 
> 	…
> 	if (has_explicit_init (decl))
> 	  {
> 	   …;     /* existing handling.  */
> 	  }
> 	else if (is_var_need_auto_init (decl))      /*. New code. */
> 	  {
> 	    gimple_add_init_for_auto_var (….);   /*  new code.  */
> 	    ...
> 	  }
>       }
> 
> 
> Since the “DECL_VALUE_EXPR (decl)” is NOT a DECL_EXPR, it will not be scanned and added initialization.
> 
> if we do not add initialization for a decl that has DECL_VALUE_EXPR, then the “DECL_VALUE_EXPR (decl)” will not be added an initialization either.  We will miss adding initializations for these decls.
> 
> So, I think that the current implementation is correct.
> 
> And if C++ FE will not mark “this” as READONLY, only mark DECL_VALUE_EXPR(this) as READONLY, the proposed fix is correct too.
> 
> Let me know your opinion on this.

The problem with this test is not whether the 'this' proxy is marked 
READONLY, the problem is that you're trying to initialize lambda capture 
proxies at all; the lambda capture objects were already initialized when 
forming the closure object.  So this test currently aborts with 
-ftrivial-auto-var-init=zero because you "initialize" the i capture 
field to 0 after it was previously initialized to 42:

int main()
{
   int i = 42;
   auto l = [=]() mutable { return i; };
   if (l() != i)
     __builtin_abort ();
}

I believe the same issue applies to the proxy variables in coroutines 
that work much like lambdas.

You can't just assume that a VAR_DECL with DECL_VALUE_EXPR is uninitialized.

Since there's already VLA handling in gimplify_decl_expr, you could 
remember whether you added DECL_VALUE_EXPR in that function, and only 
then do the initialization.

Jason

Re: [RFC][Patch][middle-end/PR102359]Not add initialization for READONLY variables with -ftrivial-auto-var-init

[Qing Zhao]

> On Sep 30, 2021, at 2:31 PM, Jason Merrill <jason@redhat.com> wrote:
> 
> On 9/30/21 11:42, Qing Zhao wrote:
>>> On Sep 30, 2021, at 1:54 AM, Richard Biener <rguenther@suse.de> wrote:
>>> 
>>> On Thu, 30 Sep 2021, Jason Merrill wrote:
>>> 
>>>> On 9/29/21 17:30, Qing Zhao wrote:
>>>>> Hi,
>>>>> 
>>>>> PR102359 (ICE gimplification failed since  r12-3433-ga25e0b5e6ac8a77a)
>>>>> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=102359
>>>>> 
>>>>> Is due to -ftrivial-auto-var-init adding initialization for READONLY
>>>>> variable “this” in the following routine: (t.cpp.005t.original)
>>>>> 
>>>>> =======
>>>>> 
>>>>> ;; Function A::foo()::<lambda()> (null)
>>>>> ;; enabled by -tree-original
>>>>> 
>>>>> {
>>>>>  const struct A * const this [value-expr: &__closure->__this];
>>>>>    const struct A * const this [value-expr: &__closure->__this];
>>>>>  return <retval> = (double) ((const struct A *) this)->a;
>>>>> }
>>>>> =======
>>>>> 
>>>>> However, in the above routine, “this” is NOT marked as READONLY, but its
>>>>> value-expr "&__closure->__this” is marked as READONLY.
>>>>> 
>>>>> There are two major issues:
>>>>> 
>>>>> 1. In the routine “is_var_need_auto_init”, we should exclude “decl” that is
>>>>> marked as READONLY;
>>>>> 2. In the C++ FE, “this” should be marked as READONLY.
>>>>> 
>>>>> The idea solution will be:
>>>>> 
>>>>> 1. Fix “is_var_need_auto_init” to exclude TREE_READONLY (decl);
>>>>> 2. Fix C++ FE to mark “this” as TREE_READONLY (decl)==true;
>>>>> 
>>>>> Not sure whether it’s hard for C++ FE to fix the 2nd issue or not?
>>>>> 
>>>>> In the case it’s not a quick fix in C++FE, I proposed the following fix in
>>>>> middle end:
>>>>> 
>>>>> Let me know your comments or suggestions on this.
>>>>> 
>>>>> Thanks a lot for the help.
>>>> 
>>>> I'd think is_var_need_auto_init should be false for any variable with
>>>> DECL_HAS_VALUE_EXPR_P, as they aren't really variables, just ways of naming
>>>> objects that are initialized elsewhere.
>>> 
>>> IIRC handing variables with DECL_HAS_VALUE_EXPR_P is necessary to
>>> auto-init VLAs, otherwise I tend to agree - would we handle those
>>> when we see a DECL_EXPR then?
>> The current implementation is:
>> gimplify_decl_expr:
>> For each DECL_EXPR “decl”
>>    If (VAR_P (decl) && !DECL_EXTERNAL (decl))
>>      {
>> 	if (is_vla (decl))
>>           gimplify_vla_decl (decl, …);      /* existing handling: create a VALUE_EXPR for this vla decl*/
>> 	…
>> 	if (has_explicit_init (decl))
>> 	  {
>> 	   …;     /* existing handling.  */
>> 	  }
>> 	else if (is_var_need_auto_init (decl))      /*. New code. */
>> 	  {
>> 	    gimple_add_init_for_auto_var (….);   /*  new code.  */
>> 	    ...
>> 	  }
>>      }
>> Since the “DECL_VALUE_EXPR (decl)” is NOT a DECL_EXPR, it will not be scanned and added initialization.
>> if we do not add initialization for a decl that has DECL_VALUE_EXPR, then the “DECL_VALUE_EXPR (decl)” will not be added an initialization either.  We will miss adding initializations for these decls.
>> So, I think that the current implementation is correct.
>> And if C++ FE will not mark “this” as READONLY, only mark DECL_VALUE_EXPR(this) as READONLY, the proposed fix is correct too.
>> Let me know your opinion on this.
> 
> The problem with this test is not whether the 'this' proxy is marked READONLY, the problem is that you're trying to initialize lambda capture proxies at all; the lambda capture objects were already initialized when forming the closure object.  So this test currently aborts with -ftrivial-auto-var-init=zero because you "initialize" the i capture field to 0 after it was previously initialized to 42:
> 
> int main()
> {
>  int i = 42;
>  auto l = [=]() mutable { return i; };
>  if (l() != i)
>    __builtin_abort ();
> }
> 
> I believe the same issue applies to the proxy variables in coroutines that work much like lambdas.
So, how should the middle end determine that a variable is “proxy variable”?
Have all “proxy variables” been initialized by C++ FE already?
> 
> You can't just assume that a VAR_DECL with DECL_VALUE_EXPR is uninitialized.

So, all the VAR_DECLs with DECL_VALUE_EXPR (except the ones created by “gimplify_decl_expr”) are initialized by FE already?

> 
> Since there's already VLA handling in gimplify_decl_expr, you could remember whether you added DECL_VALUE_EXPR in that function, and only then do the initialization.

Yes, if we can guarantee that all the VAR_DECLs with DECL_VALUE_EXPR created from FEs have been initialized already by FE, we can fix this issue as this way.

thanks.

Qing
> 
> Jason
> 

Re: [RFC][Patch][middle-end/PR102359]Not add initialization for READONLY variables with -ftrivial-auto-var-init

[Jason Merrill]

On 10/1/21 10:54, Qing Zhao wrote:
> 
> 
>> On Sep 30, 2021, at 2:31 PM, Jason Merrill <jason@redhat.com> wrote:
>>
>> On 9/30/21 11:42, Qing Zhao wrote:
>>>> On Sep 30, 2021, at 1:54 AM, Richard Biener <rguenther@suse.de> wrote:
>>>>
>>>> On Thu, 30 Sep 2021, Jason Merrill wrote:
>>>>
>>>>> On 9/29/21 17:30, Qing Zhao wrote:
>>>>>> Hi,
>>>>>>
>>>>>> PR102359 (ICE gimplification failed since  r12-3433-ga25e0b5e6ac8a77a)
>>>>>> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=102359
>>>>>>
>>>>>> Is due to -ftrivial-auto-var-init adding initialization for READONLY
>>>>>> variable “this” in the following routine: (t.cpp.005t.original)
>>>>>>
>>>>>> =======
>>>>>>
>>>>>> ;; Function A::foo()::<lambda()> (null)
>>>>>> ;; enabled by -tree-original
>>>>>>
>>>>>> {
>>>>>>   const struct A * const this [value-expr: &__closure->__this];
>>>>>>     const struct A * const this [value-expr: &__closure->__this];
>>>>>>   return <retval> = (double) ((const struct A *) this)->a;
>>>>>> }
>>>>>> =======
>>>>>>
>>>>>> However, in the above routine, “this” is NOT marked as READONLY, but its
>>>>>> value-expr "&__closure->__this” is marked as READONLY.
>>>>>>
>>>>>> There are two major issues:
>>>>>>
>>>>>> 1. In the routine “is_var_need_auto_init”, we should exclude “decl” that is
>>>>>> marked as READONLY;
>>>>>> 2. In the C++ FE, “this” should be marked as READONLY.
>>>>>>
>>>>>> The idea solution will be:
>>>>>>
>>>>>> 1. Fix “is_var_need_auto_init” to exclude TREE_READONLY (decl);
>>>>>> 2. Fix C++ FE to mark “this” as TREE_READONLY (decl)==true;
>>>>>>
>>>>>> Not sure whether it’s hard for C++ FE to fix the 2nd issue or not?
>>>>>>
>>>>>> In the case it’s not a quick fix in C++FE, I proposed the following fix in
>>>>>> middle end:
>>>>>>
>>>>>> Let me know your comments or suggestions on this.
>>>>>>
>>>>>> Thanks a lot for the help.
>>>>>
>>>>> I'd think is_var_need_auto_init should be false for any variable with
>>>>> DECL_HAS_VALUE_EXPR_P, as they aren't really variables, just ways of naming
>>>>> objects that are initialized elsewhere.
>>>>
>>>> IIRC handing variables with DECL_HAS_VALUE_EXPR_P is necessary to
>>>> auto-init VLAs, otherwise I tend to agree - would we handle those
>>>> when we see a DECL_EXPR then?
>>> The current implementation is:
>>> gimplify_decl_expr:
>>> For each DECL_EXPR “decl”
>>>     If (VAR_P (decl) && !DECL_EXTERNAL (decl))
>>>       {
>>> 	if (is_vla (decl))
>>>            gimplify_vla_decl (decl, …);      /* existing handling: create a VALUE_EXPR for this vla decl*/
>>> 	…
>>> 	if (has_explicit_init (decl))
>>> 	  {
>>> 	   …;     /* existing handling.  */
>>> 	  }
>>> 	else if (is_var_need_auto_init (decl))      /*. New code. */
>>> 	  {
>>> 	    gimple_add_init_for_auto_var (….);   /*  new code.  */
>>> 	    ...
>>> 	  }
>>>       }
>>> Since the “DECL_VALUE_EXPR (decl)” is NOT a DECL_EXPR, it will not be scanned and added initialization.
>>> if we do not add initialization for a decl that has DECL_VALUE_EXPR, then the “DECL_VALUE_EXPR (decl)” will not be added an initialization either.  We will miss adding initializations for these decls.
>>> So, I think that the current implementation is correct.
>>> And if C++ FE will not mark “this” as READONLY, only mark DECL_VALUE_EXPR(this) as READONLY, the proposed fix is correct too.
>>> Let me know your opinion on this.
>>
>> The problem with this test is not whether the 'this' proxy is marked READONLY, the problem is that you're trying to initialize lambda capture proxies at all; the lambda capture objects were already initialized when forming the closure object.  So this test currently aborts with -ftrivial-auto-var-init=zero because you "initialize" the i capture field to 0 after it was previously initialized to 42:
>>
>> int main()
>> {
>>   int i = 42;
>>   auto l = [=]() mutable { return i; };
>>   if (l() != i)
>>     __builtin_abort ();
>> }
>>
>> I believe the same issue applies to the proxy variables in coroutines that work much like lambdas.

> So, how should the middle end determine that a variable is “proxy variable”?

In the front end, is_capture_proxy will identify a lambda capture proxy 
variable.  But that won't be true for the similar proxies used by 
coroutines.

> Have all “proxy variables” been initialized by C++ FE already?

Yes.

>> You can't just assume that a VAR_DECL with DECL_VALUE_EXPR is uninitialized.
> 
> So, all the VAR_DECLs with DECL_VALUE_EXPR (except the ones created by “gimplify_decl_expr”) are initialized by FE already?

In general I'd expect them to refer to previously created objects which 
may or may not have been initialized, but if they haven't been, the 
place to deal with that is at their previous creation.

>> Since there's already VLA handling in gimplify_decl_expr, you could remember whether you added DECL_VALUE_EXPR in that function, and only then do the initialization.
> 
> Yes, if we can guarantee that all the VAR_DECLs with DECL_VALUE_EXPR created from FEs have been initialized already by FE, we can fix this issue as this way.

Or more generally, check whether the argument to gimplify_decl_expr has 
DECL_VALUE_EXPR when we enter the function, and don't do the 
initialization in that case.

Jason

Re: [RFC][Patch][middle-end/PR102359]Not add initialization for READONLY variables with -ftrivial-auto-var-init

[Qing Zhao]

> On Oct 1, 2021, at 10:33 AM, Jason Merrill <jason@redhat.com> wrote:
> 
> On 10/1/21 10:54, Qing Zhao wrote:
>>> On Sep 30, 2021, at 2:31 PM, Jason Merrill <jason@redhat.com> wrote:
>>> 
>>> On 9/30/21 11:42, Qing Zhao wrote:
>>>>> On Sep 30, 2021, at 1:54 AM, Richard Biener <rguenther@suse.de> wrote:
>>>>> 
>>>>> On Thu, 30 Sep 2021, Jason Merrill wrote:
>>>>> 
>>>>>> On 9/29/21 17:30, Qing Zhao wrote:
>>>>>>> Hi,
>>>>>>> 
>>>>>>> PR102359 (ICE gimplification failed since  r12-3433-ga25e0b5e6ac8a77a)
>>>>>>> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=102359
>>>>>>> 
>>>>>>> Is due to -ftrivial-auto-var-init adding initialization for READONLY
>>>>>>> variable “this” in the following routine: (t.cpp.005t.original)
>>>>>>> 
>>>>>>> =======
>>>>>>> 
>>>>>>> ;; Function A::foo()::<lambda()> (null)
>>>>>>> ;; enabled by -tree-original
>>>>>>> 
>>>>>>> {
>>>>>>>  const struct A * const this [value-expr: &__closure->__this];
>>>>>>>    const struct A * const this [value-expr: &__closure->__this];
>>>>>>>  return <retval> = (double) ((const struct A *) this)->a;
>>>>>>> }
>>>>>>> =======
>>>>>>> 
>>>>>>> However, in the above routine, “this” is NOT marked as READONLY, but its
>>>>>>> value-expr "&__closure->__this” is marked as READONLY.
>>>>>>> 
>>>>>>> There are two major issues:
>>>>>>> 
>>>>>>> 1. In the routine “is_var_need_auto_init”, we should exclude “decl” that is
>>>>>>> marked as READONLY;
>>>>>>> 2. In the C++ FE, “this” should be marked as READONLY.
>>>>>>> 
>>>>>>> The idea solution will be:
>>>>>>> 
>>>>>>> 1. Fix “is_var_need_auto_init” to exclude TREE_READONLY (decl);
>>>>>>> 2. Fix C++ FE to mark “this” as TREE_READONLY (decl)==true;
>>>>>>> 
>>>>>>> Not sure whether it’s hard for C++ FE to fix the 2nd issue or not?
>>>>>>> 
>>>>>>> In the case it’s not a quick fix in C++FE, I proposed the following fix in
>>>>>>> middle end:
>>>>>>> 
>>>>>>> Let me know your comments or suggestions on this.
>>>>>>> 
>>>>>>> Thanks a lot for the help.
>>>>>> 
>>>>>> I'd think is_var_need_auto_init should be false for any variable with
>>>>>> DECL_HAS_VALUE_EXPR_P, as they aren't really variables, just ways of naming
>>>>>> objects that are initialized elsewhere.
>>>>> 
>>>>> IIRC handing variables with DECL_HAS_VALUE_EXPR_P is necessary to
>>>>> auto-init VLAs, otherwise I tend to agree - would we handle those
>>>>> when we see a DECL_EXPR then?
>>>> The current implementation is:
>>>> gimplify_decl_expr:
>>>> For each DECL_EXPR “decl”
>>>>    If (VAR_P (decl) && !DECL_EXTERNAL (decl))
>>>>      {
>>>> 	if (is_vla (decl))
>>>>           gimplify_vla_decl (decl, …);      /* existing handling: create a VALUE_EXPR for this vla decl*/
>>>> 	…
>>>> 	if (has_explicit_init (decl))
>>>> 	  {
>>>> 	   …;     /* existing handling.  */
>>>> 	  }
>>>> 	else if (is_var_need_auto_init (decl))      /*. New code. */
>>>> 	  {
>>>> 	    gimple_add_init_for_auto_var (….);   /*  new code.  */
>>>> 	    ...
>>>> 	  }
>>>>      }
>>>> Since the “DECL_VALUE_EXPR (decl)” is NOT a DECL_EXPR, it will not be scanned and added initialization.
>>>> if we do not add initialization for a decl that has DECL_VALUE_EXPR, then the “DECL_VALUE_EXPR (decl)” will not be added an initialization either.  We will miss adding initializations for these decls.
>>>> So, I think that the current implementation is correct.
>>>> And if C++ FE will not mark “this” as READONLY, only mark DECL_VALUE_EXPR(this) as READONLY, the proposed fix is correct too.
>>>> Let me know your opinion on this.
>>> 
>>> The problem with this test is not whether the 'this' proxy is marked READONLY, the problem is that you're trying to initialize lambda capture proxies at all; the lambda capture objects were already initialized when forming the closure object.  So this test currently aborts with -ftrivial-auto-var-init=zero because you "initialize" the i capture field to 0 after it was previously initialized to 42:
>>> 
>>> int main()
>>> {
>>>  int i = 42;
>>>  auto l = [=]() mutable { return i; };
>>>  if (l() != i)
>>>    __builtin_abort ();
>>> }
>>> 
>>> I believe the same issue applies to the proxy variables in coroutines that work much like lambdas.
> 
>> So, how should the middle end determine that a variable is “proxy variable”?
> 
> In the front end, is_capture_proxy will identify a lambda capture proxy variable.  But that won't be true for the similar proxies used by coroutines.

Does this mean that in middle end, especially in gimplification phase, there is Not a simple way to determine whether a variable is a proxy variable?
> 
>> Have all “proxy variables” been initialized by C++ FE already?
> 
> Yes.
> 
>>> You can't just assume that a VAR_DECL with DECL_VALUE_EXPR is uninitialized.
>> So, all the VAR_DECLs with DECL_VALUE_EXPR (except the ones created by “gimplify_decl_expr”) are initialized by FE already?
> 
> In general I'd expect them to refer to previously created objects which may or may not have been initialized, but if they haven't been, the place to deal with that is at their previous creation.

Still a little confuse..., do you mean, even for VAL_DECLS with DECL_VALUE_EXPR that were created by FE, we cannot guarantee they have been initialized? 

What did you mean by “the place to deal with that is at there previous creation”?


> 
>>> Since there's already VLA handling in gimplify_decl_expr, you could remember whether you added DECL_VALUE_EXPR in that function, and only then do the initialization.
>> Yes, if we can guarantee that all the VAR_DECLs with DECL_VALUE_EXPR created from FEs have been initialized already by FE, we can fix this issue as this way.
> 
> Or more generally, check whether the argument to gimplify_decl_expr has DECL_VALUE_EXPR when we enter the function, and don't do the initialization in that case.

Yes, we can do that.

However, the major thing I need to make sure is: 

can we guarantee that for All the VAL_DECLS with DECL_VALUE_EXPR created by FE are initialized already?

thanks.

Qing
> 
> Jason

Re: [RFC][Patch][middle-end/PR102359]Not add initialization for READONLY variables with -ftrivial-auto-var-init

[Richard Biener]

On Fri, 1 Oct 2021, Qing Zhao wrote:

> 
> 
> > On Oct 1, 2021, at 10:33 AM, Jason Merrill <jason@redhat.com> wrote:
> > 
> > On 10/1/21 10:54, Qing Zhao wrote:
> >>> On Sep 30, 2021, at 2:31 PM, Jason Merrill <jason@redhat.com> wrote:
> >>> 
> >>> On 9/30/21 11:42, Qing Zhao wrote:
> >>>>> On Sep 30, 2021, at 1:54 AM, Richard Biener <rguenther@suse.de> wrote:
> >>>>> 
> >>>>> On Thu, 30 Sep 2021, Jason Merrill wrote:
> >>>>> 
> >>>>>> On 9/29/21 17:30, Qing Zhao wrote:
> >>>>>>> Hi,
> >>>>>>> 
> >>>>>>> PR102359 (ICE gimplification failed since  r12-3433-ga25e0b5e6ac8a77a)
> >>>>>>> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=102359
> >>>>>>> 
> >>>>>>> Is due to -ftrivial-auto-var-init adding initialization for READONLY
> >>>>>>> variable “this” in the following routine: (t.cpp.005t.original)
> >>>>>>> 
> >>>>>>> =======
> >>>>>>> 
> >>>>>>> ;; Function A::foo()::<lambda()> (null)
> >>>>>>> ;; enabled by -tree-original
> >>>>>>> 
> >>>>>>> {
> >>>>>>>  const struct A * const this [value-expr: &__closure->__this];
> >>>>>>>    const struct A * const this [value-expr: &__closure->__this];
> >>>>>>>  return <retval> = (double) ((const struct A *) this)->a;
> >>>>>>> }
> >>>>>>> =======
> >>>>>>> 
> >>>>>>> However, in the above routine, “this” is NOT marked as READONLY, but its
> >>>>>>> value-expr "&__closure->__this” is marked as READONLY.
> >>>>>>> 
> >>>>>>> There are two major issues:
> >>>>>>> 
> >>>>>>> 1. In the routine “is_var_need_auto_init”, we should exclude “decl” that is
> >>>>>>> marked as READONLY;
> >>>>>>> 2. In the C++ FE, “this” should be marked as READONLY.
> >>>>>>> 
> >>>>>>> The idea solution will be:
> >>>>>>> 
> >>>>>>> 1. Fix “is_var_need_auto_init” to exclude TREE_READONLY (decl);
> >>>>>>> 2. Fix C++ FE to mark “this” as TREE_READONLY (decl)==true;
> >>>>>>> 
> >>>>>>> Not sure whether it’s hard for C++ FE to fix the 2nd issue or not?
> >>>>>>> 
> >>>>>>> In the case it’s not a quick fix in C++FE, I proposed the following fix in
> >>>>>>> middle end:
> >>>>>>> 
> >>>>>>> Let me know your comments or suggestions on this.
> >>>>>>> 
> >>>>>>> Thanks a lot for the help.
> >>>>>> 
> >>>>>> I'd think is_var_need_auto_init should be false for any variable with
> >>>>>> DECL_HAS_VALUE_EXPR_P, as they aren't really variables, just ways of naming
> >>>>>> objects that are initialized elsewhere.
> >>>>> 
> >>>>> IIRC handing variables with DECL_HAS_VALUE_EXPR_P is necessary to
> >>>>> auto-init VLAs, otherwise I tend to agree - would we handle those
> >>>>> when we see a DECL_EXPR then?
> >>>> The current implementation is:
> >>>> gimplify_decl_expr:
> >>>> For each DECL_EXPR “decl”
> >>>>    If (VAR_P (decl) && !DECL_EXTERNAL (decl))
> >>>>      {
> >>>> 	if (is_vla (decl))
> >>>>           gimplify_vla_decl (decl, …);      /* existing handling: create a VALUE_EXPR for this vla decl*/
> >>>> 	…
> >>>> 	if (has_explicit_init (decl))
> >>>> 	  {
> >>>> 	   …;     /* existing handling.  */
> >>>> 	  }
> >>>> 	else if (is_var_need_auto_init (decl))      /*. New code. */
> >>>> 	  {
> >>>> 	    gimple_add_init_for_auto_var (….);   /*  new code.  */
> >>>> 	    ...
> >>>> 	  }
> >>>>      }
> >>>> Since the “DECL_VALUE_EXPR (decl)” is NOT a DECL_EXPR, it will not be scanned and added initialization.
> >>>> if we do not add initialization for a decl that has DECL_VALUE_EXPR, then the “DECL_VALUE_EXPR (decl)” will not be added an initialization either.  We will miss adding initializations for these decls.
> >>>> So, I think that the current implementation is correct.
> >>>> And if C++ FE will not mark “this” as READONLY, only mark DECL_VALUE_EXPR(this) as READONLY, the proposed fix is correct too.
> >>>> Let me know your opinion on this.
> >>> 
> >>> The problem with this test is not whether the 'this' proxy is marked READONLY, the problem is that you're trying to initialize lambda capture proxies at all; the lambda capture objects were already initialized when forming the closure object.  So this test currently aborts with -ftrivial-auto-var-init=zero because you "initialize" the i capture field to 0 after it was previously initialized to 42:
> >>> 
> >>> int main()
> >>> {
> >>>  int i = 42;
> >>>  auto l = [=]() mutable { return i; };
> >>>  if (l() != i)
> >>>    __builtin_abort ();
> >>> }
> >>> 
> >>> I believe the same issue applies to the proxy variables in coroutines that work much like lambdas.
> > 
> >> So, how should the middle end determine that a variable is “proxy variable”?
> > 
> > In the front end, is_capture_proxy will identify a lambda capture proxy variable.  But that won't be true for the similar proxies used by coroutines.
> 
> Does this mean that in middle end, especially in gimplification phase, there is Not a simple way to determine whether a variable is a proxy variable?
> > 
> >> Have all “proxy variables” been initialized by C++ FE already?
> > 
> > Yes.
> > 
> >>> You can't just assume that a VAR_DECL with DECL_VALUE_EXPR is uninitialized.
> >> So, all the VAR_DECLs with DECL_VALUE_EXPR (except the ones created by “gimplify_decl_expr”) are initialized by FE already?
> > 
> > In general I'd expect them to refer to previously created objects which may or may not have been initialized, but if they haven't been, the place to deal with that is at their previous creation.
> 
> Still a little confuse..., do you mean, even for VAL_DECLS with DECL_VALUE_EXPR that were created by FE, we cannot guarantee they have been initialized? 
> 
> What did you mean by “the place to deal with that is at there previous creation”?
> 
> 
> > 
> >>> Since there's already VLA handling in gimplify_decl_expr, you could remember whether you added DECL_VALUE_EXPR in that function, and only then do the initialization.
> >> Yes, if we can guarantee that all the VAR_DECLs with DECL_VALUE_EXPR created from FEs have been initialized already by FE, we can fix this issue as this way.
> > 
> > Or more generally, check whether the argument to gimplify_decl_expr has DECL_VALUE_EXPR when we enter the function, and don't do the initialization in that case.
> 
> Yes, we can do that.
> 
> However, the major thing I need to make sure is: 
> 
> can we guarantee that for All the VAL_DECLS with DECL_VALUE_EXPR created 
> by FE are initialized already?

I think so.

Richard.

Re: [RFC][Patch][middle-end/PR102359]Not add initialization for READONLY variables with -ftrivial-auto-var-init

[Qing Zhao]

> On Oct 4, 2021, at 1:44 AM, Richard Biener <rguenther@suse.de> wrote:
> 
> On Fri, 1 Oct 2021, Qing Zhao wrote:
> 
>> 
>> 
>>> On Oct 1, 2021, at 10:33 AM, Jason Merrill <jason@redhat.com> wrote:
>>> 
>>> On 10/1/21 10:54, Qing Zhao wrote:
>>>>> On Sep 30, 2021, at 2:31 PM, Jason Merrill <jason@redhat.com> wrote:
>>>>> 
>>>>> On 9/30/21 11:42, Qing Zhao wrote:
>>>>>>> On Sep 30, 2021, at 1:54 AM, Richard Biener <rguenther@suse.de> wrote:
>>>>>>> 
>>>>>>> On Thu, 30 Sep 2021, Jason Merrill wrote:
>>>>>>> 
>>>>>>>> On 9/29/21 17:30, Qing Zhao wrote:
>>>>>>>>> Hi,
>>>>>>>>> 
>>>>>>>>> PR102359 (ICE gimplification failed since  r12-3433-ga25e0b5e6ac8a77a)
>>>>>>>>> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=102359
>>>>>>>>> 
>>>>>>>>> Is due to -ftrivial-auto-var-init adding initialization for READONLY
>>>>>>>>> variable “this” in the following routine: (t.cpp.005t.original)
>>>>>>>>> 
>>>>>>>>> =======
>>>>>>>>> 
>>>>>>>>> ;; Function A::foo()::<lambda()> (null)
>>>>>>>>> ;; enabled by -tree-original
>>>>>>>>> 
>>>>>>>>> {
>>>>>>>>> const struct A * const this [value-expr: &__closure->__this];
>>>>>>>>>   const struct A * const this [value-expr: &__closure->__this];
>>>>>>>>> return <retval> = (double) ((const struct A *) this)->a;
>>>>>>>>> }
>>>>>>>>> =======
>>>>>>>>> 
>>>>>>>>> However, in the above routine, “this” is NOT marked as READONLY, but its
>>>>>>>>> value-expr "&__closure->__this” is marked as READONLY.
>>>>>>>>> 
>>>>>>>>> There are two major issues:
>>>>>>>>> 
>>>>>>>>> 1. In the routine “is_var_need_auto_init”, we should exclude “decl” that is
>>>>>>>>> marked as READONLY;
>>>>>>>>> 2. In the C++ FE, “this” should be marked as READONLY.
>>>>>>>>> 
>>>>>>>>> The idea solution will be:
>>>>>>>>> 
>>>>>>>>> 1. Fix “is_var_need_auto_init” to exclude TREE_READONLY (decl);
>>>>>>>>> 2. Fix C++ FE to mark “this” as TREE_READONLY (decl)==true;
>>>>>>>>> 
>>>>>>>>> Not sure whether it’s hard for C++ FE to fix the 2nd issue or not?
>>>>>>>>> 
>>>>>>>>> In the case it’s not a quick fix in C++FE, I proposed the following fix in
>>>>>>>>> middle end:
>>>>>>>>> 
>>>>>>>>> Let me know your comments or suggestions on this.
>>>>>>>>> 
>>>>>>>>> Thanks a lot for the help.
>>>>>>>> 
>>>>>>>> I'd think is_var_need_auto_init should be false for any variable with
>>>>>>>> DECL_HAS_VALUE_EXPR_P, as they aren't really variables, just ways of naming
>>>>>>>> objects that are initialized elsewhere.
>>>>>>> 
>>>>>>> IIRC handing variables with DECL_HAS_VALUE_EXPR_P is necessary to
>>>>>>> auto-init VLAs, otherwise I tend to agree - would we handle those
>>>>>>> when we see a DECL_EXPR then?
>>>>>> The current implementation is:
>>>>>> gimplify_decl_expr:
>>>>>> For each DECL_EXPR “decl”
>>>>>>   If (VAR_P (decl) && !DECL_EXTERNAL (decl))
>>>>>>     {
>>>>>> 	if (is_vla (decl))
>>>>>>          gimplify_vla_decl (decl, …);      /* existing handling: create a VALUE_EXPR for this vla decl*/
>>>>>> 	…
>>>>>> 	if (has_explicit_init (decl))
>>>>>> 	  {
>>>>>> 	   …;     /* existing handling.  */
>>>>>> 	  }
>>>>>> 	else if (is_var_need_auto_init (decl))      /*. New code. */
>>>>>> 	  {
>>>>>> 	    gimple_add_init_for_auto_var (….);   /*  new code.  */
>>>>>> 	    ...
>>>>>> 	  }
>>>>>>     }
>>>>>> Since the “DECL_VALUE_EXPR (decl)” is NOT a DECL_EXPR, it will not be scanned and added initialization.
>>>>>> if we do not add initialization for a decl that has DECL_VALUE_EXPR, then the “DECL_VALUE_EXPR (decl)” will not be added an initialization either.  We will miss adding initializations for these decls.
>>>>>> So, I think that the current implementation is correct.
>>>>>> And if C++ FE will not mark “this” as READONLY, only mark DECL_VALUE_EXPR(this) as READONLY, the proposed fix is correct too.
>>>>>> Let me know your opinion on this.
>>>>> 
>>>>> The problem with this test is not whether the 'this' proxy is marked READONLY, the problem is that you're trying to initialize lambda capture proxies at all; the lambda capture objects were already initialized when forming the closure object.  So this test currently aborts with -ftrivial-auto-var-init=zero because you "initialize" the i capture field to 0 after it was previously initialized to 42:
>>>>> 
>>>>> int main()
>>>>> {
>>>>> int i = 42;
>>>>> auto l = [=]() mutable { return i; };
>>>>> if (l() != i)
>>>>>   __builtin_abort ();
>>>>> }
>>>>> 
>>>>> I believe the same issue applies to the proxy variables in coroutines that work much like lambdas.
>>> 
>>>> So, how should the middle end determine that a variable is “proxy variable”?
>>> 
>>> In the front end, is_capture_proxy will identify a lambda capture proxy variable.  But that won't be true for the similar proxies used by coroutines.
>> 
>> Does this mean that in middle end, especially in gimplification phase, there is Not a simple way to determine whether a variable is a proxy variable?
>>> 
>>>> Have all “proxy variables” been initialized by C++ FE already?
>>> 
>>> Yes.
>>> 
>>>>> You can't just assume that a VAR_DECL with DECL_VALUE_EXPR is uninitialized.
>>>> So, all the VAR_DECLs with DECL_VALUE_EXPR (except the ones created by “gimplify_decl_expr”) are initialized by FE already?
>>> 
>>> In general I'd expect them to refer to previously created objects which may or may not have been initialized, but if they haven't been, the place to deal with that is at their previous creation.
>> 
>> Still a little confuse..., do you mean, even for VAL_DECLS with DECL_VALUE_EXPR that were created by FE, we cannot guarantee they have been initialized? 
>> 
>> What did you mean by “the place to deal with that is at there previous creation”?
>> 
>> 
>>> 
>>>>> Since there's already VLA handling in gimplify_decl_expr, you could remember whether you added DECL_VALUE_EXPR in that function, and only then do the initialization.
>>>> Yes, if we can guarantee that all the VAR_DECLs with DECL_VALUE_EXPR created from FEs have been initialized already by FE, we can fix this issue as this way.
>>> 
>>> Or more generally, check whether the argument to gimplify_decl_expr has DECL_VALUE_EXPR when we enter the function, and don't do the initialization in that case.
>> 
>> Yes, we can do that.
>> 
>> However, the major thing I need to make sure is: 
>> 
>> can we guarantee that for All the VAL_DECLS with DECL_VALUE_EXPR created 
>> by FE are initialized already?
> 
> I think so.

Okay. 

Will fix this bug based on this.

Thanks.

Qing

> 
> Richard.