[i386] : Fix PR 56807

Submitted by Kai Tietz on Dec. 6, 2013, 5:06 p.m.

Details

Message ID CAEwic4YRnyDhML9GYvM1+OyrTdX+-F4HagKJWyfjDnNhLkM4wQ@mail.gmail.com
State New
Headers show

Commit Message

Kai Tietz Dec. 6, 2013, 5:06 p.m.
Hi,


ChangeLog

2013-12-06  Kai Tietz  <ktietz@redhat.com>

    PR target/56807
    * config/i386/i386.c (ix86_expand_prologue):

Tested for i686-w64-mingw32, x86_64-unknown-linux-gnu.  Ok for apply?

Regards,
Kai

Comments

H.J. Lu Dec. 6, 2013, 5:10 p.m.
On Fri, Dec 6, 2013 at 9:06 AM, Kai Tietz <ktietz70@googlemail.com> wrote:
> Hi,
>
>
> ChangeLog
>
> 2013-12-06  Kai Tietz  <ktietz@redhat.com>
>
>     PR target/56807
>     * config/i386/i386.c (ix86_expand_prologue):
>

Incomplete ChangeLog entry.
Kai Tietz Dec. 6, 2013, 5:12 p.m.
Upps ... here is the missing Changlog

ChangeLog

2013-12-06  Kai Tietz  <ktietz@redhat.com>

    PR target/56807
    * config/i386/i386.c (ix86_expand_prologue): Address saved
    registers stack-relative, not via frame-pointer.
Kai Tietz Dec. 9, 2013, 11:24 a.m.
ping
Jan Hubicka Dec. 10, 2013, 11:48 a.m.
> Hi,
> 
> 
> ChangeLog
> 
> 2013-12-06  Kai Tietz  <ktietz@redhat.com>
> 
>     PR target/56807
>     * config/i386/i386.c (ix86_expand_prologue):
> 
> Tested for i686-w64-mingw32, x86_64-unknown-linux-gnu.  Ok for apply?

So the code in question does spilling relative to stack pointer before function
call and relative to base pointer after the function call and this leads to mismatch
when stack alignment is in the way?

The chagne seems OK if that is the case.

