Re: [Patch][V2]Enable -Wuninitialized + -ftrivial-auto-var-init for address taken variables

[Richard Biener <richard.guenther@gmail.com>]

On Thu, Dec 16, 2021 at 5:00 PM Qing Zhao <qing.zhao@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.  It's also a bit awkward
to build another
copy of all decl names :/  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).

+         /* 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?

+         /* 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.  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?

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.

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