From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp-out2.suse.de (smtp-out2.suse.de [195.135.220.29]) by sourceware.org (Postfix) with ESMTPS id 7CDC63858010 for ; Wed, 18 Aug 2021 07:19:36 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 7CDC63858010 Received: from relay2.suse.de (relay2.suse.de [149.44.160.134]) by smtp-out2.suse.de (Postfix) with ESMTP id 7CDE320059; Wed, 18 Aug 2021 07:19:35 +0000 (UTC) Received: from murzim.suse.de (murzim.suse.de [10.160.4.192]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by relay2.suse.de (Postfix) with ESMTPS id 551CEA3B99; Wed, 18 Aug 2021 07:19:35 +0000 (UTC) Date: Wed, 18 Aug 2021 09:19:35 +0200 (CEST) From: Richard Biener To: Qing Zhao cc: kees Cook , Jakub Jelinek , Richard Sandiford , Nick Alcock via Gcc-patches Subject: Re: [patch][version 6] add -ftrivial-auto-var-init and variable attribute "uninitialized" to gcc In-Reply-To: <7BF55F94-3C5C-416B-A5D2-8D1EFD1AC89B@oracle.com> Message-ID: References: <52E29277-1403-4755-901A-528116C43FB8@oracle.com> <6FD42B95-F73D-4B75-B83A-BAC4925B1714@oracle.com> <9DD34DA7-4CD7-48B7-B5E4-A4107B84F5D8@oracle.com> <7BF55F94-3C5C-416B-A5D2-8D1EFD1AC89B@oracle.com> User-Agent: Alpine 2.21 (LSU 202 2017-01-01) MIME-Version: 1.0 X-Spam-Status: No, score=-4.8 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.4 X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) on server2.sourceware.org Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8BIT X-Content-Filtered-By: Mailman/MimeDel 2.1.29 X-BeenThere: gcc-patches@gcc.gnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gcc-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 18 Aug 2021 07:19:46 -0000 On Tue, 17 Aug 2021, Qing Zhao wrote: > > > > On Aug 17, 2021, at 10:04 AM, Qing Zhao via Gcc-patches wrote: > > > > > > > >> On Aug 16, 2021, at 11:48 AM, Qing Zhao via Gcc-patches wrote: > >> > >>>> From the above IR file after “FRE”, we can see that the major issue with this IR is: > >>>> > >>>> The address taken auto variable “alt_reloc” has been completely replaced by the temporary variable “_1” in all > >>>> the uses of the original “alt_reloc”. > >>> > >>> Well, this can happen with regular code as well, there's no need for > >>> .DEFERRED_INIT. This is the usual problem with reporting uninitialized > >>> uses late. > >>> > >>> IMHO this shouldn't be a blocker. The goal of zero "regressions" wrt > >>> -Wuninitialized isn't really achievable. > >> > >> Okay. Sounds reasonable to me too. > >> > >>> > >>>> The major problem with such IR is, during uninitialized analysis phase, the original use of “alt_reloc” disappeared completely. > >>>> So, the warning cannot be reported. > >>>> > >>>> > >>>> My questions: > >>>> > >>>> 1. Is it possible to get the original “alt_reloc” through the temporary variable “_1” with some available information recorded in the IR? > >>>> 2. If not, then we have to record the relationship between “alt_reloc” and “_1” when the original “alt_reloc” is replaced by “_1” and get such relationship during > >>>> Uninitialized analysis phase. Is this doable? > >>> > >>> Well, you could add a fake argument to .DEFERRED_INIT for the purpose of > >>> diagnostics. The difficulty is to avoid tracking it as actual use so > >>> you could for example pass a string with the declarations name though > >>> this wouldn't give the association with the actual decl. > >> Good suggestion, I can try this a little bit. > > > > I tried this yesterday, added the 4th argument to .DEFERRED_INIT as: > > > > 1st argument: SIZE of the DECL; > > 2nd argument: INIT_TYPE; > > 3rd argument: IS_VLA, 0 NO, 1 YES; > > + 4th argument: The NAME for the DECL; > > > > - as LHS = DEFERRED_INIT (SIZE of the DECL, INIT_TYPE, IS_VLA) > > + as LHS = DEFERRED_INIT (SIZE of the DECL, INIT_TYPE, IS_VLA, NAME) > > > > + tree name_node > > + = build_string_literal (IDENTIFIER_LENGTH (DECL_NAME (decl)), > > + IDENTIFIER_POINTER (DECL_NAME (decl))); > > > > tree call = build_call_expr_internal_loc (UNKNOWN_LOCATION, IFN_DEFERRED_INIT, > > - TREE_TYPE (decl), 3, > > + TREE_TYPE (decl), 4, > > decl_size, init_type_node, > > - is_vla_node); > > + is_vla_node, name_node); > > > > > > And got the following IR in .uninit1 dump: > > > > > > …. > > > > _1 = .DEFERRED_INIT (4, 2, 0, &"alt_reloc"[0]); > > if (_1 != 0) > > …. > > > > > > My questions: > > > > 1. Is “build_string_literal” the correct utility routine to use for this new argument? > > 2. Will Such string literal nodes have potential other impact? > > I tried to get the 4th argument from the call to .DEFERED_INIT during uninitialized variable analysis in tree-ssa-uninit.c: > > @@ -197,18 +197,25 @@ warn_uninit (enum opt_code wc, tree t, tree expr, tree var, > the COMPLEX_EXPRs real part in that case. See PR71581. */ > if (expr == NULL_TREE > && var == NULL_TREE > - && SSA_NAME_VAR (t) == NULL_TREE > - && is_gimple_assign (SSA_NAME_DEF_STMT (t)) > - && gimple_assign_rhs_code (SSA_NAME_DEF_STMT (t)) == COMPLEX_EXPR) > - { > - tree v = gimple_assign_rhs1 (SSA_NAME_DEF_STMT (t)); > - if (TREE_CODE (v) == SSA_NAME > - && has_undefined_value_p (v) > - && zerop (gimple_assign_rhs2 (SSA_NAME_DEF_STMT (t)))) > + && SSA_NAME_VAR (t) == NULL_TREE) > + { > + if (is_gimple_assign (SSA_NAME_DEF_STMT (t)) > + && (gimple_assign_rhs_code (SSA_NAME_DEF_STMT (t)) == COMPLEX_EXPR)) > { > - expr = SSA_NAME_VAR (v); > - var = expr; > + tree v = gimple_assign_rhs1 (SSA_NAME_DEF_STMT (t)); > + if (TREE_CODE (v) == SSA_NAME > + && has_undefined_value_p (v) > + && zerop (gimple_assign_rhs2 (SSA_NAME_DEF_STMT (t)))) > + { > + expr = SSA_NAME_VAR (v); > + var = expr; > + } > } > + else if (gimple_call_internal_p (SSA_NAME_DEF_STMT (t), IFN_DEFERRED_INIT)) > + { > + expr = gimple_call_arg (SSA_NAME_DEF_STMT (t), 3); > + var = expr; > + } > } > > However, this 4th argument is not a regular variable, it’s just an ADDR_EXPR that includes the constant string for the name of > the deleted variable. > If we’d like to report the warning based on this ADDR_EXPR, a complete new code to report the warnings other than the current one that based on > “Variables” need to be added, this might make the code very ugly. > > My questions: > > 1. Is there better way to do this? Adding a variable as extra argument won't work, so no, I don't see a nice way of carrying the extra information. Btw, if you make sure to set the location of the .DEFERRED_INIT call to the DECL_SOURCE_LOCATION of the decl we initialize we should be able to diagnose sth like warning: variable is used uninitialized note: variable declared here and point to the correct declartion point which should reveal the variable name (to the user, not to the compiler). > 1. As you mentioned before, it’s very unrealistic to meet the goal of “zero regression” for -Wuninitialized, can we leave this part of work in a later patch to improve > The warning for “address taken” auto variables? Yes, as said, I'd simply ignore this particular issue for now since I don't see a good way to fix it. Richard.