[explow] PR target/85173: validize memory before passing it on to target probe_stack

Message ID 5AC630B9.1060504@foss.arm.com
State New
Headers show
Series
  • [explow] PR target/85173: validize memory before passing it on to target probe_stack
Related show

Commit Message

Kyrill Tkachov April 5, 2018, 2:20 p.m.
Hi all,

In this PR the expansion code emits an invalid memory address for the stack probe, which the backend fails to recognise.
The address is created explicitly in anti_adjust_stack_and_probe_stack_clash in explow.c and passed down to gen_probe_stack
without any validation in emit_stack_probe.

This patch fixes the ICE by calling validize_mem on the memory location before passing it down to the target.
Jakub pointed out that we also want to create valid addresses for the probe_stack_address case, so this patch
creates an expand operand and legitimizes it before passing it down to the probe_stack_address expander.

This patch passes bootstrap and testing on arm-none-linux-gnueabihf and aarch64-none-linux-gnu
and ppc64le-redhat-linux on gcc112 in the compile farm.

Is this ok for trunk?

Thanks,
Kyrill

P.S. Uros, the alpha probe_stack expander in alpha.md seems incompatible with the way the probe_stack name is
used in the midend. It accepts a const_int operand that is used as an offset from the stack pointer, rather than accepting
a full memory operand like other targets. Do you think it's better to rename the probe_stack pattern there to something
that doesn't conflict with the name the midend assumes?

2018-04-05  Kyrylo Tkachov  <kyrylo.tkachov@arm.com>

     PR target/85173
     * explow.c (emit_stack_probe): Call validize_mem on memory location
     before passing it to gen_probe_stack.  Create address operand and
     legitimize it for the probe_stack_address case.

2018-04-05  Kyrylo Tkachov  <kyrylo.tkachov@arm.com>

     PR target/85173
     * gcc.target/arm/pr85173.c: New test.

Comments

Jeff Law April 9, 2018, 10:26 p.m. | #1
On 04/05/2018 08:20 AM, Kyrill Tkachov wrote:
> Hi all,

> 

> In this PR the expansion code emits an invalid memory address for the

> stack probe, which the backend fails to recognise.

> The address is created explicitly in

> anti_adjust_stack_and_probe_stack_clash in explow.c and passed down to

> gen_probe_stack

> without any validation in emit_stack_probe.

> 

> This patch fixes the ICE by calling validize_mem on the memory location

> before passing it down to the target.

> Jakub pointed out that we also want to create valid addresses for the

> probe_stack_address case, so this patch

> creates an expand operand and legitimizes it before passing it down to

> the probe_stack_address expander.

> 

> This patch passes bootstrap and testing on arm-none-linux-gnueabihf and

> aarch64-none-linux-gnu

> and ppc64le-redhat-linux on gcc112 in the compile farm.

> 

> Is this ok for trunk?

> 

> Thanks,

> Kyrill

> 

> P.S. Uros, the alpha probe_stack expander in alpha.md seems incompatible

> with the way the probe_stack name is

> used in the midend. It accepts a const_int operand that is used as an

> offset from the stack pointer, rather than accepting

> a full memory operand like other targets. Do you think it's better to

> rename the probe_stack pattern there to something

> that doesn't conflict with the name the midend assumes?

> 

> 2018-04-05  Kyrylo Tkachov  <kyrylo.tkachov@arm.com>

> 

>     PR target/85173

>     * explow.c (emit_stack_probe): Call validize_mem on memory location

>     before passing it to gen_probe_stack.  Create address operand and

>     legitimize it for the probe_stack_address case.

> 

> 2018-04-05  Kyrylo Tkachov  <kyrylo.tkachov@arm.com>

> 

>     PR target/85173

>     * gcc.target/arm/pr85173.c: New test.

Alpha should be fixed -- the docs clearly state that the operand is "the
memory reference in the stack that needs to be probed".  Just passing in
the offset seems wrong.

Note that -fstack-clash-protection on ARM (32bit) relies on the old
-fstack-check code which has a variety of issues.  It's better than
nothing, but the real way to go here is to fix prologue generation as
well as alloca/vla handling to have stack clash specific sequences.

OK for the trunk.

