Re: [patch][version 4]add -ftrivial-auto-var-init and variable attribute "uninitialized" to gcc

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

> On Jul 12, 2021, at 2:51 AM, Richard Sandiford <richard.sandiford@arm.com> wrote:
> 
> Martin Jambor <mjambor@suse.cz> writes:
>> On Thu, Jul 08 2021, Qing Zhao wrote:
>>> (Resend this email since the previous one didn’t quote, I changed one
>>> setting in my mail client, hopefully that can fix this issue).
>>> 
>>> Hi, Martin,
>>> 
>>> Thank you for the review and comment.
>>> 
>>>> On Jul 8, 2021, at 8:29 AM, Martin Jambor <mjambor@suse.cz> wrote:
>>>>> diff --git a/gcc/tree-sra.c b/gcc/tree-sra.c
>>>>> index c05d22f3e8f1..35051d7c6b96 100644
>>>>> --- a/gcc/tree-sra.c
>>>>> +++ b/gcc/tree-sra.c
>>>>> @@ -384,6 +384,13 @@ static struct
>>>>> 
>>>>>  /* Numbber of components created when splitting aggregate parameters.  */
>>>>>  int param_reductions_created;
>>>>> +
>>>>> +  /* Number of deferred_init calls that are modified.  */
>>>>> +  int deferred_init;
>>>>> +
>>>>> +  /* Number of deferred_init calls that are created by
>>>>> +     generate_subtree_deferred_init.  */
>>>>> +  int subtree_deferred_init;
>>>>> } sra_stats;
>>>>> 
>>>>> static void
>>>>> @@ -4096,6 +4103,110 @@ get_repl_default_def_ssa_name (struct access *racc, tree reg_type)
>>>>>  return get_or_create_ssa_default_def (cfun, racc->replacement_decl);
>>>>> }
>>>>> 
>>>>> +
>>>>> +/* Generate statements to call .DEFERRED_INIT to initialize scalar replacements
>>>>> +   of accesses within a subtree ACCESS; all its children, siblings and their
>>>>> +   children are to be processed.
>>>>> +   GSI is a statement iterator used to place the new statements.  */
>>>>> +static void
>>>>> +generate_subtree_deferred_init (struct access *access,
>>>>> +				tree init_type,
>>>>> +				tree is_vla,
>>>>> +				gimple_stmt_iterator *gsi,
>>>>> +				location_t loc)
>>>>> +{
>>>>> +  do
>>>>> +    {
>>>>> +      if (access->grp_to_be_replaced)
>>>>> +	{
>>>>> +	  tree repl = get_access_replacement (access);
>>>>> +	  gimple *call
>>>>> +	    = gimple_build_call_internal (IFN_DEFERRED_INIT, 3,
>>>>> +					  TYPE_SIZE_UNIT (TREE_TYPE (repl)),
>>>>> +					  init_type, is_vla);
>>>>> +	  gimple_call_set_lhs (call, repl);
>>>>> +	  gsi_insert_before (gsi, call, GSI_SAME_STMT);
>>>>> +	  update_stmt (call);
>>>>> +	  gimple_set_location (call, loc);
>>>>> +	  sra_stats.subtree_deferred_init++;
>>>>> +	}
>>>>> +      else if (access->grp_to_be_debug_replaced)
>>>>> +	{
>>>>> +	  tree drepl = get_access_replacement (access);
>>>>> +	  tree call = build_call_expr_internal_loc
>>>>> +		     (UNKNOWN_LOCATION, IFN_DEFERRED_INIT,
>>>>> +		      TREE_TYPE (drepl), 3,
>>>>> +		      TYPE_SIZE_UNIT (TREE_TYPE (drepl)),
>>>>> +		      init_type, is_vla);
>>>>> +	  gdebug *ds = gimple_build_debug_bind (drepl, call,
>>>>> +						gsi_stmt (*gsi));
>>>>> +	  gsi_insert_before (gsi, ds, GSI_SAME_STMT);
>>>> 
>>>> Is handling of grp_to_be_debug_replaced accesses necessary here?  If so,
>>>> why?  grp_to_be_debug_replaced accesses are there only to facilitate
>>>> debug information about a part of an aggregate decl is that is likely
>>>> going to be entirely removed - so that debuggers can sometimes show to
>>>> users information about what they would contain had they not removed.
>>>> It seems strange you need to mark them as uninitialized because they
>>>> should not have any consumers.  (But perhaps it is also harmless.)
>>> 
>>> This part has been discussed during the 2nd version of the patch, but
>>> I think that more discussion might be necessary.
>>> 
>>> In the previous discussion, Richard Sandiford mentioned:
>>> (https://gcc.gnu.org/pipermail/gcc-patches/2021-April/568620.html):
>>> 
>>> =====
>>> 
>>> I guess the thing we need to decide here is whether -ftrivial-auto-var-init
>>> should affect debug-only constructs too.  If it doesn't, exmaining removed
>>> components in a debugger might show uninitialised values in cases where
>>> the user was expecting initialised ones.  There would be no security
>>> concern, but it might be surprising.
>>> 
>>> I think in principle the DRHS can contain a call to DEFERRED_INIT.
>>> Doing that would probably require further handling elsewhere though.
>>> 
>>> =====
>>> 
>>> I am still not very confident now for this part of the change.
>> 
>> I see.  I still tend to think that with or without the generation of
>> gimple_build_debug_binds, the debugger would still not display any value
>> for the component in question.  Without it there would be no information
>> about the component at a any place in code affected by this, with it the
>> component would be explicitely uninitialized.  But OK.
> 
> FTR, I don't have a strong opinion here.  You know the code better
> than I do, so if you think not generating debug binds is better then
> let's do that.

I am okay with not generating debug binds here. 

Then I will just delete the part of code that guarded with  if (access->grp_to_be_debug_replaced)?

Martin, please confirm on this.

Thanks.

Qing
> Thanks,
> Richard