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

["Richard Earnshaw (lists)" <Richard dot Earnshaw at arm dot com>]

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]));
>