Hi, Martin,
Thanks a lot for your review and comments.
> On Dec 9, 2021, at 11:43 AM, Martin Sebor <mse...@gmail.com> wrote:
>>
>> diff --git a/gcc/tree-ssa-uninit.c b/gcc/tree-ssa-uninit.c
>> index 1df0bcc42c0..85d1ba866fc 100644
>> --- a/gcc/tree-ssa-uninit.c
>> +++ b/gcc/tree-ssa-uninit.c
>> @@ -182,9 +182,22 @@ warn_uninit (opt_code opt, tree t, tree var, const char
>> *gmsgid,
>> }
>> /* Anonymous SSA_NAMEs shouldn't be uninitialized, but
>> ssa_undefined_value_p
>> - can return true if the def stmt of an anonymous SSA_NAME is
>> COMPLEX_EXPR
>> - created for conversion from scalar to complex. Use the underlying var
>> of
>> - the COMPLEX_EXPRs real part in that case. See PR71581. */
>> + can return true if the def stmt of an anonymous SSA_NAME is
>> + 1. A COMPLEX_EXPR created for conversion from scalar to complex. Use
>> the
>> + underlying var of the COMPLEX_EXPRs real part in that case. See
>> PR71581.
>> +
>> + 2. A call to .DEFERRED_INIT internal function. Since the original
>> variable
>> + has been eliminated by optimziation, we need to get the variable name,
>> + and variable declaration location from this call. At the same time, we
>> + should use the temporary variable that was created to replace the
>> original
>> + variable as a placeholder to record the information on whether the
>> warning
>> + message for the variable has been issued or not. */
>> +
>> + tree var_name = NULL_TREE;
>> + const char *var_name_str = NULL;
>> + location_t var_decl_loc = UNKNOWN_LOCATION;
>> + tree repl_var = NULL_TREE;
>> +
>> if (!var && !SSA_NAME_VAR (t))
>> {
>> gimple *def_stmt = SSA_NAME_DEF_STMT (t);
>> @@ -197,9 +210,34 @@ warn_uninit (opt_code opt, tree t, tree var, const char
>> *gmsgid,
>> && zerop (gimple_assign_rhs2 (def_stmt)))
>> var = SSA_NAME_VAR (v);
>> }
>> +
>> + if (gimple_call_internal_p (def_stmt, IFN_DEFERRED_INIT))
>> + {
>> + /* Get the variable name from the 3rd argument of call. */
>> + var_name = gimple_call_arg (def_stmt, 2);
>> + var_name = TREE_OPERAND (TREE_OPERAND (var_name, 0), 0);
>> + var_name_str = TREE_STRING_POINTER (var_name);
>> +
>> + if (is_gimple_assign (context)
>> + && TREE_CODE (gimple_assign_lhs (context)) == VAR_DECL
>> + && DECL_NAME (gimple_assign_lhs (context))
>> + && IDENTIFIER_POINTER (DECL_NAME (gimple_assign_lhs (context))))
>> + if (strcmp
>> + (IDENTIFIER_POINTER (DECL_NAME (gimple_assign_lhs (context))),
>> + var_name_str) == 0)
>
> For what it's worth, although the style in GCC is pervasive
> of repeating the same expression in operands of subsequent
> subexpressions of a logical exoression, I think it gets more
> and more difficult to read as the expression become increasingly
> deeply nested. I find it far more readable when temporaries
> are added for the subexpressions (if it makes the nesting too
> deep I take it as a sign that the logic might be better factored
> out into a helper function).
Agreed. I will add some temporaries with meaningful name to make the above more
readable.
>
>> + return;
>> +
>> + /* Get the variable declaration location from the def_stmt. */
>> + var_decl_loc = gimple_location (def_stmt);
>> +
>> + /* 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);
>> + }
>> }
>> - if (var == NULL_TREE)
>> + if (var == NULL_TREE && var_name == NULL_TREE)
>> return;
>> /* Avoid warning if we've already done so or if the warning has been
>> @@ -207,36 +245,56 @@ warn_uninit (opt_code opt, tree t, tree var, const
>> char *gmsgid,
>> if (((warning_suppressed_p (context, OPT_Wuninitialized)
>> || (gimple_assign_single_p (context)
>> && get_no_uninit_warning (gimple_assign_rhs1 (context)))))
>> - || get_no_uninit_warning (var))
>> + || (var && get_no_uninit_warning (var))
>> + || (repl_var && get_no_uninit_warning (repl_var)))
>> return;
>> /* Use either the location of the read statement or that of the PHI
>> argument, or that of the uninitialized variable, in that order,
>> whichever is valid. */
>> - location_t location;
>> + location_t location = UNKNOWN_LOCATION;
>> if (gimple_has_location (context))
>> location = gimple_location (context);
>> else if (phi_arg_loc != UNKNOWN_LOCATION)
>> location = phi_arg_loc;
>> - else
>> + else if (var)
>> location = DECL_SOURCE_LOCATION (var);
>> + else if (var_name)
>> + location = var_decl_loc;
>> +
>> location = linemap_resolve_location (line_table, location,
>> LRK_SPELLING_LOCATION, NULL);
>> auto_diagnostic_group d;
>> - if (!warning_at (location, opt, gmsgid, var))
>> + char *gmsgid_final = XNEWVEC (char, strlen (gmsgid) + 5);
>> + gmsgid_final[0] = 0;
>> + if (var)
>> + strcat (gmsgid_final, "%qD ");
>> + else if (var_name)
>> + strcat (gmsgid_final, "%qs ");
>> + strcat (gmsgid_final, gmsgid);
>> +
>> + if (var && !warning_at (location, opt, gmsgid_final, var))
>> + return;
>> + else if (var_name && !warning_at (location, opt, gmsgid_final,
>> var_name_str))
>> return;
>
> Dynamically creating the string seems quite cumbersome here, and
> it leaks the allocated block. I wonder if it might be better to
> remove the gmsgid argument from the function and assign it to
> one of the literals based on the other arguments.
>
> Since only one of var and var_name is used, I also wonder if
> the %qs form could be used for both to simplify the overall
> logic. (I.e., get the IDENTIFIER_POINTER string from var and
> use it instead of %qD).
Both are good suggestions, I will try to update the code based on this.
Thanks again.
Qing
>
