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 B40DC385743A for ; Mon, 12 Jul 2021 17:06:07 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org B40DC385743A 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 AADB821B6A; Mon, 12 Jul 2021 17:06:06 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=suse.cz; s=susede2_rsa; t=1626109566; 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=d1Yxm6H30xUlWajJYjXQDc3HGQ/7lgMUkolTuV//o/8=; b=266z/Dchl9aRG2GSE5ks2sOX/wX3ZJGCnQUbK9HNe31r8EqEKgtKQxyysXng/fusr395J+ Q3+rnwmx7dC2TpKHfliFbnPqiz8OwSzwFWwVJd/cKkNx/s4+JgIBJrQhiywioyIyjjD9i8 d9gzQhZIVklYnjaGN51Z4l4KgutOy6Q= DKIM-Signature: v=1; a=ed25519-sha256; c=relaxed/relaxed; d=suse.cz; s=susede2_ed25519; t=1626109566; 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=d1Yxm6H30xUlWajJYjXQDc3HGQ/7lgMUkolTuV//o/8=; b=SEEpPu9HRN/1anVLqMFutaesXp3pCb2eB+uNUGvVD84RjkQ96tJ9/F1dm11rEZmWKg2DF6 yqOZHDeAdlhqdQAQ== 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 97D47A3B84; Mon, 12 Jul 2021 17:06:06 +0000 (UTC) From: Martin Jambor To: Qing Zhao , Richard Sandiford Cc: 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 In-Reply-To: <1AF776B5-FB73-4900-986C-D46DFA8E72DA@oracle.com> References: <6165EBB7-9F0C-4112-9B81-EEE76F893234@oracle.com> <1AF776B5-FB73-4900-986C-D46DFA8E72DA@oracle.com> User-Agent: Notmuch/0.32.2 (https://notmuchmail.org) Emacs/27.2 (x86_64-suse-linux-gnu) Date: Mon, 12 Jul 2021 19:06:06 +0200 Message-ID: MIME-Version: 1.0 Content-Type: multipart/mixed; boundary="=-=-=" X-Spam-Status: No, score=-10.9 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, 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 17:06:09 -0000 --=-=-= Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable Hi, On Mon, Jul 12 2021, Qing Zhao wrote: >> On Jul 12, 2021, at 2:51 AM, Richard Sandiford wrote: >> >> Martin Jambor writes: >>> On Thu, Jul 08 2021, Qing Zhao wrote: >>>> (Resend this email since the previous one didn=E2=80=99t quote, I chan= ged 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 >>>>>> >>>>>> /* Numbber of components created when splitting aggregate parameter= s. */ >>>>>> 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 =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); >>>>> >>>>> 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= -init >>>> should affect debug-only constructs too. If it doesn't, exmaining rem= oved >>>> 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. I am very flattered that you think I understand debug_binds well :-) (I do= n't) > > I am okay with not generating debug binds here. > > Then I will just delete the part of code that guarded with if (access->g= rp_to_be_debug_replaced)? > But I have done some simple experiments and reached the conclusion the code is never executed and wrong. It can get executed if you change the if statement in build_access_from_expr like I explained in my previous email: static bool build_access_from_expr (tree expr, gimple *stmt, bool write) { struct access *access; access =3D build_access_from_expr_1 (expr, stmt, write); if (access) { /* This means the aggregate is accesses as a whole in a way other t= han an assign statement and thus cannot be removed even if we had a sca= lar replacement for everything. */ if (cannot_scalarize_away_bitmap && !gimple_call_internal_p (stmt, IFN_DEFERRED_INIT)) bitmap_set_bit (cannot_scalarize_away_bitmap, DECL_UID (access->b= ase)); return true; } return false; } But then it ICEs when the operand scanner attempts to grok the debug bind: test.c:43:1: internal compiler error: in get_expr_operands, at tree-ssa-o= perands.c:945 43 | } Gdb reveals the statement is what I suspected: #2 0x000000000156eea6 in operands_scanner::parse_ssa_operands (this=3D0x= 7fffffffdad0) at /home/mjambor/gcc/mine/src/gcc/tree-ssa-operands.c:973 973 get_expr_operands (gimple_debug_bind_get_value_ptr (stmt), (gdb) pgg stmt # DEBUG s2D.1971 =3D> .DEFERRED_INIT (4, 1, 0) It turns out debug binds cannot have CALL_EXPRs as operands. So yes, just do not attempt to handle grp_to_be_debug_replaced accesses in generate_subtree_deferred_init. In fact, if you do that (+ the modification described above) the results seem to be rather good on the attached testcase (which I derived from gcc.dg/guality/pr59776.c), at least on GIMPLE and at least when compiled with -O1 -g. I did not actually attempt to look at the generated dwarf. I have made a simple attempt to print uninitialized (and unused) SRAed structure in gdb with and without -ftrivial-auto-var-init=3Dpattern, but in both cases it just said it was optimized out. Martin --=-=-= Content-Type: text/x-csrc Content-Disposition: inline; filename=sra-uninit-test.c Content-Description: sra-uninit-test.c #include "nop.h" struct S { float f, g; }; __attribute__((noipa)) void foo (struct S *p, int flag) { struct S s1, s2; if (flag) { s1 = *p; s2 = s1; } else { s2 = s1; } *(int *) &s2.f = 0; asm volatile (NOP : : : "memory"); asm volatile (NOP : : : "memory"); s2 = s1; asm volatile (NOP : : : "memory"); asm volatile (NOP : : : "memory"); } int __attribute__((noipa)) getint(void) { return 1; } int main () { struct S x = { 5.0f, 6.0f }; foo (&x, getint()); return 0; } --=-=-=--