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 1522C3898520 for ; Mon, 16 Aug 2021 07:13:01 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 1522C3898520 Received: from relay2.suse.de (relay2.suse.de [149.44.160.134]) by smtp-out2.suse.de (Postfix) with ESMTP id EF2A31FE7B; Mon, 16 Aug 2021 07:12:59 +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 E6831A3B9E; Mon, 16 Aug 2021 07:12:59 +0000 (UTC) Date: Mon, 16 Aug 2021 09:12:59 +0200 (CEST) From: Richard Biener To: Qing Zhao cc: Jakub Jelinek , Richard Sandiford , Nick Alcock via Gcc-patches , Kees Cook Subject: Re: [patch][version 6] add -ftrivial-auto-var-init and variable attribute "uninitialized" to gcc In-Reply-To: <28A70D41-58C6-49FF-BDA9-661F36A2D795@oracle.com> Message-ID: References: <52E29277-1403-4755-901A-528116C43FB8@oracle.com> <6FD42B95-F73D-4B75-B83A-BAC4925B1714@oracle.com> <28A70D41-58C6-49FF-BDA9-661F36A2D795@oracle.com> User-Agent: Alpine 2.21 (LSU 202 2017-01-01) MIME-Version: 1.0 X-Spam-Status: No, score=-10.8 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, 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: Mon, 16 Aug 2021 07:13:11 -0000 On Wed, 11 Aug 2021, Qing Zhao wrote: > Hi, > > I finally decided to take another approach to resolve this issue, it resolved all the potential issues with the “address taken” auto variable. > > The basic idea is to avoid generating the temporary variable in the beginning. > As you mentioned, "The reason is that alt_reloc is memory (because it is address taken) and that GIMPLE says that register typed stores > need to use a is_gimple_val RHS which the call is not.” > In order to avoid generating the temporary variable for “address taken” auto variable, I updated the utility routine “is_gimple_val” as following: > > diff --git a/gcc/gimple-expr.c b/gcc/gimple-expr.c > index a2563a45c37d..d5ef1aef8cea 100644 > --- a/gcc/gimple-expr.c > +++ b/gcc/gimple-expr.c > @@ -787,8 +787,20 @@ is_gimple_reg (tree t) > return !DECL_NOT_GIMPLE_REG_P (t); > } > > +/* Return true if T is a call to .DEFERRED_INIT internal function. */ > +static bool > +is_deferred_init_call (tree t) > +{ > + if (TREE_CODE (t) == CALL_EXPR > + && CALL_EXPR_IFN (t) == IFN_DEFERRED_INIT) > + return true; > + return false; > +} > + > > -/* Return true if T is a GIMPLE rvalue, i.e. an identifier or a constant. */ > +/* Return true if T is a GIMPLE rvalue, i.e. an identifier or a constant, > + or a call to .DEFERRED_INIT internal function because the call to > + .DEFERRED_INIT will eventually be expanded as a constant. */ > > bool > is_gimple_val (tree t) > @@ -799,7 +811,8 @@ is_gimple_val (tree t) > && !is_gimple_reg (t)) > return false; > > - return (is_gimple_variable (t) || is_gimple_min_invariant (t)); > + return (is_gimple_variable (t) || is_gimple_min_invariant (t) > + || is_deferred_init_call (t)); > } > > With this change, the temporary variable will not be created for “address taken” auto variable, and uninitialized analysis does not need any change. > Everything works well. > > And I believe that treating “call to .DEFERRED_INIT” as “is_gimple_val” is reasonable since this call actually is a constant. > > Let me know if you have any objection on this solution. Yeah, I object to this solution. Richard. > thanks. > > Qing > > > On Aug 11, 2021, at 3:30 PM, Qing Zhao via Gcc-patches wrote: > > > > Hi, > > > > I met another issue for “address taken” auto variable, see below for details: > > > > **** the testing case: (gcc/testsuite/gcc.dg/uninit-16.c) > > > > 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; > > } > > > > ****When compiled with -ftrivial-auto-var-init=zero -O2 -Wuninitialized -fdump-tree-all: > > > > .*************gimple dump: > > > > void testfunc () > > { > > int alt_reloc; > > > > try > > { > > _1 = .DEFERRED_INIT (4, 2, 0); > > alt_reloc = _1; > > foo.0_2 = foo; > > decode_reloc (foo.0_2, &alt_reloc); > > alt_reloc.1_3 = alt_reloc; > > if (alt_reloc.1_3 != 0) goto ; else goto ; > > : > > bar = 42; > > : > > } > > finally > > { > > alt_reloc = {CLOBBER}; > > } > > } > > > > **************fre1 dump: > > > > void testfunc () > > { > > int alt_reloc; > > int _1; > > int foo.0_2; > > > > : > > _1 = .DEFERRED_INIT (4, 2, 0); > > foo.0_2 = foo; > > if (foo.0_2 > 19) > > goto ; [50.00%] > > else > > goto ; [50.00%] > > > > : > > goto ; [100.00%] > > > > : > > if (foo.0_2 > 9) > > goto ; [50.00%] > > else > > goto ; [50.00%] > > > > : > > goto ; [100.00%] > > > > : > > if (_1 != 0) > > goto ; [INV] > > else > > goto ; [INV] > > > > : > > bar = 42; > > > > : > > return; > > > > } > > > > 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”. > > > > 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? > > 3. Looks like that for “address taken” auto variable, if we have to introduce a new temporary variable and split the call to .DEFERRED_INIT into two: > > > > temp = .DEFERRED_INIT (4, 2, 0); > > alt_reloc = temp; > > > > More issues might possible. > > > > Any comments and suggestions on this issue? > > > > Qing > > > > j > >> On Aug 11, 2021, at 11:55 AM, Richard Biener wrote: > >> > >> On August 11, 2021 6:22:00 PM GMT+02:00, Qing Zhao wrote: > >>> > >>> > >>>> On Aug 11, 2021, at 10:53 AM, Richard Biener wrote: > >>>> > >>>> On August 11, 2021 5:30:40 PM GMT+02:00, Qing Zhao wrote: > >>>>> I modified the routine “gimple_add_init_for_auto_var” as the following: > >>>>> ==== > >>>>> /* Generate initialization to automatic variable DECL based on INIT_TYPE. > >>>>> Build a call to internal const function DEFERRED_INIT: > >>>>> 1st argument: SIZE of the DECL; > >>>>> 2nd argument: INIT_TYPE; > >>>>> 3rd argument: IS_VLA, 0 NO, 1 YES; > >>>>> > >>>>> as DEFERRED_INIT (SIZE of the DECL, INIT_TYPE, IS_VLA). */ > >>>>> static void > >>>>> gimple_add_init_for_auto_var (tree decl, > >>>>> enum auto_init_type init_type, > >>>>> bool is_vla, > >>>>> gimple_seq *seq_p) > >>>>> { > >>>>> gcc_assert (VAR_P (decl) && !DECL_EXTERNAL (decl) && !TREE_STATIC (decl)); > >>>>> gcc_assert (init_type > AUTO_INIT_UNINITIALIZED); > >>>>> tree decl_size = TYPE_SIZE_UNIT (TREE_TYPE (decl)); > >>>>> > >>>>> tree init_type_node > >>>>> = build_int_cst (integer_type_node, (int) init_type); > >>>>> tree is_vla_node > >>>>> = build_int_cst (integer_type_node, (int) is_vla); > >>>>> > >>>>> tree call = build_call_expr_internal_loc (UNKNOWN_LOCATION, IFN_DEFERRED_INIT, > >>>>> TREE_TYPE (decl), 3, > >>>>> decl_size, init_type_node, > >>>>> is_vla_node); > >>>>> > >>>>> /* If this DECL is a VLA, a temporary address variable for it has been > >>>>> created, the replacement for DECL is recorded in DECL_VALUE_EXPR (decl), > >>>>> we should use it as the LHS of the call. */ > >>>>> > >>>>> tree lhs_call > >>>>> = is_vla ? DECL_VALUE_EXPR (decl) : decl; > >>>>> gimplify_assign (lhs_call, call, seq_p); > >>>>> } > >>>>> > >>>>> With this change, the current issue is resolved, the gimple dump now is: > >>>>> > >>>>> (*arr.1) = .DEFERRED_INIT (D.1952, 2, 1); > >>>>> > >>>>> However, there is another new issue: > >>>>> > >>>>> For the following testing case: > >>>>> > >>>>> ====== > >>>>> [opc@qinzhao-ol8u3-x86 gcc]$ cat t.c > >>>>> int bar; > >>>>> > >>>>> extern void decode_reloc(int *); > >>>>> > >>>>> void testfunc() > >>>>> { > >>>>> int alt_reloc; > >>>>> > >>>>> decode_reloc(&alt_reloc); > >>>>> > >>>>> if (alt_reloc) /* { dg-warning "may be used uninitialized" } */ > >>>>> bar = 42; > >>>>> } > >>>>> ===== > >>>>> > >>>>> In the above, the auto var “alt_reloc” is address taken, then the gimple dump for it when compiled with -ftrivial-auto-var-init=zero is: > >>>>> > >>>>> void testfunc () > >>>>> { > >>>>> int alt_reloc; > >>>>> > >>>>> try > >>>>> { > >>>>> _1 = .DEFERRED_INIT (4, 2, 0); > >>>>> alt_reloc = _1; > >>>>> decode_reloc (&alt_reloc); > >>>>> alt_reloc.0_2 = alt_reloc; > >>>>> if (alt_reloc.0_2 != 0) goto ; else goto ; > >>>>> : > >>>>> bar = 42; > >>>>> : > >>>>> } > >>>>> finally > >>>>> { > >>>>> alt_reloc = {CLOBBER}; > >>>>> } > >>>>> } > >>>>> > >>>>> I.e, instead of the expected IR: > >>>>> > >>>>> alt_reloc = .DEFERRED_INIT (4, 2, 0); > >>>>> > >>>>> We got the following: > >>>>> > >>>>> _1 = .DEFERRED_INIT (4, 2, 0); > >>>>> alt_reloc = _1; > >>>>> > >>>>> I guess the temp “_1” is created because “alt_reloc” is address taken. > >>>> > >>>> Yes and no. The reason is that alt_reloc is memory (because it is address taken) and that GIMPLE says that register typed stores need to use a is_gimple_val RHS which the call is not. > >>> > >>> Okay. > >>>> > >>>>> My questions: > >>>>> > >>>>> Shall we accept such IR for .DEFERRED_INIT purpose when the auto var is address taken? > >>>> > >>>> I think so. Note it doesn't necessarily need address taking but any other reason that prevents SSA rewriting the variable suffices. > >>> > >>> You mean, in addition to “address taken”, there are other situations that will introduce such IR: > >>> > >>> temp = .DEFERRED_INIT(); > >>> auto_var = temp; > >>> > >>> So, such IR is unavoidable and we have to handle it? > >> > >> Yes. > >> > >>> If we have to handle it, what’ the best way to do it? > >>> > >>> The solution in my mind is: > >>> 1. During uninitialized analysis phase, following the data flow to connect .DEFERRED_INIT to “auto_var”, and then decide that “auto_var” is uninitialized. > >> > >> Yes. Basically if there's an artificial variable auto initialized you have to look at its uses. > >> > >>> 2. During RTL expansion, following the data flow to connect .DEFERRED_INIT to “auto_var”, and then delete “temp”, and then expand .DEFERRED_INIT to auto_var. > >> > >> That shouldn't be necessary. You'd initialize a temporary register which is then copied to the real variable. That's good enough and should be optimized by the RTL pipeline. > >> > >>> Let me know your comments and suggestions on this. > >>> > >>> > >>>> > >>>> The only other option is to force. DEFERED_INIT making the LHS address taken which I think could be achieved by passing it the address as argument instead of having a LHS. But let's not go down this route - it will have quite bad behavior on alias analysis and optimization. > >>> > >>> Okay. > >>> > >>> Qing > >>>> > >>>>> If so, “uninitialized analysis” phase need to be further adjusted to specially handle such IR. > >>>>> > >>>>> If not, what should we do when the auto var is address taken? > > -- Richard Biener SUSE Software Solutions Germany GmbH, Maxfeldstrasse 5, 90409 Nuernberg, Germany; GF: Felix Imendörffer; HRB 36809 (AG Nuernberg)