Hi, Richard,
> On Jan 11, 2022, at 7:43 AM, Richard Biener <richard.guent...@gmail.com>
> wrote:
>
>>>>
>>>>
>>>> 1. Add some meaningful temporaries to break the long expression to make it
>>>> Readable. And also add comments to explain the purpose of the statement;
>>>>
>>>> 2. Resolve the memory leakage of the dynamically created string.
>>>>
>>>> The patch has been bootstrapped and regressing tested on both x86 and
>>>> aarch64, no issues.
>>>> Okay for commit?
>>>
>>> tree decl_name
>>> + = build_string_literal (IDENTIFIER_LENGTH (DECL_NAME (decl)) + 1,
>>> + IDENTIFIER_POINTER (DECL_NAME (decl)));
>>>
>>> you need to deal with DECL_NAME being NULL.
>>
>> Okay.
>> Usually under what situation, the decl_name will be NULL?
>
> I don't know but it definitely happens.
>
>>> It's also a bit awkward
>>> to build another
>>> copy of all decl names :/
>>
>> Yes, this is awkward. But it might be unavoidable for address taken
>> variables since the original variable might be completely deleted by
>> optimizations.
>> See the details at:
>> https://gcc.gnu.org/pipermail/gcc-patches/2021-August/577431.html
>>
>> We had a previous discussion on this issue, and the idea of adding this 3rd
>> argument with the name of the variable was proposed by you at that time. -:)
>
> I know ... I didn't have a better idea.
I think that adding the name string of the auto variable as one parameter to
the function .DEFERRED_INIT might be the only solution to this issue? (In the
very beginning of the implementation, we added the VAR itself as one parameter
to the function .DEFERRED_INIT, but that design didn’t work out)
>
>>
>>
>>>
>>> + /* The LHS of the call is a temporary variable, we use it as a
>>> + placeholder to record the information on whether the warning
>>> + has been issued or not. */
>>> + repl_var = gimple_call_lhs (def_stmt);
>>>
>>> this seems to be a change that can be done independently?
>>
>> The major purpose of this “repl_var” is used to record the info whether the
>> warning has been issued for the variable or not, then avoid emitting it
>> again later.
>> Since the original variable has been completely deleted by optimization, we
>> have to use this “repl_var” for a placeholder to record such info.
>
> But the ... = .DEFERRED_INIT stmt itself could be used to record this
> since its location is
> already used to indicate the original location, like with
> suppress_warning/warning_suppressed_p?
Ah, I will check on this. Thanks a lot.
>
>>>
>>> + /* Ignore the call to .DEFERRED_INIT that define the original
>>> + var itself. */
>>> + if (is_gimple_assign (context))
>>> + {
>>> + if (TREE_CODE (gimple_assign_lhs (context)) == VAR_DECL)
>>> + lhs_var = gimple_assign_lhs (context);
>>> + else if (TREE_CODE (gimple_assign_lhs (context)) == SSA_NAME)
>>> + lhs_var = SSA_NAME_VAR (gimple_assign_lhs (context));
>>> + }
>>> + if (lhs_var
>>> + && (lhs_var_name = DECL_NAME (lhs_var))
>>> + && (lhs_var_name_str = IDENTIFIER_POINTER (lhs_var_name))
>>> + && (strcmp (lhs_var_name_str, var_name_str) == 0))
>>> + return;
>>>
>>> likewise but I don't really understand what you are doing here.
>>
>> The above is to exclude the following case:
>>
>> temp = .DEFERRED_INIT (4, 2, “alt_reloc");
>> alt_reloc = temp;
>>
>> i.e, a call to .DEFERRED_INIT that define the original variable itself.
>
> How can this happen? It looks like a bug to me. Do you have a testcase?
With -ftrivial-auto-var-init, During gimplification phase, almost all address
taken variables that do not have an explicit initializer will have the above IR
pattern.
For example, gcc.dg/auto-init-uninit-16.c:
[opc@qinzhao-ol8u3-x86 gcc]$ cat
/home/opc/Work/GCC/latest-gcc/gcc/testsuite/gcc.dg/auto-init-uninit-16.c
/* { dg-do compile } */
/* { dg-options "-O2 -Wuninitialized -ftrivial-auto-var-init=zero" } */
int foo, bar;
static
void decode_reloc(int reloc, int *is_alt)
{
if (reloc >= 20)
*is_alt = 1;
else if (reloc >= 10)
*is_alt = 0;
}
void testfunc()
{
int alt_reloc;
decode_reloc(foo, &alt_reloc);
if (alt_reloc) /* { dg-warning "may be used uninitialized" "" } */
bar = 42;
}
****With -ftrivial-auto-var-init=zero, the IR after gimplification is:
_1 = .DEFERRED_INIT (4, 2, &"alt_reloc"[0]);
alt_reloc = _1;
And the IR after SSA is similar as the above:
_1 = .DEFERRED_INIT (4, 2, &"alt_reloc"[0]);
alt_reloc = _1;
During the early uninitialized analysis phase, the above IR will feed to the
analyzer, we should exclude such
IR from issuing fake warnings.
>
>>
>>> I'm
>>> also not sure
>>> I understand the case we try to fix with passing the name - is that
>>> for VLA decls
>>> that get replaced by allocation?
>>
>> This whole patch is mainly to resolve the issue that has been discussed last
>> Aug as:
>>
>> https://gcc.gnu.org/pipermail/gcc-patches/2021-August/577431.html
>>
>> We have agreed at that time to resolve this issue later.
>
> Yes, I know. But the patch seems to do multiple things and there's no
> new testcase
> and the ChangeLog does not indicate the altered testcases are in any
> way now fixed.
That’s my bad, I realized this problem and separated the original patch into
two separate patch and also added more detailed
Description of the problem, hope this time the patch will be more clearer.
You have approved the 1st patch. I will update it per your suggestion and
commit to GCC12.
For the 2nd one, I will fix the concern you raised about “repl_var”, and
resubmit the patch.
Qing
