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 3D70E38515F0 for ; Mon, 9 Aug 2021 17:14:17 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 3D70E38515F0 Received: from imap1.suse-dmz.suse.de (imap1.suse-dmz.suse.de [192.168.254.73]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature ECDSA (P-521) server-digest SHA512) (No client certificate requested) by smtp-out2.suse.de (Postfix) with ESMTPS id 2CAF01FDFE; Mon, 9 Aug 2021 17:14:16 +0000 (UTC) Received: from imap1.suse-dmz.suse.de (imap1.suse-dmz.suse.de [192.168.254.73]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature ECDSA (P-521) server-digest SHA512) (No client certificate requested) by imap1.suse-dmz.suse.de (Postfix) with ESMTPS id 0D3CF13758; Mon, 9 Aug 2021 17:14:16 +0000 (UTC) Received: from dovecot-director2.suse.de ([192.168.254.65]) by imap1.suse-dmz.suse.de with ESMTPSA id i+piAmhiEWGOMQAAGKfGzw (envelope-from ); Mon, 09 Aug 2021 17:14:16 +0000 Date: Mon, 09 Aug 2021 19:14:12 +0200 From: Richard Biener To: Qing Zhao CC: Martin Jambor , Jakub Jelinek , Kees Cook , Nick Alcock via Gcc-patches , Richard Biener Subject: =?US-ASCII?Q?Re=3A_=5Bpatch=5D=5Bversion_6=5D_add_-ftrivial-auto-var-in?= =?US-ASCII?Q?it_and_variable_attribute_=22uninitialized=22_to_gcc?= User-Agent: K-9 Mail for Android In-Reply-To: <58ADBC0C-9D44-485B-BB5A-B072664B9C4F@oracle.com> References: <52E29277-1403-4755-901A-528116C43FB8@oracle.com> <58ADBC0C-9D44-485B-BB5A-B072664B9C4F@oracle.com> Message-ID: MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable X-Spam-Status: No, score=-5.4 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 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, 09 Aug 2021 17:14:27 -0000 On August 9, 2021 6:38:21 PM GMT+02:00, Qing Zhao wrote: >Hi, Richard, > >Thanks a lot for you review=2E > >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=2E I actually reviewed the 7th patch, just appearantly picked the wrong mail = to reply to=2E=2E=2E=20 > >> On Aug 9, 2021, at 9:09 AM, Richard Biener wrote: >>=20 >> On Tue, 27 Jul 2021, Qing Zhao wrote: >>=20 >>> Hi, >>>=20 >>> This is the 6th version of the patch for the new security feature for = GCC=2E >>>=20 >>> I have tested it with bootstrap on both x86 and aarch64, regression te= sting on both x86 and aarch64=2E >>> Also compile CPU2017 (running is ongoing), without any issue=2E (With = the fix to bug https://gcc=2Egnu=2Eorg/bugzilla/show_bug=2Ecgi?id=3D101586)= =2E >>>=20 >>> Please take a look and let me know any issue=2E >>=20 >> +/* Handle an "uninitialized" attribute; arguments as in >> + struct attribute_spec=2Ehandler=2E */ >> + >> +static tree >> +handle_uninitialized_attribute (tree *node, tree name, tree ARG_UNUSED= =20 >> (args), >> + int ARG_UNUSED (flags), bool=20 >> *no_add_attrs) >> +{ >> + if (!VAR_P (*node)) >> + { >> + warning (OPT_Wattributes, "%qE attribute ignored", name); >> + *no_add_attrs =3D true; >> + } >>=20 >> 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)=2E > >Right, I should warn when the attribute is placed for globals or static v= ariables=2E=20 >I will try TREE_STATIC/DECL_EXTERNAL to see whether it=E2=80=99s work or = not=2E > >>=20 >> + /* for languages that do not support BUILT_IN_CLEAR_PADDING, create = the >> + function node for padding initialization=2E */ >> + if (!fn) >> + { >> + tree ftype =3D build_function_type_list (void_type_node, >> + ptr_type_node, >>=20 >> the "appropriate" place to do this would be=20 >> tree=2Ec:build_common_builtin_nodes > >Sure, will move the creation of function node of BUILT_IN_CLEAR_PADDING = for Fortran etc=2E to tree=2Ec:build_common_builtin_nodes=2E > >>=20 >> 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?)=2E > >Yes, I added an extra argument =E2=80=9Cfor_auto_init=E2=80=9D for =E2=80= =9CBUILT_IN_CLEAR_PADDING=E2=80=9D, this argument is added to suppress erro= rs emitted during folding >BUILT_IN_CLEAR_PADDING for flexible array member =2E Such errors should N= ot be emitted when =E2=80=9CBUILT_IN_CLEAR_PADDING=E2=80=9D is called with = compiler automatic initialization=2E >Please see https://gcc=2Egnu=2Eorg/bugzilla/show_bug=2Ecgi?id=3D101586, c= omment #6 from Jakub Jelinek=2E > >> I suggest >> to re-name is_vla/for_auto_init to something more descriptive=2E > >Okay, I will=2E=20 >>=20 >> + gimple_fold_builtin_clear_padding=2E If FOR_AUTO_INIT, >> + not emit some of the error messages since doing that >> + might confuse the end user=2E */ >>=20 >> doesn't explain to me whether errors still might be raised or >> what the actual behavior is=2E > >Okay, will make this more clear in the comments=2E > >>=20 >> +static gimple * >> +build_deferred_init (tree decl, >> + enum auto_init_type init_type, >> + bool is_vla) >> +{ >> + gcc_assert ((is_vla && TREE_CODE (decl) =3D=3D WITH_SIZE_EXPR) >> + || (!is_vla && TREE_CODE (decl) !=3D WITH_SIZE_EXPR)); >>=20 >> so the is_vla parameter looks redundant (and the assert dangerous?)=2E >> 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=2E > >The routine =E2=80=9Cbuild_deferred_init=E2=80=9D is ONLY called during g= implification phase by the routine =E2=80=9Cgimple_add_init_for_auto_var", = at this place, >Is_vla should be determined by the caller to check the size of the DECL= =2E If it=E2=80=99s a vla, the =E2=80=9Cmaybe_with_size_expr=E2=80=9D will = be applied for >DECL to make it to a WITH_SIZE_EXPR=2E So, the assertion is purely to ma= ke sure this at gimplification phase=2E > >Yes, the size of the VLA decl might become a constant later due to consta= nt propagation, etc=2E but during the gimplification phase, the assertion = should be true=2E >>=20 >> + /* If the user requests to initialize automatic variables, we >> + should initialize paddings inside the variable=2E Add a ca= ll to >> + __BUILTIN_CLEAR_PADDING (&object, 0, for_auto_init =3D tru= e) to >> + initialize paddings of object always to zero regardless of >> + INIT_TYPE=2E */ >> + 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; >>=20 >> I think you want to use either auto_var_p (object) or >> auto_var_in_fn_p (object, current_function_decl)=2E 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=2E > >Thanks for the suggestion, I will do this=2E > > >>=20 >> There's another path above this calling gimplify_init_constructor >> for the case of >>=20 >> const struct S x =3D { =2E=2E=2E }; >> struct S y =3D x; >>=20 >> where it will try to init 'y' from the CTOR directly, it seems you >> do not cover this case=2E > >Yes, you are right, this case was not covered right now, and this should = be covered=2E > >Looks like that I need to move the =E2=80=9Cgimple_add_padding_init_for_a= uto_var=E2=80=9D inside the routine =E2=80=9Cgimplify_init_constructor=E2= =80=9D to >Cover all the cases=2E=20 > >> 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=2E=20 >Will update=2E > >>=20 >> +static void >> +expand_DEFERRED_INIT (internal_fn, gcall *stmt) >> +{ >> + tree var =3D gimple_call_lhs (stmt); >> + tree size_of_var =3D gimple_call_arg (stmt, 0); >> + tree vlaaddr =3D NULL_TREE; >> + tree var_type =3D TREE_TYPE (var); >> + bool is_vla =3D (bool) TREE_INT_CST_LOW (gimple_call_arg (stmt, 2)); >> + enum auto_init_type init_type >> + =3D (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=2E */ >> + if (is_vla) >> + { >> + /* The temporary address variable for this vla should have been >> + created during gimplification phase=2E Refer to gimplify_vla_= decl >> + for details=2E */ >> + tree var_decl =3D (TREE_CODE (var) =3D=3D 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)) =3D=3D=20 >> INDIRECT_REF); >> + /* Get the address of this vla variable=2E */ >> + vlaaddr =3D TREE_OPERAND (DECL_VALUE_EXPR (var_decl), 0); >>=20 >> err - isn't the address of the decl represented by the LHS=20 >> regardless whether this is a VLA or not? > >The LHS of the call to =2EDEFERRED_INIT is the DECL itself whatever it=E2= =80=99s a VLA or not=2E=20 > >In order to create a memset call, we need the Address of this DECL as the= first argument=2E=20 >If the DECL is not a VLA, we just simply apply =E2=80=9Cbuild_fold_addr_e= xpr=E2=80=9D on this DECL to get its address, >However, for VLA, during gimplification phase =E2=80=9Cgimplify_vla_decl= =E2=80=9D, we have already created a temporary >address variable for this DECL, and recorded this address variable with = =E2=80=9CDECL_VALUE_EXPR(DECL),=20 >We should use this already created address variable for VLAs=2E=20 > > >> Looking at DECL_VALUE_EXPR >> looks quite fragile since that's not sth data dependence honors=2E >> It looks you only partly gimplify the build init here? All >> DECL_VALUE_EXPRs should have been resolved=2E > >Don=E2=80=99t quite understand here=2E you mean that all the =E2=80=9CDEC= L_VALUE_EXPRs=E2=80=9D 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)=2E > >>=20 >> + if (is_vla || (!use_register_for_decl (var))) >> =2E=2E=2E >> + else >> + { >> + /* If this variable is in a register, use expand_assignment might >> + generate better code=2E */ >>=20 >> you compute the patter initializer even when not needing it, >> that's wasteful=2E > >Okay, I will restrict the pattern initializer computation when really nee= ded=2E=20 > >> 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=2E > >Thanks for the suggestion=2E=20 > >Will try this=2E=20 > >>=20 >> + /* We will not verify the arguments for the calls to =2EDEFERRED_INI= T=2E >> + Such call is not a real call, just a placeholder for a later >> + initialization during expand phase=2E >> + This is mainly to avoid assertion failure for the following >> + case: >> + >> + uni_var =3D =2EDEFERRED_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=2E = */ >> + 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=2E So for now >> leave the call arguments mostly unverified=2E Once we gimplify >> unit-at-a-time we have a chance to fix this=2E */ >>=20 >> - for (i =3D 0; i < gimple_call_num_args (stmt); ++i) >>=20 >> isn't that from the time there was a decl argument to =2EDEFERRED_INIT? > >You mean this issue is only there when the decl is the first argument (th= e old design for =2EDEFERRED_INIT)=2E >With the new design, this issue is not there anymore? > >>=20 >> + if (gimple_call_internal_p (stmt, IFN_DEFERRED_INIT)) >> + { >> + tree size_of_arg0 =3D gimple_call_arg (stmt, 0); >> + tree size_of_lhs =3D TYPE_SIZE_UNIT (TREE_TYPE (lhs)); >> + tree is_vla_node =3D gimple_call_arg (stmt, 2); >> + bool is_vla =3D (bool) TREE_INT_CST_LOW (is_vla_node); >> + >> + if (TREE_CODE (lhs) =3D=3D SSA_NAME) >> + lhs =3D SSA_NAME_VAR (lhs); >> + >>=20 >> 'lhs' is not looked at after this, no need to look at SSA_NAME_VAR=2E > >Okay, will update this=2E > >>=20 >>=20 >> Thanks and sorry for the delay in reviewing this (again)=2E > >Thanks again for your detailed review and suggestions=2E > >I will update the patch accordingly and send the updated patch soon=2E > >Qing >>=20 >> Richard=2E >>=20 >>=20 >>> Thanks >>>=20 >