Re: [PATCH][PR target/84066] Wrong shadow stack register size is saved for x32

[Uros Bizjak <ubizjak at gmail dot com>]

On Fri, Feb 2, 2018 at 10:22 AM, Tsimbalist, Igor V
<igor.v.tsimbalist@intel.com> wrote:
>> -----Original Message-----
>> From: H.J. Lu [mailto:hjl.tools@gmail.com]
>> Sent: Tuesday, January 30, 2018 3:45 PM
>> To: Uros Bizjak <ubizjak@gmail.com>
>> Cc: Tsimbalist, Igor V <igor.v.tsimbalist@intel.com>; gcc-
>> patches@gcc.gnu.org
>> Subject: Re: [PATCH][PR target/84066] Wrong shadow stack register size is
>> saved for x32
>>
>> On Tue, Jan 30, 2018 at 6:38 AM, Uros Bizjak <ubizjak@gmail.com> wrote:
>> > On Tue, Jan 30, 2018 at 3:19 PM, Tsimbalist, Igor V
>> > <igor.v.tsimbalist@intel.com> wrote:
>> >> x32 is a 64-bit process with 32-bit software pointer and kernel may
>> >> place x32 shadow stack above 4GB.  We need to save and restore 64-bit
>> >> shadow stack register for x32. builtin jmp buf size is 5 pointers.  We
>> >> have space to save 64-bit shadow stack pointer: 32-bit SP, 32-bit FP,
>> >> 32-bit IP, 64-bit SSP for x32.
>> >>
>> >>         PR target/84066
>> >>         * gcc/config/i386/i386.md: Replace Pmode with word_mode in
>> >>         builtin_setjmp_setup and builtin_longjmp to support x32.
>> >>         * gcc/testsuite/gcc.target/i386/cet-sjlj-6.c: New test.
>> >>
>> >> Ok for trunk?
>> >
>> > LGTM, but please check the testcase with -mx32
>> > -maddress-mode={short,long} nevertheless to catch any incosistencies.
>> >
>>
>> Speaking of -maddress-mode=, shouldn't
>>
>> +      reg_adj = gen_rtx_SUBREG (Pmode, reg_ssp, 0);
>>        tmp = gen_rtx_SET (reg_adj,
>>   gen_rtx_LSHIFTRT (Pmode, negate_rtx (Pmode, reg_adj),
>> -   GEN_INT ((Pmode == SImode)
>> +   GEN_INT ((word_mode == SImode)
>>      ? 2
>>      : 3)));
>>
>> be
>>
>> +      reg_adj = gen_rtx_SUBREG (ptr_mode, reg_ssp, 0);
>>        tmp = gen_rtx_SET (reg_adj,
>>   gen_rtx_LSHIFTRT (ptr_mode, negate_rtx (ptr_mode, reg_adj),
>> -   GEN_INT ((Pmode == SImode)
>> +   GEN_INT ((word_mode == SImode)
>>      ? 2
>>      : 3)));
>>
>> Pmode == word_mode for -maddress-mode=long.
>>
>> +++ b/gcc/testsuite/gcc.target/i386/cet-sjlj-6.c
>> @@ -0,0 +1,19 @@
>> +/* { dg-do compile } */
>> +/* { dg-options "-O -fcf-protection -mcet -mx32" } */
>> +/* { dg-final { scan-assembler-times "endbr64" 2 } } */
>> +/* { dg-final { scan-assembler-times "movq\t.*buf\\+12" 1 } } */
>> +/* { dg-final { scan-assembler-times "subq\tbuf\\+12" 1 } } */
>> +/* { dg-final { scan-assembler-times "rdsspq" 2 } } */
>> +/* { dg-final { scan-assembler-times "incsspq" 2 } } */
>>
>> Please add a test for
>>
>>        tmp = gen_rtx_SET (reg_adj,
>>   gen_rtx_LSHIFTRT (Pmode, negate_rtx (Pmode, reg_adj),
>> -   GEN_INT ((Pmode == SImode)
>> +   GEN_INT ((word_mode == SImode)
>>      ? 2
>>      : 3)));
>>
> Thanks for suggestion about -maddress-mode option. The patch was updated
> and new test was added. The -maddress-mode=long test is expected to
> fail as it depends on fixing of https://gcc.gnu.org/bugzilla/show_bug.cgi?id=84150
>
> Ok for trunk?

 LGTM, with a couple of testcase adjustments:

+/* { dg-do compile } */
+/* { dg-options "-O -maddress-mode=short -fcf-protection -mcet -mx32" } */

You will need:

/* { dg-do compile { target { ! ia32 } } } */

for both tests and

/* { dg-require-effective-target maybe_x32 } */

when -maddress-mode=short is used.

Uros.