From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 85468 invoked by alias); 11 Jan 2019 12:53:29 -0000 Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org Received: (qmail 85455 invoked by uid 89); 11 Jan 2019 12:53:29 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-11.9 required=5.0 tests=BAYES_00,GIT_PATCH_2,GIT_PATCH_3,SPF_PASS autolearn=ham version=3.3.2 spammy= X-HELO: mx1.suse.de Received: from mx2.suse.de (HELO mx1.suse.de) (195.135.220.15) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Fri, 11 Jan 2019 12:53:26 +0000 Received: from relay1.suse.de (unknown [195.135.220.254]) by mx1.suse.de (Postfix) with ESMTP id 6DD53B023; Fri, 11 Jan 2019 12:53:24 +0000 (UTC) Date: Fri, 11 Jan 2019 12:53:00 -0000 User-Agent: K-9 Mail for Android In-Reply-To: <20190110223855.GD30353@tucnak> References: <20190110223855.GD30353@tucnak> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable Subject: Re: [PATCH] Improve RTL DSE with -fstack-protector* (PR rtl-optimization/88796) To: Jakub Jelinek ,Jeff Law CC: gcc-patches@gcc.gnu.org From: Richard Biener Message-ID: <3BF1AE41-FA1C-40F9-A850-F849603E957D@suse.de> X-SW-Source: 2019-01/txt/msg00626.txt.bz2 On January 10, 2019 11:38:55 PM GMT+01:00, Jakub Jelinek = wrote: >Hi! > >As mentioned in the PR, RTL DSE doesn't do much with >-fstack-protector*, >because the stack canary test in the epilogue of instrumented functions >is a MEM_VOLATILE_P read out of the crtl->stack_protect_guard ssp >canary >slot in the stack frame and either a MEM_VOLATILE_P read of >__stack_chk_guard variable, or corresponding some other location (e.g. >TLS >memory on x86). > >The canary slot in the stack frame is written in the prologue using >MEM_VOLATILE_P store, so we never consider those to be DSEd and is only >read >in the epilogue, so it shouldn't alias any other stores. >Similarly, __stack_chk_guard variable or say the TLS ssp slot or >whatever >else is used to hold the random pointer-sized value really shouldn't be >changed in -fstack-protector* instrumented functions, as that would >mean >they remembered one value in the prologue and would fail comparison in >the >epilogue if it changed in between. So, I believe we can safely ignore >the >whole stack_pointer_test instruction in RTL DSE. > >Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk? Isn't it enough to have the decl marked DECL_NONALIASED? Alias analysis sho= uld not consider any address aliasing this (well, any with a mem_expr I gue= ss).=20 Richard.=20 >2019-01-10 Jakub Jelinek > > PR rtl-optimization/88796 > * emit-rtl.h (struct rtl_data): Add stack_protect_guard_decl field. > * cfgexpand.c (stack_protect_prologue): Initialize > crtl->stack_protect_guard_decl. > * function.c (stack_protect_epilogue): Use it instead of calling > targetm.stack_protect_guard again. > * dse.c (check_mem_read_rtx): Ignore MEM_VOLATILE_P reads from > MEMs with MEM_EXPR equal to crtl->stack_protect_guard or > crtl->stack_protect_guard_decl. > * config/i386/i386.c (ix86_stack_protect_guard): Set >TREE_THIS_VOLATILE > on the returned MEM_EXPR. > > * gcc.target/i386/pr88796.c: New test. > >--- gcc/emit-rtl.h.jj 2019-01-10 11:43:14.390377646 +0100 >+++ gcc/emit-rtl.h 2019-01-10 21:38:38.682055891 +0100 >@@ -87,6 +87,10 @@ struct GTY(()) rtl_data { > Used for detecting stack clobbers. */ > tree stack_protect_guard; >=20 >+ /* The __stack_chk_guard variable or expression holding the stack >+ protector canary value. */ >+ tree stack_protect_guard_decl; >+ >/* List (chain of INSN_LIST) of labels heading the current handlers for > nonlocal gotos. */ > rtx_insn_list *x_nonlocal_goto_handler_labels; >--- gcc/cfgexpand.c.jj 2019-01-07 09:50:26.774650762 +0100 >+++ gcc/cfgexpand.c 2019-01-10 21:40:08.714589919 +0100 >@@ -6219,6 +6219,7 @@ stack_protect_prologue (void) > tree guard_decl =3D targetm.stack_protect_guard (); > rtx x, y; >=20 >+ crtl->stack_protect_guard_decl =3D guard_decl; > x =3D expand_normal (crtl->stack_protect_guard); >=20 > if (targetm.have_stack_protect_combined_set () && guard_decl) >--- gcc/function.c.jj 2019-01-10 16:43:54.802481070 +0100 >+++ gcc/function.c 2019-01-10 21:40:49.326928642 +0100 >@@ -4902,7 +4902,7 @@ init_function_start (tree subr) > void > stack_protect_epilogue (void) > { >- tree guard_decl =3D targetm.stack_protect_guard (); >+ tree guard_decl =3D crtl->stack_protect_guard_decl; > rtx_code_label *label =3D gen_label_rtx (); > rtx x, y; > rtx_insn *seq =3D NULL; >--- gcc/dse.c.jj 2019-01-10 11:43:12.345411240 +0100 >+++ gcc/dse.c 2019-01-10 21:48:07.224799798 +0100 >@@ -2072,8 +2072,29 @@ check_mem_read_rtx (rtx *loc, bb_info_t > insn_info =3D bb_info->last_insn; >=20 > if ((MEM_ALIAS_SET (mem) =3D=3D ALIAS_SET_MEMORY_BARRIER) >- || (MEM_VOLATILE_P (mem))) >+ || MEM_VOLATILE_P (mem)) > { >+ if (crtl->stack_protect_guard >+ && (MEM_EXPR (mem) =3D=3D crtl->stack_protect_guard >+ || (crtl->stack_protect_guard_decl >+ && MEM_EXPR (mem) =3D=3D crtl->stack_protect_guard_decl)) >+ && MEM_VOLATILE_P (mem)) >+ { >+ /* This is either the stack protector canary on the stack, >+ which ought to be written by a MEM_VOLATILE_P store and >+ thus shouldn't be deleted and is read at the very end of >+ function, but shouldn't conflict with any other store. >+ Or it is __stack_chk_guard variable or TLS or whatever else >+ MEM holding the canary value, which really shouldn't be >+ ever modified in -fstack-protector* protected functions, >+ otherwise the prologue store wouldn't match the epilogue >+ check. */ >+ if (dump_file && (dump_flags & TDF_DETAILS)) >+ fprintf (dump_file, " stack protector canary read ignored.\n"); >+ insn_info->cannot_delete =3D true; >+ return; >+ } >+ > if (dump_file && (dump_flags & TDF_DETAILS)) > fprintf (dump_file, " adding wild read, volatile or barrier.\n"); > add_wild_read (bb_info); >--- gcc/config/i386/i386.c.jj 2019-01-10 11:43:17.534325998 +0100 >+++ gcc/config/i386/i386.c 2019-01-10 21:35:39.588972002 +0100 >@@ -45093,6 +45093,7 @@ ix86_stack_protect_guard (void) > t =3D build_int_cst (asptrtype, ix86_stack_protector_guard_offset); > t =3D build2 (MEM_REF, asptrtype, t, > build_int_cst (asptrtype, 0)); >+ TREE_THIS_VOLATILE (t) =3D 1; > } >=20 > return t; >--- gcc/testsuite/gcc.target/i386/pr88796.c.jj 2019-01-10 >21:58:48.878354306 +0100 >+++ gcc/testsuite/gcc.target/i386/pr88796.c 2019-01-10 >21:58:42.468458654 +0100 >@@ -0,0 +1,8 @@ >+/* PR rtl-optimization/88796 */ >+/* { dg-do compile { target { ! ia32 } } } */ >+/* { dg-options "-O2 -fstack-protector-strong" } */ >+/* { dg-require-effective-target fstack_protector } */ >+ >+#include "pr87370.c" >+ >+/* { dg-final { scan-assembler-not "xmm" } } */ > > Jakub