On Tue, Jan 11, 2022 at 5:32 PM Qing Zhao <qing.z...@oracle.com> wrote: > > Hi, Richard, > > > On Jan 11, 2022, at 7:43 AM, Richard Biener <richard.guent...@gmail.com> > > wrote: > > > >>>> > >>>> > >>>> 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. > > I think that adding the name string of the auto variable as one parameter to > the function .DEFERRED_INIT might be the only solution to this issue? (In the > very beginning of the implementation, we added the VAR itself as one > parameter to the function .DEFERRED_INIT, but that design didn’t work out) > > > >> > >> > >>> > >>> + /* 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? > > Ah, I will check on this. Thanks a lot. > > > >>> > >>> + /* 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? > With -ftrivial-auto-var-init, During gimplification phase, almost all address > taken variables that do not have an explicit initializer will have the above > IR pattern. > > For example, gcc.dg/auto-init-uninit-16.c: > > [opc@qinzhao-ol8u3-x86 gcc]$ cat > /home/opc/Work/GCC/latest-gcc/gcc/testsuite/gcc.dg/auto-init-uninit-16.c > /* { dg-do compile } */ > /* { dg-options "-O2 -Wuninitialized -ftrivial-auto-var-init=zero" } */ > > int foo, bar; > > static > void decode_reloc(int reloc, int *is_alt) > { > if (reloc >= 20) > *is_alt = 1; > else if (reloc >= 10) > *is_alt = 0; > } > > void testfunc() > { > int alt_reloc; > > decode_reloc(foo, &alt_reloc); > > if (alt_reloc) /* { dg-warning "may be used uninitialized" "" } */ > bar = 42; > } > > ****With -ftrivial-auto-var-init=zero, the IR after gimplification is: > > _1 = .DEFERRED_INIT (4, 2, &"alt_reloc"[0]); > alt_reloc = _1; > > And the IR after SSA is similar as the above: > > _1 = .DEFERRED_INIT (4, 2, &"alt_reloc"[0]); > alt_reloc = _1; > > During the early uninitialized analysis phase, the above IR will feed to the > analyzer, we should exclude such > IR from issuing fake warnings.
Yes, but how do we get to a fake warning here? We should eventually run into the _1 def being used by alt_reloc = ...; and then by means of using the string "alt_reloc" warn about an uninitialized use of alt_reloc? Is it the point of the use that is "misdiagnosed"? I was expecting the first patch to fix this. I'll wait for an update to the second patch. Richard. > > > > >> > >>> 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. > > That’s my bad, I realized this problem and separated the original patch into > two separate patch and also added more detailed > Description of the problem, hope this time the patch will be more clearer. > > You have approved the 1st patch. I will update it per your suggestion and > commit to GCC12. > > For the 2nd one, I will fix the concern you raised about “repl_var”, and > resubmit the patch. > > Qing >
