Re: [PING]: [PATCH]: Conditionally include target specific files while building TSAN
Hi Jakub, Thank you and I committed the patch https://gcc.gnu.org/viewcvs/gcc?view=revision&revision=220083. regards, Venkat. On 24 January 2015 at 20:38, Jakub Jelinek wrote: > On Sat, Jan 24, 2015 at 08:09:24PM +0530, Venkataramanan Kumar wrote: >> Index: libsanitizer/ChangeLog >> === >> --- libsanitizer/ChangeLog(revision 220079) >> +++ libsanitizer/ChangeLog(working copy) >> @@ -1,5 +1,11 @@ >> 2015-01-25 Venkataramanan Kumar >> >> + * configure.ac (TSAN_TARGET_DEPENDENT_OBJECTS): Undefine. >> + * configure: Regenerate. >> + * configure.tgt (TSAN_TARGET_DEPENDENT_OBJECTS): Define. > > Ok. > > Jakub
Re: [PING]: [PATCH]: Conditionally include target specific files while building TSAN
Hi Jakub,
On 24 January 2015 at 14:40, Jakub Jelinek wrote:
> On Sat, Jan 24, 2015 at 01:23:22PM +0530, Venkataramanan Kumar wrote:
>> I reused libgcc's "host_address" test and the patch passed normal
>> bootstrap in x86_64.
>>
>> Can you please check if this is fine ?
>
> Can't you just use what configure.tgt already uses?
>
> x86_64-*-linux* | i?86-*-linux*)
> if test x$ac_cv_sizeof_void_p = x8; then
> TSAN_SUPPORTED=yes
> LSAN_SUPPORTED=yes
> fi
> ;;
>
> Just make sure AC_CHECK_SIZEOF([void *]) is above this (seems it is).
>
> So
>
> TSAN_TARGET_DEPENDENT_OBJECTS=
> case "${target}" in
> x86_64-*-linux* | i?86-*-linux*)
> if test x$ac_cv_sizeof_void_p = x8; then
> TSAN_TARGET_DEPENDENT_OBJECTS=tsan_rtl_amd64.lo
> fi;;
> esac
> AC_SUBST([TSAN_TARGET_DEPENDENT_OBJECTS])
>
> ?
> Or even better move the TSAN_TARGET_DEPENDENT_OBJECTS initialization
> to configure.tgt and just keep AC_SUBST([TSAN_TARGET_DEPENDENT_OBJECTS])
> in configure.ac.
>
> Jakub
As per you suggestion, I moved the TSAN_TARGET_DEPENDENT_OBJECTS to
"configure.tgt" also it includes i?86 targets.
Bootstraped on x86_64 and Aarch64.
regards,
Venkat.
Index: libsanitizer/ChangeLog
=======
--- libsanitizer/ChangeLog (revision 220079)
+++ libsanitizer/ChangeLog (working copy)
@@ -1,5 +1,11 @@
2015-01-25 Venkataramanan Kumar
+ * configure.ac (TSAN_TARGET_DEPENDENT_OBJECTS): Undefine.
+ * configure: Regenerate.
+ * configure.tgt (TSAN_TARGET_DEPENDENT_OBJECTS): Define.
+
+2015-01-25 Venkataramanan Kumar
+
* configure.ac (TSAN_TARGET_DEPENDENT_OBJECTS): Define.
* configure: Regenerate.
* tsan/Makefile.am (EXTRA_libtsan_la_SOURCES): Define.
Index: libsanitizer/configure
===
--- libsanitizer/configure (revision 220079)
+++ libsanitizer/configure (working copy)
@@ -16363,10 +16363,6 @@
fi
-case "${target}" in
- x86_64-*-linux-*) TSAN_TARGET_DEPENDENT_OBJECTS='tsan_rtl_amd64.lo' ;;
- *) TSAN_TARGET_DEPENDENT_OBJECTS='' ;;
-esac
cat >confcache <<\_ACEOF
Index: libsanitizer/configure.ac
===
--- libsanitizer/configure.ac (revision 220079)
+++ libsanitizer/configure.ac (working copy)
@@ -346,10 +346,6 @@
])
fi
-case "${target}" in
- x86_64-*-linux-*) TSAN_TARGET_DEPENDENT_OBJECTS='tsan_rtl_amd64.lo' ;;
- *) TSAN_TARGET_DEPENDENT_OBJECTS='' ;;
-esac
AC_SUBST([TSAN_TARGET_DEPENDENT_OBJECTS])
AC_OUTPUT
Index: libsanitizer/configure.tgt
===
--- libsanitizer/configure.tgt (revision 220079)
+++ libsanitizer/configure.tgt (working copy)
@@ -19,11 +19,13 @@
# lets us skip running autoconf when modifying target specific information.
# Filter out unsupported systems.
+TSAN_TARGET_DEPENDENT_OBJECTS=
case "${target}" in
x86_64-*-linux* | i?86-*-linux*)
if test x$ac_cv_sizeof_void_p = x8; then
TSAN_SUPPORTED=yes
LSAN_SUPPORTED=yes
+ TSAN_TARGET_DEPENDENT_OBJECTS=tsan_rtl_amd64.lo
fi
;;
powerpc*le-*-linux*)
Re: [PING]: [PATCH]: Conditionally include target specific files while building TSAN
Hi Rainer,
Please find the corrected patch attached. I removed some eval
statements I added for debugging.
regards,
Venkat,
On 24 January 2015 at 13:23, Venkataramanan Kumar
wrote:
> Hi Rainer,
>
> I reused libgcc's "host_address" test and the patch passed normal
> bootstrap in x86_64.
>
> Can you please check if this is fine ?
>
> regards,
> Venkat.
>
> On 24 January 2015 at 12:53, Rainer Orth
> wrote:
>> Hi Venkat,
>>
>>> Yes thanks I will work on fixing this. Let me know if I need to revert
>>> the patch meanwhile.
>>
>> I don't think this is urgent enough to justify reversion.
>>
>> Thanks.
>> Rainer
>>
>> --
>> -
>> Rainer Orth, Center for Biotechnology, Bielefeld University
Index: libsanitizer/ChangeLog
===
--- libsanitizer/ChangeLog (revision 220077)
+++ libsanitizer/ChangeLog (working copy)
@@ -1,5 +1,10 @@
2015-01-25 Venkataramanan Kumar
+ * configure.ac: Set host_address to 64 or 32.
+ * configure: Regenerate.
+
+2015-01-25 Venkataramanan Kumar
+
* configure.ac (TSAN_TARGET_DEPENDENT_OBJECTS): Define.
* configure: Regenerate.
* tsan/Makefile.am (EXTRA_libtsan_la_SOURCES): Define.
Index: libsanitizer/configure
===
--- libsanitizer/configure (revision 220077)
+++ libsanitizer/configure (working copy)
@@ -16363,12 +16363,29 @@
fi
-case "${target}" in
- x86_64-*-linux-*) TSAN_TARGET_DEPENDENT_OBJECTS='tsan_rtl_amd64.lo' ;;
- *) TSAN_TARGET_DEPENDENT_OBJECTS='' ;;
-esac
+# Check 32bit or 64bit. In the case of MIPS, this really determines the
+# word size rather than the address size.
+cat > conftest.c <confcache <<\_ACEOF
# This file is a shell script that caches the results of configure
# tests run on this system so they can be shared between configure
Index: libsanitizer/configure.ac
===
--- libsanitizer/configure.ac (revision 220077)
+++ libsanitizer/configure.ac (working copy)
@@ -346,10 +346,27 @@
])
fi
-case "${target}" in
- x86_64-*-linux-*) TSAN_TARGET_DEPENDENT_OBJECTS='tsan_rtl_amd64.lo' ;;
- *) TSAN_TARGET_DEPENDENT_OBJECTS='' ;;
-esac
+# Check 32bit or 64bit. In the case of MIPS, this really determines the
+# word size rather than the address size.
+cat > conftest.c <
Re: [PING]: [PATCH]: Conditionally include target specific files while building TSAN
Hi Rainer,
I reused libgcc's "host_address" test and the patch passed normal
bootstrap in x86_64.
Can you please check if this is fine ?
regards,
Venkat.
On 24 January 2015 at 12:53, Rainer Orth wrote:
> Hi Venkat,
>
>> Yes thanks I will work on fixing this. Let me know if I need to revert
>> the patch meanwhile.
>
> I don't think this is urgent enough to justify reversion.
>
> Thanks.
> Rainer
>
> --
> -
> Rainer Orth, Center for Biotechnology, Bielefeld University
Index: libsanitizer/ChangeLog
===
--- libsanitizer/ChangeLog (revision 220077)
+++ libsanitizer/ChangeLog (working copy)
@@ -1,5 +1,10 @@
2015-01-25 Venkataramanan Kumar
+ * configure.ac: Set host_address to 64 or 32.
+ * configure: Regenerate.
+
+2015-01-25 Venkataramanan Kumar
+
* configure.ac (TSAN_TARGET_DEPENDENT_OBJECTS): Define.
* configure: Regenerate.
* tsan/Makefile.am (EXTRA_libtsan_la_SOURCES): Define.
Index: libsanitizer/configure
===
--- libsanitizer/configure (revision 220077)
+++ libsanitizer/configure (working copy)
@@ -16363,12 +16363,31 @@
fi
-case "${target}" in
- x86_64-*-linux-*) TSAN_TARGET_DEPENDENT_OBJECTS='tsan_rtl_amd64.lo' ;;
- *) TSAN_TARGET_DEPENDENT_OBJECTS='' ;;
-esac
+# Check 32bit or 64bit. In the case of MIPS, this really determines the
+# word size rather than the address size.
+cat > conftest.c <confcache <<\_ACEOF
# This file is a shell script that caches the results of configure
# tests run on this system so they can be shared between configure
Index: libsanitizer/configure.ac
===
--- libsanitizer/configure.ac (revision 220077)
+++ libsanitizer/configure.ac (working copy)
@@ -346,10 +346,29 @@
])
fi
-case "${target}" in
- x86_64-*-linux-*) TSAN_TARGET_DEPENDENT_OBJECTS='tsan_rtl_amd64.lo' ;;
- *) TSAN_TARGET_DEPENDENT_OBJECTS='' ;;
-esac
+# Check 32bit or 64bit. In the case of MIPS, this really determines the
+# word size rather than the address size.
+cat > conftest.c <
Re: [PING]: [PATCH]: Conditionally include target specific files while building TSAN
Hi Rainer, Yes thanks I will work on fixing this. Let me know if I need to revert the patch meanwhile. regards, Venkat. On 24 January 2015 at 02:23, Rainer Orth wrote: > Hi Venkat, > >> I committed the patch with the change log corrections you said. >> >> https://gcc.gnu.org/viewcvs/gcc?view=revision&revision=220034 > > unfortunately, it broke bootstrap for an i686-unknown-linux-gnu > --enable-targets=all build: the 64-bit libtsan.so fails to link: > > .libs/tsan_interface_atomic.o: In function > `__tsan::TraceAddEvent(__tsan::ThreadState*, __tsan::FastState, > __tsan::EventType, unsigned long long)': > /vol/gcc/src/hg/trunk/local/libsanitizer/tsan/tsan_rtl.h:715: undefined > reference to `__tsan_trace_switch_thunk' > [... > .libs/tsan_rtl_mutex.o:/vol/gcc/src/hg/trunk/local/libsanitizer/tsan/tsan_rtl.h:715: > more undefined references to `__tsan_trace_switch_thunk' follow > /vol/gcc/bin/i686/gld-2.24: .libs/libtsan.so.0.0.0: hidden symbol > `__tsan_report_race_thunk' isn't defined > /vol/gcc/bin/i686/gld-2.24: final link failed: Bad valu > collect2: error: ld returned 1 exit status > make[6]: *** [libtsan.la] Error 1 > > The problem is that libsanitizer/configure.ac checks for target > x86_64-*-linux-*, which is wrong in this case. I believe that you need > something like libgcc's host_address instead and then check for either > x86_64-*-linux-* or i?86-*-linux-* and host_address=64. > > Rainer > > -- > - > Rainer Orth, Center for Biotechnology, Bielefeld University
Re: [PING]: [PATCH]: Conditionally include target specific files while building TSAN
Hi Jakub, I committed the patch with the change log corrections you said. https://gcc.gnu.org/viewcvs/gcc?view=revision&revision=220034 regards, Venkat. On 23 January 2015 at 02:14, Jakub Jelinek wrote: > On Thu, Jan 22, 2015 at 06:16:53PM +0400, Dmitry Vyukov wrote: >> On Thu, Jan 22, 2015 at 5:03 PM, Jakub Jelinek wrote: >> > On Thu, Jan 22, 2015 at 07:30:50PM +0530, Venkataramanan Kumar wrote: >> >> ping. >> >> >> >> Forgot to mention, GCC bootstraps and regression testing passed on x86_64. >> > >> > Well, without a change from upstream to guard the HACKY_CALL and actual >> > tsan >> > port to non-x86_64 this patch doesn't solve anything. >> >> >> I've just committed that change upstream: >> http://llvm.org/viewvc/llvm-project?view=revision&revision=226829 >> Now we need to summon +Kostya to update gcc version of runtime. > > Here is what I've committed: > > 2015-01-22 Jakub Jelinek > > * tsan/tsan_rtl.h: Cherry pick upstream r226829. > > --- libsanitizer/tsan/tsan_rtl.h(revision 226828) > +++ libsanitizer/tsan/tsan_rtl.h(revision 226829) > @@ -679,7 +679,7 @@ void AcquireReleaseImpl(ThreadState *thr > // The trick is that the call preserves all registers and the compiler > // does not treat it as a call. > // If it does not work for you, use normal call. > -#if TSAN_DEBUG == 0 > +#if TSAN_DEBUG == 0 && defined(__x86_64__) > // The caller may not create the stack frame for itself at all, > // so we create a reserve stack frame for it (1024b must be enough). > #define HACKY_CALL(f) \ > > > Jakub
Re: [PING] : [RFC] Tighten memory type assumption in RTL combiner pass.
Thank you Segher, I will send an updated patch for stage 1. regards, Venkat. On 22 January 2015 at 21:46, Segher Boessenkool wrote: > On Thu, Jan 22, 2015 at 07:29:28PM +0530, Venkataramanan Kumar wrote: >> ping. Segher do you any comments from your side. > > I agree combine should not transform shifts into multiplication (except > in addresses, where it is canonical). I haven't looked at the actual > patch in detail though, it will have to wait until stage1, it is not > fixing a regression and it affects all targets. > > > Segher
Re: [PING]: [PATCH]: Conditionally include target specific files while building TSAN
Hi Jakub, Thank you for the reply. Yes there is no functional change. I got some comments in PR63850 about pushing a patch since is GCC only change. Ok I will wait for LLVM merge of TSAN which needs this patch. regards, Venkat. On 22 January 2015 at 19:33, Jakub Jelinek wrote: > On Thu, Jan 22, 2015 at 07:30:50PM +0530, Venkataramanan Kumar wrote: >> ping. >> >> Forgot to mention, GCC bootstraps and regression testing passed on x86_64. > > Well, without a change from upstream to guard the HACKY_CALL and actual tsan > port to non-x86_64 this patch doesn't solve anything. > > Jakub
[PING]: [PATCH]: Conditionally include target specific files while building TSAN
ping. Forgot to mention, GCC bootstraps and regression testing passed on x86_64. regards, Venkat. On 20 January 2015 at 18:51, Venkataramanan Kumar wrote: > Hi all, > > This patch changes make file and configure under libsanitizer, to > separate out X86_64 specific file "tsan_rtl_amd64.S" from getting > build for targets other than X86_64. > > Ok for trunk? > > Please review. > > regards, > Venkat, > > > ChangeLog > > 2015-01-19 Venkataramanan Kumar > > * configure.ac (TSAN_TARGET_DEPENDENT_OBJECTS): Define. > * configure: Regenerate. > * tsan/Makefile.am > (EXTRA_libtsan_la_SOURCES): Define, > (libtsan_la_DEPENDENCIES): Likewise. > * Makefile.in: Regenerate. > * asan/Makefile.in: Regenerate. > * interception/Makefile.in: Regenerate. > * libbacktrace/Makefile.in: Regenerate. > * lsan/Makefile.in: Regenerate. > * sanitizer_common/Makefile.in: Regenerate. > * tsan/Makefile.in: Regenerate. > * ubsan/Makefile.in: Regenerate.
[PING] : [RFC] Tighten memory type assumption in RTL combiner pass.
ping. Segher do you any comments from your side.
regards,
Venkat.
On 14 January 2015 at 16:57, Venkataramanan Kumar
wrote:
> Hi all,
>
> When trying to debug GCC combiner pass with the test case in PR63949
> ref https://gcc.gnu.org/bugzilla/show_bug.cgi?id=63949 I came across this
> code.
>
> This code in "make_compound_operation" assumes that all PLUS and MINUS
> RTX are "MEM" type for scalar int modes and tries to optimize based on
> that assumption.
>
> /* Select the code to be used in recursive calls. Once we are inside an
> address, we stay there. If we have a comparison, set to COMPARE,
> but once inside, go back to our default of SET. */
>
>next_code = (code == MEM ? MEM
> : ((code == PLUS || code == MINUS)
>&& SCALAR_INT_MODE_P (mode)) ? MEM
> : ((code == COMPARE || COMPARISON_P (x))
>&& XEXP (x, 1) == const0_rtx) ? COMPARE
> : in_code == COMPARE ? SET : in_code);
>
> next_code is passed as in_code via recursive calls to
> "make_compound_operation". Based on that we are converting shift
> pattern to MULT pattern.
>
> case ASHIFT:
> /* Convert shifts by constants into multiplications if inside
> an address. */
> if (in_code == MEM && CONST_INT_P (XEXP (x, 1))
> && INTVAL (XEXP (x, 1)) < HOST_BITS_PER_WIDE_INT
> && INTVAL (XEXP (x, 1)) >= 0
> && SCALAR_INT_MODE_P (mode))
> {
>
> Now I tried to tighten it further by adding check to see in_code is
> also MEM type.
> Not sure if this right thing to do. But this assumption about MEM
> seems to be very relaxed before.
>
> diff --git a/gcc/combine.c b/gcc/combine.c
> index 101cf35..1353f54 100644
> --- a/gcc/combine.c
> +++ b/gcc/combine.c
> @@ -7696,7 +7696,8 @@ make_compound_operation (rtx x, enum rtx_code in_code)
>
>next_code = (code == MEM ? MEM
>: ((code == PLUS || code == MINUS)
> - && SCALAR_INT_MODE_P (mode)) ? MEM
> + && SCALAR_INT_MODE_P (mode)
> + && (in_code == MEM)) ? MEM
>: ((code == COMPARE || COMPARISON_P (x))
> && XEXP (x, 1) == const0_rtx) ? COMPARE
>: in_code == COMPARE ? SET : in_code);
>
>
> This passed bootstrap on x86_64 and GCC tests are not regressing.
> On Aarch64 passed bootstrap tests, test case in PR passed, but few
> tests failed (failed to generate adds and subs), because there are
> patterns (extended adds and subs) based on multiplication only in
> Aarch64 backend.
>
> if this change is correct then I may need to add patterns in Aarch64
> based on shifts. Not sure about targets also.
>
> Requesting further comments/help about this.
>
> I am looking to get it fixed in stage 1.
>
> regards,
> Venkat.
[PATCH]: Conditionally include target specific files while building TSAN
Hi all,
This patch changes make file and configure under libsanitizer, to
separate out X86_64 specific file "tsan_rtl_amd64.S" from getting
build for targets other than X86_64.
Ok for trunk?
Please review.
regards,
Venkat,
ChangeLog
2015-01-19 Venkataramanan Kumar
* configure.ac (TSAN_TARGET_DEPENDENT_OBJECTS): Define.
* configure: Regenerate.
* tsan/Makefile.am
(EXTRA_libtsan_la_SOURCES): Define,
(libtsan_la_DEPENDENCIES): Likewise.
* Makefile.in: Regenerate.
* asan/Makefile.in: Regenerate.
* interception/Makefile.in: Regenerate.
* libbacktrace/Makefile.in: Regenerate.
* lsan/Makefile.in: Regenerate.
* sanitizer_common/Makefile.in: Regenerate.
* tsan/Makefile.in: Regenerate.
* ubsan/Makefile.in: Regenerate.
diff --git a/libsanitizer/Makefile.in b/libsanitizer/Makefile.in
index 0b89245..79a1be6 100644
--- a/libsanitizer/Makefile.in
+++ b/libsanitizer/Makefile.in
@@ -185,6 +185,7 @@ SED = @SED@
SET_MAKE = @SET_MAKE@
SHELL = @SHELL@
STRIP = @STRIP@
+TSAN_TARGET_DEPENDENT_OBJECTS = @TSAN_TARGET_DEPENDENT_OBJECTS@
VERSION = @VERSION@
VIEW_FILE = @VIEW_FILE@
abs_builddir = @abs_builddir@
diff --git a/libsanitizer/asan/Makefile.in b/libsanitizer/asan/Makefile.in
index 1a65944..e61ceda 100644
--- a/libsanitizer/asan/Makefile.in
+++ b/libsanitizer/asan/Makefile.in
@@ -194,6 +194,7 @@ SED = @SED@
SET_MAKE = @SET_MAKE@
SHELL = @SHELL@
STRIP = @STRIP@
+TSAN_TARGET_DEPENDENT_OBJECTS = @TSAN_TARGET_DEPENDENT_OBJECTS@
VERSION = @VERSION@
VIEW_FILE = @VIEW_FILE@
abs_builddir = @abs_builddir@
diff --git a/libsanitizer/configure b/libsanitizer/configure
index 108b1fd..4a90acf 100755
--- a/libsanitizer/configure
+++ b/libsanitizer/configure
@@ -604,6 +604,7 @@ ac_subst_vars='am__EXEEXT_FALSE
am__EXEEXT_TRUE
LTLIBOBJS
LIBOBJS
+TSAN_TARGET_DEPENDENT_OBJECTS
LIBBACKTRACE_SUPPORTED_FALSE
LIBBACKTRACE_SUPPORTED_TRUE
BACKTRACE_SUPPORTS_THREADS
@@ -12019,7 +12020,7 @@ else
lt_dlunknown=0; lt_dlno_uscore=1; lt_dlneed_uscore=2
lt_status=$lt_dlunknown
cat > conftest.$ac_ext <<_LT_EOF
-#line 12022 "configure"
+#line 12023 "configure"
#include "confdefs.h"
#if HAVE_DLFCN_H
@@ -12125,7 +12126,7 @@ else
lt_dlunknown=0; lt_dlno_uscore=1; lt_dlneed_uscore=2
lt_status=$lt_dlunknown
cat > conftest.$ac_ext <<_LT_EOF
-#line 12128 "configure"
+#line 12129 "configure"
#include "confdefs.h"
#if HAVE_DLFCN_H
@@ -16362,6 +16363,12 @@ if test "x$TSAN_SUPPORTED" = "xyes"; then
fi
+case "${target}" in
+ x86_64-*-linux-*) TSAN_TARGET_DEPENDENT_OBJECTS='tsan_rtl_amd64.lo' ;;
+ *) TSAN_TARGET_DEPENDENT_OBJECTS='' ;;
+esac
+
+
cat >confcache <<\_ACEOF
# This file is a shell script that caches the results of configure
# tests run on this system so they can be shared between configure
diff --git a/libsanitizer/configure.ac b/libsanitizer/configure.ac
index e672131..03208db 100644
--- a/libsanitizer/configure.ac
+++ b/libsanitizer/configure.ac
@@ -346,4 +346,10 @@ _EOF
])
fi
+case "${target}" in
+ x86_64-*-linux-*) TSAN_TARGET_DEPENDENT_OBJECTS='tsan_rtl_amd64.lo' ;;
+ *) TSAN_TARGET_DEPENDENT_OBJECTS='' ;;
+esac
+AC_SUBST([TSAN_TARGET_DEPENDENT_OBJECTS])
+
AC_OUTPUT
diff --git a/libsanitizer/interception/Makefile.in
b/libsanitizer/interception/Makefile.in
index 8ce4fd0..0e261b4 100644
--- a/libsanitizer/interception/Makefile.in
+++ b/libsanitizer/interception/Makefile.in
@@ -150,6 +150,7 @@ SED = @SED@
SET_MAKE = @SET_MAKE@
SHELL = @SHELL@
STRIP = @STRIP@
+TSAN_TARGET_DEPENDENT_OBJECTS = @TSAN_TARGET_DEPENDENT_OBJECTS@
VERSION = @VERSION@
VIEW_FILE = @VIEW_FILE@
abs_builddir = @abs_builddir@
diff --git a/libsanitizer/libbacktrace/Makefile.in
b/libsanitizer/libbacktrace/Makefile.in
index a4f9912..7d2e244 100644
--- a/libsanitizer/libbacktrace/Makefile.in
+++ b/libsanitizer/libbacktrace/Makefile.in
@@ -192,6 +192,7 @@ SED = @SED@
SET_MAKE = @SET_MAKE@
SHELL = @SHELL@
STRIP = @STRIP@
+TSAN_TARGET_DEPENDENT_OBJECTS = @TSAN_TARGET_DEPENDENT_OBJECTS@
VERSION = @VERSION@
VIEW_FILE = @VIEW_FILE@
abs_builddir = @abs_builddir@
diff --git a/libsanitizer/lsan/Makefile.in b/libsanitizer/lsan/Makefile.in
index bb6f95f..3ad4401 100644
--- a/libsanitizer/lsan/Makefile.in
+++ b/libsanitizer/lsan/Makefile.in
@@ -185,6 +185,7 @@ SED = @SED@
SET_MAKE = @SET_MAKE@
SHELL = @SHELL@
STRIP = @STRIP@
+TSAN_TARGET_DEPENDENT_OBJECTS = @TSAN_TARGET_DEPENDENT_OBJECTS@
VERSION = @VERSION@
VIEW_FILE = @VIEW_FILE@
abs_builddir = @abs_builddir@
diff --git a/libsanitizer/sanitizer_common/Makefile.in
b/libsanitizer/sanitizer_common/Makefile.in
index 86bf787..4a0e727 100644
--- a/libsanitizer/sanitizer_common/Makefile.in
+++ b/libsanitizer/sanitizer_common/Makefile.in
@@ -178,6 +178,7 @@ SED = @SED@
SET_MAKE = @SET_MAKE@
SHELL = @SHELL@
STRIP = @STRIP@
+TSAN_TARGET_DEPENDENT_OBJECTS = @TSAN_TARGET_DE
Re: [RFC] Tighten memory type assumption in RTL combiner pass.
Hi jeff and Richard
On 15 January 2015 at 03:10, Jeff Law wrote:
> On 01/14/15 04:27, Venkataramanan Kumar wrote:
>>
>> Hi all,
>>
>> When trying to debug GCC combiner pass with the test case in PR63949
>> ref https://gcc.gnu.org/bugzilla/show_bug.cgi?id=63949 I came across this
>> code.
>>
>> This code in "make_compound_operation" assumes that all PLUS and MINUS
>> RTX are "MEM" type for scalar int modes and tries to optimize based on
>> that assumption.
>>
>> /* Select the code to be used in recursive calls. Once we are inside an
>>address, we stay there. If we have a comparison, set to COMPARE,
>>but once inside, go back to our default of SET. */
>>
>> next_code = (code == MEM ? MEM
>> : ((code == PLUS || code == MINUS)
>> && SCALAR_INT_MODE_P (mode)) ? MEM
>> : ((code == COMPARE || COMPARISON_P (x))
>> && XEXP (x, 1) == const0_rtx) ? COMPARE
>> : in_code == COMPARE ? SET : in_code);
>>
>> next_code is passed as in_code via recursive calls to
>> "make_compound_operation". Based on that we are converting shift
>> pattern to MULT pattern.
>>
>> case ASHIFT:
>>/* Convert shifts by constants into multiplications if inside
>> an address. */
>>if (in_code == MEM && CONST_INT_P (XEXP (x, 1))
>>&& INTVAL (XEXP (x, 1)) < HOST_BITS_PER_WIDE_INT
>>&& INTVAL (XEXP (x, 1)) >= 0
>>&& SCALAR_INT_MODE_P (mode))
>> {
>>
>> Now I tried to tighten it further by adding check to see in_code is
>> also MEM type.
>> Not sure if this right thing to do. But this assumption about MEM
>> seems to be very relaxed before.
>>
>> diff --git a/gcc/combine.c b/gcc/combine.c
>> index 101cf35..1353f54 100644
>> --- a/gcc/combine.c
>> +++ b/gcc/combine.c
>> @@ -7696,7 +7696,8 @@ make_compound_operation (rtx x, enum rtx_code
>> in_code)
>>
>> next_code = (code == MEM ? MEM
>> : ((code == PLUS || code == MINUS)
>> - && SCALAR_INT_MODE_P (mode)) ? MEM
>> + && SCALAR_INT_MODE_P (mode)
>> + && (in_code == MEM)) ? MEM
>> : ((code == COMPARE || COMPARISON_P (x))
>>&& XEXP (x, 1) == const0_rtx) ? COMPARE
>> : in_code == COMPARE ? SET : in_code);
>>
>>
>> This passed bootstrap on x86_64 and GCC tests are not regressing.
>> On Aarch64 passed bootstrap tests, test case in PR passed, but few
>> tests failed (failed to generate adds and subs), because there are
>> patterns (extended adds and subs) based on multiplication only in
>> Aarch64 backend.
>>
>> if this change is correct then I may need to add patterns in Aarch64
>> based on shifts. Not sure about targets also.
>>
>> Requesting further comments/help about this.
>>
>> I am looking to get it fixed in stage 1.
>
> So the first question I would ask here is what precisely are you trying to
> accomplish? Is there some code where making this change is important or is
> it strictly a theoretical problem? If the latter, can we make it concrete
> with a well crafted testcase?
>
> jeff
The below test case which I am working on is from the PR63949
int subsi_sxth (int a, short i)
{
/* { dg-final { scan-assembler "sub\tw\[0-9\]+,.*sxth #?1" } } */
return a - ((int)i << 1);
}
The expression "a - ((int)i << 1)" is not a memory expression.
But combiner assumes that MINUS RTX as memory, and process all RTX
sub expressions with the memory assumption.
While processing ((int)i << 1) in the combiner, it first hits this code.
/* See if we have operations between an ASHIFTRT and an ASHIFT.
If so, try to merge the shifts into a SIGN_EXTEND. We could
also do this for some cases of SIGN_EXTRACT, but it doesn't
seem worth the effort; the case checked for occurs on Alpha. */
if (!OBJECT_P (lhs)
&& ! (GET_CODE (lhs) == SUBREG
&& (OBJECT_P (SUBREG_REG (lhs
&& CONST_INT_P (rhs)
&& INTVAL (rhs) < HOST_BITS_PER_WIDE_INT
&& INTVAL (rhs) < mode_width
&& (new_rtx = extract_left_shift (lhs, INTVAL (rhs))) != 0)
new_rtx = make_extraction
(mode,make_compound_operation(
new_rtx,next_code),
0, NULL_RTX, mode_width - I
[RFC] Tighten memory type assumption in RTL combiner pass.
Hi all,
When trying to debug GCC combiner pass with the test case in PR63949
ref https://gcc.gnu.org/bugzilla/show_bug.cgi?id=63949 I came across this code.
This code in "make_compound_operation" assumes that all PLUS and MINUS
RTX are "MEM" type for scalar int modes and tries to optimize based on
that assumption.
/* Select the code to be used in recursive calls. Once we are inside an
address, we stay there. If we have a comparison, set to COMPARE,
but once inside, go back to our default of SET. */
next_code = (code == MEM ? MEM
: ((code == PLUS || code == MINUS)
&& SCALAR_INT_MODE_P (mode)) ? MEM
: ((code == COMPARE || COMPARISON_P (x))
&& XEXP (x, 1) == const0_rtx) ? COMPARE
: in_code == COMPARE ? SET : in_code);
next_code is passed as in_code via recursive calls to
"make_compound_operation". Based on that we are converting shift
pattern to MULT pattern.
case ASHIFT:
/* Convert shifts by constants into multiplications if inside
an address. */
if (in_code == MEM && CONST_INT_P (XEXP (x, 1))
&& INTVAL (XEXP (x, 1)) < HOST_BITS_PER_WIDE_INT
&& INTVAL (XEXP (x, 1)) >= 0
&& SCALAR_INT_MODE_P (mode))
{
Now I tried to tighten it further by adding check to see in_code is
also MEM type.
Not sure if this right thing to do. But this assumption about MEM
seems to be very relaxed before.
diff --git a/gcc/combine.c b/gcc/combine.c
index 101cf35..1353f54 100644
--- a/gcc/combine.c
+++ b/gcc/combine.c
@@ -7696,7 +7696,8 @@ make_compound_operation (rtx x, enum rtx_code in_code)
next_code = (code == MEM ? MEM
: ((code == PLUS || code == MINUS)
- && SCALAR_INT_MODE_P (mode)) ? MEM
+ && SCALAR_INT_MODE_P (mode)
+ && (in_code == MEM)) ? MEM
: ((code == COMPARE || COMPARISON_P (x))
&& XEXP (x, 1) == const0_rtx) ? COMPARE
: in_code == COMPARE ? SET : in_code);
This passed bootstrap on x86_64 and GCC tests are not regressing.
On Aarch64 passed bootstrap tests, test case in PR passed, but few
tests failed (failed to generate adds and subs), because there are
patterns (extended adds and subs) based on multiplication only in
Aarch64 backend.
if this change is correct then I may need to add patterns in Aarch64
based on shifts. Not sure about targets also.
Requesting further comments/help about this.
I am looking to get it fixed in stage 1.
regards,
Venkat.
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 wrote: > On Thu, Sep 4, 2014 at 1:18 AM, James Greenhalgh > 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 >>> >>>* config/aarch64/aarch64.md (stack_protect_test_) 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_" >>> - [(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 >>> 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 >>> > >>> >* config/aarch64/aarch64.md (stack_protect_test_) 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_" >>> > - [(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 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 wrote: > On 4 September 2014 19:19, Venkataramanan Kumar > 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 >> >> * config/aarch64/aarch64.md (stack_protect_test_) 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.
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 * config/aarch64/aarch64.md (stack_protect_test_) 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_" - [(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%3, %x1\;ldr\t%0, %x2\;eor\t%0, %3, %0" [(set_attr "length" "12") regards, Venkat, On 4 September 2014 13:48, James Greenhalgh 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 >> >>* config/aarch64/aarch64.md (stack_protect_test_) 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_" >> - [(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 >> 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 >> > >> >* config/aarch64/aarch64.md (stack_protect_test_) 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_" >> > - [(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
* config/aarch64/aarch64.md (stack_protect_test_) 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_"
- [(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
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
>
>* config/aarch64/aarch64.md (stack_protect_test_) 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_"
> - [(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 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 * config/aarch64/aarch64.md (stack_protect_test_) 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_" - [(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][AArch64] Improve TARGET_LEGITIMIZE_ADDRESS_P hook
Hi Jiong,
(Snip)
+ && (op0 == virtual_stack_vars_rtx
+ || op0 == frame_pointer_rtx
+ || op0 == arg_pointer_rtx)
(Snip)
The above check is means that these are the ways to access the frame.
is it possible to have stack_pointer_rtx has op0?
On 1 August 2014 14:01, Jiong Wang wrote:
> currently, aarch64 LEGITIMIZE_ADDRESS_P hook will reject all "reg + offset"
> address given
> "offset" is beyond supported range.
>
> while this may be too strict. we should honor the "strict_p" parameter in
> the hook. before
> reload, we accept all offset if it's a frame access, because the offset may
> change during
> later register elimination.
>
> the early reject of "reg + offset" may cause extra registers created, and if
> that register
> live range is across function invoking then callee saved reg needed, thus
> introduce extra
> reg save/restore also.
>
> give a simple example as:
>
> int
> test15 (void)
> {
> unsigned char a[480];
> initialize_array (a, 480);
>
> if (a[0] == 0x10)
> return 1;
>
> return 0;
> }
>
> .S before the patch
> (-O2 -fPIC)
> ===
> test15:
> sub sp, sp, #480
> mov w1, 480
> stp x29, x30, [sp, -32]!
> add x29, sp, 0
> str x19, [sp, 16]
> add x19, x29, 32
> mov x0, x19
> bl initialize_array
> ldrbw0, [x19]
> ldr x19, [sp, 16]
> ldp x29, x30, [sp], 32
> cmp w0, 16
> csetw0, eq
> add sp, sp, 480
> ret
>
> .S after the patch
> ===
> test15:
> stp x29, x30, [sp, -496]!
> mov w1, 480
> add x29, sp, 0
> add x0, x29, 16
> bl initialize_array
> ldrbw0, [x29, 16]
> ldp x29, x30, [sp], 496
> cmp w0, 16
> csetw0, eq
> ret
>
> test done
> =
> no regression on aarch64-none-elf bare metal.
> bootstrap OK on aarch64.
>
> OK for trunk?
>
> thanks.
>
> gcc/
> * config/aarch64/aarch64.c (aarch64_classify_address): Accept all offset
> for frame access
> when strict_p is false.
>
> gcc/testsuite
> * gcc.target/aarch64/legitimize_stack_var_before_reload_1.c: New testcase.
Re: [RFC][ARM]: Fix reload spill failure (PR 60617)
Hi Ramana/Maxim,
On 18 June 2014 16:05, Venkataramanan Kumar
wrote:
> Hi Ramana,
>
> On 18 June 2014 15:29, Ramana Radhakrishnan wrote:
>> On Mon, Jun 16, 2014 at 1:53 PM, Venkataramanan Kumar
>> wrote:
>>> Hi Maintainers,
>>>
>>> This patch fixes the PR 60617 that occurs when we turn on reload pass
>>> in thumb2 mode.
>>>
>>> It occurs for the pattern "*ior_scc_scc" that gets generated for the 3
>>> argument of the below function call.
>>>
>>> JIT:emitStoreInt32(dst,regT0m, (op1 == dst || op2 == dst)));
>>>
>>>
>>> (snip---)
>>> (insn 634 633 635 27 (parallel [
>>> (set (reg:SI 3 r3)
>>> (ior:SI (eq:SI (reg/v:SI 110 [ dst ]) <== This operand
>>> r5 is registers gets assigned
>>> (reg/v:SI 112 [ op2 ]))
>>> (eq:SI (reg/v:SI 110 [ dst ]) <== This operand
>>> (reg/v:SI 111 [ op1 ]
>>> (clobber (reg:CC 100 cc))
>>> ]) ../Source/JavaScriptCore/jit/JITArithmetic32_64.cpp:179 300
>>> {*ior_scc_scc
>>> (snip---)
>>>
>>> The issue here is that the above pattern demands 5 registers (LO_REGS).
>>>
>>> But when we are in reload, registers r0 is used for pointer to the
>>> class, r1 and r2 for first and second argument. r7 is used for stack
>>> pointer.
>>>
>>> So we are left with r3,r4,r5 and r6. But the above patterns needs five
>>> LO_REGS. Hence we get spill failure when processing the last register
>>> operand in that pattern,
>>>
>>> In ARM port, TARGET_LIKELY_SPILLED_CLASS is defined for Thumb-1 and
>>> for thumb 2 mode there is mention of using LO_REG in the comment as
>>> below.
>>>
>>> "Care should be taken to avoid adding thumb-2 patterns that require
>>> many low registers"
>>>
>>> So conservative fix is not to allow this pattern for Thumb-2 mode.
>>
>> I don't have an additional solution off the top of my head and
>> probably need to go do some digging.
>>
>> It sounds like the conservative fix but what's the impact of doing so
>> ? Have you measured that in terms of performance or code size on a
>> range of benchmarks ?
>>
>>>
>
> I haven't done any benchmark testing. I will try and run some
> benchmarks with my patch.
>
>
>>> I allowed these pattern for Thumb2 when we have constant operands for
>>> comparison. That makes the target tests arm/thum2-cond-cmp-1.c to
>>> thum2-cond-cmp-4.c pass.
>>
>> That sounds fine and fair - no trouble there.
>>
>> My concern is with removing the register alternatives and loosing the
>> ability to trigger conditional compares on 4.9 and trunk for Thumb1
>> till the time the "new" conditional compare work makes it in.
>>
>> Ramana
I tested this conservative fix with Coremark (ran it on chromebook)and
SPEC cpu2006 (cross compiled and compared assembly differences).
With Coremark there are no performance issues. In fact there no
assembly differences with CPU flags for A15 and A9.
For SPEC2006 I cross compiled and compared assembly differences with
and without patch (-O3 -fno-common).
I have not run these bechmarks.
There are major code differences and are due to conditional compare
instructions not getting generated as you expected. This also results
in different physical register numbers assigned in the generated code
and also there are code scheduling differences when comparing it with
base.
I am showing a simple test case to showcase the conditional compare
difference I am seeing in SPEC2006 benchmarks.
char a,b;
int i;
int f( int j)
{
if ( (i >= a) ? (j <= a) : 1)
return 1;
else
return 0;
}
GCC FSF 4.9
---
movwr2, #:lower16:a
movwr3, #:lower16:i
movtr2, #:upper16:a
movtr3, #:upper16:i
ldrbr2, [r2]@ zero_extendqisi2
ldr r3, [r3]
cmp r2, r3
it le
cmple r2, r0 <== conditional compare instrucion generated.
ite ge
movge r0, #1
movlt r0, #0
bx lr
With patch
-
movwr2, #:lower16:a
movwr3, #:lower16:i
movtr2, #:upper16:a
movtr3, #:upper16:i
ldr r3, [r3]
ldrbr2, [r2]@ zero_extendqisi2
cmp r2, r3
ite le
movle r3, #0 <== Unoptimal moves generated.
movgt r3, #1 <==
cmp r2, r0
ite lt
movlt
Re: [RFC][ARM]: Fix reload spill failure (PR 60617)
Hi Ramana,
On 18 June 2014 15:29, Ramana Radhakrishnan wrote:
> On Mon, Jun 16, 2014 at 1:53 PM, Venkataramanan Kumar
> wrote:
>> Hi Maintainers,
>>
>> This patch fixes the PR 60617 that occurs when we turn on reload pass
>> in thumb2 mode.
>>
>> It occurs for the pattern "*ior_scc_scc" that gets generated for the 3
>> argument of the below function call.
>>
>> JIT:emitStoreInt32(dst,regT0m, (op1 == dst || op2 == dst)));
>>
>>
>> (snip---)
>> (insn 634 633 635 27 (parallel [
>> (set (reg:SI 3 r3)
>> (ior:SI (eq:SI (reg/v:SI 110 [ dst ]) <== This operand
>> r5 is registers gets assigned
>> (reg/v:SI 112 [ op2 ]))
>> (eq:SI (reg/v:SI 110 [ dst ]) <== This operand
>> (reg/v:SI 111 [ op1 ]
>> (clobber (reg:CC 100 cc))
>> ]) ../Source/JavaScriptCore/jit/JITArithmetic32_64.cpp:179 300
>> {*ior_scc_scc
>> (snip---)
>>
>> The issue here is that the above pattern demands 5 registers (LO_REGS).
>>
>> But when we are in reload, registers r0 is used for pointer to the
>> class, r1 and r2 for first and second argument. r7 is used for stack
>> pointer.
>>
>> So we are left with r3,r4,r5 and r6. But the above patterns needs five
>> LO_REGS. Hence we get spill failure when processing the last register
>> operand in that pattern,
>>
>> In ARM port, TARGET_LIKELY_SPILLED_CLASS is defined for Thumb-1 and
>> for thumb 2 mode there is mention of using LO_REG in the comment as
>> below.
>>
>> "Care should be taken to avoid adding thumb-2 patterns that require
>> many low registers"
>>
>> So conservative fix is not to allow this pattern for Thumb-2 mode.
>
> I don't have an additional solution off the top of my head and
> probably need to go do some digging.
>
> It sounds like the conservative fix but what's the impact of doing so
> ? Have you measured that in terms of performance or code size on a
> range of benchmarks ?
>
>>
I haven't done any benchmark testing. I will try and run some
benchmarks with my patch.
>> I allowed these pattern for Thumb2 when we have constant operands for
>> comparison. That makes the target tests arm/thum2-cond-cmp-1.c to
>> thum2-cond-cmp-4.c pass.
>
> That sounds fine and fair - no trouble there.
>
> My concern is with removing the register alternatives and loosing the
> ability to trigger conditional compares on 4.9 and trunk for Thumb1
> till the time the "new" conditional compare work makes it in.
>
> Ramana
This bug does not occur when LRA is enabled. In 4.9 FSF and trunk, the
LRA pass is enabled by default now .
May be too conservative, but is there a way to enable this pattern
when we have LRA pass and prevent it we have old reload pass?
regards,
Venkat.
>
>>
>> Regression tested with gcc 4.9 branch since in trunk this bug is
>> masked revision 209897.
>>
>> Please provide your suggestion on this patch
>>
>> regards,
>> Venkat.
[RFC][ARM]: Fix reload spill failure (PR 60617)
Hi Maintainers,
This patch fixes the PR 60617 that occurs when we turn on reload pass
in thumb2 mode.
It occurs for the pattern "*ior_scc_scc" that gets generated for the 3
argument of the below function call.
JIT:emitStoreInt32(dst,regT0m, (op1 == dst || op2 == dst)));
(snip---)
(insn 634 633 635 27 (parallel [
(set (reg:SI 3 r3)
(ior:SI (eq:SI (reg/v:SI 110 [ dst ]) <== This operand
r5 is registers gets assigned
(reg/v:SI 112 [ op2 ]))
(eq:SI (reg/v:SI 110 [ dst ]) <== This operand
(reg/v:SI 111 [ op1 ]
(clobber (reg:CC 100 cc))
]) ../Source/JavaScriptCore/jit/JITArithmetic32_64.cpp:179 300
{*ior_scc_scc
(snip---)
The issue here is that the above pattern demands 5 registers (LO_REGS).
But when we are in reload, registers r0 is used for pointer to the
class, r1 and r2 for first and second argument. r7 is used for stack
pointer.
So we are left with r3,r4,r5 and r6. But the above patterns needs five
LO_REGS. Hence we get spill failure when processing the last register
operand in that pattern,
In ARM port, TARGET_LIKELY_SPILLED_CLASS is defined for Thumb-1 and
for thumb 2 mode there is mention of using LO_REG in the comment as
below.
"Care should be taken to avoid adding thumb-2 patterns that require
many low registers"
So conservative fix is not to allow this pattern for Thumb-2 mode.
I allowed these pattern for Thumb2 when we have constant operands for
comparison. That makes the target tests arm/thum2-cond-cmp-1.c to
thum2-cond-cmp-4.c pass.
Regression tested with gcc 4.9 branch since in trunk this bug is
masked revision 209897.
Please provide your suggestion on this patch
regards,
Venkat.
diff --git a/gcc/config/arm/arm.md b/gcc/config/arm/arm.md
index 0284f95..e8fbb11 100644
--- a/gcc/config/arm/arm.md
+++ b/gcc/config/arm/arm.md
@@ -10654,7 +10654,7 @@
[(match_operand:SI 4 "s_register_operand" "r")
(match_operand:SI 5 "arm_add_operand" "rIL")])))
(clobber (reg:CC CC_REGNUM))]
- "TARGET_32BIT
+ "TARGET_ARM
&& (arm_select_dominance_cc_mode (operands[3], operands[6], DOM_CC_X_OR_Y)
!= CCmode)"
"#"
@@ -10675,6 +10675,36 @@
(set_attr "type" "multiple")]
)
+(define_insn_and_split "*ior_scc_scc_imm"
+ [(set (match_operand:SI 0 "s_register_operand" "=Ts")
+(ior:SI (match_operator:SI 3 "arm_comparison_operator"
+ [(match_operand:SI 1 "s_register_operand" "r")
+ (match_operand:SI 2 "arm_addimm_operand" "IL")])
+(match_operator:SI 6 "arm_comparison_operator"
+ [(match_operand:SI 4 "s_register_operand" "r")
+ (match_operand:SI 5 "arm_addimm_operand" "IL")])))
+ (clobber (reg:CC CC_REGNUM))]
+ "TARGET_THUMB2
+ && (arm_select_dominance_cc_mode (operands[3], operands[6], DOM_CC_X_OR_Y)
+ != CCmode)"
+ "#"
+ "TARGET_THUMB2 && reload_completed"
+ [(set (match_dup 7)
+(compare
+ (ior:SI
+ (match_op_dup 3 [(match_dup 1) (match_dup 2)])
+ (match_op_dup 6 [(match_dup 4) (match_dup 5)]))
+ (const_int 0)))
+ (set (match_dup 0) (ne:SI (match_dup 7) (const_int 0)))]
+ "operands[7]
+ = gen_rtx_REG (arm_select_dominance_cc_mode (operands[3], operands[6],
+ DOM_CC_X_OR_Y),
+CC_REGNUM);"
+ [(set_attr "conds" "clob")
+ (set_attr "length" "16")
+ (set_attr "type" "multiple")]
+)
+
; If the above pattern is followed by a CMP insn, then the compare is
; redundant, since we can rework the conditional instruction that follows.
(define_insn_and_split "*ior_scc_scc_cmp"
@@ -10714,7 +10744,7 @@
[(match_operand:SI 4 "s_register_operand" "r")
(match_operand:SI 5 "arm_add_operand" "rIL")])))
(clobber (reg:CC CC_REGNUM))]
- "TARGET_32BIT
+ "TARGET_ARM
&& (arm_select_dominance_cc_mode (operands[3], operands[6], DOM_CC_X_AND_Y)
!= CCmode)"
"#"
@@ -10737,6 +10767,38 @@
(set_attr "type" "multiple")]
)
+(define_insn_and_split "*and_scc_scc_imm"
+ [(set (match_operand:SI 0 "s_register_operand" "=Ts")
+(and:SI (match_operator:SI 3 "arm_comparison_operator"
+ [(match_operand:SI 1 "s_register_operand" "r")
+ (match_operand:SI 2 "arm_addimm_operand" "IL")])
+(match_operator:SI 6 "arm_comparison_operator"
+ [(match_operand:SI 4 "s_register_operand" "r")
+ (match_operand:SI 5 "arm_addimm_operand" "IL")])))
+ (clobber (reg:CC CC_REGNUM))]
+ "TARGET_THUMB2
+ && (arm_select_dominance_cc_mode (operands[3], operands[6], DOM_CC_X_AND_Y)
+ != CCmode)"
+ "#"
+ "TARGET_THUMB2 && reload_completed
+ && (arm_select_dominance_cc_mode (operands[3], operands[6], DOM_CC_X_AND_Y)
+ != CCmode)"
+ [(set (match_dup 7)
+(compare
+ (and:SI
+ (match
[PATCH 1/2, AARCH64]: Machine descriptions: Re: [RFC] [PATCH, AARCH64] : Using standard patterns for stack protection.
Hi Marcus,
On 14 March 2014 19:42, Marcus Shawcroft wrote:
> Hi Venkat
>
> On 5 February 2014 10:29, Venkataramanan Kumar
> wrote:
>> Hi Marcus,
>>
>>> + "ldr\\t%x2, %1\;str\\t%x2, %0\;mov\t%x2,0"
>>> + [(set_attr "length" "12")])
>>>
>>> This pattern emits an opaque sequence of instructions that cannot be
>>> scheduled, is that necessary? Can we not expand individual
>>> instructions or at least split ?
>>
>> Almost all the ports emits a template of assembly instructions.
>> I m not sure why they have to be generated this way.
>> But usage of these pattern is to clear the register that holds canary
>> value immediately after its usage.
>
> I've just read the thread Andrew pointed out, thanks, I'm happy that
> there is a good reason to do it this way. Andrew, thanks for
> providing the background.
>
> + [(set_attr "length" "12")])
> +
>
> These patterns should also set the "type" attribute, a reasonable
> value would be "multiple".
>
I have incorporated your review comments and split the patch into two.
The first patch attached here contains Aarch64 machine descriptions
for the stack protect patterns.
ChangeLog.
2014-03-19 Venkataramanan Kumar
* config/aarch64/aarch64.md (stack_protect_set, stack_protect_test)
(stack_protect_set_, stack_protect_test_): Add
machine descriptions for Stack Smashing Protector.
Tested for aarch64-none-linux-gnu target under QEMU .
regards,
Venkat.
Index: gcc/config/aarch64/aarch64.md
===
--- gcc/config/aarch64/aarch64.md (revision 208609)
+++ gcc/config/aarch64/aarch64.md (working copy)
@@ -102,6 +102,8 @@
UNSPEC_TLSDESC
UNSPEC_USHL_2S
UNSPEC_VSTRUCTDUMMY
+UNSPEC_SP_SET
+UNSPEC_SP_TEST
])
(define_c_enum "unspecv" [
@@ -3634,6 +3636,67 @@
DONE;
})
+;; Named patterns for stack smashing protection.
+(define_expand "stack_protect_set"
+ [(match_operand 0 "memory_operand")
+ (match_operand 1 "memory_operand")]
+ ""
+{
+ enum machine_mode mode = GET_MODE (operands[0]);
+
+ emit_insn ((mode == DImode
+ ? gen_stack_protect_set_di
+ : gen_stack_protect_set_si) (operands[0], operands[1]));
+ DONE;
+})
+
+(define_insn "stack_protect_set_"
+ [(set (match_operand:PTR 0 "memory_operand" "=m")
+ (unspec:PTR [(match_operand:PTR 1 "memory_operand" "m")]
+UNSPEC_SP_SET))
+ (set (match_scratch:PTR 2 "=&r") (const_int 0))]
+ ""
+ "ldr\\t%x2, %1\;str\\t%x2, %0\;mov\t%x2,0"
+ [(set_attr "length" "12")
+ (set_attr "type" "multiple")])
+
+(define_expand "stack_protect_test"
+ [(match_operand 0 "memory_operand")
+ (match_operand 1 "memory_operand")
+ (match_operand 2)]
+ ""
+{
+
+ rtx result = gen_reg_rtx (Pmode);
+
+ enum machine_mode mode = GET_MODE (operands[0]);
+
+ emit_insn ((mode == DImode
+ ? gen_stack_protect_test_di
+ : gen_stack_protect_test_si) (result,
+ operands[0],
+ operands[1]));
+
+ if (mode == DImode)
+emit_jump_insn (gen_cbranchdi4 (gen_rtx_EQ (VOIDmode, result, const0_rtx),
+ result, const0_rtx, operands[2]));
+ else
+emit_jump_insn (gen_cbranchsi4 (gen_rtx_EQ (VOIDmode, result, const0_rtx),
+ result, const0_rtx, operands[2]));
+ DONE;
+})
+
+(define_insn "stack_protect_test_"
+ [(set (match_operand:PTR 0 "register_operand")
+ (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"))]
+ ""
+ "ldr\t%x3, %x1\;ldr\t%x0, %x2\;eor\t%x0, %x3, %x0"
+ [(set_attr "length" "12")
+ (set_attr "type" "multiple")])
+
;; AdvSIMD Stuff
(include "aarch64-simd.md")
[PATCH 2/2, AARCH64] Test case changes: Re: [RFC] [PATCH, AARCH64] : Using standard patterns for stack protection.
Hi Marcus,
On 14 March 2014 19:42, Marcus Shawcroft wrote:
>>>
>>> Do we need a new effective target test, why is the existing
>>> "fstack_protector" not appropriate?
>>
>> "stack_protector" does a run time test. It failed in cross compilation
>> environment and these are compile only tests.
>
> This works fine in my cross environment, how does yours fail?
>
>
>> Also I thought richard suggested me to add a new option for this.
>> ref: http://gcc.gnu.org/ml/gcc-patches/2013-11/msg03358.html
>
> I read that comment to mean use an effective target test instead of
> matching triples. I don't see that re-using an existing effective
> target test contradicts that suggestion.
>
> Looking through the test suite I see that there are:
>
> 6 tests that use dg-do compile with dg-require-effective-target
> fstack_protector
>
> 4 tests that use dg-do run with dg-require-effective-target fstack_protector
>
> 2 tests that use dg-do run {target native} dg-require-effective-target
> fstack_protector
>
> and finally the 2 tests we are discussing that use dg-compile with a
> triple test.
>
> so there are already tests in the testsuite that use dg-do compile
> with the existing effective target test.
>
> I see no immediately obvious reason why the two tests that require
> target native require the native constraint... but I guess that is a
> different issue.
>
I used the existing dg-require-effective-target check,
"stack_protector" and added it in a separate line.
ChangeLog.
2014-03-19 Venkataramanan Kumar
* g++.dg/fstack-protector-strong.C: Add effetive target check for
stack protection.
* gcc.dg/fstack-protector-strong.c: Likewise.
These two tests are passing now for aarch64-none-linux-gnu target under QEMU.
Let me know if I can upstream these two patches.
regards,
Venkat.
Index: gcc/testsuite/g++.dg/fstack-protector-strong.C
===
--- gcc/testsuite/g++.dg/fstack-protector-strong.C (revision 208609)
+++ gcc/testsuite/g++.dg/fstack-protector-strong.C (working copy)
@@ -1,7 +1,8 @@
/* Test that stack protection is done on chosen functions. */
-/* { dg-do compile { target i?86-*-* x86_64-*-* } } */
+/* { dg-do compile } */
/* { dg-options "-O2 -fstack-protector-strong" } */
+/* { dg-require-effective-target fstack_protector } */
class A
{
Index: gcc/testsuite/gcc.dg/fstack-protector-strong.c
===
--- gcc/testsuite/gcc.dg/fstack-protector-strong.c (revision 208609)
+++ gcc/testsuite/gcc.dg/fstack-protector-strong.c (working copy)
@@ -1,7 +1,8 @@
/* Test that stack protection is done on chosen functions. */
-/* { dg-do compile { target i?86-*-* x86_64-*-* rs6000-*-* s390x-*-* } } */
+/* { dg-do compile } */
/* { dg-options "-O2 -fstack-protector-strong" } */
+/* { dg-require-effective-target fstack_protector } */
#include
Re: [RFC] [PATCH, AARCH64] : Using standard patterns for stack protection.
Hi Marcus,
> + "ldr\\t%x2, %1\;str\\t%x2, %0\;mov\t%x2,0"
> + [(set_attr "length" "12")])
>
> This pattern emits an opaque sequence of instructions that cannot be
> scheduled, is that necessary? Can we not expand individual
> instructions or at least split ?
Almost all the ports emits a template of assembly instructions.
I m not sure why they have to be generated this way.
But usage of these pattern is to clear the register that holds canary
value immediately after its usage.
> -/* { dg-do compile { target i?86-*-* x86_64-*-* rs6000-*-* s390x-*-* } } */
> +/* { dg-do compile { target stack_protection } } */
> /* { dg-options "-O2 -fstack-protector-strong" } */
>
> Do we need a new effective target test, why is the existing
> "fstack_protector" not appropriate?
"stack_protector" does a run time test. It failed in cross compilation
environment and these are compile only tests.
Also I thought richard suggested me to add a new option for this.
ref: http://gcc.gnu.org/ml/gcc-patches/2013-11/msg03358.html
regards,
Venkat.
On 4 February 2014 21:39, Marcus Shawcroft wrote:
> Hi Venkat,
>
> On 22 January 2014 16:57, Venkataramanan Kumar
> wrote:
>> Hi Marcus,
>>
>> After we changed the frame growing direction (downwards) in Aarch64,
>> the back-end now generates stack smashing set and test based on
>> generic code available in GCC.
>>
>> But most of the ports (i386, spu, rs6000, s390, sh, sparc, tilepro and
>> tilegx) define machine descriptions using standard pattern names
>> 'stack_protect_set' and 'stack_protect_test'. This is done for both
>> TLS model as well as global variable based stack guard model.
>
> + ""
> + "ldr\\t%x2, %1\;str\\t%x2, %0\;mov\t%x2,0"
> + [(set_attr "length" "12")])
>
> This pattern emits an opaque sequence of instructions that cannot be
> scheduled, is that necessary? Can we not expand individual
> instructions or at least split ?
>
> + "ldr\t%x3, %x1\;ldr\t%x0, %x2\;eor\t%x0, %x3, %x0"
> + [(set_attr "length" "12")])
>
> Likewise.
>
> -/* { dg-do compile { target i?86-*-* x86_64-*-* rs6000-*-* s390x-*-* } } */
> +/* { dg-do compile { target stack_protection } } */
> /* { dg-options "-O2 -fstack-protector-strong" } */
>
> Do we need a new effective target test, why is the existing
> "fstack_protector" not appropriate?
>
> Cheers
> /Marcus
[Ping]: [RFC] [PATCH, AARCH64] : Using standard patterns for stack protection.
Can someone review this please. regards, Venkat. On 22 January 2014 22:27, Venkataramanan Kumar wrote: > Hi Marcus, > > After we changed the frame growing direction (downwards) in Aarch64, > the back-end now generates stack smashing set and test based on > generic code available in GCC. > > But most of the ports (i386, spu, rs6000, s390, sh, sparc, tilepro and > tilegx) define machine descriptions using standard pattern names > 'stack_protect_set' and 'stack_protect_test'. This is done for both > TLS model as well as global variable based stack guard model. > > Also all these ports in their machine descriptions, have cleared the > register that loaded the canary value using an additional instruction. > > (GCC internals) > 'stack_protect_set' > This pattern, if defined, moves a ptr_mode value from the memory in operand > 1 to the memory in operand 0 without leaving the value in a register > afterward. > This is to avoid leaking the value some place that an attacker might use to > rewrite the stack guard slot after having clobbered it. > If this pattern is not defined, then a plain move pattern is generated. > (GCC internals) > > I believe this is done for extra security. Also each target can > control the way of clearing the register that loaded the canary value. > > In the attached patch, I have written machine descriptions patterns > for stack_protect_set and stack_protect_test for Aarch64. > Also I am clearing the register by moving 0 to the register while > setting the stack and using "eor" instruction while testing the stack. > > However this generates un-optimal code when compared to generic GCC code. > > While setting up stack canary , > > Generic code > > adrpx19, __stack_chk_guard > ldr x1, [x19,#:lo12:__stack_chk_guard] > str x1, [x29,40] > > > Patch > > adrpx19, __stack_chk_guard > add x1, x19, :lo12:__stack_chk_guard > ldr x2, [x1] >str x1, [x29,40] >mov x2, 0 > > while testing stack canary > > generic code > ldr x1, [x29,40] > ldr x0, [x19,#:lo12:__stack_chk_guard] > cmp x1, x0 > bne .L7 > > patch > add x19, x19, :lo12:__stack_chk_guard > ldr x1, [x29,40] > ldr x0, [x19] > eor x0, x1, x0 > cbnzx0, .L7 > > Please let me know if this change is fine for Aarch64. > > 2014-01-22 Venkataramanan Kumar > * config/aarch64/aarch64.md (stack_protect_set, stack_protect_test) > (stack_protect_set_, stack_protect_test_): Add > machine descriptions for Stack Smashing Protector. > > 2014-01-22 Venkataramanan Kumar > * lib/target-supports.exp > (check_effective_target_stack_protection): New procedure. > * g++.dg/fstack-protector-strong.C: Add target check for > stack protection. > * gcc.dg/fstack-protector-strong.c: Likewise. > > > regards, > Venkat.
Re: [RFC] [PATCH, AARCH64] : Using standard patterns for stack protection.
ping. On 22 January 2014 22:27, Venkataramanan Kumar wrote: > Hi Marcus, > > After we changed the frame growing direction (downwards) in Aarch64, > the back-end now generates stack smashing set and test based on > generic code available in GCC. > > But most of the ports (i386, spu, rs6000, s390, sh, sparc, tilepro and > tilegx) define machine descriptions using standard pattern names > ‘stack_protect_set’ and ‘stack_protect_test’. This is done for both > TLS model as well as global variable based stack guard model. > > Also all these ports in their machine descriptions, have cleared the > register that loaded the canary value using an additional instruction. > > (GCC internals) > ‘stack_protect_set’ > This pattern, if defined, moves a ptr_mode value from the memory in operand > 1 to the memory in operand 0 without leaving the value in a register > afterward. > This is to avoid leaking the value some place that an attacker might use to > rewrite the stack guard slot after having clobbered it. > If this pattern is not defined, then a plain move pattern is generated. > (GCC internals) > > I believe this is done for extra security. Also each target can > control the way of clearing the register that loaded the canary value. > > In the attached patch, I have written machine descriptions patterns > for stack_protect_set and stack_protect_test for Aarch64. > Also I am clearing the register by moving 0 to the register while > setting the stack and using "eor" instruction while testing the stack. > > However this generates un-optimal code when compared to generic GCC code. > > While setting up stack canary , > > Generic code > > adrpx19, __stack_chk_guard > ldr x1, [x19,#:lo12:__stack_chk_guard] > str x1, [x29,40] > > > Patch > > adrpx19, __stack_chk_guard > add x1, x19, :lo12:__stack_chk_guard > ldr x2, [x1] >str x1, [x29,40] >mov x2, 0 > > while testing stack canary > > generic code > ldr x1, [x29,40] > ldr x0, [x19,#:lo12:__stack_chk_guard] > cmp x1, x0 > bne .L7 > > patch > add x19, x19, :lo12:__stack_chk_guard > ldr x1, [x29,40] > ldr x0, [x19] > eor x0, x1, x0 > cbnzx0, .L7 > > Please let me know if this change is fine for Aarch64. > > 2014-01-22 Venkataramanan Kumar > * config/aarch64/aarch64.md (stack_protect_set, stack_protect_test) > (stack_protect_set_, stack_protect_test_): Add > machine descriptions for Stack Smashing Protector. > > 2014-01-22 Venkataramanan Kumar > * lib/target-supports.exp > (check_effective_target_stack_protection): New procedure. > * g++.dg/fstack-protector-strong.C: Add target check for > stack protection. > * gcc.dg/fstack-protector-strong.c: Likewise. > > > regards, > Venkat.
[RFC] [PATCH, AARCH64] : Using standard patterns for stack protection.
Hi Marcus,
After we changed the frame growing direction (downwards) in Aarch64,
the back-end now generates stack smashing set and test based on
generic code available in GCC.
But most of the ports (i386, spu, rs6000, s390, sh, sparc, tilepro and
tilegx) define machine descriptions using standard pattern names
‘stack_protect_set’ and ‘stack_protect_test’. This is done for both
TLS model as well as global variable based stack guard model.
Also all these ports in their machine descriptions, have cleared the
register that loaded the canary value using an additional instruction.
(GCC internals)
‘stack_protect_set’
This pattern, if defined, moves a ptr_mode value from the memory in operand
1 to the memory in operand 0 without leaving the value in a register afterward.
This is to avoid leaking the value some place that an attacker might use to
rewrite the stack guard slot after having clobbered it.
If this pattern is not defined, then a plain move pattern is generated.
(GCC internals)
I believe this is done for extra security. Also each target can
control the way of clearing the register that loaded the canary value.
In the attached patch, I have written machine descriptions patterns
for stack_protect_set and stack_protect_test for Aarch64.
Also I am clearing the register by moving 0 to the register while
setting the stack and using "eor" instruction while testing the stack.
However this generates un-optimal code when compared to generic GCC code.
While setting up stack canary ,
Generic code
adrpx19, __stack_chk_guard
ldr x1, [x19,#:lo12:__stack_chk_guard]
str x1, [x29,40]
Patch
adrpx19, __stack_chk_guard
add x1, x19, :lo12:__stack_chk_guard
ldr x2, [x1]
str x1, [x29,40]
mov x2, 0
while testing stack canary
generic code
ldr x1, [x29,40]
ldr x0, [x19,#:lo12:__stack_chk_guard]
cmp x1, x0
bne .L7
patch
add x19, x19, :lo12:__stack_chk_guard
ldr x1, [x29,40]
ldr x0, [x19]
eor x0, x1, x0
cbnzx0, .L7
Please let me know if this change is fine for Aarch64.
2014-01-22 Venkataramanan Kumar
* config/aarch64/aarch64.md (stack_protect_set, stack_protect_test)
(stack_protect_set_, stack_protect_test_): Add
machine descriptions for Stack Smashing Protector.
2014-01-22 Venkataramanan Kumar
* lib/target-supports.exp
(check_effective_target_stack_protection): New procedure.
* g++.dg/fstack-protector-strong.C: Add target check for
stack protection.
* gcc.dg/fstack-protector-strong.c: Likewise.
regards,
Venkat.
Index: gcc/config/aarch64/aarch64.md
===
--- gcc/config/aarch64/aarch64.md (revision 206874)
+++ gcc/config/aarch64/aarch64.md (working copy)
@@ -99,6 +99,8 @@
UNSPEC_TLSDESC
UNSPEC_USHL_2S
UNSPEC_VSTRUCTDUMMY
+UNSPEC_SP_SET
+UNSPEC_SP_TEST
])
(define_c_enum "unspecv" [
@@ -3631,6 +3633,65 @@
DONE;
})
+;; Named patterns for stack smashing protection.
+(define_expand "stack_protect_set"
+ [(match_operand 0 "memory_operand")
+ (match_operand 1 "memory_operand")]
+ ""
+{
+ enum machine_mode mode = GET_MODE (operands[0]);
+
+ emit_insn ((mode == DImode
+ ? gen_stack_protect_set_di
+ : gen_stack_protect_set_si) (operands[0], operands[1]));
+ DONE;
+})
+
+(define_insn "stack_protect_set_"
+ [(set (match_operand:PTR 0 "memory_operand" "=m")
+ (unspec:PTR [(match_operand:PTR 1 "memory_operand" "m")]
+UNSPEC_SP_SET))
+ (set (match_scratch:PTR 2 "=&r") (const_int 0))]
+ ""
+ "ldr\\t%x2, %1\;str\\t%x2, %0\;mov\t%x2,0"
+ [(set_attr "length" "12")])
+
+(define_expand "stack_protect_test"
+ [(match_operand 0 "memory_operand")
+ (match_operand 1 "memory_operand")
+ (match_operand 2)]
+ ""
+{
+
+ rtx result = gen_reg_rtx (Pmode);
+
+ enum machine_mode mode = GET_MODE (operands[0]);
+
+ emit_insn ((mode == DImode
+ ? gen_stack_protect_test_di
+ : gen_stack_protect_test_si) (result,
+ operands[0],
+ operands[1]));
+
+ if (mode == DImode)
+emit_jump_insn (gen_cbranchdi4 (gen_rtx_EQ (VOIDmode, result, const0_rtx),
+ result, const0_rtx, operands[2]));
+ else
+emit_jump_insn (gen_cbranchsi4 (gen_rtx_EQ (VOIDmode, result, const0_rtx),
+ result, const0_rtx, operands[2]));
+ DONE;
+})
+
+(define_insn "stack_protect_test_"
+ [(set (match_operand:PTR 0 "register_operand")
+ (unspec:P
[Ping] Re: [RFC] [PATCH V2, AARCH64]: Re: [RFC] [PATCH, AARCH64] Machine descriptions to support stack smashing protection
Hi Richard,
Pinging for further comments.
regards,
Venkat.
On 27 November 2013 14:24, Venkataramanan Kumar
wrote:
> Hi Richard,
>
>> I don't think it's good to have long lists of targets on generic tests.
>> Can we factor this out into a target-supports option?
>
> I have updated the patch as per your recommendation. Please let me
> know if it is fine.
>
> 2013-11-26 Venkataramanan Kumar
> * configure.ac (gcc_cv_libc_provides_tls_ssp): Add test to
> check TLS support in target C library for Aarch64.
> * configure: Regenerate.
> * config.in: Regenerate.
> * config/aarch64/aarch64.md (stack_protect_set, stack_protect_test)
> (stack_protect_set_, stack_protect_test_): Add
> initial machine description for Stack Smashing Protector.
> * config/aarch64/aarch64-linux.h (TARGET_THREAD_SSP_OFFSET): Define.
>
> 2013-11-26 Venkataramanan Kumar
> * lib/target-supports.exp
> (check_effective_target_stack_protection): New procedure.
> * g++.dg/fstack-protector-strong.C: Add target check for
> stack protection.
> * gcc.dg/fstack-protector-strong.c: Likewise.
>
> regards,
> Venkat.
>
>
> On 26 November 2013 20:23, Richard Earnshaw wrote:
>> On 26/11/13 14:16, Venkataramanan Kumar wrote:
>>> Index: gcc/testsuite/gcc.dg/fstack-protector-strong.c
>>> ===
>>> --- gcc/testsuite/gcc.dg/fstack-protector-strong.c(revision 205378)
>>> +++ gcc/testsuite/gcc.dg/fstack-protector-strong.c(working copy)
>>> @@ -1,6 +1,6 @@
>>> /* Test that stack protection is done on chosen functions. */
>>>
>>> -/* { dg-do compile { target i?86-*-* x86_64-*-* rs6000-*-* s390x-*-* } } */
>>> +/* { dg-do compile { target i?86-*-* x86_64-*-* rs6000-*-* s390x-*-*
>>> aarch64-*-* } } */
>>> /* { dg-options "-O2 -fstack-protector-strong" } */
>>>
>>
>> I don't think it's good to have long lists of targets on generic tests.
>> Can we factor this out into a target-supports option?
>>
>> R.
>>
Re: [RFC] [PATCH V2, AARCH64]: Re: [RFC] [PATCH, AARCH64] Machine descriptions to support stack smashing protection
Hi Richard,
> I don't think it's good to have long lists of targets on generic tests.
> Can we factor this out into a target-supports option?
I have updated the patch as per your recommendation. Please let me
know if it is fine.
2013-11-26 Venkataramanan Kumar
* configure.ac (gcc_cv_libc_provides_tls_ssp): Add test to
check TLS support in target C library for Aarch64.
* configure: Regenerate.
* config.in: Regenerate.
* config/aarch64/aarch64.md (stack_protect_set, stack_protect_test)
(stack_protect_set_, stack_protect_test_): Add
initial machine description for Stack Smashing Protector.
* config/aarch64/aarch64-linux.h (TARGET_THREAD_SSP_OFFSET): Define.
2013-11-26 Venkataramanan Kumar
* lib/target-supports.exp
(check_effective_target_stack_protection): New procedure.
* g++.dg/fstack-protector-strong.C: Add target check for
stack protection.
* gcc.dg/fstack-protector-strong.c: Likewise.
regards,
Venkat.
On 26 November 2013 20:23, Richard Earnshaw wrote:
> On 26/11/13 14:16, Venkataramanan Kumar wrote:
>> Index: gcc/testsuite/gcc.dg/fstack-protector-strong.c
>> ===
>> --- gcc/testsuite/gcc.dg/fstack-protector-strong.c(revision 205378)
>> +++ gcc/testsuite/gcc.dg/fstack-protector-strong.c(working copy)
>> @@ -1,6 +1,6 @@
>> /* Test that stack protection is done on chosen functions. */
>>
>> -/* { dg-do compile { target i?86-*-* x86_64-*-* rs6000-*-* s390x-*-* } } */
>> +/* { dg-do compile { target i?86-*-* x86_64-*-* rs6000-*-* s390x-*-*
>> aarch64-*-* } } */
>> /* { dg-options "-O2 -fstack-protector-strong" } */
>>
>
> I don't think it's good to have long lists of targets on generic tests.
> Can we factor this out into a target-supports option?
>
> R.
>
Index: gcc/configure.ac
===
--- gcc/configure.ac(revision 205378)
+++ gcc/configure.ac(working copy)
@@ -4896,6 +4896,24 @@
[Define if your target C library provides stack protector support])
fi
+# Test for tls based stack protector support in target C library.
+AC_CACHE_CHECK(TLS stack guard support in target C library,
+ gcc_cv_libc_provides_tls_ssp,
+ [gcc_cv_libc_provides_tls_ssp=no
+ case "$target" in
+ aarch64-*-linux* | aarch64-*-kfreebsd*-gnu | aarch64-*-knetbsd*-gnu)
+ # glibc 2.19 and later provides TLS access to stack guard canary
+ # currently set for Aarch64.
+ GCC_GLIBC_VERSION_GTE_IFELSE([2], [19],
[gcc_cv_libc_provides_tls_ssp=yes])
+ ;;
+ *) gcc_cv_libc_provides_tls_ssp=no ;;
+ esac])
+
+if test x$gcc_cv_libc_provides_tls_ssp = xyes; then
+ AC_DEFINE(TARGET_LIBC_PROVIDES_TLS_SSP, 1,
+ [Define if your target C library provides TLS stack protector
support.])
+fi
+
# Test for on the target.
GCC_TARGET_TEMPLATE([HAVE_SYS_SDT_H])
AC_MSG_CHECKING(sys/sdt.h in the target C library)
Index: gcc/configure
===
--- gcc/configure (revision 205378)
+++ gcc/configure (working copy)
@@ -27127,6 +27127,35 @@
fi
+# Test for tls based stack protector support in target C library.
+{ $as_echo "$as_me:${as_lineno-$LINENO}: checking TLS stack guard support in
target C library" >&5
+$as_echo_n "checking TLS stack guard support in target C library... " >&6; }
+if test "${gcc_cv_libc_provides_tls_ssp+set}" = set; then :
+ $as_echo_n "(cached) " >&6
+else
+ gcc_cv_libc_provides_tls_ssp=no
+ case "$target" in
+ aarch64-*-linux* | aarch64-*-kfreebsd*-gnu | aarch64-*-knetbsd*-gnu)
+ # glibc 2.19 and later provides TLS access to stack guard canary
+ # currently set for Aarch64.
+
+if test $glibc_version_major -gt 2 \
+ || ( test $glibc_version_major -eq 2 && test $glibc_version_minor -ge 19 );
then :
+ gcc_cv_libc_provides_tls_ssp=yes
+fi
+ ;;
+ *) gcc_cv_libc_provides_tls_ssp=no ;;
+ esac
+fi
+{ $as_echo "$as_me:${as_lineno-$LINENO}: result:
$gcc_cv_libc_provides_tls_ssp" >&5
+$as_echo "$gcc_cv_libc_provides_tls_ssp" >&6; }
+
+if test x$gcc_cv_libc_provides_tls_ssp = xyes; then
+
+$as_echo "#define TARGET_LIBC_PROVIDES_TLS_SSP 1" >>confdefs.h
+
+fi
+
# Test for on the target.
{ $as_echo "$as_me:${as_lineno-$LINENO}: checking sys/sdt.h in the target C
library" >&5
Index: gcc/testsuite/lib/target-supports.exp
===
--- gcc/testsuite/lib/target-supports.exp (revision 205378)
+++ gcc/testsuite/lib/target-supports.exp (working copy)
@@ -808,6 +808,17 @@
}
[RFC] [PATCH V2, AARCH64]: Re: [RFC] [PATCH, AARCH64] Machine descriptions to support stack smashing protection
Hi Joseph/Jakub,
Attached is Version 2 patch that adds machine descriptions for stack
protection in Aarch64. I have removed the incorrect test case changes
from the previous patch.
To make GCC compatible with glibc, I have added a test for aarch64 in
"GCC/configure".
This tests for the glibc version >= 2.19, generate TLS based stack
guard access, otherwise will fall back to global variable
__stack_chk_guard based access.
Also I will be submitting a glibc patch to export __stack_chk_guard
and initialize it with proper value, to make sure that it coexists
with TLS based stack guard access for aarch64.
I have posted code snippet here.
http://gcc.gnu.org/ml/gcc-patches/2013-11/msg02968.html
ChangeLog:
2013-11-26 Venkataramanan Kumar
* configure.ac (gcc_cv_libc_provides_tls_ssp): Add test to
check TLS support in target C library for Aarch64.
* configure: Regenerate.
* config.in: Regenerate.
* config/aarch64/aarch64.md (stack_protect_set, stack_protect_test)
(stack_protect_set_, stack_protect_test_): Add
initial machine description for Stack Smashing Protector.
* config/aarch64/aarch64-linux.h (TARGET_THREAD_SSP_OFFSET): Define.
2013-11-26 Venkataramanan Kumar
* g++.dg/fstack-protector-strong.C: Add aarch64 target.
* gcc.dg/fstack-protector-strong.c: Likewise.
Let me know your feed back and please advice on improving it further.
regards,
Venkat.
On 23 November 2013 09:32, Venkataramanan Kumar
wrote:
> Hi Joseph,
>
> Thank you for the detail explanation.
>
>> You need to ensure that, when new glibc is built, whatever compiler it's
>> built with, it continues to export the __stack_chk_guard symbol at version
>> GLIBC_2.17. Furthermore, if any released GCC version would generate
>> references to __stack_chk_guard when compiling code for AArch64 with stack
>> protection, you need to ensure __stack_chk_guard is a normal symbol not a
>> compat symbol so that people can continue to link against new glibc when
>> using old GCC. If it's only a limited range of development versions of
>> GCC that could have generated code using __stack_chk_guard because
>> released versions didn't support stack protection on AArch64 at all, a
>> compat symbol would probably be OK (but you should still ensure that the
>> variable gets initialized with the correct value for any applications
>> built with those development versions of GCC).
>
> As you said when THREAD_SET_STACK_GUARD is set glibc does not export
> __stack_chk_guard. So I am planning to change the export logic by
> adding a new macro EXPORT_GLOBAL_STACK_GUARD
> and set it for Aarch64 port in glibc.
>
> snip
> --- a/csu/libc-start.c
> +++ b/csu/libc-start.c
> -# ifndef THREAD_SET_STACK_GUARD
> +
> +#if !defined(THREAD_SET_STACK_GUARD) || defined(EXPORT_GLOBAL_STACK_GUARD)
> /* Only exported for architectures that don't store the stack guard canary
> in thread local area. */
> uintptr_t __stack_chk_guard attribute_relro;
> -# endif
> +#endif
> +
> snip
>
> I will find a better way to version that symbol as well. I will sent a
> RFC patch to glibc mailing list.
>
> On the GCC side, trunk GCC configure script checks and sets
> TARGET_LIBC_PROVIDES_SSP support when glibc is >=2.4
>
> -snip
> # Test for stack protector support in target C library.
> { $as_echo "$as_me:${as_lineno-$LINENO}: checking __stack_chk_fail in
> target C library" >&5
> $as_echo_n "checking __stack_chk_fail in target C library... " >&6; }
> if test "${gcc_cv_libc_provides_ssp+set}" = set; then :
> $as_echo_n "(cached) " >&6
> else
> gcc_cv_libc_provides_ssp=no
> case "$target" in
>*-*-linux* | *-*-kfreebsd*-gnu | *-*-knetbsd*-gnu)
> # glibc 2.4 and later provides __stack_chk_fail and
> # either __stack_chk_guard, or TLS access to stack guard canary.
>
> if test $glibc_version_major -gt 2 \
> || ( test $glibc_version_major -eq 2 && test $glibc_version_minor
> -ge 4 ); then :
> gcc_cv_libc_provides_ssp=yes
>
>
> if test x$gcc_cv_libc_provides_ssp = xyes; then
>
> $as_echo "#define TARGET_LIBC_PROVIDES_SSP 1" >>confdefs.h
> fi
> snip-
>
> To make GCC for AArch64 generate TLS based stack access for glibc >=
> 2.19 I need to introduce a new macro
> TARGET_LIBC_PROVIDES_TLS_SSP and check and set it for glibc >= 2.19 in
> GCC configure .
>
> Any better approach to this since it is specific to Aarch64?
>
> regards,
> Venkat.
>
> On 20 November 2013 22:38, Joseph S. Myers wrote:
>> On Wed, 20 Nov 2013, Venkataramanan Kumar wrote:
>>
Re: [RFC] [PATCH, AARCH64] Machine descriptions to support stack smashing protection
Hi Joseph,
Thank you for the detail explanation.
> You need to ensure that, when new glibc is built, whatever compiler it's
> built with, it continues to export the __stack_chk_guard symbol at version
> GLIBC_2.17. Furthermore, if any released GCC version would generate
> references to __stack_chk_guard when compiling code for AArch64 with stack
> protection, you need to ensure __stack_chk_guard is a normal symbol not a
> compat symbol so that people can continue to link against new glibc when
> using old GCC. If it's only a limited range of development versions of
> GCC that could have generated code using __stack_chk_guard because
> released versions didn't support stack protection on AArch64 at all, a
> compat symbol would probably be OK (but you should still ensure that the
> variable gets initialized with the correct value for any applications
> built with those development versions of GCC).
As you said when THREAD_SET_STACK_GUARD is set glibc does not export
__stack_chk_guard. So I am planning to change the export logic by
adding a new macro EXPORT_GLOBAL_STACK_GUARD
and set it for Aarch64 port in glibc.
snip
--- a/csu/libc-start.c
+++ b/csu/libc-start.c
-# ifndef THREAD_SET_STACK_GUARD
+
+#if !defined(THREAD_SET_STACK_GUARD) || defined(EXPORT_GLOBAL_STACK_GUARD)
/* Only exported for architectures that don't store the stack guard canary
in thread local area. */
uintptr_t __stack_chk_guard attribute_relro;
-# endif
+#endif
+
snip
I will find a better way to version that symbol as well. I will sent a
RFC patch to glibc mailing list.
On the GCC side, trunk GCC configure script checks and sets
TARGET_LIBC_PROVIDES_SSP support when glibc is >=2.4
-snip
# Test for stack protector support in target C library.
{ $as_echo "$as_me:${as_lineno-$LINENO}: checking __stack_chk_fail in
target C library" >&5
$as_echo_n "checking __stack_chk_fail in target C library... " >&6; }
if test "${gcc_cv_libc_provides_ssp+set}" = set; then :
$as_echo_n "(cached) " >&6
else
gcc_cv_libc_provides_ssp=no
case "$target" in
*-*-linux* | *-*-kfreebsd*-gnu | *-*-knetbsd*-gnu)
# glibc 2.4 and later provides __stack_chk_fail and
# either __stack_chk_guard, or TLS access to stack guard canary.
if test $glibc_version_major -gt 2 \
|| ( test $glibc_version_major -eq 2 && test $glibc_version_minor
-ge 4 ); then :
gcc_cv_libc_provides_ssp=yes
if test x$gcc_cv_libc_provides_ssp = xyes; then
$as_echo "#define TARGET_LIBC_PROVIDES_SSP 1" >>confdefs.h
fi
snip-
To make GCC for AArch64 generate TLS based stack access for glibc >=
2.19 I need to introduce a new macro
TARGET_LIBC_PROVIDES_TLS_SSP and check and set it for glibc >= 2.19 in
GCC configure .
Any better approach to this since it is specific to Aarch64?
regards,
Venkat.
On 20 November 2013 22:38, Joseph S. Myers wrote:
> On Wed, 20 Nov 2013, Venkataramanan Kumar wrote:
>
>> > I would like to see a clear description of what happens with all eight
>> > combinations of (glibc version does or doesn't support this, GCC building
>> > glibc does or doesn't support this, GCC building user program does or
>> > doesn't support this). Which of the (GCC building glibc, glibc)
>> > combinations will successfully build glibc? Will all such glibcs be
>> > binary-compatible? Will both old and new GCC work for building user
>> > programs with both old and new glibc?
>>
>> Can you please clarify why we need to consider "the gcc compiler that
>> builds the glibc" in the combinations you want me to describe. I am
>> not able to understand that.
>
> Let's imagine this support goes in GCC 4.9 and the glibc support goes in
> glibc 2.19, whereas GCC 4.8 and glibc 2.18 are versions without this
> support.
>
> * Building glibc 2.18 with GCC 4.8 already works (I presume).
>
> * Suppose you use GCC 4.9 to build glibc 2.18. Does this work? If it
> works, it must produce a glibc binary that's ABI compatible with one built
> with GCC 4.8, meaning same symbols exported at same symbol versions.
>
> * Suppose you build glibc 2.19 with GCC 4.8. Does this work? If it does,
> then it must be ABI compatible with 2.18 (meaning the symbols exported at
> GLIBC_2.18 or earlier versions must be exactly the same as exported at
> those versions in 2.18).
>
> * Suppose you build glibc 2.19 with GCC 4.9. This needs to work and must
> again produce a binary compatible with the previous ones.
>
> Note: there is no current glibc support for architectures that gained
> TLS-based stack guards after 2.4 (meaning they need both a TLS-based
> scheme and backwards compatibility for binaries using __st
Re: [RFC] [PATCH, AARCH64] Machine descriptions to support stack smashing protection
Hi Joseph,
On 19 November 2013 21:53, Joseph S. Myers wrote:
> On Tue, 19 Nov 2013, Jakub Jelinek wrote:
>
>> On Tue, Nov 19, 2013 at 04:30:21PM +0530, Venkataramanan Kumar wrote:
>> > This is RFC patch that adds machine descriptions to support stack
>> > smashing protection in AArch64.
>>
>> Most of the testsuite changes look wrong. The fact that aarch64
>> gets stack protector support doesn't mean all other targets do as well.
>> So please leave all the changes that remove native or stack_protector
>> requirements out.
>
> The tests for "target native" look wrong for me ("native" conditionals
> only make sense for special cases such as guality tests that expect to
> exec another tool on the host) - so I think they should be removed, but
> that removal needs submitting as a separate patch.
>
Yes apologies for that, I will send another patch.
> I would like to see a clear description of what happens with all eight
> combinations of (glibc version does or doesn't support this, GCC building
> glibc does or doesn't support this, GCC building user program does or
> doesn't support this). Which of the (GCC building glibc, glibc)
> combinations will successfully build glibc? Will all such glibcs be
> binary-compatible? Will both old and new GCC work for building user
> programs with both old and new glibc?
Can you please clarify why we need to consider "the gcc compiler that
builds the glibc" in the combinations you want me to describe. I am
not able to understand that.
I tested the below three GCC compiler versions for building user programs.
1) GCC trunk (without my patch) generates code for "stack protection
set/test" when -fstack-protector-all is enabled.
This is based on global variable "__stack_chk_guard" access, which
glibc supports from version 2.4.
-snip-
adrpx0, __stack_chk_guard
add x0, x0, :lo12:__stack_chk_guard
ldr x0, [x0]
str x0, [x29,24]
-snip-
GCC has generic code emitted for stack protection, enabled for targets
where frame growth is downwards. The patch for frame growing downwards
in Aarch64 is recently committed in trunk.
2) Now with the patch, GCC compiler will generate TLS based access.
snip-
mrs x0, tpidr_el0
ldr x1, [x0,-8]
str x1, [x29,24]
-snip-
I need to check if target glibc for Aarch64 supports TLS based ssp
support. Currently some targets check and generate TLS based access
when glibc version is >=2.4.
3) GCC linaro compiler packaged with open embedded image and which I
boot in V8 foundation model as I don't have access to Aarch64 hardware
yet.
It will not emit any stack protection code.
test.c:1:0: warning: -fstack-protector not supported for this target
[enabled by default]
regards,
Venkat.
[RFC] [PATCH, AARCH64] Machine descriptions to support stack smashing protection
Hi Maintainers,
This is RFC patch that adds machine descriptions to support stack
smashing protection in AArch64.
I have written a very simple patch that prints "stack set" and "stack
test" as template of instructions.
I had 2 assumptions.
1) For "stack_protect_set" and "stack_protect_test", I
used "memory_operand" as predicate.
GCC pushes the memory operand in a register much
earlier during expand phase before these patterns are invoked.
So assuming that I will get a memory operand "__stack_chk_gaurd" in a
register when we are not using TLS based stack guard.
2) For the TLS case, assuming stack guard value will be stored at "-8"
offset from "tp" GCC generates below code for stack set.
mrs x0, tpidr_el0
ldr x1, [x0,-8]
str x1, [x29,24]
mov x1,0
I submitted Glibc patches some time before
https://sourceware.org/ml/libc-ports/2013-08/msg00044.html.
There are few regressions, the pthread_cancel tests in glibc fails I
am currently debugging :(.
GCC with the patch generates below code for stack test
ldr x1, [x29,24]
ldr x0, [x0,-8]
eor x0, x1, x0
cbnzx0, .L4
.
..
.L4:
bl __stack_chk_f
I generate "eor" since it has 2 purpose one for checking equality, and
two for clearing the canary loaded register.
Request your feedback to shape this into a better patch.
regards,
Venkat.
Index: gcc/testsuite/gcc.dg/pr46440.c
===
--- gcc/testsuite/gcc.dg/pr46440.c (revision 204932)
+++ gcc/testsuite/gcc.dg/pr46440.c (working copy)
@@ -1,7 +1,6 @@
/* PR rtl-optimization/46440 */
/* { dg-do compile } */
/* { dg-options "-O -fstack-protector -fno-tree-dominator-opts -fno-tree-fre"
} */
-/* { dg-require-effective-target fstack_protector } */
int i;
Index: gcc/testsuite/gcc.dg/ssp-1.c
===
--- gcc/testsuite/gcc.dg/ssp-1.c(revision 204932)
+++ gcc/testsuite/gcc.dg/ssp-1.c(working copy)
@@ -1,6 +1,4 @@
-/* { dg-do run { target native } } */
/* { dg-options "-fstack-protector" } */
-/* { dg-require-effective-target fstack_protector } */
#include
Index: gcc/testsuite/gcc.dg/pr47766.c
===
--- gcc/testsuite/gcc.dg/pr47766.c (revision 204932)
+++ gcc/testsuite/gcc.dg/pr47766.c (working copy)
@@ -1,6 +1,5 @@
/* { dg-do compile } */
/* { dg-options "-O2 -fstack-protector" } */
-/* { dg-require-effective-target fstack_protector } */
int
parse_opt (int key)
Index: gcc/testsuite/gcc.dg/ssp-2.c
===
--- gcc/testsuite/gcc.dg/ssp-2.c(revision 204932)
+++ gcc/testsuite/gcc.dg/ssp-2.c(working copy)
@@ -1,7 +1,5 @@
-/* { dg-do run { target native } } */
/* { dg-options "-fstack-protector" } */
/* { dg-options "-fstack-protector -Wl,-multiply_defined,suppress" { target
*-*-darwin* } } */
-/* { dg-require-effective-target fstack_protector } */
#include
Index: gcc/testsuite/gcc.dg/fstack-protector-strong.c
===
--- gcc/testsuite/gcc.dg/fstack-protector-strong.c (revision 204932)
+++ gcc/testsuite/gcc.dg/fstack-protector-strong.c (working copy)
@@ -1,6 +1,6 @@
/* Test that stack protection is done on chosen functions. */
-/* { dg-do compile { target i?86-*-* x86_64-*-* rs6000-*-* s390x-*-* } } */
+/* { dg-do compile { target i?86-*-* x86_64-*-* rs6000-*-* s390x-*-*
aarch64-*-*} } */
/* { dg-options "-O2 -fstack-protector-strong" } */
#include
Index: gcc/testsuite/g++.dg/fstack-protector-strong.C
===
--- gcc/testsuite/g++.dg/fstack-protector-strong.C (revision 204932)
+++ gcc/testsuite/g++.dg/fstack-protector-strong.C (working copy)
@@ -1,6 +1,6 @@
/* Test that stack protection is done on chosen functions. */
-/* { dg-do compile { target i?86-*-* x86_64-*-* } } */
+/* { dg-do compile { target i?86-*-* x86_64-*-* aarch64-*-* } } */
/* { dg-options "-O2 -fstack-protector-strong" } */
class A
Index: gcc/config/aarch64/aarch64-linux.h
===
--- gcc/config/aarch64/aarch64-linux.h (revision 204932)
+++ gcc/config/aarch64/aarch64-linux.h (working copy)
@@ -43,4 +43,9 @@
} \
while (0)
+#ifdef TARGET_LIBC_PROVIDES_SSP
+/* Aarch64 glibc provides __stack_chk_guard in [tp - 0x8]. */
+#define TARGET_THREAD_SSP_OFFSET (-1 * GET_MODE_SIZE (ptr_mode))
+#endif
+
#endif /* GCC_AARCH64_LINUX_H */
Index: gcc/config/aarch64/aarch64.md
===
--- gcc/config/aarch64/aarch64.md (revision 204932)
+++ gcc/config/aarch64
Re: [RFC Patch, Aarch64] : Macros for profile code generation to enable gprof support
Hi Marcus,
I have re-based the patch and tested for aarch64-none-elf with no regressions.
Also for aarch64-unknown-linux-gnu the following test cases passes.
Before:
UNSUPPORTED: gcc.dg/nested-func-4.c
UNSUPPORTED: gcc.dg/pr43643.c:
UNSUPPORTED: gcc.dg/nest.c
UNSUPPORTED: gcc.dg/20021014-1.c
UNSUPPORTED: gcc.dg/pr32450.c
UNSUPPORTED: g++.dg/other/profile1.C -std=gnu++98
UNSUPPORTED: g++.dg/other/profile1.C -std=gnu++11
After:
---
PASS: gcc.dg/nested-func-4.c (test for excess errors)
PASS: gcc.dg/nested-func-4.c execution test
PASS: gcc.dg/pr43643.c (test for excess errors)
PASS: gcc.dg/pr43643.c execution test
PASS: gcc.dg/nest.c (test for excess errors)
PASS: gcc.dg/nest.c execution test
PASS: gcc.dg/20021014-1.c (test for excess errors)
PASS: gcc.dg/20021014-1.c execution test
PASS: gcc.dg/pr32450.c (test for excess errors)
PASS: gcc.dg/pr32450.c execution test
PASS: g++.dg/other/profile1.C -std=gnu++98 (test for excess errors)
PASS: g++.dg/other/profile1.C -std=gnu++98 execution test
PASS: g++.dg/other/profile1.C -std=gnu++11 (test for excess errors)
PASS: g++.dg/other/profile1.C -std=gnu++11 execution test
Please let me know if I can commit it to trunk, given that glibc
patches are upstreamed.
2013-10-28 Venkataramanan Kumar
* config/aarch64/aarch64.h (MCOUNT_NAME): Define.
(NO_PROFILE_COUNTERS): Likewise.
(PROFILE_HOOK): Likewise.
(FUNCTION_PROFILER): Likewise.
* config/aarch64/aarch64.c (aarch64_function_profiler): Remove.
regards,
Venkat.
On 27 August 2013 13:05, Marcus Shawcroft wrote:
> Hi Venkat,
>
> On 3 August 2013 19:01, Venkataramanan Kumar
> wrote:
>
>> This patch adds macros to support gprof in Aarch64. The difference
>> from the previous patch is that the compiler, while generating
>> "mcount" routine for an instrumented function, also passes the return
>> address as argument.
>>
>> The "mcount" routine in glibc will be modified as follows.
>>
>> (-Snip-)
>> #define MCOUNT \
>> -void __mcount (void)
>> \
>> +void __mcount (void* frompc)
>>\
>> {
>> \
>> - mcount_internal ((u_long) RETURN_ADDRESS (1), (u_long) RETURN_ADDRESS
>> (0)); \
>> + mcount_internal ((u_long) frompc, (u_long) RETURN_ADDRESS (0)); \
>> }
>> (-Snip-)
>
>
>> If this is Ok I will send the patch to glibc as well.
>
>> 2013-08-02 Venkataramanan Kumar
>>
>> * config/aarch64/aarch64.h (MCOUNT_NAME): Define.
>>(NO_PROFILE_COUNTERS): Likewise.
>>(PROFILE_HOOK): Likewise.
>>(FUNCTION_PROFILER): Likewise.
>> * config/aarch64/aarch64.c (aarch64_function_profiler): Remove.
>>.
>>
>> regards,
>> Venkat.
>
> + emit_library_call (fun, LCT_NORMAL, VOIDmode, 1,lr,Pmode); \
> +}
>
> GNU coding style requires spaces after the commas, but otherwise I
> have no further comments on this patch. Post the glibc patch please.
>
> Thanks
> /Marcus
Index: gcc/config/aarch64/aarch64.c
===
--- gcc/config/aarch64/aarch64.c(revision 202934)
+++ gcc/config/aarch64/aarch64.c(working copy)
@@ -3857,13 +3857,6 @@
output_addr_const (f, x);
}
-void
-aarch64_function_profiler (FILE *f ATTRIBUTE_UNUSED,
- int labelno ATTRIBUTE_UNUSED)
-{
- sorry ("function profiling");
-}
-
bool
aarch64_label_mentioned_p (rtx x)
{
Index: gcc/config/aarch64/aarch64.h
===
--- gcc/config/aarch64/aarch64.h(revision 202934)
+++ gcc/config/aarch64/aarch64.h(working copy)
@@ -783,9 +783,23 @@
#define PRINT_OPERAND_ADDRESS(STREAM, X) \
aarch64_print_operand_address (STREAM, X)
-#define FUNCTION_PROFILER(STREAM, LABELNO) \
- aarch64_function_profiler (STREAM, LABELNO)
+#define MCOUNT_NAME "_mcount"
+#define NO_PROFILE_COUNTERS 1
+
+/* Emit rtl for profiling. Output assembler code to FILE
+ to call "_mcount" for profiling a function entry. */
+#define PROFILE_HOOK(LABEL)\
+{ \
+ rtx fun,lr; \
+ lr = get_hard_reg_initial_val (Pmode, LR_REGNUM);\
+ fun = gen_rtx_SYMBOL_REF (Pmode, MCOUNT_NAME); \
+ emit_library_call (fun, LCT_NORMAL, VOIDmode, 1, lr, Pmode); \
+}
+
+/* All the work done in PROFILE_HOOK, but still required. */
+#define FUNCTION_PROFILER(STREAM, LABELNO) do { } while (0)
+
/* For some reason, the Linux h
Re: [RFC Patch, Aarch64] : Macros for profile code generation to enable gprof support
ping!
On 3 August 2013 23:31, Venkataramanan Kumar
wrote:
> Hi Maintainers,
>
> This patch adds macros to support gprof in Aarch64. The difference
> from the previous patch is that the compiler, while generating
> "mcount" routine for an instrumented function, also passes the return
> address as argument.
>
> The "mcount" routine in glibc will be modified as follows.
>
> (-Snip-)
> #define MCOUNT \
> -void __mcount (void)
> \
> +void __mcount (void* frompc)
>\
> {
> \
> - mcount_internal ((u_long) RETURN_ADDRESS (1), (u_long) RETURN_ADDRESS
> (0)); \
> + mcount_internal ((u_long) frompc, (u_long) RETURN_ADDRESS (0)); \
> }
> (-Snip-)
>
> Also in Aarch64 cases__builtin_return_adderess(n) where n>0, still be
> returning 0 as it was doing before.
>
> If this is Ok I will send the patch to glibc as well.
>
> 2013-08-02 Venkataramanan Kumar
>
> * config/aarch64/aarch64.h (MCOUNT_NAME): Define.
>(NO_PROFILE_COUNTERS): Likewise.
>(PROFILE_HOOK): Likewise.
>(FUNCTION_PROFILER): Likewise.
> * config/aarch64/aarch64.c (aarch64_function_profiler): Remove.
>.
>
> regards,
> Venkat.
[RFC Patch, Aarch64] : Macros for profile code generation to enable gprof support
Hi Maintainers,
This patch adds macros to support gprof in Aarch64. The difference
from the previous patch is that the compiler, while generating
"mcount" routine for an instrumented function, also passes the return
address as argument.
The "mcount" routine in glibc will be modified as follows.
(-Snip-)
#define MCOUNT \
-void __mcount (void) \
+void __mcount (void* frompc)
\
{\
- mcount_internal ((u_long) RETURN_ADDRESS (1), (u_long) RETURN_ADDRESS (0)); \
+ mcount_internal ((u_long) frompc, (u_long) RETURN_ADDRESS (0)); \
}
(-Snip-)
Also in Aarch64 cases__builtin_return_adderess(n) where n>0, still be
returning 0 as it was doing before.
If this is Ok I will send the patch to glibc as well.
2013-08-02 Venkataramanan Kumar
* config/aarch64/aarch64.h (MCOUNT_NAME): Define.
(NO_PROFILE_COUNTERS): Likewise.
(PROFILE_HOOK): Likewise.
(FUNCTION_PROFILER): Likewise.
* config/aarch64/aarch64.c (aarch64_function_profiler): Remove.
.
regards,
Venkat.
gcc.gprof.diff
Description: Binary data
Re: [Patch, Aarch64]: Handle return address via. frame pointer
Hi Andrew, I can see that rs6000 port uses a flag "cfun->machine->ra_needs_full_frame = 1". But I need to check if this flag helps to generate frame for all the functions in a compilation unit. The requirement is that frame need to be preserved by any function that calls our function which uses _builtin_return_address(1), even when -fomit-frame-pointer is used. regards, venkat. On 29 July 2013 11:57, Andrew Pinski wrote: > On Sun, Jul 28, 2013 at 3:53 AM, Venkataramanan Kumar > wrote: >> Hi Maintainers, >> >> This patch adds supports to handle return address via. frame pointer. > > I noticed this patch causes undefined behavior when > -fomit-frame-pointer and __builtin_return_address(1) is used. On > PowerPC it is defined correctly that is it generates a frame for that > case. Now on x86_64 it might be undefined but I think that is just > wrong since __builtin_return_address is just used for debugging > anyways. > > Thanks, > Andrew > >> >> gcc/ChangeLog >> - >> >> 2013-07-28 Venkataramanan Kumar >> >>* config/aarch64/aarch64.c (aarch64_return_addr): Handle returning >> address from a frame. >> >> >> Regression tested with aarch64-none-elf with V8 foundation model. >> >> regards, >> Venkat.
[Patch, Aarch64] : Macros for profile code generation to enable gprof support
Hi Maintainers, This patch defines some macros that are needed for profile generation support in Aarch64. I tested this patch on top of the patch http://gcc.gnu.org/ml/gcc-patches/2013-07/msg01333.html Regression tested with aarch64-none-elf with V8 foundation model after re basing to latest gcc trunk. Also ran profile related tests against the latest trunk for aarch64-unknown-linux-gnu. Below tests are passing with the patch. Before: UNSUPPORTED: gcc.dg/nested-func-4.c UNSUPPORTED: gcc.dg/pr43643.c: UNSUPPORTED: gcc.dg/nest.c UNSUPPORTED: gcc.dg/20021014-1.c UNSUPPORTED: gcc.dg/pr32450.c UNSUPPORTED: g++.dg/other/profile1.C -std=gnu++98 UNSUPPORTED: g++.dg/other/profile1.C -std=gnu++11 After: --- PASS: gcc.dg/nested-func-4.c (test for excess errors) PASS: gcc.dg/nested-func-4.c execution test PASS: gcc.dg/pr43643.c (test for excess errors) PASS: gcc.dg/pr43643.c execution test PASS: gcc.dg/nest.c (test for excess errors) PASS: gcc.dg/nest.c execution test PASS: gcc.dg/20021014-1.c (test for excess errors) PASS: gcc.dg/20021014-1.c execution test PASS: gcc.dg/pr32450.c (test for excess errors) PASS: gcc.dg/pr32450.c execution test PASS: g++.dg/other/profile1.C -std=gnu++98 (test for excess errors) PASS: g++.dg/other/profile1.C -std=gnu++98 execution test PASS: g++.dg/other/profile1.C -std=gnu++11 (test for excess errors) PASS: g++.dg/other/profile1.C -std=gnu++11 execution test regards, Venkat. gcc.gprof.diff Description: Binary data
[Patch, Aarch64]: Handle return address via. frame pointer
Hi Maintainers, This patch adds supports to handle return address via. frame pointer. gcc/ChangeLog - 2013-07-28 Venkataramanan Kumar * config/aarch64/aarch64.c (aarch64_return_addr): Handle returning address from a frame. Regression tested with aarch64-none-elf with V8 foundation model. regards, Venkat. gcc.return_addr_from_frame_pointer.diff Description: Binary data
Re: [Patch, Aarch64] [RFC] : Macros for profile code generation to enable gprof support
Hi Marcus,
Please find in that attachment two separate patches.
One for getting return address from frame, and second one conating
macros for profile code generation.
I have incorporated your review comments.
> +# We support profiling for AArch64 linux target.
> +if { [istarget aarch64*-linux-*]
> + && ($test_what == "-p" || $test_what == "-pg") } {
> + return 1
> }
>
> Can't this clause be removed completely now ?
We are not supporting profiling in "newlib". So it is still disabled
for aarch64-none-elf targets which uses newlib.
Patch1
-
gcc/ChangeLog
-
2013-05-12 Venkataramanan Kumar
* config/aarch64/aarch64.c (aarch64_return_addr): Handle returning
address from a frame.
Patch2
--
2013-05-12 Venkataramanan Kumar
* config/aarch64/aarch64.h (MCOUNT_NAME): Define.
(NO_PROFILE_COUNTERS): Likewise.
(PROFILE_HOOK): Likewise.
(FUNCTION_PROFILER): Likewise.
* config/aarch64/aarch64.c (aarch64_function_profiler): Remove.
(aarch64_frame_pointer_required): Enable frame pointer when profiling.
gcc/testsuite/ChangeLog
---
2013-05-12 Venkataramanan Kumar
* lib/target-supports.exp (check_profiling_available): Enable
aarch64*-linux-*
regards,
Venkat.
On 9 May 2013 15:02, Marcus Shawcroft wrote:
> On 9 May 2013 07:48, Venkataramanan Kumar
> wrote:
>> Hi Maintainers,
>>
>> The attach patch contains the following for aarch64 backend to enable
>> gprof support.
>>
>> 1. Changes to "aarch64_return_addr" to get return address from a stack frame.
>> 2. Defines macros associated with generating code for profiling.
>
> Hi Venkat,
> Can we split this into separate patches for 1) and 2) please.
>
> General comment, GNU coding style places two space after each period,
> please update each of the comments that don't conform.
>
> +
> +/* All the work done in PROFILE_HOOK, but still required. */
> +#define FUNCTION_PROFILER(FILE, LABELNO) do { } while (0)
> +
>
> Please don't move the location of FUNCTION_PROFILER, just change its
> definition.
>
>
> +/* Implement RETURN_ADDR_RTX. As per Aarch64 ABI return address
> + is stored at an offset 8 from the frame pointer of a function. */
>
> Aarch64 -> AArch64
>
> Comma after ABI ?
>
> Specify 'bytes' after the 8.
>
> + if(count != 0)
> +{
>
> GNU coding style places a space before the '('.
>
> + mem_lr = gen_frame_mem (DImode,
>
> Double space after "=" -> single space.
>
> -# We don't yet support profiling for AArch64.
> -if { [istarget aarch64*-*-*]
> - && ([lindex $test_what 1] == "-p"
> - || [lindex $test_what 1] == "-pg") } {
> - return 0
> +# We support profiling for AArch64 linux target.
> +if { [istarget aarch64*-linux-*]
> + && ($test_what == "-p" || $test_what == "-pg") } {
> + return 1
> }
>
> Can't this clause be removed completely now ?
>
> Cheers
> /Marcus
gcc.return_addr_from_frame.diff
Description: Binary data
gcc.profile.diff
Description: Binary data
[Patch, Aarch64] [RFC] : Macros for profile code generation to enable gprof support
Hi Maintainers, The attach patch contains the following for aarch64 backend to enable gprof support. 1. Changes to "aarch64_return_addr" to get return address from a stack frame. 2. Defines macros associated with generating code for profiling. gcc/ChangeLog - 2013-05-09 Venkataramanan Kumar * config/aarch64/aarch64.h (MCOUNT_NAME): Define. (NO_PROFILE_COUNTERS): Likewise. (PROFILE_HOOK): Likewise. (FUNCTION_PROFILER): Likewise. * config/aarch64/aarch64.c (aarch64_return_addr): Handle returning address from a frame. (aarch64_function_profiler): Remove. (aarch64_frame_pointer_required): Enable frame pointer when profiling. gcc/testsuite/ChangeLog --- 2013-05-09 Venkataramanan Kumar * lib/target-supports.exp (check_profiling_available): Enable aarch64*-linux-* Testing: --- gcc regression tests for aarch64-none-elf passed. gcc regression tests for aarch64-linux-none-gnu is underway. With a small change in glibc, gcc now emits gmon.out when compiled with -pg and run on openembedded/V8 model. regards, Venkat. gcc.profile.diff Description: Binary data
[Patch, AArch64, AArch64-4.7] Backport Optimize cmp in some cases patch
Hi Maintainers,
The attached patch backports the gcc trunk patch
http://gcc.gnu.org/ml/gcc-patches/2013-01/msg00143.html to
"ARM/aarch64-4.7-branch" branch.
ChangeLog.aarch64
2013-01-27 Venkataramanan Kumar
Backport from mainline.
2013-01-04 Andrew Pinski
* config/aarch64/aarch64.c (aarch64_fixed_condition_code_regs):
New function.
(TARGET_FIXED_CONDITION_CODE_REGS): Define
Path is attached. Please let me know if I can change "-1" to
"INVALID_REGNUM" and commit.
Built gcc and tested the gcc testsuites for the aarch64-none-elf
target with ARMv8 Foundation model. No new regressions.
Ok to for the ARM/aarch64-4.7-branch ?
regards,
Venkat.
Index: gcc/config/aarch64/aarch64.c
===
--- gcc/config/aarch64/aarch64.c(revision 195486)
+++ gcc/config/aarch64/aarch64.c(working copy)
@@ -2971,6 +2971,16 @@
return REAL_VALUES_EQUAL (r, dconst0);
}
+/* Return the fixed registers used for condition codes. */
+
+static bool
+aarch64_fixed_condition_code_regs (unsigned int *p1, unsigned int *p2)
+{
+ *p1 = CC_REGNUM;
+ *p2 = -1;
+ return true;
+}
+
enum machine_mode
aarch64_select_cc_mode (RTX_CODE code, rtx x, rtx y)
{
@@ -7809,6 +7819,9 @@
#undef TARGET_EXPAND_BUILTIN_VA_START
#define TARGET_EXPAND_BUILTIN_VA_START aarch64_expand_builtin_va_start
+#undef TARGET_FIXED_CONDITION_CODE_REG
+#define TARGET_FIXED_CONDITION_CODE_REGS aarch64_fixed_condition_code_regs
+
#undef TARGET_FUNCTION_ARG
#define TARGET_FUNCTION_ARG aarch64_function_arg
Index: gcc/testsuite/gcc.target/aarch64/cmp-1.c
===
--- gcc/testsuite/gcc.target/aarch64/cmp-1.c(revision 0)
+++ gcc/testsuite/gcc.target/aarch64/cmp-1.c(working copy)
@@ -0,0 +1,15 @@
+/* { dg-do compile } */
+/* { dg-options "-O2" } */
+
+int f(int a, int b)
+{
+ if(ab)
+return -1;
+ return 0;
+}
+
+/* We should optimize away the second cmp. */
+/* { dg-final { scan-assembler-times "cmp\tw" 1 } } */
+
