diff mbox

[i386] : Fix PR target/48723

Message ID 201104302255.11621.ebotcazou@adacore.com
State New
Headers show

Commit Message

Eric Botcazou April 30, 2011, 8:55 p.m. UTC 
On Saturday 23 April 2011 09:35:38 Uros Bizjak wrote:
> Index: i386.c
> ===================================================================
> --- i386.c	(revision 172866)
> +++ i386.c	(working copy)
> @@ -10149,7 +10149,7 @@ ix86_adjust_stack_and_probe (const HOST_
>    /* Even if the stack pointer isn't the CFA register, we need to
> correctly describe the adjustments made to it, in particular differentiate
> the frame-related ones from the frame-unrelated ones.  */
> -  if (size > 0)
> +  if (size != 0)
>      {
>        rtx expr = gen_rtx_SEQUENCE (VOIDmode, rtvec_alloc (2));
>        XVECEXP (expr, 0, 0)

Probing for a negative size doesn't make much sense; we shouldn't be there if 
the size to allocate is negative.  Is that really intended?  The code without 
stack checking is:

        pushq   %rbp
.LCFI0:
        movq    %rsp, %rbp
.LCFI1:
        andq    $-32, %rsp
        addq    $16, %rsp

which looks weird.  If this is nevertheless correct, then the fix for the 
problem is attached.  Tested on x86_64-suse-linux.


	PR target/48723
	* config/i386/i386.c (ix86_expand_prologue): Do not probe the stack
	for -fstack-check if the size to allocate is negative.

Comments

H.J. Lu April 30, 2011, 9:48 p.m. UTC | #1 
On Sat, Apr 30, 2011 at 1:55 PM, Eric Botcazou <ebotcazou@adacore.com> wrote:
> On Saturday 23 April 2011 09:35:38 Uros Bizjak wrote:
>> Index: i386.c
>> ===================================================================
>> --- i386.c    (revision 172866)
>> +++ i386.c    (working copy)
>> @@ -10149,7 +10149,7 @@ ix86_adjust_stack_and_probe (const HOST_
>>    /* Even if the stack pointer isn't the CFA register, we need to
>> correctly describe the adjustments made to it, in particular differentiate
>> the frame-related ones from the frame-unrelated ones.  */
>> -  if (size > 0)
>> +  if (size != 0)
>>      {
>>        rtx expr = gen_rtx_SEQUENCE (VOIDmode, rtvec_alloc (2));
>>        XVECEXP (expr, 0, 0)
>
> Probing for a negative size doesn't make much sense; we shouldn't be there if
> the size to allocate is negative.  Is that really intended?  The code without
> stack checking is:
>
>        pushq   %rbp
> .LCFI0:
>        movq    %rsp, %rbp
> .LCFI1:
>        andq    $-32, %rsp
>        addq    $16, %rsp
>
> which looks weird.  If this is nevertheless correct, then the fix for the
> problem is attached.  Tested on x86_64-suse-linux.
>

This code aligns stack to 32byte for AVX.
Eric Botcazou April 30, 2011, 9:56 p.m. UTC | #2 
> This code aligns stack to 32byte for AVX.

Right, the strange line is the next one.
H.J. Lu April 30, 2011, 10:37 p.m. UTC | #3 
On Sat, Apr 30, 2011 at 2:56 PM, Eric Botcazou <ebotcazou@adacore.com> wrote:
>> This code aligns stack to 32byte for AVX.
>
> Right, the strange line is the next one.
>

What is wrong? x86-64 has 128byte redzone.
Eric Botcazou May 1, 2011, 8:15 a.m. UTC | #4 
> What is wrong? x86-64 has 128byte redzone.

Nothing wrong, just strange.  ISTM that it doesn't serve any useful purpose.
And I'm not sure that the rest of the code in ix86_expand_prologue is really 
prepared for a negative size either.
diff mbox

Patch

Index: config/i386/i386.c
===================================================================
--- config/i386/i386.c	(revision 173209)
+++ config/i386/i386.c	(working copy)
@@ -10619,8 +10619,8 @@  ix86_expand_prologue (void)
     }
 
   /* The stack has already been decremented by the instruction calling us
-     so we need to probe unconditionally to preserve the protection area.  */
-  if (flag_stack_check == STATIC_BUILTIN_STACK_CHECK)
+     so probe if the size is non-negative to preserve the protection area.  */
+  if (allocate >= 0 && flag_stack_check == STATIC_BUILTIN_STACK_CHECK)
     {
       /* We expect the registers to be saved when probes are used.  */
       gcc_assert (int_registers_saved);