[Patch][V2]Enable -Wuninitialized + -ftrivial-auto-var-init for address taken variables

[Qing Zhao]

> On Jan 12, 2022, at 4:46 AM, Richard Biener <richard.guenther@gmail.com> wrote:
> 
> On Tue, Jan 11, 2022 at 5:32 PM Qing Zhao <qing.zhao@oracle.com> wrote:
>> 
>>>>> +         /* 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
>> 
>> ****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.
> 
> Yes, but how do we get to a fake warning here?  We should eventually
> run into the _1 def being used
> by alt_reloc = ...; and then by means of using the string "alt_reloc"
> warn about an uninitialized use of
> alt_reloc?  Is it the point of the use that is "misdiagnosed"?  

Yes, that’s the issue, the use point would be misdiagnosed without excluding this self definition chain. 

For the following: cat /home/opc/Work/GCC/latest-gcc/gcc/testsuite/gcc.dg/auto-init-uninit-16.c
  1 /* { dg-do compile } */
  2 /* { dg-options "-O2 -Wuninitialized -ftrivial-auto-var-init=zero" } */
  3 
  4 int foo, bar;
  5 
  6 static
  7 void decode_reloc(int reloc, int *is_alt)
  8 {
  9   if (reloc >= 20)
 10       *is_alt = 1;
 11   else if (reloc >= 10)
 12       *is_alt = 0;
 13 }
 14 
 15 void testfunc()
 16 {
 17   int alt_reloc;
 18 
 19   decode_reloc(foo, &alt_reloc);
 20 
 21   if (alt_reloc) /* { dg-warning "may be used uninitialized" "" }  */
 22     bar = 42;
 23 }

If I deleted the following from tree-ssa-uninit.c:

+         /* 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;

We will get the following warning:

/home/opc/Work/GCC/latest_gcc/gcc/testsuite/gcc.dg/auto-init-uninit-16.c: In function ‘testfunc’:
/home/opc/Work/GCC/latest_gcc/gcc/testsuite/gcc.dg/auto-init-uninit-16.c:17:7: warning: ‘alt_reloc’ is used uninitialized [-Wuninitialized]

In which the line number of the usage point “17:7” is wrong, which is the declaration point of the variable. 
This warning is issued during “pass_early_warn_uninitialized” for the following IR:

_1 = .DEFERRED_INIT (4, 2, &"alt_reloc"[0]);
alt_reloc = _1;

The above usage point “alt_reloc = _1” actually is a fake usage, we should exclude this usage from issuing warning.

The correct warning should be issued during “pass_late_warn_uninitialized” and for line 21 “if alt_reloc” for the following IR (the use point is “if (_1 != 0), which is for line 21:

_1 = .DEFERRED_INIT (4, 2, &"alt_reloc"[0]);
  if (_1 != 0)

Hope this is clear now.

> I was
> expecting the first patch to fix this.
> 
> I'll wait for an update to the second patch.
I will update it today.

thanks.

Qing
> 
> Richard.
> 
>> 
>>> 
>>>> 
>>>>> 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