[PATCH] Improve RTL DSE with -fstack-protector* (PR rtl-optimization/88796)

[PATCH] Improve RTL DSE with -fstack-protector* (PR rtl-optimization/88796)

[Jakub Jelinek]

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?

2019-01-10  Jakub Jelinek  <jakub@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

Re: [PATCH] Improve RTL DSE with -fstack-protector* (PR rtl-optimization/88796)

[Richard Biener]

On January 10, 2019 11:38:55 PM GMT+01:00, Jakub Jelinek <jakub@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  <jakub@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

Re: [PATCH] Improve RTL DSE with -fstack-protector* (PR rtl-optimization/88796)

[Jakub Jelinek]

On Fri, Jan 11, 2019 at 01:53:21PM +0100, Richard Biener wrote:
> >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).

No.  RTL DSE gives up completely in all MEM_VOLATILE_P reads.
  if ((MEM_ALIAS_SET (mem) == ALIAS_SET_MEMORY_BARRIER)
      || (MEM_VOLATILE_P (mem)))
    {
      if (dump_file && (dump_flags & TDF_DETAILS))
        fprintf (dump_file, " adding wild read, volatile or barrier.\n");
      add_wild_read (bb_info);
      insn_info->cannot_delete = true;
      return;
    }
so it doesn't make into the alias oracle in any way, no idea why this has
been added in that form, seems to be a big hammer to me, but it is like that
(we obviously shouldn't try to replace_read those, but otherwise, I'd say
that whether a volatile or non-volatile read kills some store or not doesn't
really depend on whether it is volatile or not, but on the address;
I guess stage4 isn't the right time to change that though, it is this way
since r123530 when dse.c has been added).

Furthermore, the MEM_EXPR isn't always a DECL on which DECL_NONALIASED could be
applied, e.g. on x86_64-linux it is a MEM_REF built for the TLS memory slot.
Those were killing all the stores too.

	Jakub

Re: [PATCH] Improve RTL DSE with -fstack-protector* (PR rtl-optimization/88796)

[Richard Biener]

On Fri, 11 Jan 2019, Jakub Jelinek wrote:

> On Fri, Jan 11, 2019 at 01:53:21PM +0100, Richard Biener wrote:
> > >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).
> 
> No.  RTL DSE gives up completely in all MEM_VOLATILE_P reads.
>   if ((MEM_ALIAS_SET (mem) == ALIAS_SET_MEMORY_BARRIER)
>       || (MEM_VOLATILE_P (mem)))
>     {
>       if (dump_file && (dump_flags & TDF_DETAILS))
>         fprintf (dump_file, " adding wild read, volatile or barrier.\n");
>       add_wild_read (bb_info);
>       insn_info->cannot_delete = true;
>       return;
>     }
> so it doesn't make into the alias oracle in any way, no idea why this has
> been added in that form, seems to be a big hammer to me, but it is like that
> (we obviously shouldn't try to replace_read those, but otherwise, I'd say
> that whether a volatile or non-volatile read kills some store or not doesn't
> really depend on whether it is volatile or not, but on the address;
> I guess stage4 isn't the right time to change that though, it is this way
> since r123530 when dse.c has been added).
> 
> Furthermore, the MEM_EXPR isn't always a DECL on which DECL_NONALIASED could be
> applied, e.g. on x86_64-linux it is a MEM_REF built for the TLS memory slot.
> Those were killing all the stores too.

Ah, OK.

Well, the patch is OK then I suppose.

Thanks,
Richard.