On Wed, Jan 5, 2022 at 5:59 PM Qing Zhao <qing.z...@oracle.com> 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?
I don't know but it definitely happens. > > 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. -:) I know ... I didn't have a better idea. > 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. > > > > > + /* 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. But the ... = .DEFERRED_INIT stmt itself could be used to record this since its location is already used to indicate the original location, like with suppress_warning/warning_suppressed_p? > > > > + /* 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. How can this happen? It looks like a bug to me. Do you have a testcase? > > > 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. Yes, I know. But the patch seems to do multiple things and there's no new testcase and the ChangeLog does not indicate the altered testcases are in any way now fixed. Sorry for the delay btw, Richard. > > 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. > > > > 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: >