jeff
Uros Bizjak April 10, 2018, 7:37 a.m. | #2
On Tue, Apr 10, 2018 at 12:26 AM, Jeff Law <law@redhat.com> wrote:
> On 04/05/2018 08:20 AM, Kyrill Tkachov wrote:

>> Hi all,

>>

>> In this PR the expansion code emits an invalid memory address for the

>> stack probe, which the backend fails to recognise.

>> The address is created explicitly in

>> anti_adjust_stack_and_probe_stack_clash in explow.c and passed down to

>> gen_probe_stack

>> without any validation in emit_stack_probe.

>>

>> This patch fixes the ICE by calling validize_mem on the memory location

>> before passing it down to the target.

>> Jakub pointed out that we also want to create valid addresses for the

>> probe_stack_address case, so this patch

>> creates an expand operand and legitimizes it before passing it down to

>> the probe_stack_address expander.

>>

>> This patch passes bootstrap and testing on arm-none-linux-gnueabihf and

>> aarch64-none-linux-gnu

>> and ppc64le-redhat-linux on gcc112 in the compile farm.

>>

>> Is this ok for trunk?

>>

>> Thanks,

>> Kyrill

>>

>> P.S. Uros, the alpha probe_stack expander in alpha.md seems incompatible

>> with the way the probe_stack name is

>> used in the midend. It accepts a const_int operand that is used as an

>> offset from the stack pointer, rather than accepting

>> a full memory operand like other targets. Do you think it's better to

>> rename the probe_stack pattern there to something

>> that doesn't conflict with the name the midend assumes?

>>

>> 2018-04-05  Kyrylo Tkachov  <kyrylo.tkachov@arm.com>

>>

>>     PR target/85173

>>     * explow.c (emit_stack_probe): Call validize_mem on memory location

>>     before passing it to gen_probe_stack.  Create address operand and

>>     legitimize it for the probe_stack_address case.

>>

>> 2018-04-05  Kyrylo Tkachov  <kyrylo.tkachov@arm.com>

>>

>>     PR target/85173

>>     * gcc.target/arm/pr85173.c: New test.

> Alpha should be fixed -- the docs clearly state that the operand is "the

> memory reference in the stack that needs to be probed".  Just passing in

> the offset seems wrong.


This pattern has to be renamed to not clash with the standard pattern name.

I'm testing the attached patch.

Uros.
diff --git a/gcc/config/alpha/alpha.c b/gcc/config/alpha/alpha.c
index a039f045262c..3222cb716b63 100644
--- a/gcc/config/alpha/alpha.c
+++ b/gcc/config/alpha/alpha.c
@@ -7771,13 +7771,13 @@ alpha_expand_prologue (void)
 	  int probed;
 
 	  for (probed = 4096; probed < probed_size; probed += 8192)
-	    emit_insn (gen_probe_stack (GEN_INT (-probed)));
+	    emit_insn (gen_stack_probe (GEN_INT (-probed)));
 
 	  /* We only have to do this probe if we aren't saving registers or
 	     if we are probing beyond the frame because of -fstack-check.  */
 	  if ((sa_size == 0 && probed_size > probed - 4096)
 	      || flag_stack_check || flag_stack_clash_protection)
-	    emit_insn (gen_probe_stack (GEN_INT (-probed_size)));
+	    emit_insn (gen_stack_probe (GEN_INT (-probed_size)));
 	}
 
       if (frame_size != 0)
diff --git a/gcc/config/alpha/alpha.md b/gcc/config/alpha/alpha.md
index 5d82e5a4adf7..6b99fce643b4 100644
--- a/gcc/config/alpha/alpha.md
+++ b/gcc/config/alpha/alpha.md
@@ -4851,7 +4851,7 @@
 
 
 ;; Subroutine of stack space allocation.  Perform a stack probe.
