From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from foss.arm.com (foss.arm.com [217.140.110.172]) by sourceware.org (Postfix) with ESMTP id 8757E383600E for ; Mon, 12 Jul 2021 07:51:11 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 8757E383600E Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id 1DD1F1FB; Mon, 12 Jul 2021 00:51:11 -0700 (PDT) Received: from localhost (e121540-lin.manchester.arm.com [10.32.98.126]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 495143F694; Mon, 12 Jul 2021 00:51:10 -0700 (PDT) From: Richard Sandiford To: Martin Jambor Mail-Followup-To: Martin Jambor , Qing Zhao , Richard Biener , kees cook , gcc-patches Qing Zhao via , richard.sandiford@arm.com Cc: Qing Zhao , Richard Biener , kees cook , gcc-patches Qing Zhao via Subject: Re: [patch][version 4]add -ftrivial-auto-var-init and variable attribute "uninitialized" to gcc References: <6165EBB7-9F0C-4112-9B81-EEE76F893234@oracle.com> Date: Mon, 12 Jul 2021 08:51:09 +0100 In-Reply-To: (Martin Jambor's message of "Fri, 09 Jul 2021 18:18:58 +0200") Message-ID: User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/26.3 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable X-Spam-Status: No, score=-12.4 required=5.0 tests=BAYES_00, GIT_PATCH_0, KAM_DMARC_STATUS, 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, 12 Jul 2021 07:51:14 -0000 Martin Jambor writes: > On Thu, Jul 08 2021, Qing Zhao wrote: >> (Resend this email since the previous one didn=E2=80=99t quote, I change= d one >> setting in my mail client, hopefully that can fix this issue). >> >> Hi, Martin, >> >> Thank you for the review and comment. >> >>> On Jul 8, 2021, at 8:29 AM, Martin Jambor wrote: >>>> 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 >>>>=20 >>>> /* 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; >>>>=20 >>>> 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); >>>> } >>>>=20 >>>> + >>>> +/* Generate statements to call .DEFERRED_INIT to initialize scalar re= placements >>>> + of accesses within a subtree ACCESS; all its children, siblings an= d 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 =3D get_access_replacement (access); >>>> + gimple *call >>>> + =3D 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 =3D get_access_replacement (access); >>>> + tree call =3D 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 =3D gimple_build_debug_bind (drepl, call, >>>> + gsi_stmt (*gsi)); >>>> + gsi_insert_before (gsi, ds, GSI_SAME_STMT); >>>=20 >>> 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.) >> >> This part has been discussed during the 2nd version of the patch, but >> I think that more discussion might be necessary. >> >> In the previous discussion, Richard Sandiford mentioned: >> (https://gcc.gnu.org/pipermail/gcc-patches/2021-April/568620.html): >> >> =3D=3D=3D=3D=3D >> >> I guess the thing we need to decide here is whether -ftrivial-auto-var-i= nit >> should affect debug-only constructs too. If it doesn't, exmaining remov= ed >> components in a debugger might show uninitialised values in cases where >> the user was expecting initialised ones. There would be no security >> concern, but it might be surprising. >> >> I think in principle the DRHS can contain a call to DEFERRED_INIT. >> Doing that would probably require further handling elsewhere though. >> >> =3D=3D=3D=3D=3D >> >> I am still not very confident now for this part of the change. > > I see. I still tend to think that with or without the generation of > gimple_build_debug_binds, the debugger would still not display any value > for the component in question. Without it there would be no information > about the component at a any place in code affected by this, with it the > component would be explicitely uninitialized. But OK. FTR, I don't have a strong opinion here. You know the code better than I do, so if you think not generating debug binds is better then let's do that. Thanks, Richard