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

[Richard Biener <richard.guenther@gmail.com>]

On Tue, Jan 11, 2022 at 5:32 PM Qing Zhao <qing.zhao@oracle.com> wrote:
>
> Hi, Richard,
>
> > On Jan 11, 2022, at 7:43 AM, Richard Biener <richard.guenther@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.

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"?  I was
expecting the first patch to fix this.

I'll wait for an update to the second patch.

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
>