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.