-(define_expand "probe_stack"
+(define_expand "stack_probe"
   [(set (match_dup 1) (match_operand:DI 0 "const_int_operand"))]
   ""
 {
@@ -4886,12 +4886,12 @@
 
 	  int probed = 4096;
 
-	  emit_insn (gen_probe_stack (GEN_INT (- probed)));
+	  emit_insn (gen_stack_probe (GEN_INT (- probed)));
 	  while (probed + 8192 < INTVAL (operands[1]))
-	    emit_insn (gen_probe_stack (GEN_INT (- (probed += 8192))));
+	    emit_insn (gen_stack_probe (GEN_INT (- (probed += 8192))));
 
 	  if (probed + 4096 < INTVAL (operands[1]))
-	    emit_insn (gen_probe_stack (GEN_INT (- INTVAL(operands[1]))));
+	    emit_insn (gen_stack_probe (GEN_INT (- INTVAL(operands[1]))));
 	}
 
       operands[1] = GEN_INT (- INTVAL (operands[1]));
Richard Earnshaw (lists) April 10, 2018, 8:45 a.m. | #3
On 10/04/18 08:37, Uros Bizjak wrote:
> On Tue, Apr 10, 2018 at 12:26 AM, Jeff Law <law@redhat.com> wrote:

>> On 04/05/2018 08:20 AM, Kyrill Tkachov wrote:

>>> Hi all,

>>>

>>> In this PR the expansion code emits an invalid memory address for the

>>> stack probe, which the backend fails to recognise.

>>> The address is created explicitly in

>>> anti_adjust_stack_and_probe_stack_clash in explow.c and passed down to

>>> gen_probe_stack

>>> without any validation in emit_stack_probe.

>>>

>>> This patch fixes the ICE by calling validize_mem on the memory location

>>> before passing it down to the target.

>>> Jakub pointed out that we also want to create valid addresses for the

>>> probe_stack_address case, so this patch

>>> creates an expand operand and legitimizes it before passing it down to

>>> the probe_stack_address expander.

>>>

>>> This patch passes bootstrap and testing on arm-none-linux-gnueabihf and

>>> aarch64-none-linux-gnu

>>> and ppc64le-redhat-linux on gcc112 in the compile farm.

>>>

>>> Is this ok for trunk?

>>>

>>> Thanks,

>>> Kyrill

>>>

>>> P.S. Uros, the alpha probe_stack expander in alpha.md seems incompatible

>>> with the way the probe_stack name is

>>> used in the midend. It accepts a const_int operand that is used as an

>>> offset from the stack pointer, rather than accepting

>>> a full memory operand like other targets. Do you think it's better to

>>> rename the probe_stack pattern there to something

>>> that doesn't conflict with the name the midend assumes?

>>>

>>> 2018-04-05  Kyrylo Tkachov  <kyrylo.tkachov@arm.com>

>>>

>>>     PR target/85173

>>>     * explow.c (emit_stack_probe): Call validize_mem on memory location

>>>     before passing it to gen_probe_stack.  Create address operand and

>>>     legitimize it for the probe_stack_address case.

>>>

>>> 2018-04-05  Kyrylo Tkachov  <kyrylo.tkachov@arm.com>

>>>

>>>     PR target/85173

>>>     * gcc.target/arm/pr85173.c: New test.

>> Alpha should be fixed -- the docs clearly state that the operand is "the

>> memory reference in the stack that needs to be probed".  Just passing in

>> the offset seems wrong.

> 

> This pattern has to be renamed to not clash with the standard pattern name.

> 

> I'm testing the attached patch.

> 


Ugh!  Two different APIs, one called gen_stack_probe and one
gen_probe_stack?  That has to be a recipe for disaster!

R.

> Uros.

> 

> 

> a.diff.txt

> 

> 

> diff --git a/gcc/config/alpha/alpha.c b/gcc/config/alpha/alpha.c

> index a039f045262c..3222cb716b63 100644

> --- a/gcc/config/alpha/alpha.c

> +++ b/gcc/config/alpha/alpha.c

> @@ -7771,13 +7771,13 @@ alpha_expand_prologue (void)

>  	  int probed;

>  

>  	  for (probed = 4096; probed < probed_size; probed += 8192)

> -	    emit_insn (gen_probe_stack (GEN_INT (-probed)));

> +	    emit_insn (gen_stack_probe (GEN_INT (-probed)));

>  

>  	  /* We only have to do this probe if we aren't saving registers or

>  	     if we are probing beyond the frame because of -fstack-check.  */

>  	  if ((sa_size == 0 && probed_size > probed - 4096)

>  	      || flag_stack_check || flag_stack_clash_protection)

> -	    emit_insn (gen_probe_stack (GEN_INT (-probed_size)));

> +	    emit_insn (gen_stack_probe (GEN_INT (-probed_size)));

>  	}

>  

>        if (frame_size != 0)

> diff --git a/gcc/config/alpha/alpha.md b/gcc/config/alpha/alpha.md

> index 5d82e5a4adf7..6b99fce643b4 100644

> --- a/gcc/config/alpha/alpha.md

> +++ b/gcc/config/alpha/alpha.md

> @@ -4851,7 +4851,7 @@

>  

>  

>  ;; Subroutine of stack space allocation.  Perform a stack probe.

> -(define_expand "probe_stack"

> +(define_expand "stack_probe"

>    [(set (match_dup 1) (match_operand:DI 0 "const_int_operand"))]

>    ""

>  {

> @@ -4886,12 +4886,12 @@

>  

>  	  int probed = 4096;

>  

> -	  emit_insn (gen_probe_stack (GEN_INT (- probed)));

> +	  emit_insn (gen_stack_probe (GEN_INT (- probed)));

>  	  while (probed + 8192 < INTVAL (operands[1]))

> -	    emit_insn (gen_probe_stack (GEN_INT (- (probed += 8192))));

> +	    emit_insn (gen_stack_probe (GEN_INT (- (probed += 8192))));

>  

>  	  if (probed + 4096 < INTVAL (operands[1]))

> -	    emit_insn (gen_probe_stack (GEN_INT (- INTVAL(operands[1]))));

> +	    emit_insn (gen_stack_probe (GEN_INT (- INTVAL(operands[1]))));

>  	}

>  

>        operands[1] = GEN_INT (- INTVAL (operands[1]));

>
Uros Bizjak April 10, 2018, 9:14 a.m. | #4
On Tue, Apr 10, 2018 at 10:45 AM, Richard Earnshaw (lists)
<Richard.Earnshaw@arm.com> wrote:

>>> Alpha should be fixed -- the docs clearly state that the operand is "the

>>> memory reference in the stack that needs to be probed".  Just passing in

>>> the offset seems wrong.

>>

>> This pattern has to be renamed to not clash with the standard pattern name.

>>

>> I'm testing the attached patch.

>>

>

> Ugh!  Two different APIs, one called gen_stack_probe and one

> gen_probe_stack?  That has to be a recipe for disaster!


It is just an unforunatelly named helper expander. Maybe
"stack_probe_internal" is indeed a bettern name.

Uros.

>

> R.

>

>> Uros.

>>

>>

>> a.diff.txt

>>

>>

>> diff --git a/gcc/config/alpha/alpha.c b/gcc/config/alpha/alpha.c

>> index a039f045262c..3222cb716b63 100644

>> --- a/gcc/config/alpha/alpha.c

>> +++ b/gcc/config/alpha/alpha.c

>> @@ -7771,13 +7771,13 @@ alpha_expand_prologue (void)

>>         int probed;

>>

>>         for (probed = 4096; probed < probed_size; probed += 8192)

>> -         emit_insn (gen_probe_stack (GEN_INT (-probed)));

>> +         emit_insn (gen_stack_probe (GEN_INT (-probed)));

>>

>>         /* We only have to do this probe if we aren't saving registers or

>>            if we are probing beyond the frame because of -fstack-check.  */

>>         if ((sa_size == 0 && probed_size > probed - 4096)

>>             || flag_stack_check || flag_stack_clash_protection)

>> -         emit_insn (gen_probe_stack (GEN_INT (-probed_size)));

>> +         emit_insn (gen_stack_probe (GEN_INT (-probed_size)));

>>       }

>>

>>        if (frame_size != 0)

>> diff --git a/gcc/config/alpha/alpha.md b/gcc/config/alpha/alpha.md

>> index 5d82e5a4adf7..6b99fce643b4 100644

>> --- a/gcc/config/alpha/alpha.md

>> +++ b/gcc/config/alpha/alpha.md

>> @@ -4851,7 +4851,7 @@

>>

>>

>>  ;; Subroutine of stack space allocation.  Perform a stack probe.

>> -(define_expand "probe_stack"

>> +(define_expand "stack_probe"

>>    [(set (match_dup 1) (match_operand:DI 0 "const_int_operand"))]

>>    ""

>>  {

>> @@ -4886,12 +4886,12 @@

>>

>>         int probed = 4096;

>>

>> -       emit_insn (gen_probe_stack (GEN_INT (- probed)));

>> +       emit_insn (gen_stack_probe (GEN_INT (- probed)));

>>         while (probed + 8192 < INTVAL (operands[1]))

>> -         emit_insn (gen_probe_stack (GEN_INT (- (probed += 8192))));

>> +         emit_insn (gen_stack_probe (GEN_INT (- (probed += 8192))));

>>

>>         if (probed + 4096 < INTVAL (operands[1]))

>> -         emit_insn (gen_probe_stack (GEN_INT (- INTVAL(operands[1]))));

>> +         emit_insn (gen_stack_probe (GEN_INT (- INTVAL(operands[1]))));

>>       }

>>

>>        operands[1] = GEN_INT (- INTVAL (operands[1]));

>>

>
Uros Bizjak April 11, 2018, 3:08 p.m. | #5
On Tue, Apr 10, 2018 at 11:14 AM, Uros Bizjak <ubizjak@gmail.com> wrote:
> On Tue, Apr 10, 2018 at 10:45 AM, Richard Earnshaw (lists)

> <Richard.Earnshaw@arm.com> wrote:

>

>>>> Alpha should be fixed -- the docs clearly state that the operand is "the

>>>> memory reference in the stack that needs to be probed".  Just passing in

>>>> the offset seems wrong.

>>>

>>> This pattern has to be renamed to not clash with the standard pattern name.

>>>

>>> I'm testing the attached patch.

>>>

>>

>> Ugh!  Two different APIs, one called gen_stack_probe and one

>> gen_probe_stack?  That has to be a recipe for disaster!

>

> It is just an unforunatelly named helper expander. Maybe

> "stack_probe_internal" is indeed a bettern name.


Now committed with the above change and following ChangeLog entry:

2018-04-11  Uros Bizjak  <ubizjak@gmail.com>

    * config/alpha/alpha.md (stack_probe_internal): Rename
    from "probe_stack".  Update all callers.

Bootstrapped and regression tested on alphaev68-linux-gnu.

Uros.

Patch

commit dc4e225eb394eaba8765425c0c7076c13d107580
Author: Kyrylo Tkachov <kyrylo.tkachov@arm.com>
Date:   Tue Apr 3 16:46:15 2018 +0100

    [explow] PR target/85173: validize memory before passing it on to target probe_stack

diff --git a/gcc/explow.c b/gcc/explow.c
index 042e719..fb2b7ff 100644
--- a/gcc/explow.c
+++ b/gcc/explow.c
@@ -1626,18 +1626,25 @@  void
 emit_stack_probe (rtx address)
 {
   if (targetm.have_probe_stack_address ())
-    emit_insn (targetm.gen_probe_stack_address (address));
+    {
+      struct expand_operand ops[1];
+      insn_code icode = targetm.code_for_probe_stack_address;
+      create_address_operand (ops, address);
+      maybe_legitimize_operands (icode, 0, 1, ops);
+      expand_insn (icode, 1, ops);
+    }
   else
     {
       rtx memref = gen_rtx_MEM (word_mode, address);
 
       MEM_VOLATILE_P (memref) = 1;
+      memref = validize_mem (memref);
 
       /* See if we have an insn to probe the stack.  */
       if (targetm.have_probe_stack ())
-        emit_insn (targetm.gen_probe_stack (memref));
+	emit_insn (targetm.gen_probe_stack (memref));
       else
-        emit_move_insn (memref, const0_rtx);
+	emit_move_insn (memref, const0_rtx);
     }
 }
 
diff --git a/gcc/testsuite/gcc.target/arm/pr85173.c b/gcc/testsuite/gcc.target/arm/pr85173.c
new file mode 100644
index 0000000..36105c9
--- /dev/null
+++ b/gcc/testsuite/gcc.target/arm/pr85173.c
@@ -0,0 +1,20 @@ 
+/* PR target/85173.  */
+
+/* { dg-do compile } */
+/* { dg-options "-O2 -fstack-clash-protection --param stack-clash-protection-probe-interval=14" } */
+/* { dg-require-effective-target arm_thumb2_ok } */
+
+__attribute__((noinline, noclone)) void
+foo (char *p)
+{
+  asm volatile ("" : : "r" (p) : "memory");
+}
+
+/* Nonconstant alloca, small local frame.  */
+__attribute__((noinline, noclone)) void
+f5 (int x)
+{
+  char locals[128];
+  char *vla = __builtin_alloca (x);
+  foo (vla);
+}