> On Jan 5, 2022, at 10:59 AM, Qing Zhao via Gcc-patches
> <gcc-patches@gcc.gnu.org> wrote:
>
> Hi, Richard,
>
> Thanks a lot for the review and questions.
> See my reply embedded below:
>
>
>> On Jan 5, 2022, at 4:33 AM, Richard Biener <richard.guent...@gmail.com>
>> wrote:
>>
>> On Thu, Dec 16, 2021 at 5:00 PM Qing Zhao <qing.z...@oracle.com> wrote:
>>>
>>> Hi,
>>>
>>> This is the 2nd version of the patch.
>>> The original patch is at:
>>>
>>> https://gcc.gnu.org/pipermail/gcc-patches/2021-December/586341.html
>>>
>>> In addition to resolve the two issues mentioned in the original patch,
>>> This patch also can be used as a very good workaround for the issue in
>>> PR103720
>>> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=103720
>>>
>>> And as I checked, the patch can fix all the bogus uninitialized warnings
>>> when
>>> building kernel with -O2 + -ftrivial-auto-var-init=zero + -Wuninitialized.
>>>
>>> So, this is a very important patch that need to be included into gcc12.
>>>
>>> Compared to the 1st patch, the major changes are to resolve Martin’s
>>> comments on
>>> tree-ssa-uninit.c
>>>
>>> 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?
Anyway, I made the following change in gimplify.c to address this concern:
[opc@qinzhao-ol8u3-x86 gcc]$ git diff
diff --git a/gcc/gimplify.c b/gcc/gimplify.c
index cc6b643312e..a9714c9f9c4 100644
--- a/gcc/gimplify.c
+++ b/gcc/gimplify.c
@@ -1764,9 +1764,12 @@ gimple_add_init_for_auto_var (tree decl,
tree init_type_node
= build_int_cst (integer_type_node, (int) init_type);
+ const char *decl_name_str = DECL_NAME (decl)
+ ? IDENTIFIER_POINTER (DECL_NAME (decl))
+ : "<anonymous>";
tree decl_name
- = build_string_literal (IDENTIFIER_LENGTH (DECL_NAME (decl)) + 1,
- IDENTIFIER_POINTER (DECL_NAME (decl)));
+ = build_string_literal (strlen (decl_name_str) + 1,
+ decl_name_str);
Let me know if you see any issue with this solution.
>
>> 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. -:)
> And I think that’s a good solution to this problem.
>
>
>> The changes in the uninit warning are also
>> quite ugly, refactoring
>> things to always pass down a name / location pair might improve that
>> (but I'd like to
>> understand the case to fix first).
>
> I will try this idea to see whether it will work.
Martin Sebor raised the similar concern and similar suggestions in the previous
round of review, and I tried to pass the name string and use “%qs” consistently.
However, that didn’t work since there are some special cases that %qD handles
specially other than %qs. There were multiple testing cases failed after the
change.
In order to resolve the regression, we have to copy multiple details from the
handling of “%D” to uninit warning.
Please see:
https://gcc.gnu.org/pipermail/gcc-patches/2021-December/586814.html
For more details.
>
>>
>> + /* 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.
>>
>> + /* 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.
>
>> 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.
>
> And this patch is to resolve this issue.
> Hope this is clear.
>
> Thanks.
>
> Qimg
>
>
>>
>> The patch adjusts testcases but doesn't add new ones but each of the
>> above changes
>> would warrant one and make it easier to understand the motivation of
>> the changes.
Adjusting the following testing cases is the main purpose of this patch:
gcc/testsuite/gcc.dg/auto-init-uninit-16.c | 4 +-
gcc/testsuite/gcc.dg/auto-init-uninit-34.c | 8 +-
gcc/testsuite/gcc.dg/auto-init-uninit-37.c | 44 ++++----
gcc/testsuite/gcc.dg/auto-init-uninit-B.c
Previously, the warnings on address taken variables were missing, I added
“xfail” at the warning lines.
With this patch, all the warnings for address taken variables can be issued
correctly, so, I removed all the “xfail” at the warning lines.
All the other adjustment are for the 3rd parameter of .DEFERRED_INIT.
Let me know whether it’s clear about the purpose of this patch.
If you still have questions on the purpose of this patch, please let me know.
Thanks a lot.
Qing
>>
>> Richard.
>>
>>> thanks.
>>>
>>> Qing
>>>
>>> =================================================
>>>
>>> ******Compared to the 1st version, the code change is:
>>>
>>> --- 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,
>>> @@ -798,26 +798,35 @@
>>> 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,
>>> +@@ -197,9 +210,43 @@ 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))
>>> + {
>>> ++ tree lhs_var = NULL_TREE;
>>> ++ tree lhs_var_name = NULL_TREE;
>>> ++ const char *lhs_var_name_str = NULL;
>>> + /* 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)
>>> -+ return;
>>> ++ /* 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;
>>> +
>>> + /* Get the variable declaration location from the def_stmt. */
>>> + var_decl_loc = gimple_location (def_stmt);
>>> @@ -834,7 +843,7 @@
>>> 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,
>>> +@@ -207,36 +254,54 @@ 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)))))
>>> @@ -863,25 +872,24 @@
>>>
>>> auto_diagnostic_group d;
>>> - if (!warning_at (location, opt, gmsgid, var))
>>> +- return;
>>> + 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, var ? "%qD " : "%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;
>>> ++ if ((var && !warning_at (location, opt, gmsgid_final, var))
>>> ++ || (var_name && !warning_at (location, opt, gmsgid_final,
>>> var_name_str)))
>>> ++ {
>>> ++ XDELETE (gmsgid_final);
>>> ++ return;
>>> ++ }
>>> ++ XDELETE (gmsgid_final);
>>>
>>> /* Avoid subsequent warnings for reads of the same variable again. */
>>> - suppress_warning (var, opt);
>>> -+ if (var)
>>> -+ suppress_warning (var, opt);
>>> -+ else if (repl_var)
>>> -+ suppress_warning (repl_var, opt);
>>> ++ if (var || repl_var)
>>> ++ suppress_warning (var ? var : repl_var, opt);
>>>
>>> /* Issue a note pointing to the read variable unless the warning
>>> is at the same location. */
>>> @@ -898,7 +906,7 @@
>>> }
>>>
>>> ******The complete patch is:
