Re: [PATCH AArch64]: Add constraint letter for stack_protect_test pattern)
On 18 September 2014 11:15, James Greenhalgh james.greenha...@arm.com wrote: gcc/ 2014-09-18 James Greenhalgh james.greenha...@arm.com * config/aarch64/aarch64.md (stack_protect_test_mode): Mark scratch register as an output to placate register renaming. gcc/testsuite/ 2014-09-18 James Greenhalgh james.greenha...@arm.com * gcc.dg/ssp-3.c: New. * gcc.dg/ssp-4.c: Likewise. OK, thanks /Marcus
Re: [PATCH AArch64]: Add constraint letter for stack_protect_test pattern)
On 17 September 2014 15:43, James Greenhalgh james.greenha...@arm.com wrote: On Wed, Sep 17, 2014 at 09:30:31AM +0100, Richard Earnshaw wrote: =r is correct for an early-clobbered scratch. R. In that case... How is the attached patch for trunk? I've bootstrapped it on AArch64 with -fstack-protector-strong and -frename-registers in the BOOT_CFLAGS without seeing any issues. OK? Thanks, James --- gcc/ 2014-09-15 James Greenhalgh james.greenha...@arm.com * config/aarch64/aarch64.md (stack_protect_test_mode): Mark scratch register as an output to placate register renaming. OK for this part. gcc/testsuite/ 2014-09-15 James Greenhalgh james.greenha...@arm.com * gcc.target/aarch64/stack_protector_set_1.c: New. * gcc.target/aarch64/stack_protector_set_2.c: Likewise. I agree with Andrew, these don't need to be aarch64 specific. /Marcus
Re: [PATCH AArch64]: Add constraint letter for stack_protect_test pattern)
On Thu, Sep 18, 2014 at 09:18:53AM +0100, Marcus Shawcroft wrote:
gcc/testsuite/
2014-09-15 James Greenhalgh james.greenha...@arm.com
* gcc.target/aarch64/stack_protector_set_1.c: New.
* gcc.target/aarch64/stack_protector_set_2.c: Likewise.
I agree with Andrew, these don't need to be aarch64 specific.
Well, guess the 16 needs to be replaced with sizeof buffer, because
sizeof (unsigned int) is not 4 on all architectures.
And
/* { dg-require-effective-target fstack_protector } */
is needed, not all targets support -fstack-protector*.
Jakub
Re: [PATCH AArch64]: Add constraint letter for stack_protect_test pattern)
On Wed, Sep 17, 2014 at 03:50:55PM +0100, pins...@gmail.com wrote:
On Sep 17, 2014, at 7:43 AM, James Greenhalgh james.greenha...@arm.com
wrote:
On Wed, Sep 17, 2014 at 09:30:31AM +0100, Richard Earnshaw wrote:
=r is correct for an early-clobbered scratch.
R.
In that case...
How is the attached patch for trunk? I've bootstrapped it on AArch64
with -fstack-protector-strong and -frename-registers in the BOOT_CFLAGS
without seeing any issues.
There is nothing aarch64 specific about this testcase so I would place them
under gcc.dg and add the extra marker which says this testcase requires stack
protector.
That sounds reasonable to me. Updated as attached, along with Jakub's
suggestions.
And maybe even use compile instead of just assemble too.
Compile is weaker than assemble. Assemble takes you up to an object file,
which is as far as we need to go.
Thanks,
James
---
gcc/
2014-09-18 James Greenhalgh james.greenha...@arm.com
* config/aarch64/aarch64.md (stack_protect_test_mode): Mark
scratch register as an output to placate register renaming.
gcc/testsuite/
2014-09-18 James Greenhalgh james.greenha...@arm.com
* gcc.dg/ssp-3.c: New.
* gcc.dg/ssp-4.c: Likewise.
diff --git a/gcc/config/aarch64/aarch64.md b/gcc/config/aarch64/aarch64.md
index c60038a9015d614f40f6d9e3fd228ad3e2b247a8..f15a516bb0559c86bea7512f91d60dc179ec9149 100644
--- a/gcc/config/aarch64/aarch64.md
+++ b/gcc/config/aarch64/aarch64.md
@@ -4031,7 +4031,7 @@ (define_insn stack_protect_test_mode
(unspec:PTR [(match_operand:PTR 1 memory_operand m)
(match_operand:PTR 2 memory_operand m)]
UNSPEC_SP_TEST))
- (clobber (match_scratch:PTR 3 r))]
+ (clobber (match_scratch:PTR 3 =r))]
ldr\t%w3, %x1\;ldr\t%w0, %x2\;eor\t%w0, %w3, %w0
[(set_attr length 12)
diff --git a/gcc/testsuite/gcc.dg/ssp-3.c b/gcc/testsuite/gcc.dg/ssp-3.c
new file mode 100644
index 000..98c12da
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/ssp-3.c
@@ -0,0 +1,16 @@
+/* { dg-do assemble } */
+/* { dg-options -fstack-protector-strong -O1 -frename-registers } */
+/* { dg-require-effective-target fstack_protector } */
+
+extern int bar (const char *s, int *argc);
+extern int baz (const char *s);
+
+char
+foo (const char *s)
+{
+ int argc;
+ int ret;
+ if ( !bar (s, argc))
+ret = baz (s);
+ return *s;
+}
diff --git a/gcc/testsuite/gcc.dg/ssp-4.c b/gcc/testsuite/gcc.dg/ssp-4.c
new file mode 100644
index 000..402033c
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/ssp-4.c
@@ -0,0 +1,18 @@
+/* { dg-do assemble } */
+/* { dg-options -fstack-protector-strong -O1 -frename-registers } */
+/* { dg-require-effective-target fstack_protector } */
+
+typedef unsigned int uint32_t;
+struct ctx
+{
+ uint32_t A;
+};
+
+void *
+buffer_copy (const struct ctx *ctx, void *resbuf)
+{
+ uint32_t buffer[4];
+ buffer[0] = (ctx-A);
+ __builtin_memcpy (resbuf, buffer, sizeof (buffer));
+ return resbuf;
+}
Re: [PATCH v2 AArch64]: Re: [PATCH AArch64]: Add constraint letter for stack_protect_test pattern.
Am 17.09.2014 um 15:14 schrieb Matthias Klose: Am 17.09.2014 um 00:03 schrieb James Greenhalgh: If you have any other suggestions, or if =r is actually correct and I am misreading the documentation please let me know. with this patch I see a lot of ICEs in the testsuite for test cases built with -O3 (and a build defaulting to -fstack-protector-strong by default), all of the form: I tested with the wrong patch. No regression, and fixing the original issue with the attached patch. Tested with the trunk and the Linaro 4.9 branch, Matthias --- gcc/config/aarch64/aarch64.md +++ gcc/config/aarch64/aarch64.md @@ -4031,7 +4031,7 @@ (unspec:PTR [(match_operand:PTR 1 memory_operand m) (match_operand:PTR 2 memory_operand m)] UNSPEC_SP_TEST)) - (clobber (match_scratch:PTR 3 r))] + (clobber (match_scratch:PTR 3 =r))] ldr\t%w3, %x1\;ldr\t%w0, %x2\;eor\t%w0, %w3, %w0 [(set_attr length 12)
Re: [PATCH v2 AArch64]: Re: [PATCH AArch64]: Add constraint letter for stack_protect_test pattern.
On 16/09/14 23:03, James Greenhalgh wrote: On Tue, Sep 16, 2014 at 10:36:08PM +0100, Andrew Pinski wrote: On Thu, Sep 4, 2014 at 1:18 AM, James Greenhalgh james.greenha...@arm.com wrote: On Thu, Sep 04, 2014 at 08:42:31AM +0100, Venkataramanan Kumar wrote: Hi maintainers, I just added =r and retested it. I had a very similar patch to this sitting in my local tree. However, I am surprised you have left operand 3 as an output operand. In my tree I had marked operand 3 as r. What do you think? The clobber needs to be =r as you are writing to the register and not just reading from it. I think this is causing some issues including linaro bugzilla #667 (https://bugs.linaro.org/show_bug.cgi?id=667). (+CC Matthias Klose and Steve McIntyre who have also been in contact with me regarding this bug) I've seen this bug locally, and had considered sending the patch you suggested, which does indeed fix the bug. However, it feels wrong as the operand is not a formal output of the pattern. It is clobbered - and indeed earlyclobbered - so yes it is written to, but it isn't an output. This makes the fix look like a band-aid around the real problem. The bug looks similar to pr52573 - regrename fails to spot that it should not rename to a register used in an earlyclobber operand of any type, rather than just an output+earlyclobber operand as it does now. I've played about with a fix that sits in regrename, and forces it to think of all earlyclobber operands as starting and ending chains but this didn't bootstrap clean - we end up with what I believe are false reports of stack smashing in libstdc++. I was planning to look again at my approach tomorrow, I would like to convince myself that this isn't a deficiency in regrename before I would support just marking this operand =r. If you have any other suggestions, or if =r is actually correct and I am misreading the documentation please let me know. Thanks, James =r is correct for an early-clobbered scratch. R.
Re: [PATCH v2 AArch64]: Re: [PATCH AArch64]: Add constraint letter for stack_protect_test pattern.
Am 17.09.2014 um 00:03 schrieb James Greenhalgh:
If you have any other suggestions, or if =r is actually correct and
I am misreading the documentation please let me know.
with this patch I see a lot of ICEs in the testsuite for test cases built with
-O3 (and a build defaulting to -fstack-protector-strong by default), all of the
form:
Executing on host: /home/doko/gcc/4.9/gcc-4.9-4.9.1/build/gcc/xgcc
-B/home/doko/gcc/4.9/gcc-4.9-4.9.1/build/gcc/ -fno-diagnostics-show-caret -fdia
gnostics-color=never -O3 -fomit-frame-pointer -funroll-all-loops
-finline-functions -w -c -o 900116-1.o /home/doko/gcc/4.9/gcc-4.9-4.9.1/src/g
cc/testsuite/gcc.c-torture/compile/900116-1.c(timeout = 300)
spawn /home/doko/gcc/4.9/gcc-4.9-4.9.1/build/gcc/xgcc
-B/home/doko/gcc/4.9/gcc-4.9-4.9.1/build/gcc/ -fno-diagnostics-show-caret
-fdiagnostics-color
=never -O3 -fomit-frame-pointer -funroll-all-loops -finline-functions -w -c -o
900116-1.o /home/doko/gcc/4.9/gcc-4.9-4.9.1/src/gcc/testsuite/gcc.c-
torture/compile/900116-1.c
/home/doko/gcc/4.9/gcc-4.9-4.9.1/src/gcc/testsuite/gcc.c-torture/compile/900116-1.c:
In function 'zloop':
/home/doko/gcc/4.9/gcc-4.9-4.9.1/src/gcc/testsuite/gcc.c-torture/compile/900116-1.c:14:1:
error: insn does not satisfy its constraints:
(insn 228 225 230 9 (parallel [
(set (reg:DI 1 x1 [279])
(unspec:DI [
(mem/v/f/c:DI (plus:DI (reg/f:DI 31 sp)
(const_int 24 [0x18])) [4 D.2626+0 S8 A64])
(mem/v/f/c:DI (reg/f:DI 2 x2 [277]) [4
__stack_chk_guard+0 S8 A64])
] UNSPEC_SP_TEST))
(clobber (reg:DI 2 x2 [320]))
])
/home/doko/gcc/4.9/gcc-4.9-4.9.1/src/gcc/testsuite/gcc.c-torture/compile/900116-1.c:14
741 {stack_protect_test_di}
(expr_list:REG_DEAD (reg/f:DI 13 x13 [277])
(expr_list:REG_UNUSED (reg:DI 2 x2 [320])
(nil
/home/doko/gcc/4.9/gcc-4.9-4.9.1/src/gcc/testsuite/gcc.c-torture/compile/900116-1.c:14:1:
internal compiler error: in copyprop_hardreg_forward_1, at regcprop.c:775
Please submit a full bug report,
with preprocessed source if appropriate.
for now only tested with the 4.9 linaro branch, now testing with trunk.
Matthias
--- - 2014-09-17 13:13:05.245022015 +
+++ test-summary2014-09-17 03:02:55.916771634 +
@@ -1,4 +1,4 @@
-Results for 4.9.1 (Ubuntu/Linaro 4.9.1-14ubuntu2) testsuite on
aarch64-unknown-linux-gnu
+Results for 4.9.1 (Ubuntu/Linaro 4.9.1-14ubuntu2.1) testsuite on
aarch64-unknown-linux-gnu
LAST_UPDATED: Fri Sep 12 17:12:16 UTC 2014 (revision 215228)
Native configuration is aarch64-unknown-linux-gnu
@@ -78,19 +78,16 @@
# of expected passes 116
# of unexpected failures 6
-/build/buildd/gcc-4.9-4.9.1/build/./gcc/gccgo version 4.9.1 (Ubuntu/Linaro
4.9.1-14ubuntu2)
+/home/doko/gcc/4.9/gcc-4.9-4.9.1/build/./gcc/gccgo version 4.9.1
(Ubuntu/Linaro 4.9.1-14ubuntu2.1)
=== libgomp tests ===
Running target unix
-WARNING: program timed out.
-FAIL: libgomp.graphite/force-parallel-6.c execution test
=== libgomp Summary for unix ===
-# of expected passes 3240
-# of unexpected failures 1
+# of expected passes 3241
# of unsupported tests 36
Running target unix/-fno-stack-protector
@@ -102,8 +99,7 @@
=== libgomp Summary ===
-# of expected passes 6481
-# of unexpected failures 1
+# of expected passes 6482
# of unsupported tests 72
=== libitm tests ===
@@ -198,14 +194,29 @@
# of expected failures 82
# of unsupported tests 504
Target: aarch64-linux-gnu
-gcc version 4.9.1 (Ubuntu/Linaro 4.9.1-14ubuntu2)
+gcc version 4.9.1 (Ubuntu/Linaro 4.9.1-14ubuntu2.1)
=== g++ tests ===
Running target unix
+FAIL: g++.dg/opt/unroll1.C -std=gnu++98 (internal compiler error)
+FAIL: g++.dg/opt/unroll1.C -std=gnu++98 (test for excess errors)
+UNRESOLVED: g++.dg/opt/unroll1.C -std=gnu++98 compilation failed to produce
executable
+FAIL: g++.dg/opt/unroll1.C -std=gnu++11 (internal compiler error)
+FAIL: g++.dg/opt/unroll1.C -std=gnu++11 (test for excess errors)
+UNRESOLVED: g++.dg/opt/unroll1.C -std=gnu++11 compilation failed to produce
executable
+FAIL: g++.dg/opt/unroll1.C -std=gnu++1y (internal compiler error)
+FAIL: g++.dg/opt/unroll1.C -std=gnu++1y (test for excess errors)
+UNRESOLVED: g++.dg/opt/unroll1.C -std=gnu++1y compilation failed to produce
executable
XPASS: g++.dg/tls/thread_local-order2.C -std=c++11 execution test
XPASS: g++.dg/tls/thread_local-order2.C -std=c++1y execution test
+FAIL: c-c++-common/torture/vector-shift.c -O3 -fomit-frame-pointer
-funroll-loops (internal compiler error)
+FAIL: c-c++-common/torture/vector-shift.c -O3 -fomit-frame-pointer
-funroll-loops (test for excess errors)
+UNRESOLVED: c-c++-common/torture/vector-shift.c -O3 -fomit-frame-pointer
-funroll-loops
Re: [PATCH AArch64]: Add constraint letter for stack_protect_test pattern)
On Wed, Sep 17, 2014 at 09:30:31AM +0100, Richard Earnshaw wrote:
=r is correct for an early-clobbered scratch.
R.
In that case...
How is the attached patch for trunk? I've bootstrapped it on AArch64
with -fstack-protector-strong and -frename-registers in the BOOT_CFLAGS
without seeing any issues.
OK?
Thanks,
James
---
gcc/
2014-09-15 James Greenhalgh james.greenha...@arm.com
* config/aarch64/aarch64.md (stack_protect_test_mode): Mark
scratch register as an output to placate register renaming.
gcc/testsuite/
2014-09-15 James Greenhalgh james.greenha...@arm.com
* gcc.target/aarch64/stack_protector_set_1.c: New.
* gcc.target/aarch64/stack_protector_set_2.c: Likewise.
diff --git a/gcc/config/aarch64/aarch64.md b/gcc/config/aarch64/aarch64.md
index c60038a9015d614f40f6d9e3fd228ad3e2b247a8..f15a516bb0559c86bea7512f91d60dc179ec9149 100644
--- a/gcc/config/aarch64/aarch64.md
+++ b/gcc/config/aarch64/aarch64.md
@@ -4031,7 +4031,7 @@ (define_insn stack_protect_test_mode
(unspec:PTR [(match_operand:PTR 1 memory_operand m)
(match_operand:PTR 2 memory_operand m)]
UNSPEC_SP_TEST))
- (clobber (match_scratch:PTR 3 r))]
+ (clobber (match_scratch:PTR 3 =r))]
ldr\t%w3, %x1\;ldr\t%w0, %x2\;eor\t%w0, %w3, %w0
[(set_attr length 12)
diff --git a/gcc/testsuite/gcc.target/aarch64/stack_protector_set_1.c b/gcc/testsuite/gcc.target/aarch64/stack_protector_set_1.c
new file mode 100644
index 000..df0d26b
--- /dev/null
+++ b/gcc/testsuite/gcc.target/aarch64/stack_protector_set_1.c
@@ -0,0 +1,15 @@
+/* { dg-do assemble } */
+/* { dg-options -fstack-protector-strong -O1 -frename-registers } */
+
+extern int bar (const char *s, int *argc);
+extern int baz (const char *s);
+
+char
+foo (const char *s)
+{
+ int argc;
+ int ret;
+ if ( !bar (s, argc))
+ret = baz (s);
+ return *s;
+}
diff --git a/gcc/testsuite/gcc.target/aarch64/stack_protector_set_2.c b/gcc/testsuite/gcc.target/aarch64/stack_protector_set_2.c
new file mode 100644
index 000..b94a2d6
--- /dev/null
+++ b/gcc/testsuite/gcc.target/aarch64/stack_protector_set_2.c
@@ -0,0 +1,17 @@
+/* { dg-do assemble } */
+/* { dg-options -fstack-protector-strong -O1 -frename-registers } */
+
+typedef unsigned int uint32_t;
+struct ctx
+{
+ uint32_t A;
+};
+
+void *
+buffer_copy (const struct ctx *ctx, void *resbuf)
+{
+ uint32_t buffer[4];
+ buffer[0] = (ctx-A);
+ __builtin_memcpy (resbuf, buffer, 16);
+ return resbuf;
+}
Re: [PATCH AArch64]: Add constraint letter for stack_protect_test pattern)
On Sep 17, 2014, at 7:43 AM, James Greenhalgh james.greenha...@arm.com wrote: On Wed, Sep 17, 2014 at 09:30:31AM +0100, Richard Earnshaw wrote: =r is correct for an early-clobbered scratch. R. In that case... How is the attached patch for trunk? I've bootstrapped it on AArch64 with -fstack-protector-strong and -frename-registers in the BOOT_CFLAGS without seeing any issues. OK? Thanks, James --- gcc/ 2014-09-15 James Greenhalgh james.greenha...@arm.com * config/aarch64/aarch64.md (stack_protect_test_mode): Mark scratch register as an output to placate register renaming. gcc/testsuite/ 2014-09-15 James Greenhalgh james.greenha...@arm.com * gcc.target/aarch64/stack_protector_set_1.c: New. * gcc.target/aarch64/stack_protector_set_2.c: Likewise. There is nothing aarch64 specific about this testcase so I would place them under gcc.dg and add the extra marker which says this testcase requires stack protector. And maybe even use compile instead of just assemble too. Thanks, Andrew 0001-Re-PATCH-AArch64-Add-constraint-letter-for-stack_pro.patch
Re: [PATCH v2 AArch64]: Re: [PATCH AArch64]: Add constraint letter for stack_protect_test pattern.
Am 17.09.2014 um 15:14 schrieb Matthias Klose:
Am 17.09.2014 um 00:03 schrieb James Greenhalgh:
If you have any other suggestions, or if =r is actually correct and
I am misreading the documentation please let me know.
with this patch I see a lot of ICEs in the testsuite for test cases built with
-O3 (and a build defaulting to -fstack-protector-strong by default), all of
the
form:
Executing on host: /home/doko/gcc/4.9/gcc-4.9-4.9.1/build/gcc/xgcc
-B/home/doko/gcc/4.9/gcc-4.9-4.9.1/build/gcc/ -fno-diagnostics-show-caret
-fdia
gnostics-color=never -O3 -fomit-frame-pointer -funroll-all-loops
-finline-functions -w -c -o 900116-1.o
/home/doko/gcc/4.9/gcc-4.9-4.9.1/src/g
cc/testsuite/gcc.c-torture/compile/900116-1.c(timeout = 300)
spawn /home/doko/gcc/4.9/gcc-4.9-4.9.1/build/gcc/xgcc
-B/home/doko/gcc/4.9/gcc-4.9-4.9.1/build/gcc/ -fno-diagnostics-show-caret
-fdiagnostics-color
=never -O3 -fomit-frame-pointer -funroll-all-loops -finline-functions -w -c -o
900116-1.o /home/doko/gcc/4.9/gcc-4.9-4.9.1/src/gcc/testsuite/gcc.c-
torture/compile/900116-1.c
/home/doko/gcc/4.9/gcc-4.9-4.9.1/src/gcc/testsuite/gcc.c-torture/compile/900116-1.c:
In function 'zloop':
/home/doko/gcc/4.9/gcc-4.9-4.9.1/src/gcc/testsuite/gcc.c-torture/compile/900116-1.c:14:1:
error: insn does not satisfy its constraints:
(insn 228 225 230 9 (parallel [
(set (reg:DI 1 x1 [279])
(unspec:DI [
(mem/v/f/c:DI (plus:DI (reg/f:DI 31 sp)
(const_int 24 [0x18])) [4 D.2626+0 S8 A64])
(mem/v/f/c:DI (reg/f:DI 2 x2 [277]) [4
__stack_chk_guard+0 S8 A64])
] UNSPEC_SP_TEST))
(clobber (reg:DI 2 x2 [320]))
])
/home/doko/gcc/4.9/gcc-4.9-4.9.1/src/gcc/testsuite/gcc.c-torture/compile/900116-1.c:14
741 {stack_protect_test_di}
(expr_list:REG_DEAD (reg/f:DI 13 x13 [277])
(expr_list:REG_UNUSED (reg:DI 2 x2 [320])
(nil
/home/doko/gcc/4.9/gcc-4.9-4.9.1/src/gcc/testsuite/gcc.c-torture/compile/900116-1.c:14:1:
internal compiler error: in copyprop_hardreg_forward_1, at regcprop.c:775
Please submit a full bug report,
with preprocessed source if appropriate.
for now only tested with the 4.9 linaro branch, now testing with trunk.
seen with trunk r215323 as well, after disabling itm (--disable-libitm) which
currently doesn't seem to build.
Matthias
Re: [PATCH v2 AArch64]: Re: [PATCH AArch64]: Add constraint letter for stack_protect_test pattern.
On Thu, Sep 4, 2014 at 1:18 AM, James Greenhalgh james.greenha...@arm.com wrote: On Thu, Sep 04, 2014 at 08:42:31AM +0100, Venkataramanan Kumar wrote: Hi maintainers, I just added =r and retested it. I had a very similar patch to this sitting in my local tree. However, I am surprised you have left operand 3 as an output operand. In my tree I had marked operand 3 as r. What do you think? The clobber needs to be =r as you are writing to the register and not just reading from it. I think this is causing some issues including linaro bugzilla #667 (https://bugs.linaro.org/show_bug.cgi?id=667). Thanks, Andrew Pinski Thanks, James gcc/ChangeLog 2014-09-04 Venkataramanan Kumar venkataramanan.ku...@linaro.org * config/aarch64/aarch64.md (stack_protect_test_mode) Add register constraint for operand 0. diff --git a/gcc/config/aarch64/aarch64.md b/gcc/config/aarch64/aarch64.md index b5be79c..ed6e602 100644 --- a/gcc/config/aarch64/aarch64.md +++ b/gcc/config/aarch64/aarch64.md @@ -4026,7 +4026,7 @@ }) (define_insn stack_protect_test_mode - [(set (match_operand:PTR 0 register_operand) + [(set (match_operand:PTR 0 register_operand =r) (unspec:PTR [(match_operand:PTR 1 memory_operand m) (match_operand:PTR 2 memory_operand m)] UNSPEC_SP_TEST)) regards, venkat. On 4 September 2014 12:42, Venkataramanan Kumar venkataramanan.ku...@linaro.org wrote: Hi Maintainers, Below patch adds register constraint r for destination operand in stack_protect_test pattern. We need a general register here and adding r will avoid vector register getting allocated. regression tested on aarch64-unknown-linux-gnu. Ok for trunk? regards, Venkat. gcc/ChangeLog 2014-09-04 Venkataramanan Kumar venkataramanan.ku...@linaro.org * config/aarch64/aarch64.md (stack_protect_test_mode) Add register constraint for operand 0. diff --git a/gcc/config/aarch64/aarch64.md b/gcc/config/aarch64/aarch64.md index b5be79c..77588b9 100644 --- a/gcc/config/aarch64/aarch64.md +++ b/gcc/config/aarch64/aarch64.md @@ -4026,7 +4026,7 @@ }) (define_insn stack_protect_test_mode - [(set (match_operand:PTR 0 register_operand) + [(set (match_operand:PTR 0 register_operand r) (unspec:PTR [(match_operand:PTR 1 memory_operand m) (match_operand:PTR 2 memory_operand m)] UNSPEC_SP_TEST))
Re: [PATCH v2 AArch64]: Re: [PATCH AArch64]: Add constraint letter for stack_protect_test pattern.
Hi Andrew, Thanks for pointing that. I thought modifier is enough to say that operand is early clobbered and so GCC will use a different register and it will not allocate same register that was given to a input operand. Lookign at the the bug it looks like = is needed for the clobber, so that GCC will allocate a fresh register. regards, Venkat. On 17 September 2014 03:06, Andrew Pinski pins...@gmail.com wrote: On Thu, Sep 4, 2014 at 1:18 AM, James Greenhalgh james.greenha...@arm.com wrote: On Thu, Sep 04, 2014 at 08:42:31AM +0100, Venkataramanan Kumar wrote: Hi maintainers, I just added =r and retested it. I had a very similar patch to this sitting in my local tree. However, I am surprised you have left operand 3 as an output operand. In my tree I had marked operand 3 as r. What do you think? The clobber needs to be =r as you are writing to the register and not just reading from it. I think this is causing some issues including linaro bugzilla #667 (https://bugs.linaro.org/show_bug.cgi?id=667). Thanks, Andrew Pinski Thanks, James gcc/ChangeLog 2014-09-04 Venkataramanan Kumar venkataramanan.ku...@linaro.org * config/aarch64/aarch64.md (stack_protect_test_mode) Add register constraint for operand 0. diff --git a/gcc/config/aarch64/aarch64.md b/gcc/config/aarch64/aarch64.md index b5be79c..ed6e602 100644 --- a/gcc/config/aarch64/aarch64.md +++ b/gcc/config/aarch64/aarch64.md @@ -4026,7 +4026,7 @@ }) (define_insn stack_protect_test_mode - [(set (match_operand:PTR 0 register_operand) + [(set (match_operand:PTR 0 register_operand =r) (unspec:PTR [(match_operand:PTR 1 memory_operand m) (match_operand:PTR 2 memory_operand m)] UNSPEC_SP_TEST)) regards, venkat. On 4 September 2014 12:42, Venkataramanan Kumar venkataramanan.ku...@linaro.org wrote: Hi Maintainers, Below patch adds register constraint r for destination operand in stack_protect_test pattern. We need a general register here and adding r will avoid vector register getting allocated. regression tested on aarch64-unknown-linux-gnu. Ok for trunk? regards, Venkat. gcc/ChangeLog 2014-09-04 Venkataramanan Kumar venkataramanan.ku...@linaro.org * config/aarch64/aarch64.md (stack_protect_test_mode) Add register constraint for operand 0. diff --git a/gcc/config/aarch64/aarch64.md b/gcc/config/aarch64/aarch64.md index b5be79c..77588b9 100644 --- a/gcc/config/aarch64/aarch64.md +++ b/gcc/config/aarch64/aarch64.md @@ -4026,7 +4026,7 @@ }) (define_insn stack_protect_test_mode - [(set (match_operand:PTR 0 register_operand) + [(set (match_operand:PTR 0 register_operand r) (unspec:PTR [(match_operand:PTR 1 memory_operand m) (match_operand:PTR 2 memory_operand m)] UNSPEC_SP_TEST))
Re: [PATCH v2 AArch64]: Re: [PATCH AArch64]: Add constraint letter for stack_protect_test pattern.
On Tue, Sep 16, 2014 at 10:36:08PM +0100, Andrew Pinski wrote: On Thu, Sep 4, 2014 at 1:18 AM, James Greenhalgh james.greenha...@arm.com wrote: On Thu, Sep 04, 2014 at 08:42:31AM +0100, Venkataramanan Kumar wrote: Hi maintainers, I just added =r and retested it. I had a very similar patch to this sitting in my local tree. However, I am surprised you have left operand 3 as an output operand. In my tree I had marked operand 3 as r. What do you think? The clobber needs to be =r as you are writing to the register and not just reading from it. I think this is causing some issues including linaro bugzilla #667 (https://bugs.linaro.org/show_bug.cgi?id=667). (+CC Matthias Klose and Steve McIntyre who have also been in contact with me regarding this bug) I've seen this bug locally, and had considered sending the patch you suggested, which does indeed fix the bug. However, it feels wrong as the operand is not a formal output of the pattern. It is clobbered - and indeed earlyclobbered - so yes it is written to, but it isn't an output. This makes the fix look like a band-aid around the real problem. The bug looks similar to pr52573 - regrename fails to spot that it should not rename to a register used in an earlyclobber operand of any type, rather than just an output+earlyclobber operand as it does now. I've played about with a fix that sits in regrename, and forces it to think of all earlyclobber operands as starting and ending chains but this didn't bootstrap clean - we end up with what I believe are false reports of stack smashing in libstdc++. I was planning to look again at my approach tomorrow, I would like to convince myself that this isn't a deficiency in regrename before I would support just marking this operand =r. If you have any other suggestions, or if =r is actually correct and I am misreading the documentation please let me know. Thanks, James
Re: [PATCH v2 AArch64]: Re: [PATCH AArch64]: Add constraint letter for stack_protect_test pattern.
On Tue, Sep 16, 2014 at 3:03 PM, James Greenhalgh james.greenha...@arm.com wrote: On Tue, Sep 16, 2014 at 10:36:08PM +0100, Andrew Pinski wrote: On Thu, Sep 4, 2014 at 1:18 AM, James Greenhalgh james.greenha...@arm.com wrote: On Thu, Sep 04, 2014 at 08:42:31AM +0100, Venkataramanan Kumar wrote: Hi maintainers, I just added =r and retested it. I had a very similar patch to this sitting in my local tree. However, I am surprised you have left operand 3 as an output operand. In my tree I had marked operand 3 as r. What do you think? The clobber needs to be =r as you are writing to the register and not just reading from it. I think this is causing some issues including linaro bugzilla #667 (https://bugs.linaro.org/show_bug.cgi?id=667). (+CC Matthias Klose and Steve McIntyre who have also been in contact with me regarding this bug) I've seen this bug locally, and had considered sending the patch you suggested, which does indeed fix the bug. However, it feels wrong as the operand is not a formal output of the pattern. It is clobbered - and indeed earlyclobbered - so yes it is written to, but it isn't an output. This makes the fix look like a band-aid around the real problem. The bug looks similar to pr52573 - regrename fails to spot that it should not rename to a register used in an earlyclobber operand of any type, rather than just an output+earlyclobber operand as it does now. I've played about with a fix that sits in regrename, and forces it to think of all earlyclobber operands as starting and ending chains but this didn't bootstrap clean - we end up with what I believe are false reports of stack smashing in libstdc++. I was planning to look again at my approach tomorrow, I would like to convince myself that this isn't a deficiency in regrename before I would support just marking this operand =r. If you have any other suggestions, or if =r is actually correct and I am misreading the documentation please let me know. I think you misread the documentation. Or rather the documentation is not fully clear here. https://gcc.gnu.org/onlinedocs/gccint/Modifiers.html says this for : ‘’ does not obviate the need to write ‘=’ or ‘+’. Which means you need = if you write to the register. Match_scratch is the same as match_operand except it is able to be added back during combine. Also = means: Means that this operand is write-only for this instruction: the previous value is discarded and replaced by output data. That is not necessary a formal output of the pattern. Thanks, Andrew Thanks, Andrew Pinski Thanks, James
Re: [PATCH v2 AArch64]: Re: [PATCH AArch64]: Add constraint letter for stack_protect_test pattern.
Hi Marcus, I up streamed the changes to trunk. There is no support for stack protection in FSF GCC 4.9 branch yet. So I need to back port r209712 and this change together. regards, Venkat. On 5 September 2014 21:17, Marcus Shawcroft marcus.shawcr...@gmail.com wrote: On 4 September 2014 19:19, Venkataramanan Kumar venkataramanan.ku...@linaro.org wrote: Hi James, Yes we can just mark operand 3 as r. PFB, the updated patch. Ok for trunk? regards, Venkat. gcc/ChangeLog 2014-09-04 Venkataramanan Kumar venkataramanan.ku...@linaro.org * config/aarch64/aarch64.md (stack_protect_test_mode) Add register constraint for operand 0 and remove write only constraint from operand 3. OK include pr63190 in the ChangeLog entry and backport to 4.9 please. Thanks /Marcus
Re: [PATCH v2 AArch64]: Re: [PATCH AArch64]: Add constraint letter for stack_protect_test pattern.
On 8 September 2014 16:36, Venkataramanan Kumar venkataramanan.ku...@linaro.org wrote: Hi Marcus, I up streamed the changes to trunk. There is no support for stack protection in FSF GCC 4.9 branch yet. Quite right, ignore my back port request. Cheers /Marcus
Re: [PATCH v2 AArch64]: Re: [PATCH AArch64]: Add constraint letter for stack_protect_test pattern.
On 4 September 2014 19:19, Venkataramanan Kumar venkataramanan.ku...@linaro.org wrote: Hi James, Yes we can just mark operand 3 as r. PFB, the updated patch. Ok for trunk? regards, Venkat. gcc/ChangeLog 2014-09-04 Venkataramanan Kumar venkataramanan.ku...@linaro.org * config/aarch64/aarch64.md (stack_protect_test_mode) Add register constraint for operand 0 and remove write only constraint from operand 3. OK include pr63190 in the ChangeLog entry and backport to 4.9 please. Thanks /Marcus
[PATCH AArch64]: Add constraint letter for stack_protect_test pattern.
Hi Maintainers, Below patch adds register constraint r for destination operand in stack_protect_test pattern. We need a general register here and adding r will avoid vector register getting allocated. regression tested on aarch64-unknown-linux-gnu. Ok for trunk? regards, Venkat. gcc/ChangeLog 2014-09-04 Venkataramanan Kumar venkataramanan.ku...@linaro.org * config/aarch64/aarch64.md (stack_protect_test_mode) Add register constraint for operand 0. diff --git a/gcc/config/aarch64/aarch64.md b/gcc/config/aarch64/aarch64.md index b5be79c..77588b9 100644 --- a/gcc/config/aarch64/aarch64.md +++ b/gcc/config/aarch64/aarch64.md @@ -4026,7 +4026,7 @@ }) (define_insn stack_protect_test_mode - [(set (match_operand:PTR 0 register_operand) + [(set (match_operand:PTR 0 register_operand r) (unspec:PTR [(match_operand:PTR 1 memory_operand m) (match_operand:PTR 2 memory_operand m)] UNSPEC_SP_TEST))
[PATCH v2 AArch64]: Re: [PATCH AArch64]: Add constraint letter for stack_protect_test pattern.
Hi maintainers, I just added =r and retested it. gcc/ChangeLog 2014-09-04 Venkataramanan Kumar venkataramanan.ku...@linaro.org * config/aarch64/aarch64.md (stack_protect_test_mode) Add register constraint for operand 0. diff --git a/gcc/config/aarch64/aarch64.md b/gcc/config/aarch64/aarch64.md index b5be79c..ed6e602 100644 --- a/gcc/config/aarch64/aarch64.md +++ b/gcc/config/aarch64/aarch64.md @@ -4026,7 +4026,7 @@ }) (define_insn stack_protect_test_mode - [(set (match_operand:PTR 0 register_operand) + [(set (match_operand:PTR 0 register_operand =r) (unspec:PTR [(match_operand:PTR 1 memory_operand m) (match_operand:PTR 2 memory_operand m)] UNSPEC_SP_TEST)) regards, venkat. On 4 September 2014 12:42, Venkataramanan Kumar venkataramanan.ku...@linaro.org wrote: Hi Maintainers, Below patch adds register constraint r for destination operand in stack_protect_test pattern. We need a general register here and adding r will avoid vector register getting allocated. regression tested on aarch64-unknown-linux-gnu. Ok for trunk? regards, Venkat. gcc/ChangeLog 2014-09-04 Venkataramanan Kumar venkataramanan.ku...@linaro.org * config/aarch64/aarch64.md (stack_protect_test_mode) Add register constraint for operand 0. diff --git a/gcc/config/aarch64/aarch64.md b/gcc/config/aarch64/aarch64.md index b5be79c..77588b9 100644 --- a/gcc/config/aarch64/aarch64.md +++ b/gcc/config/aarch64/aarch64.md @@ -4026,7 +4026,7 @@ }) (define_insn stack_protect_test_mode - [(set (match_operand:PTR 0 register_operand) + [(set (match_operand:PTR 0 register_operand r) (unspec:PTR [(match_operand:PTR 1 memory_operand m) (match_operand:PTR 2 memory_operand m)] UNSPEC_SP_TEST))
Re: [PATCH v2 AArch64]: Re: [PATCH AArch64]: Add constraint letter for stack_protect_test pattern.
On Thu, Sep 04, 2014 at 08:42:31AM +0100, Venkataramanan Kumar wrote: Hi maintainers, I just added =r and retested it. I had a very similar patch to this sitting in my local tree. However, I am surprised you have left operand 3 as an output operand. In my tree I had marked operand 3 as r. What do you think? Thanks, James gcc/ChangeLog 2014-09-04 Venkataramanan Kumar venkataramanan.ku...@linaro.org * config/aarch64/aarch64.md (stack_protect_test_mode) Add register constraint for operand 0. diff --git a/gcc/config/aarch64/aarch64.md b/gcc/config/aarch64/aarch64.md index b5be79c..ed6e602 100644 --- a/gcc/config/aarch64/aarch64.md +++ b/gcc/config/aarch64/aarch64.md @@ -4026,7 +4026,7 @@ }) (define_insn stack_protect_test_mode - [(set (match_operand:PTR 0 register_operand) + [(set (match_operand:PTR 0 register_operand =r) (unspec:PTR [(match_operand:PTR 1 memory_operand m) (match_operand:PTR 2 memory_operand m)] UNSPEC_SP_TEST)) regards, venkat. On 4 September 2014 12:42, Venkataramanan Kumar venkataramanan.ku...@linaro.org wrote: Hi Maintainers, Below patch adds register constraint r for destination operand in stack_protect_test pattern. We need a general register here and adding r will avoid vector register getting allocated. regression tested on aarch64-unknown-linux-gnu. Ok for trunk? regards, Venkat. gcc/ChangeLog 2014-09-04 Venkataramanan Kumar venkataramanan.ku...@linaro.org * config/aarch64/aarch64.md (stack_protect_test_mode) Add register constraint for operand 0. diff --git a/gcc/config/aarch64/aarch64.md b/gcc/config/aarch64/aarch64.md index b5be79c..77588b9 100644 --- a/gcc/config/aarch64/aarch64.md +++ b/gcc/config/aarch64/aarch64.md @@ -4026,7 +4026,7 @@ }) (define_insn stack_protect_test_mode - [(set (match_operand:PTR 0 register_operand) + [(set (match_operand:PTR 0 register_operand r) (unspec:PTR [(match_operand:PTR 1 memory_operand m) (match_operand:PTR 2 memory_operand m)] UNSPEC_SP_TEST))
Re: [PATCH v2 AArch64]: Re: [PATCH AArch64]: Add constraint letter for stack_protect_test pattern.
Hi James, Yes we can just mark operand 3 as r. PFB, the updated patch. Ok for trunk? regards, Venkat. gcc/ChangeLog 2014-09-04 Venkataramanan Kumar venkataramanan.ku...@linaro.org * config/aarch64/aarch64.md (stack_protect_test_mode) Add register constraint for operand 0 and remove write only constraint from operand 3. diff --git a/gcc/config/aarch64/aarch64.md b/gcc/config/aarch64/aarch64.md index b5be79c..cf6fdb0 100644 --- a/gcc/config/aarch64/aarch64.md +++ b/gcc/config/aarch64/aarch64.md @@ -4026,11 +4026,11 @@ }) (define_insn stack_protect_test_mode - [(set (match_operand:PTR 0 register_operand) + [(set (match_operand:PTR 0 register_operand =r) (unspec:PTR [(match_operand:PTR 1 memory_operand m) (match_operand:PTR 2 memory_operand m)] UNSPEC_SP_TEST)) - (clobber (match_scratch:PTR 3 =r))] + (clobber (match_scratch:PTR 3 r))] ldr\t%w3, %x1\;ldr\t%w0, %x2\;eor\t%w0, %w3, %w0 [(set_attr length 12) regards, Venkat, On 4 September 2014 13:48, James Greenhalgh james.greenha...@arm.com wrote: On Thu, Sep 04, 2014 at 08:42:31AM +0100, Venkataramanan Kumar wrote: Hi maintainers, I just added =r and retested it. I had a very similar patch to this sitting in my local tree. However, I am surprised you have left operand 3 as an output operand. In my tree I had marked operand 3 as r. What do you think? Thanks, James gcc/ChangeLog 2014-09-04 Venkataramanan Kumar venkataramanan.ku...@linaro.org * config/aarch64/aarch64.md (stack_protect_test_mode) Add register constraint for operand 0. diff --git a/gcc/config/aarch64/aarch64.md b/gcc/config/aarch64/aarch64.md index b5be79c..ed6e602 100644 --- a/gcc/config/aarch64/aarch64.md +++ b/gcc/config/aarch64/aarch64.md @@ -4026,7 +4026,7 @@ }) (define_insn stack_protect_test_mode - [(set (match_operand:PTR 0 register_operand) + [(set (match_operand:PTR 0 register_operand =r) (unspec:PTR [(match_operand:PTR 1 memory_operand m) (match_operand:PTR 2 memory_operand m)] UNSPEC_SP_TEST)) regards, venkat. On 4 September 2014 12:42, Venkataramanan Kumar venkataramanan.ku...@linaro.org wrote: Hi Maintainers, Below patch adds register constraint r for destination operand in stack_protect_test pattern. We need a general register here and adding r will avoid vector register getting allocated. regression tested on aarch64-unknown-linux-gnu. Ok for trunk? regards, Venkat. gcc/ChangeLog 2014-09-04 Venkataramanan Kumar venkataramanan.ku...@linaro.org * config/aarch64/aarch64.md (stack_protect_test_mode) Add register constraint for operand 0. diff --git a/gcc/config/aarch64/aarch64.md b/gcc/config/aarch64/aarch64.md index b5be79c..77588b9 100644 --- a/gcc/config/aarch64/aarch64.md +++ b/gcc/config/aarch64/aarch64.md @@ -4026,7 +4026,7 @@ }) (define_insn stack_protect_test_mode - [(set (match_operand:PTR 0 register_operand) + [(set (match_operand:PTR 0 register_operand r) (unspec:PTR [(match_operand:PTR 1 memory_operand m) (match_operand:PTR 2 memory_operand m)] UNSPEC_SP_TEST))
