On January 10, 2019 11:38:55 PM GMT+01:00, Jakub Jelinek <ja...@redhat.com>
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 should
not consider any address aliasing this (well, any with a mem_expr I guess).
Richard.
>2019-01-10 Jakub Jelinek <ja...@redhat.com>
>
> 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;
>
>+ /* 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 = targetm.stack_protect_guard ();
> rtx x, y;
>
>+ crtl->stack_protect_guard_decl = guard_decl;
> x = expand_normal (crtl->stack_protect_guard);
>
> 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 = targetm.stack_protect_guard ();
>+ tree guard_decl = crtl->stack_protect_guard_decl;
> rtx_code_label *label = gen_label_rtx ();
> rtx x, y;
> rtx_insn *seq = 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 = bb_info->last_insn;
>
> if ((MEM_ALIAS_SET (mem) == ALIAS_SET_MEMORY_BARRIER)
>- || (MEM_VOLATILE_P (mem)))
>+ || MEM_VOLATILE_P (mem))
> {
>+ if (crtl->stack_protect_guard
>+ && (MEM_EXPR (mem) == crtl->stack_protect_guard
>+ || (crtl->stack_protect_guard_decl
>+ && MEM_EXPR (mem) == 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 = 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 = build_int_cst (asptrtype, ix86_stack_protector_guard_offset);
> t = build2 (MEM_REF, asptrtype, t,
> build_int_cst (asptrtype, 0));
>+ TREE_THIS_VOLATILE (t) = 1;
> }
>
> 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
