| Message ID | 20110728181337.GA17842@intel.com |
|---|---|
| State | New |
| Headers | show |
On Thu, Jul 28, 2011 at 8:13 PM, H.J. Lu <hongjiu.lu@intel.com> wrote:
> This patch adds x32 support to UNSPEC_SP_XXX patterns. OK for trunk?
http://gcc.gnu.org/contribute.html#patches
Uros.
On Thu, Jul 28, 2011 at 11:55 AM, Uros Bizjak <ubizjak@gmail.com> wrote: > On Thu, Jul 28, 2011 at 8:13 PM, H.J. Lu <hongjiu.lu@intel.com> wrote: > >> This patch adds x32 support to UNSPEC_SP_XXX patterns. OK for trunk? > > http://gcc.gnu.org/contribute.html#patches > Sorry. I should have mentioned testcase in: http://gcc.gnu.org/bugzilla/show_bug.cgi?id=47766 Actually, they are in gcc testsuite. I noticed them when I run gcc testsuite on x32.
On Thu, Jul 28, 2011 at 9:03 PM, H.J. Lu <hjl.tools@gmail.com> wrote: >>> This patch adds x32 support to UNSPEC_SP_XXX patterns. OK for trunk? >> >> http://gcc.gnu.org/contribute.html#patches >> > > Sorry. I should have mentioned testcase in: > > http://gcc.gnu.org/bugzilla/show_bug.cgi?id=47766 > > Actually, they are in gcc testsuite. I noticed them when > I run gcc testsuite on x32. This looks like a middle-end problem to me. According to the documentation: --quote-- `stack_protect_set' This pattern, if defined, moves a `Pmode' 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. `stack_protect_test' This pattern, if defined, compares a `Pmode' value from the memory in operand 1 with the memory in operand 0 without leaving the value in a register afterward and branches to operand 2 if the values weren't equal. If this pattern is not defined, then a plain compare pattern and conditional branch pattern is used. --quote-- According to the documentation, x86 patterns are correct. However, middle-end fails to extend ptr_mode value to Pmode, and in function.c, stack_protect_prologue/stack_protect_epilogue, we already have ptr_mode (SImode) operand: (mem/v/f/c/i:SI (plus:DI (reg/f:DI 54 virtual-stack-vars) (const_int -4 [0xfffffffffffffffc])) [2 D.2704+0 S4 A32]) (mem/v/f/c/i:SI (symbol_ref:DI ("__stack_chk_guard") [flags 0x40] <var_decl 0x7ffc35aa0be0 __stack_chk_guard>) [2 __stack_chk_guard+0 S4 A32]) An opinion of a RTL maintainer (CC'd) is needed here. Target definition is OK in its current form. Uros.
On Thu, Jul 28, 2011 at 1:03 PM, Uros Bizjak <ubizjak@gmail.com> wrote: > On Thu, Jul 28, 2011 at 9:03 PM, H.J. Lu <hjl.tools@gmail.com> wrote: > >>>> This patch adds x32 support to UNSPEC_SP_XXX patterns. OK for trunk? >>> >>> http://gcc.gnu.org/contribute.html#patches >>> >> >> Sorry. I should have mentioned testcase in: >> >> http://gcc.gnu.org/bugzilla/show_bug.cgi?id=47766 >> >> Actually, they are in gcc testsuite. I noticed them when >> I run gcc testsuite on x32. > > This looks like a middle-end problem to me. > > According to the documentation: > > --quote-- > `stack_protect_set' > This pattern, if defined, moves a `Pmode' 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. > > `stack_protect_test' > This pattern, if defined, compares a `Pmode' value from the memory > in operand 1 with the memory in operand 0 without leaving the > value in a register afterward and branches to operand 2 if the > values weren't equal. > > If this pattern is not defined, then a plain compare pattern and > conditional branch pattern is used. > --quote-- > > According to the documentation, x86 patterns are correct. However, > middle-end fails to extend ptr_mode value to Pmode, and in function.c, > stack_protect_prologue/stack_protect_epilogue, we already have > ptr_mode (SImode) operand: > > (mem/v/f/c/i:SI (plus:DI (reg/f:DI 54 virtual-stack-vars) > (const_int -4 [0xfffffffffffffffc])) [2 D.2704+0 S4 A32]) > > (mem/v/f/c/i:SI (symbol_ref:DI ("__stack_chk_guard") [flags 0x40] > <var_decl 0x7ffc35aa0be0 __stack_chk_guard>) [2 __stack_chk_guard+0 S4 > A32]) > > An opinion of a RTL maintainer (CC'd) is needed here. Target > definition is OK in its current form. When -fstack-protector was added, there are tree default_stack_protect_guard (void) { tree t = stack_chk_guard_decl; if (t == NULL) { rtx x; t = build_decl (UNKNOWN_LOCATION, VAR_DECL, get_identifier ("__stack_chk_guard"), ptr_type_node); ^^^^^^^^^^^^^^^^^ I think ptr_mode is intended and Pmode is just a typo. Jakub, Richard, what are your thoughts on this? Thanks.
On Fri, Jul 29, 2011 at 3:14 PM, H.J. Lu <hjl.tools@gmail.com> wrote: > On Thu, Jul 28, 2011 at 1:03 PM, Uros Bizjak <ubizjak@gmail.com> wrote: >> On Thu, Jul 28, 2011 at 9:03 PM, H.J. Lu <hjl.tools@gmail.com> wrote: >> >>>>> This patch adds x32 support to UNSPEC_SP_XXX patterns. OK for trunk? >>>> >>>> http://gcc.gnu.org/contribute.html#patches >>>> >>> >>> Sorry. I should have mentioned testcase in: >>> >>> http://gcc.gnu.org/bugzilla/show_bug.cgi?id=47766 >>> >>> Actually, they are in gcc testsuite. I noticed them when >>> I run gcc testsuite on x32. >> >> This looks like a middle-end problem to me. >> >> According to the documentation: >> >> --quote-- >> `stack_protect_set' >> This pattern, if defined, moves a `Pmode' 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. >> >> `stack_protect_test' >> This pattern, if defined, compares a `Pmode' value from the memory >> in operand 1 with the memory in operand 0 without leaving the >> value in a register afterward and branches to operand 2 if the >> values weren't equal. >> >> If this pattern is not defined, then a plain compare pattern and >> conditional branch pattern is used. >> --quote-- >> >> According to the documentation, x86 patterns are correct. However, >> middle-end fails to extend ptr_mode value to Pmode, and in function.c, >> stack_protect_prologue/stack_protect_epilogue, we already have >> ptr_mode (SImode) operand: >> >> (mem/v/f/c/i:SI (plus:DI (reg/f:DI 54 virtual-stack-vars) >> (const_int -4 [0xfffffffffffffffc])) [2 D.2704+0 S4 A32]) >> >> (mem/v/f/c/i:SI (symbol_ref:DI ("__stack_chk_guard") [flags 0x40] >> <var_decl 0x7ffc35aa0be0 __stack_chk_guard>) [2 __stack_chk_guard+0 S4 >> A32]) >> >> An opinion of a RTL maintainer (CC'd) is needed here. Target >> definition is OK in its current form. > > When -fstack-protector was added, there are > > tree > default_stack_protect_guard (void) > { > tree t = stack_chk_guard_decl; > > if (t == NULL) > { > rtx x; > > t = build_decl (UNKNOWN_LOCATION, > VAR_DECL, get_identifier ("__stack_chk_guard"), > ptr_type_node); > ^^^^^^^^^^^^^^^^^ > > I think ptr_mode is intended and Pmode is just a typo. Jakub, Richard, > what are your thoughts on this? > Stack protector does use ptr_mode. We have /* i386 glibc provides __stack_chk_guard in %gs:0x14, x32 glibc provides it in %fs:0x18. x86_64 glibc provides it in %fs:0x28. */ #define TARGET_THREAD_SSP_OFFSET \ (TARGET_64BIT ? (TARGET_X32 ? 0x18 : 0x28) : 0x14) in gnu-user64.h and typedef struct { void *tcb; /* Pointer to the TCB. Not necessarily the thread descriptor used by libpthread. */ dtv_t *dtv; void *self; /* Pointer to the thread descriptor. */ int multiple_threads; int gscope_flag; uintptr_t sysinfo; uintptr_t stack_guard; uintptr_t pointer_guard; in sysdeps/x86_64/tls.h. I believe it is a mistake to use Pmode in the stack protector documentation.
diff --git a/gcc/config/i386/i386.md b/gcc/config/i386/i386.md index f33b8a0..f4717b5 100644 --- a/gcc/config/i386/i386.md +++ b/gcc/config/i386/i386.md @@ -951,6 +951,11 @@ ;; This mode iterator allows :P to be used for patterns that operate on ;; pointer-sized quantities. Exactly one of the two alternatives will match. (define_mode_iterator P [(SI "Pmode == SImode") (DI "Pmode == DImode")]) + +;; This mode iterator allows :PTR to be used for patterns that operate on +;; ptr_mode sized quantities. +(define_mode_iterator PTR + [(SI "ptr_mode == SImode") (DI "ptr_mode == DImode")]) ;; Scheduling descriptions @@ -17347,11 +17379,11 @@ #ifdef TARGET_THREAD_SSP_OFFSET operands[1] = GEN_INT (TARGET_THREAD_SSP_OFFSET); - insn = (TARGET_64BIT + insn = (TARGET_LP64 ? gen_stack_tls_protect_set_di : gen_stack_tls_protect_set_si); #else - insn = (TARGET_64BIT + insn = (TARGET_LP64 ? gen_stack_protect_set_di : gen_stack_protect_set_si); #endif @@ -17361,19 +17393,20 @@ }) (define_insn "stack_protect_set_<mode>" - [(set (match_operand:P 0 "memory_operand" "=m") - (unspec:P [(match_operand:P 1 "memory_operand" "m")] UNSPEC_SP_SET)) - (set (match_scratch:P 2 "=&r") (const_int 0)) + [(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)) (clobber (reg:CC FLAGS_REG))] "" "mov{<imodesuffix>}\t{%1, %2|%2, %1}\;mov{<imodesuffix>}\t{%2, %0|%0, %2}\;xor{l}\t%k2, %k2" [(set_attr "type" "multi")]) (define_insn "stack_tls_protect_set_<mode>" - [(set (match_operand:P 0 "memory_operand" "=m") - (unspec:P [(match_operand:P 1 "const_int_operand" "i")] - UNSPEC_SP_TLS_SET)) - (set (match_scratch:P 2 "=&r") (const_int 0)) + [(set (match_operand:PTR 0 "memory_operand" "=m") + (unspec:PTR [(match_operand:PTR 1 "const_int_operand" "i")] + UNSPEC_SP_TLS_SET)) + (set (match_scratch:PTR 2 "=&r") (const_int 0)) (clobber (reg:CC FLAGS_REG))] "" "mov{<imodesuffix>}\t{%@:%P1, %2|%2, <iptrsize> PTR %@:%P1}\;mov{<imodesuffix>}\t{%2, %0|%0, %2}\;xor{l}\t%k2, %k2" @@ -17391,11 +17424,11 @@ #ifdef TARGET_THREAD_SSP_OFFSET operands[1] = GEN_INT (TARGET_THREAD_SSP_OFFSET); - insn = (TARGET_64BIT + insn = (TARGET_LP64 ? gen_stack_tls_protect_test_di : gen_stack_tls_protect_test_si); #else - insn = (TARGET_64BIT + insn = (TARGET_LP64 ? gen_stack_protect_test_di : gen_stack_protect_test_si); #endif @@ -17409,20 +17442,20 @@ (define_insn "stack_protect_test_<mode>" [(set (match_operand:CCZ 0 "flags_reg_operand" "") - (unspec:CCZ [(match_operand:P 1 "memory_operand" "m") - (match_operand:P 2 "memory_operand" "m")] + (unspec:CCZ [(match_operand:PTR 1 "memory_operand" "m") + (match_operand:PTR 2 "memory_operand" "m")] UNSPEC_SP_TEST)) - (clobber (match_scratch:P 3 "=&r"))] + (clobber (match_scratch:PTR 3 "=&r"))] "" "mov{<imodesuffix>}\t{%1, %3|%3, %1}\;xor{<imodesuffix>}\t{%2, %3|%3, %2}" [(set_attr "type" "multi")]) (define_insn "stack_tls_protect_test_<mode>" [(set (match_operand:CCZ 0 "flags_reg_operand" "") - (unspec:CCZ [(match_operand:P 1 "memory_operand" "m") - (match_operand:P 2 "const_int_operand" "i")] + (unspec:CCZ [(match_operand:PTR 1 "memory_operand" "m") + (match_operand:PTR 2 "const_int_operand" "i")] UNSPEC_SP_TLS_TEST)) - (clobber (match_scratch:P 3 "=r"))] + (clobber (match_scratch:PTR 3 "=r"))] "" "mov{<imodesuffix>}\t{%1, %3|%3, %1}\;xor{<imodesuffix>}\t{%@:%P2, %3|%3, <iptrsize> PTR %@:%P2}" [(set_attr "type" "multi")])