> 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:

Reply via email to