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