From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp-out1.suse.de (smtp-out1.suse.de [195.135.220.28]) by sourceware.org (Postfix) with ESMTPS id 39866385380E for ; Thu, 8 Jul 2021 13:29:47 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 39866385380E Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=suse.cz Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=suse.cz Received: from relay2.suse.de (relay2.suse.de [149.44.160.134]) by smtp-out1.suse.de (Postfix) with ESMTP id 43DBE2237A; Thu, 8 Jul 2021 13:29:46 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=suse.cz; s=susede2_rsa; t=1625750986; h=from:from:reply-to:date:date:message-id:message-id:to:to:cc:cc: mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=Q5ax9dAf1XNwVmwitBL96ATzQAeIcTRp8R97kLwHIVA=; b=yOyxm3mk5SvOwxoksaYpF1mWXFpImP+mYiNlpNih1U6pvJiuEVooOb3NMdWMmZ1a7K/9lD 2JaNM6mTyPPx5UbYtgZX2CGcThPf8OA1k023L+ln301kaCqprGsuXvL/XczjSS425Q8DKo eig/Vl+43up4/ZyJeINu6QOWjRL1+gc= DKIM-Signature: v=1; a=ed25519-sha256; c=relaxed/relaxed; d=suse.cz; s=susede2_ed25519; t=1625750986; h=from:from:reply-to:date:date:message-id:message-id:to:to:cc:cc: mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=Q5ax9dAf1XNwVmwitBL96ATzQAeIcTRp8R97kLwHIVA=; b=gOmEicQmCPGjmUKAPdMBQwLoO2HX8OT55+R5hKfG+fFzsXlIkUpSEJDKTREU1m2WZUJpDM oV4Mwgh2zq7AniAQ== Received: from suse.cz (virgil.suse.cz [10.100.13.50]) (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 19E83A3B84; Thu, 8 Jul 2021 13:29:46 +0000 (UTC) From: Martin Jambor To: Qing Zhao , Richard Biener , Richard Sandiford , kees cook Cc: gcc-patches Qing Zhao via Subject: Re: [patch][version 4]add -ftrivial-auto-var-init and variable attribute "uninitialized" to gcc In-Reply-To: References: User-Agent: Notmuch/0.32.2 (https://notmuchmail.org) Emacs/27.2 (x86_64-suse-linux-gnu) Date: Thu, 08 Jul 2021 15:29:46 +0200 Message-ID: MIME-Version: 1.0 Content-Type: text/plain X-Spam-Status: No, score=-10.7 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 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: Thu, 08 Jul 2021 13:29:49 -0000 Hi, On Wed, Jul 07 2021, Qing Zhao via Gcc-patches wrote: > Hi, > > This is the 4th version of the patch for the new security feature for GCC. I have been following the threads about this feature only very lightly, so please accept my apologies if my comments are about something which has been already discussed, but... [...] > diff --git a/gcc/tree-sra.c b/gcc/tree-sra.c > index c05d22f3e8f1..35051d7c6b96 100644 > --- a/gcc/tree-sra.c > +++ b/gcc/tree-sra.c > @@ -384,6 +384,13 @@ static struct > > /* Numbber of components created when splitting aggregate parameters. */ > int param_reductions_created; > + > + /* Number of deferred_init calls that are modified. */ > + int deferred_init; > + > + /* Number of deferred_init calls that are created by > + generate_subtree_deferred_init. */ > + int subtree_deferred_init; > } sra_stats; > > static void > @@ -4096,6 +4103,110 @@ get_repl_default_def_ssa_name (struct access *racc, tree reg_type) > return get_or_create_ssa_default_def (cfun, racc->replacement_decl); > } > > + > +/* Generate statements to call .DEFERRED_INIT to initialize scalar replacements > + of accesses within a subtree ACCESS; all its children, siblings and their > + children are to be processed. > + GSI is a statement iterator used to place the new statements. */ > +static void > +generate_subtree_deferred_init (struct access *access, > + tree init_type, > + tree is_vla, > + gimple_stmt_iterator *gsi, > + location_t loc) > +{ > + do > + { > + if (access->grp_to_be_replaced) > + { > + tree repl = get_access_replacement (access); > + gimple *call > + = gimple_build_call_internal (IFN_DEFERRED_INIT, 3, > + TYPE_SIZE_UNIT (TREE_TYPE (repl)), > + init_type, is_vla); > + gimple_call_set_lhs (call, repl); > + gsi_insert_before (gsi, call, GSI_SAME_STMT); > + update_stmt (call); > + gimple_set_location (call, loc); > + sra_stats.subtree_deferred_init++; > + } > + else if (access->grp_to_be_debug_replaced) > + { > + tree drepl = get_access_replacement (access); > + tree call = build_call_expr_internal_loc > + (UNKNOWN_LOCATION, IFN_DEFERRED_INIT, > + TREE_TYPE (drepl), 3, > + TYPE_SIZE_UNIT (TREE_TYPE (drepl)), > + init_type, is_vla); > + gdebug *ds = gimple_build_debug_bind (drepl, call, > + gsi_stmt (*gsi)); > + gsi_insert_before (gsi, ds, GSI_SAME_STMT); Is handling of grp_to_be_debug_replaced accesses necessary here? If so, why? grp_to_be_debug_replaced accesses are there only to facilitate debug information about a part of an aggregate decl is that is likely going to be entirely removed - so that debuggers can sometimes show to users information about what they would contain had they not removed. It seems strange you need to mark them as uninitialized because they should not have any consumers. (But perhaps it is also harmless.) On a related note, if the intent of the feature is for optimizers to behave (almost?) as if it was not taking place, I believe you need to handle specially, and probably just ignore, calls to IFN_DEFERRED_INIT in scan_function in tree-sra.c. Otherwise the generated SRA access structures will have extra write flags turned on in them and that will lead to different behavior of the pass. Martin > + } > + if (access->first_child) > + generate_subtree_deferred_init (access->first_child, init_type, > + is_vla, gsi, loc); > + > + access = access ->next_sibling; > + } > + while (access); > +} > + > +/* For a call to .DEFERRED_INIT: > + var = .DEFERRED_INIT (size_of_var, init_type, is_vla); > + examine the LHS variable VAR and replace it with a scalar replacement if > + there is one, also replace the RHS call to a call to .DEFERRED_INIT of > + the corresponding scalar relacement variable. Examine the subtree and > + do the scalar replacements in the subtree too. STMT is the call, GSI is > + the statment iterator to place newly created statement. */ > + > +static enum assignment_mod_result > +sra_modify_deferred_init (gimple *stmt, gimple_stmt_iterator *gsi) > +{ > + tree lhs = gimple_call_lhs (stmt); > + tree init_type = gimple_call_arg (stmt, 1); > + tree is_vla = gimple_call_arg (stmt, 2); > + > + struct access *lhs_access = get_access_for_expr (lhs); > + if (!lhs_access) > + return SRA_AM_NONE; > + > + location_t loc = gimple_location (stmt); > + > + if (lhs_access->grp_to_be_replaced) > + { > + tree lhs_repl = get_access_replacement (lhs_access); > + gimple_call_set_lhs (stmt, lhs_repl); > + tree arg0_repl = TYPE_SIZE_UNIT (TREE_TYPE (lhs_repl)); > + gimple_call_set_arg (stmt, 0, arg0_repl); > + sra_stats.deferred_init++; > + } > + else if (lhs_access->grp_to_be_debug_replaced) > + { > + tree lhs_drepl = get_access_replacement (lhs_access); > + tree call = build_call_expr_internal_loc > + (UNKNOWN_LOCATION, IFN_DEFERRED_INIT, > + TREE_TYPE (lhs_drepl), 3, > + TYPE_SIZE_UNIT (TREE_TYPE (lhs_drepl)), > + init_type, is_vla); > + gdebug *ds = gimple_build_debug_bind (lhs_drepl, call, > + gsi_stmt (*gsi)); > + gsi_insert_before (gsi, ds, GSI_SAME_STMT); > + } > + > + if (lhs_access->first_child) > + generate_subtree_deferred_init (lhs_access->first_child, > + init_type, is_vla, gsi, loc); > + if (lhs_access->grp_covered) > + { > + unlink_stmt_vdef (stmt); > + gsi_remove (gsi, true); > + release_defs (stmt); > + return SRA_AM_REMOVED; > + } > + > + return SRA_AM_MODIFIED; > +} > + > /* Examine both sides of the assignment statement pointed to by STMT, replace > them with a scalare replacement if there is one and generate copying of > replacements if scalarized aggregates have been used in the assignment. GSI > @@ -4460,17 +4571,27 @@ sra_modify_function_body (void) > break; > > case GIMPLE_CALL: > - /* Operands must be processed before the lhs. */ > - for (i = 0; i < gimple_call_num_args (stmt); i++) > + /* Handle calls to .DEFERRED_INIT specially. */ > + if (gimple_call_internal_p (stmt, IFN_DEFERRED_INIT)) > { > - t = gimple_call_arg_ptr (stmt, i); > - modified |= sra_modify_expr (t, &gsi, false); > + assign_result = sra_modify_deferred_init (stmt, &gsi); > + modified |= assign_result == SRA_AM_MODIFIED; > + deleted = assign_result == SRA_AM_REMOVED; > } > - > - if (gimple_call_lhs (stmt)) > + else > { > - t = gimple_call_lhs_ptr (stmt); > - modified |= sra_modify_expr (t, &gsi, true); > + /* Operands must be processed before the lhs. */ > + for (i = 0; i < gimple_call_num_args (stmt); i++) > + { > + t = gimple_call_arg_ptr (stmt, i); > + modified |= sra_modify_expr (t, &gsi, false); > + } > + > + if (gimple_call_lhs (stmt)) > + { > + t = gimple_call_lhs_ptr (stmt); > + modified |= sra_modify_expr (t, &gsi, true); > + } > } > break; >