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 055BD3857824 for ; Tue, 10 Aug 2021 07:36:41 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 055BD3857824 Received: from relay2.suse.de (relay2.suse.de [149.44.160.134]) by smtp-out2.suse.de (Postfix) with ESMTP id E8CB92006F; Tue, 10 Aug 2021 07:36:39 +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 DF597A3BAD; Tue, 10 Aug 2021 07:36:39 +0000 (UTC) Date: Tue, 10 Aug 2021 09:36:39 +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: <58ADBC0C-9D44-485B-BB5A-B072664B9C4F@oracle.com> 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=-4.7 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, KAM_SHORT, 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 07:36:51 -0000 On Mon, 9 Aug 2021, Qing Zhao wrote: > Hi, Richard, > > Thanks a lot for you review. > > Although these comments are not made on the latest patch (7th version) :-), all the comments are valid since the parts you commented > are not changed in the 7th version. > > > > On Aug 9, 2021, at 9:09 AM, Richard Biener wrote: > > > > On Tue, 27 Jul 2021, Qing Zhao wrote: > > > >> Hi, > >> > >> This is the 6th version of the patch for the new security feature for GCC. > >> > >> I have tested it with bootstrap on both x86 and aarch64, regression testing on both x86 and aarch64. > >> Also compile CPU2017 (running is ongoing), without any issue. (With the fix to bug https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101586). > >> > >> Please take a look and let me know any issue. > > > > +/* Handle an "uninitialized" attribute; arguments as in > > + struct attribute_spec.handler. */ > > + > > +static tree > > +handle_uninitialized_attribute (tree *node, tree name, tree ARG_UNUSED > > (args), > > + int ARG_UNUSED (flags), bool > > *no_add_attrs) > > +{ > > + if (!VAR_P (*node)) > > + { > > + warning (OPT_Wattributes, "%qE attribute ignored", name); > > + *no_add_attrs = true; > > + } > > > > you are documenting this attribute for automatic variables but > > here you allow placement on globals as well (not sure if at this > > point TREE_STATIC / DECL_EXTERNAL are set correctly). > > Right, I should warn when the attribute is placed for globals or static variables. > I will try TREE_STATIC/DECL_EXTERNAL to see whether it’s work or not. > > > > > + /* for languages that do not support BUILT_IN_CLEAR_PADDING, create the > > + function node for padding initialization. */ > > + if (!fn) > > + { > > + tree ftype = build_function_type_list (void_type_node, > > + ptr_type_node, > > > > the "appropriate" place to do this would be > > tree.c:build_common_builtin_nodes > > Sure, will move the creation of function node of BUILT_IN_CLEAR_PADDING for Fortran etc. to tree.c:build_common_builtin_nodes. > > > > > You seem to marshall the is_vla argument as for_auto_init when > > expanding/folding the builtin and there it's used to suppress > > diagnostics (and make covered pieces not initialized?). > > Yes, I added an extra argument “for_auto_init” for “BUILT_IN_CLEAR_PADDING”, this argument is added to suppress errors emitted during folding > BUILT_IN_CLEAR_PADDING for flexible array member . Such errors should Not be emitted when “BUILT_IN_CLEAR_PADDING” is called with compiler automatic initialization. > Please see https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101586, comment #6 from Jakub Jelinek. > > > I suggest > > to re-name is_vla/for_auto_init to something more descriptive. > > Okay, I will. > > > > + gimple_fold_builtin_clear_padding. If FOR_AUTO_INIT, > > + not emit some of the error messages since doing that > > + might confuse the end user. */ > > > > doesn't explain to me whether errors still might be raised or > > what the actual behavior is. > > Okay, will make this more clear in the comments. > > > > > +static gimple * > > +build_deferred_init (tree decl, > > + enum auto_init_type init_type, > > + bool is_vla) > > +{ > > + gcc_assert ((is_vla && TREE_CODE (decl) == WITH_SIZE_EXPR) > > + || (!is_vla && TREE_CODE (decl) != WITH_SIZE_EXPR)); > > > > so the is_vla parameter looks redundant (and the assert dangerous?). > > Either the caller knows it deals with a VLA, then that should be > > passed through - constant sizes can also later appear during > > optimization after all - or is_vla should be determined here > > based on whether the size at gimplification time is constant. > > The routine “build_deferred_init” is ONLY called during gimplification phase by the routine “gimple_add_init_for_auto_var", at this place, > Is_vla should be determined by the caller to check the size of the DECL. If it’s a vla, the “maybe_with_size_expr” will be applied for > DECL to make it to a WITH_SIZE_EXPR. So, the assertion is purely to make sure this at gimplification phase. > > Yes, the size of the VLA decl might become a constant later due to constant propagation, etc. but during the gimplification phase, the assertion should be true. > > > > + /* If the user requests to initialize automatic variables, we > > + should initialize paddings inside the variable. Add a call to > > + __BUILTIN_CLEAR_PADDING (&object, 0, for_auto_init = true) to > > + initialize paddings of object always to zero regardless of > > + INIT_TYPE. */ > > + if (opt_for_fn (current_function_decl, flag_auto_var_init) > > + > AUTO_INIT_UNINITIALIZED > > + && VAR_P (object) > > + && !DECL_EXTERNAL (object) > > + && !TREE_STATIC (object)) > > + gimple_add_padding_init_for_auto_var (object, false, pre_p); > > + return ret; > > > > I think you want to use either auto_var_p (object) or > > auto_var_in_fn_p (object, current_function_decl). Don't you also > > want to check for the 'uninitialized' attribute here? I suggest > > to abstract the check on whether 'object' should be subject > > to autoinit to a helper function. > > Thanks for the suggestion, I will do this. > > > > > > There's another path above this calling gimplify_init_constructor > > for the case of > > > > const struct S x = { ... }; > > struct S y = x; > > > > where it will try to init 'y' from the CTOR directly, it seems you > > do not cover this case. > > Yes, you are right, this case was not covered right now, and this should be covered. > > Looks like that I need to move the “gimple_add_padding_init_for_auto_var” inside the routine “gimplify_init_constructor” to > Cover all the cases. > > > I also think that the above place applies > > to all aggregate assignment statements, not only to INIT_EXPRs? > > > So don't you want to restrict clear-padding emit here? > > You are right, I might need to restrict it Only to INIT_EXPR. > Will update. > > > > > +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. The gimplifier usually does this in gimplify_var_or_parm_decl, but you can of course substitute DECL_VALUE_EXPR yourself if the decl was already gimplified (was it?) > > > Looking at DECL_VALUE_EXPR > > looks quite fragile since that's not sth data dependence honors. > > It looks you only partly gimplify the build init here? All > > DECL_VALUE_EXPRs should have been resolved. > > Don’t quite understand here. you mean that all the “DECL_VALUE_EXPRs” have been resolved at the phase RTL expansion, > So I cannot use this to get the address variable of the VLA? > > (However, my unit testing cases for VLAs are all looks fine). > > > > > + if (is_vla || (!use_register_for_decl (var))) > > ... > > + else > > + { > > + /* If this variable is in a register, use expand_assignment might > > + generate better code. */ > > > > you compute the patter initializer even when not needing it, > > that's wasteful. > > Okay, I will restrict the pattern initializer computation when really needed. > > > It's also quite ugly, IMHO you should > > use can_native_interpret_type_p (var_type) and native_interpret > > a char [] array initialized to the pattern and if > > !can_native_interpret_type_p () go the memset route. > > Thanks for the suggestion. > > Will try this. > > > > > + /* We will not verify the arguments for the calls to .DEFERRED_INIT. > > + Such call is not a real call, just a placeholder for a later > > + initialization during expand phase. > > + This is mainly to avoid assertion failure for the following > > + case: > > + > > + uni_var = .DEFERRED_INIT (var_size, INIT_TYPE, is_vla); > > + foo (&uni_var); > > + > > + in the above, the uninitialized auto variable "uni_var" is > > + addressable, therefore should not be in registers, resulting > > + the assertion failure in the following argument verification. */ > > + if (gimple_call_internal_p (stmt, IFN_DEFERRED_INIT)) > > + return false; > > + > > /* ??? The C frontend passes unpromoted arguments in case it > > didn't see a function declaration before the call. So for now > > leave the call arguments mostly unverified. Once we gimplify > > unit-at-a-time we have a chance to fix this. */ > > > > - for (i = 0; i < gimple_call_num_args (stmt); ++i) > > > > isn't that from the time there was a decl argument to .DEFERRED_INIT? > > You mean this issue is only there when the decl is the first argument (the old design for .DEFERRED_INIT). > With the new design, this issue is not there anymore? I think so, yes - the change should no longer be needed. Ricahrd. > > > > + if (gimple_call_internal_p (stmt, IFN_DEFERRED_INIT)) > > + { > > + tree size_of_arg0 = gimple_call_arg (stmt, 0); > > + tree size_of_lhs = TYPE_SIZE_UNIT (TREE_TYPE (lhs)); > > + tree is_vla_node = gimple_call_arg (stmt, 2); > > + bool is_vla = (bool) TREE_INT_CST_LOW (is_vla_node); > > + > > + if (TREE_CODE (lhs) == SSA_NAME) > > + lhs = SSA_NAME_VAR (lhs); > > + > > > > 'lhs' is not looked at after this, no need to look at SSA_NAME_VAR. > > Okay, will update this. > > > > > > > Thanks and sorry for the delay in reviewing this (again). > > Thanks again for your detailed review and suggestions. > > I will update the patch accordingly and send the updated patch soon. > > Qing > > > > Richard. > > > > > >> Thanks > >> > > -- Richard Biener SUSE Software Solutions Germany GmbH, Maxfeldstrasse 5, 90409 Nuernberg, Germany; GF: Felix Imendörffer; HRB 36809 (AG Nuernberg)