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

[Qing Zhao <qing.zhao@oracle.com>]

> 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
>