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 7273438618AF for ; Tue, 10 Aug 2021 15:22:37 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 7273438618AF Received: from relay2.suse.de (relay2.suse.de [149.44.160.134]) by smtp-out2.suse.de (Postfix) with ESMTP id 3F9B3200CC; Tue, 10 Aug 2021 15:22:36 +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 35F50A3B8E; Tue, 10 Aug 2021 15:22:36 +0000 (UTC) Date: Tue, 10 Aug 2021 17:22:36 +0200 (CEST) From: Richard Biener To: Qing Zhao cc: Martin Jambor , Jakub Jelinek , Kees Cook , Nick Alcock via Gcc-patches , Richard Biener Subject: Re: [patch][version 6] add -ftrivial-auto-var-init and variable attribute "uninitialized" to gcc In-Reply-To: Message-ID: References: <52E29277-1403-4755-901A-528116C43FB8@oracle.com> <58ADBC0C-9D44-485B-BB5A-B072664B9C4F@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: Tue, 10 Aug 2021 15:22:47 -0000 On Tue, 10 Aug 2021, Qing Zhao wrote: > Hi, > > > On Aug 10, 2021, at 9:16 AM, Richard Biener wrote: > > > > On Tue, 10 Aug 2021, Qing Zhao wrote: > > > >>>>> > >>>>> +static void > >>>>> +expand_DEFERRED_INIT (internal_fn, gcall *stmt) > >>>>> +{ > >>>>> + tree var = gimple_call_lhs (stmt); > >>>>> + tree size_of_var = gimple_call_arg (stmt, 0); > >>>>> + tree vlaaddr = NULL_TREE; > >>>>> + tree var_type = TREE_TYPE (var); > >>>>> + bool is_vla = (bool) TREE_INT_CST_LOW (gimple_call_arg (stmt, 2)); > >>>>> + enum auto_init_type init_type > >>>>> + = (enum auto_init_type) TREE_INT_CST_LOW (gimple_call_arg (stmt, 1)); > >>>>> + > >>>>> + gcc_assert (init_type > AUTO_INIT_UNINITIALIZED); > >>>>> + > >>>>> + /* if this variable is a VLA, get its SIZE and ADDR first. */ > >>>>> + if (is_vla) > >>>>> + { > >>>>> + /* The temporary address variable for this vla should have been > >>>>> + created during gimplification phase. Refer to gimplify_vla_decl > >>>>> + for details. */ > >>>>> + tree var_decl = (TREE_CODE (var) == SSA_NAME) ? > >>>>> + SSA_NAME_VAR (var) : var; > >>>>> + gcc_assert (DECL_HAS_VALUE_EXPR_P (var_decl)); > >>>>> + gcc_assert (TREE_CODE (DECL_VALUE_EXPR (var_decl)) == > >>>>> INDIRECT_REF); > >>>>> + /* Get the address of this vla variable. */ > >>>>> + vlaaddr = TREE_OPERAND (DECL_VALUE_EXPR (var_decl), 0); > >>>>> > >>>>> err - isn't the address of the decl represented by the LHS > >>>>> regardless whether this is a VLA or not? > >>>> > >>>> The LHS of the call to .DEFERRED_INIT is the DECL itself whatever it’s a VLA or not. > >>>> > >>>> In order to create a memset call, we need the Address of this DECL as the first argument. > >>>> If the DECL is not a VLA, we just simply apply “build_fold_addr_expr” on this DECL to get its address, > >>>> However, for VLA, during gimplification phase “gimplify_vla_decl”, we have already created a temporary > >>>> address variable for this DECL, and recorded this address variable with “DECL_VALUE_EXPR(DECL), > >>>> We should use this already created address variable for VLAs. > >>> > >>> So the issue is that the LHS of the .DEFERRED_INIT call is not properly > >>> gimplified. We should not have such decl there but I see we do not > >>> have IL verification that covers this. > >> > >> Don’t quite understand here: do you mean all the LHS of .DEFERRED_INIT call are not properly gimplified, or > >> Only the LHS of .DEFERRED_INIT call for VLA are not properly gimplified? > > > > Especially in the VLA case but likely also in general (though unlikely > > since usually the receiver of initializations are simple enough). I'd > > expect the VLA case end up as > > > > *ptr_to_decl = .DEFERRED_INIT (...); > > > > where *ptr_to_decl is the DECL_VALUE_EXPR of the decl. > > So, for the following small testing case: > > ==== > extern void bar (int); > > void foo(int n) > { > int arr[n]; > bar (arr[2]); > return; > } > ===== > > If I compile it with -ftrivial-auto-var-init=zero -fdump-tree-gimple -S -o auto-init-11.s -fdump-rtl-expand, the *.gimple dump is: > > ===== > void foo (int n) > { > int n.0; > sizetype D.1950; > bitsizetype D.1951; > sizetype D.1952; > bitsizetype D.1953; > sizetype D.1954; > int[0:D.1950] * arr.1; > void * saved_stack.2; > int arr[0:D.1950] [value-expr: *arr.1]; > > saved_stack.2 = __builtin_stack_save (); > try > { > n.0 = n; > _1 = (long int) n.0; > _2 = _1 + -1; > _3 = (sizetype) _2; > D.1950 = _3; > _4 = (sizetype) n.0; > _5 = (bitsizetype) _4; > _6 = _5 * 32; > D.1951 = _6; > _7 = (sizetype) n.0; > _8 = _7 * 4; > D.1952 = _8; > _9 = (sizetype) n.0; > _10 = (bitsizetype) _9; > _11 = _10 * 32; > D.1953 = _11; > _12 = (sizetype) n.0; > _13 = _12 * 4; > D.1954 = _13; > arr.1 = __builtin_alloca_with_align (D.1954, 32); > arr = .DEFERRED_INIT (D.1952, 2, 1); > _14 = (*arr.1)[2]; > bar (_14); > return; > } > finally > { > __builtin_stack_restore (saved_stack.2); > } > } > > ==== > > You think that the above .DEFEERED_INIT is not correct? > It should be: > > *arr.1 = .DEFERRED_INIT (D.1952. 2, 1); > > ? Yes. > > > >> What do you mean by “such” decl? A decl whole “DECL_VALUE_EXPR(DECL)” is valid? > > > > A 'decl' that has a DECL_VALUE_EXPR should not appear in the IL, it should > > always be refered to as its DECL_VALUE_EXPR. > > Okay. I'm going to test diff --git a/gcc/tree-ssa-operands.c b/gcc/tree-ssa-operands.c index ebf7eea3b04..15c73b6d6f4 100644 --- a/gcc/tree-ssa-operands.c +++ b/gcc/tree-ssa-operands.c @@ -799,10 +799,11 @@ operands_scanner::get_expr_operands (tree *expr_p, int flags) flags | opf_not_non_addressable | opf_address_taken); return; - case SSA_NAME: case VAR_DECL: case PARM_DECL: case RESULT_DECL: + gcc_checking_assert (!DECL_HAS_VALUE_EXPR_P (expr)); + case SSA_NAME: case STRING_CST: case CONST_DECL: if (!(flags & opf_address_taken)) which should pass on unmodified trunk (fingers crossing ;)), but it would likely trip on the current -ftrivial-auto-init patch. The issue with the current IL is that nothing keeps arr.1 live and thus the allocation could be DCEd but the .DEFERRED_INIT call would remain, eventually being expanded to zero storage that isn't there. Richard.