Honza
> 
> Regards,
> Kai
> 
> Index: config/i386/i386.c
> ===================================================================
> --- config/i386/i386.c    (Revision 205719)
> +++ config/i386/i386.c    (Arbeitskopie)
> @@ -10934,18 +10937,21 @@ ix86_expand_prologue (void)
>      }
>        m->fs.sp_offset += allocate;
> 
> +      /* Use stack_pointer_rtx for relative addressing so that code
> +     works for realigned stack, too.  */
>        if (r10_live && eax_live)
>          {
> -      t = choose_baseaddr (m->fs.sp_offset - allocate);
> +      t = plus_constant (Pmode, stack_pointer_rtx, allocate);
>        emit_move_insn (gen_rtx_REG (word_mode, R10_REG),
>                gen_frame_mem (word_mode, t));
> -      t = choose_baseaddr (m->fs.sp_offset - allocate - UNITS_PER_WORD);
> +      t = plus_constant (Pmode, stack_pointer_rtx,
> +                 allocate - UNITS_PER_WORD);
>        emit_move_insn (gen_rtx_REG (word_mode, AX_REG),
>                gen_frame_mem (word_mode, t));
>      }
>        else if (eax_live || r10_live)
>      {
> -      t = choose_baseaddr (m->fs.sp_offset - allocate);
> +      t = plus_constant (Pmode, stack_pointer_rtx, allocate);
>        emit_move_insn (gen_rtx_REG (word_mode,
>                         (eax_live ? AX_REG : R10_REG)),
>                gen_frame_mem (word_mode, t));
Jakub Jelinek Dec. 10, 2013, 11:56 a.m.
On Tue, Dec 10, 2013 at 12:48:20PM +0100, Jan Hubicka wrote:
> > Hi,
> > 
> > 
> > ChangeLog
> > 
> > 2013-12-06  Kai Tietz  <ktietz@redhat.com>
> > 
> >     PR target/56807
> >     * config/i386/i386.c (ix86_expand_prologue):
> > 
> > Tested for i686-w64-mingw32, x86_64-unknown-linux-gnu.  Ok for apply?
> 
> So the code in question does spilling relative to stack pointer before function
> call and relative to base pointer after the function call and this leads to mismatch
> when stack alignment is in the way?
> 
> The chagne seems OK if that is the case.

The ChangeLog entry is not ok though.

	Jakub
Jan Hubicka Dec. 10, 2013, 12:01 p.m.
> On Tue, Dec 10, 2013 at 12:48:20PM +0100, Jan Hubicka wrote:
> > > Hi,
> > > 
> > > 
> > > ChangeLog
> > > 
> > > 2013-12-06  Kai Tietz  <ktietz@redhat.com>
> > > 
> > >     PR target/56807
> > >     * config/i386/i386.c (ix86_expand_prologue):
> > > 
> > > Tested for i686-w64-mingw32, x86_64-unknown-linux-gnu.  Ok for apply?
> > 
> > So the code in question does spilling relative to stack pointer before function
> > call and relative to base pointer after the function call and this leads to mismatch
> > when stack alignment is in the way?
> > 
> > The chagne seems OK if that is the case.
> 
> The ChangeLog entry is not ok though.

Kai sent updated one in reply to Jeff's email I beleive.

honza
> 
> 	Jakub
Kai Tietz Dec. 10, 2013, 1:05 p.m.
Yes, I sent update to HJ's comment.

2013/12/6 Kai Tietz <ktietz70@googlemail.com>:
> Upps ... here is the missing Changlog
>
> ChangeLog
>
> 2013-12-06  Kai Tietz  <ktietz@redhat.com>
>
>     PR target/56807
>     * config/i386/i386.c (ix86_expand_prologue): Address saved
>     registers stack-relative, not via frame-pointer.
Jakub Jelinek Dec. 16, 2013, 6:13 p.m.
On Fri, Dec 06, 2013 at 06:06:14PM +0100, Kai Tietz wrote:
> --- config/i386/i386.c    (Revision 205719)
> +++ config/i386/i386.c    (Arbeitskopie)
> @@ -10934,18 +10937,21 @@ ix86_expand_prologue (void)
>      }
>        m->fs.sp_offset += allocate;
> 
> +      /* Use stack_pointer_rtx for relative addressing so that code
> +     works for realigned stack, too.  */
>        if (r10_live && eax_live)
>          {
> -      t = choose_baseaddr (m->fs.sp_offset - allocate);
> +      t = plus_constant (Pmode, stack_pointer_rtx, allocate);
>        emit_move_insn (gen_rtx_REG (word_mode, R10_REG),
>                gen_frame_mem (word_mode, t));
> -      t = choose_baseaddr (m->fs.sp_offset - allocate - UNITS_PER_WORD);
> +      t = plus_constant (Pmode, stack_pointer_rtx,
> +                 allocate - UNITS_PER_WORD);

Somebody just asked on IRC whether this shouldn't have been
allocate + UNITS_PER_WORD.

Dunno when would be eax_live true on x86_64 though (except for uninitialized
var uses).

>        emit_move_insn (gen_rtx_REG (word_mode, AX_REG),
>                gen_frame_mem (word_mode, t));
>      }
>        else if (eax_live || r10_live)
>      {
> -      t = choose_baseaddr (m->fs.sp_offset - allocate);
> +      t = plus_constant (Pmode, stack_pointer_rtx, allocate);
>        emit_move_insn (gen_rtx_REG (word_mode,
>                         (eax_live ? AX_REG : R10_REG)),
>                gen_frame_mem (word_mode, t));

	Jakub
Kai Tietz Dec. 16, 2013, 7:16 p.m.
2013/12/16 Jakub Jelinek <jakub@redhat.com>:
> On Fri, Dec 06, 2013 at 06:06:14PM +0100, Kai Tietz wrote:
>> --- config/i386/i386.c    (Revision 205719)
>> +++ config/i386/i386.c    (Arbeitskopie)
>> @@ -10934,18 +10937,21 @@ ix86_expand_prologue (void)
>>      }
>>        m->fs.sp_offset += allocate;
>>
>> +      /* Use stack_pointer_rtx for relative addressing so that code
>> +     works for realigned stack, too.  */
>>        if (r10_live && eax_live)
>>          {
>> -      t = choose_baseaddr (m->fs.sp_offset - allocate);
>> +      t = plus_constant (Pmode, stack_pointer_rtx, allocate);
>>        emit_move_insn (gen_rtx_REG (word_mode, R10_REG),
>>                gen_frame_mem (word_mode, t));
>> -      t = choose_baseaddr (m->fs.sp_offset - allocate - UNITS_PER_WORD);
>> +      t = plus_constant (Pmode, stack_pointer_rtx,
>> +                 allocate - UNITS_PER_WORD);
>
> Somebody just asked on IRC whether this shouldn't have been
> allocate + UNITS_PER_WORD.

Well, I had over weekend same discussion on irc.  AFAIR it was BugMaster ...
and yes, in the case (for x86_64 possible only) that r10_live and
eax_live there seems to be a bug.

The addressing of save-region for r10, which is saved after rax, is
correct.  The restore-address of rax is wrong.  It should be
         t = plus_constant (Pmode, stack_pointer_rtx,
                            allocate + UNITS_PER_WORD);
instead.

> Dunno when would be eax_live true on x86_64 though (except for uninitialized
> var uses).
>

Kai

Patch hide | download patch | download mbox

Index: config/i386/i386.c
===================================================================
--- config/i386/i386.c    (Revision 205719)
+++ config/i386/i386.c    (Arbeitskopie)
@@ -10934,18 +10937,21 @@  ix86_expand_prologue (void)
     }
       m->fs.sp_offset += allocate;

+      /* Use stack_pointer_rtx for relative addressing so that code
+     works for realigned stack, too.  */
       if (r10_live && eax_live)
         {
-      t = choose_baseaddr (m->fs.sp_offset - allocate);
+      t = plus_constant (Pmode, stack_pointer_rtx, allocate);
       emit_move_insn (gen_rtx_REG (word_mode, R10_REG),
               gen_frame_mem (word_mode, t));
-      t = choose_baseaddr (m->fs.sp_offset - allocate - UNITS_PER_WORD);
+      t = plus_constant (Pmode, stack_pointer_rtx,
+                 allocate - UNITS_PER_WORD);
       emit_move_insn (gen_rtx_REG (word_mode, AX_REG),
               gen_frame_mem (word_mode, t));
     }
       else if (eax_live || r10_live)
     {
-      t = choose_baseaddr (m->fs.sp_offset - allocate);
+      t = plus_constant (Pmode, stack_pointer_rtx, allocate);
       emit_move_insn (gen_rtx_REG (word_mode,
                        (eax_live ? AX_REG : R10_REG)),
               gen_frame_mem (word_mode, t));