On 01/24/2018 12:11 AM, Uros Bizjak wrote:
> On Wed, Jan 24, 2018 at 12:15 AM, Jeff Law <suzanne.jeff....@gmail.com> wrote:
>>
>> pr83994 is a code generation bug in the stack-clash support that affects
>> openssl (we've turned on stack-clash-protection by default for the F28
>> builds).
>>
>> The core problem is stack-clash (like stack-check) will emit a probing
>> loop if the prologue allocates enough stack space. When emitting a loop
>> both implementations will need a scratch register.
>>
>> They use get_scratch_register_at_entry to find a suitable scratch
>> register. This routine assumes that callee registers saves are
>> completed at the point where the scratch register is going to be used.
>>
>> In this particular testcase we select %ebx because ax,cx,dx are used for
>> parameter passing. That's fine. The problem is %ebx hasn't been saved yet!
>>
>> -fstack-check has a bit of code in the frame setup/layout code which
>> forces the prologue to use pushes rather than reg->mem moves for saving
>> registers. There's a gcc_assert in the prologue expander to catch any
>> case where the registers aren't saved.
>>
>> -fstack-clash-protection doesn't have that same bit of magic in the
>> frame setup/layout code and it bypasses the assertion due to a change I
>> made back in Nov 2017 due to not being aware of this particular issue.
>>
>> This patch reverts the assertion bypass I added back in Nov 2017 and
>> adds clarifying comments. The patch also forces use of push to save
>> integer registers for a stack-clash protected prologue if probes are
>> going to be needed.
>>
>> Bootstrapped and regression tested on x86_64.
>>
>> While the bug is not marked as a regression, ISTM this needs to be fixed
>> for gcc-8.
>>
>> OK for the trunk?
>>
>> Jeff
>>
>> * i386.c (get_probe_interval): Move to earlier point.
>> (ix86_compute_frame_layout): If -fstack-clash-protection and
>> the frame is larger than the probe interval, then use pushes
>> to save registers rather than reg->mem moves.
>> (ix86_expand_prologue): Remove conditional for int_registers_saved
>> assertion.
>>
>> * gcc.target/i386/pr83994.c: New test.
>
> OK with the fixed testcase (see below).
>
> Thanks,
> Uros.
[ ... snip ... ]
>> diff --git a/gcc/testsuite/gcc.target/i386/pr83994.c
>> b/gcc/testsuite/gcc.target/i386/pr83994.c
>> new file mode 100644
>> index 0000000..b57b04b
>> --- /dev/null
>> +++ b/gcc/testsuite/gcc.target/i386/pr83994.c
>> @@ -0,0 +1,15 @@
>> +/* { dg-do compile } */
>> +/* { dg-options "-O2 -m32 -march=i686 -fpic -fstack-clash-protection" } */
>
> Please use
>
> /* { dg-require-effective-target ia32 } */
>
> and remove "-m32" from dg-options.
Done. Thanks for the quick turnaround.
jeff
