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

[Richard Biener <rguenther@suse.de>]

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 we should be able to diagnose sth like

warning: variable is used uninitialized
note: variable declared here

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.

Richard.