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

[Uros Bizjak]

On Tue, Apr 10, 2018 at 11:14 AM, Uros Bizjak  wrote:
> On Tue, Apr 10, 2018 at 10:45 AM, Richard Earnshaw (lists)
>  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  

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

Bootstrapped and regression tested on alphaev68-linux-gnu.

Uros.

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

[Uros Bizjak]

On Tue, Apr 10, 2018 at 10:45 AM, Richard Earnshaw (lists)
 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]));
>>
>

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

[Richard Earnshaw (lists)]

On 10/04/18 08:37, Uros Bizjak wrote:
> On Tue, Apr 10, 2018 at 12:26 AM, Jeff Law  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  
>>>
>>> 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  
>>>
>>> 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]));
> 

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

[Uros Bizjak]

On Tue, Apr 10, 2018 at 12:26 AM, Jeff Law  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  
>>
>> 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  
>>
>> 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]));

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

[Jeff Law]

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  
> 
>     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  
> 
>     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