| 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 | expand |
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
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]));
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])); >
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])); >> >
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.
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);
+}
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.