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

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

> On Aug 18, 2021, at 2:19 AM, Richard Biener <rguenther@suse.de> wrote:
> 
> On Tue, 17 Aug 2021, Qing Zhao wrote:
> 
>> 
>> 
>>> On Aug 17, 2021, at 10:04 AM, Qing Zhao via Gcc-patches <gcc-patches@gcc.gnu.org> wrote:
>>> 
>>> 
>>> 
>>>> On Aug 16, 2021, at 11:48 AM, Qing Zhao via Gcc-patches <gcc-patches@gcc.gnu.org> wrote:
>>>> 
>>>>>> From the above IR file after “FRE”, we can see that the major issue with this IR is:
>>>>>> 
>>>>>> The address taken auto variable “alt_reloc” has been completely replaced by the temporary variable “_1” in all
>>>>>> the uses of the original “alt_reloc”. 
>>>>> 
>>>>> Well, this can happen with regular code as well, there's no need for
>>>>> .DEFERRED_INIT.  This is the usual problem with reporting uninitialized
>>>>> uses late.
>>>>> 
>>>>> IMHO this shouldn't be a blocker.  The goal of zero "regressions" wrt
>>>>> -Wuninitialized isn't really achievable.
>>>> 
>>>> Okay. Sounds reasonable to me too.
>>>> 
>>>>> 
>>>>>> The major problem with such IR is,  during uninitialized analysis phase, the original use of “alt_reloc” disappeared completely.
>>>>>> So, the warning cannot be reported.
>>>>>> 
>>>>>> 
>>>>>> My questions:
>>>>>> 
>>>>>> 1. Is it possible to get the original “alt_reloc” through the temporary variable “_1” with some available information recorded in the IR?
>>>>>> 2. If not, then we have to record the relationship between “alt_reloc” and “_1” when the original “alt_reloc” is replaced by “_1” and get such relationship during
>>>>>> Uninitialized analysis phase.  Is this doable?
>>>>> 
>>>>> Well, you could add a fake argument to .DEFERRED_INIT for the purpose of
>>>>> diagnostics.  The difficulty is to avoid tracking it as actual use so
>>>>> you could for example pass a string with the declarations name though
>>>>> this wouldn't give the association with the actual decl.
>>>> Good suggestion, I can try this a little bit. 
>>> 
>>> I tried this yesterday, added the 4th argument to .DEFERRED_INIT as:
>>> 
>>>   1st argument: SIZE of the DECL;
>>>   2nd argument: INIT_TYPE;
>>>   3rd argument: IS_VLA, 0 NO, 1 YES;
>>> +   4th argument: The NAME for the DECL;
>>> 
>>> -   as LHS = DEFERRED_INIT (SIZE of the DECL, INIT_TYPE, IS_VLA)
>>> +   as LHS = DEFERRED_INIT (SIZE of the DECL, INIT_TYPE, IS_VLA, NAME)
>>> 
>>> +  tree name_node
>>> +    = build_string_literal (IDENTIFIER_LENGTH (DECL_NAME (decl)),
>>> +                           IDENTIFIER_POINTER (DECL_NAME (decl)));
>>> 
>>>  tree call = build_call_expr_internal_loc (UNKNOWN_LOCATION, IFN_DEFERRED_INIT,
>>> -                                           TREE_TYPE (decl), 3,
>>> +                                           TREE_TYPE (decl), 4,
>>>                                           decl_size, init_type_node,
>>> -                                           is_vla_node);
>>> +                                           is_vla_node, name_node);
>>> 
>>> 
>>> And got the following IR in .uninit1 dump:
>>> 
>>> 
>>> ….
>>> 
>>> _1 = .DEFERRED_INIT (4, 2, 0, &"alt_reloc"[0]);
>>> if (_1 != 0)
>>> ….
>>> 
>>> 
>>> My questions:
>>> 
>>> 1. Is “build_string_literal” the correct utility routine to use for this new argument? 
>>> 2. Will Such string literal nodes have potential other impact?
>> 
>> I tried to get the 4th argument from the call to .DEFERED_INIT during uninitialized variable analysis in tree-ssa-uninit.c:
>> 
>> @@ -197,18 +197,25 @@ warn_uninit (enum opt_code wc, tree t, tree expr, tree var,
>>      the COMPLEX_EXPRs real part in that case.  See PR71581.  */
>>   if (expr == NULL_TREE
>>       && var == NULL_TREE
>> -      && SSA_NAME_VAR (t) == NULL_TREE
>> -      && is_gimple_assign (SSA_NAME_DEF_STMT (t))
>> -      && gimple_assign_rhs_code (SSA_NAME_DEF_STMT (t)) == COMPLEX_EXPR)
>> -    {
>> -      tree v = gimple_assign_rhs1 (SSA_NAME_DEF_STMT (t));
>> -      if (TREE_CODE (v) == SSA_NAME
>> -         && has_undefined_value_p (v)
>> -         && zerop (gimple_assign_rhs2 (SSA_NAME_DEF_STMT (t))))
>> +      && SSA_NAME_VAR (t) == NULL_TREE)
>> +    {
>> +      if (is_gimple_assign (SSA_NAME_DEF_STMT (t))
>> +         && (gimple_assign_rhs_code (SSA_NAME_DEF_STMT (t)) == COMPLEX_EXPR))
>>        {
>> -         expr = SSA_NAME_VAR (v);
>> -         var = expr;
>> +         tree v = gimple_assign_rhs1 (SSA_NAME_DEF_STMT (t));
>> +         if (TREE_CODE (v) == SSA_NAME
>> +             && has_undefined_value_p (v)
>> +             && zerop (gimple_assign_rhs2 (SSA_NAME_DEF_STMT (t))))
>> +           {
>> +             expr = SSA_NAME_VAR (v);
>> +             var = expr;
>> +           }
>>        }
>> +      else if (gimple_call_internal_p (SSA_NAME_DEF_STMT (t), IFN_DEFERRED_INIT))
>> +      {
>> +       expr = gimple_call_arg (SSA_NAME_DEF_STMT (t), 3);
>> +       var = expr;
>> +      }
>>     }
>> 
>> However, this 4th argument is not a regular variable, it’s just an ADDR_EXPR that includes the constant string for the name of 
>> the deleted variable. 
>> If we’d like to report the warning based on this ADDR_EXPR, a complete new code to report the warnings other than the current one that based on 
>> “Variables” need to be added, this might make the code very ugly. 
>> 
>> My questions:
>> 
>> 1. Is there better way to do this?
> 
> Adding a variable as extra argument won't work, so no, I don't see a nice
> way of carrying the extra information.  Btw, if you make sure to set
> the location of the .DEFERRED_INIT call to the DECL_SOURCE_LOCATION
> of the decl we initialize,

This should be easy to do.

> we should be able to diagnose sth like
> 
> warning: variable is used uninitialized
> note: variable declared here

I.e, report the warnings without the name of the variable?
> 
> and point to the correct declartion point which should reveal the
> variable name (to the user, not to the compiler).
> 
>> 1. As you mentioned before, it’s very unrealistic to meet the goal of “zero regression” for -Wuninitialized, can we leave this part of work in a later patch to improve
>> The warning for “address taken” auto variables?
> 
> Yes, as said, I'd simply ignore this particular issue for now since I
> don't see a good way to fix it.

Okay, I will just ignore this issue for now and resolve it in a later patch.

How about the testing cases that are currently failed due to this issue? Should I keep them but mark them as expected failure?

Qing

> 
> Richard.