[Bug libffi/45677] Bad stack allocation for ffi function calls on x86-64

[Bug libffi/45677] Bad stack allocation for ffi function calls on x86-64

[dwitte at gmail dot com]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=45677

--- Comment #11 from Dan Witte <dwitte at gmail dot com> 2010-09-27 23:40:05 UTC ---
Jakub -- did you commit this fix to gcc's libffi? If not, I'll push upstream to
Anthony.

[Bug libffi/45677] Bad stack allocation for ffi function calls on x86-64

[jakub at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=45677

--- Comment #12 from Jakub Jelinek <jakub at gcc dot gnu.org> 2010-10-01 07:31:55 UTC ---
Author: jakub
Date: Fri Oct  1 07:31:49 2010
New Revision: 164829

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=164829
Log:
    PR libffi/45677
    * src/x86/ffi64.c (ffi_prep_cif_machdep): Ensure cif->bytes is
    a multiple of 8.
    * testsuite/libffi.call/many2.c: New test.

Added:
    trunk/libffi/testsuite/libffi.call/many2.c
Modified:
    trunk/libffi/ChangeLog
    trunk/libffi/src/x86/ffi64.c

[Bug libffi/45677] Bad stack allocation for ffi function calls on x86-64

[jakub at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=45677

--- Comment #13 from Jakub Jelinek <jakub at gcc dot gnu.org> 2010-10-01 07:33:14 UTC ---
Author: jakub
Date: Fri Oct  1 07:33:09 2010
New Revision: 164830

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=164830
Log:
    PR libffi/45677
    * src/x86/ffi64.c (ffi_prep_cif_machdep): Ensure cif->bytes is
    a multiple of 8.
    * testsuite/libffi.call/many2.c: New test.

Added:
    branches/gcc-4_5-branch/libffi/testsuite/libffi.call/many2.c
Modified:
    branches/gcc-4_5-branch/libffi/ChangeLog
    branches/gcc-4_5-branch/libffi/src/x86/ffi64.c

[Bug libffi/45677] Bad stack allocation for ffi function calls on x86-64

[jakub at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=45677

--- Comment #14 from Jakub Jelinek <jakub at gcc dot gnu.org> 2010-10-01 12:45:33 UTC ---
Author: jakub
Date: Fri Oct  1 12:45:25 2010
New Revision: 164882

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=164882
Log:
    PR libffi/45677
    * src/x86/ffi64.c (ffi_prep_cif_machdep): Ensure cif->bytes is
    a multiple of 8.
    * testsuite/libffi.call/many2.c: New test.

Added:
    branches/gcc-4_4-branch/libffi/testsuite/libffi.call/many2.c
Modified:
    branches/gcc-4_4-branch/libffi/ChangeLog
    branches/gcc-4_4-branch/libffi/src/x86/ffi64.c

[Bug libffi/45677] Bad stack allocation for ffi function calls on x86-64

[jakub at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=45677

Jakub Jelinek <jakub at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |jakub at gcc dot gnu.org
         AssignedTo|unassigned at gcc dot       |jakub at gcc dot gnu.org
                   |gnu.org                     |

--- Comment #15 from Jakub Jelinek <jakub at gcc dot gnu.org> 2010-10-01 13:27:51 UTC ---
Fixed.

[Bug libffi/45677] Bad stack allocation for ffi function calls on x86-64

[jakub at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=45677

Jakub Jelinek <jakub at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|UNCONFIRMED                 |RESOLVED
         Resolution|                            |FIXED

--- Comment #16 from Jakub Jelinek <jakub at gcc dot gnu.org> 2010-10-01 13:28:50 UTC ---
.

[Bug libffi/45677] Bad stack allocation for ffi function calls on x86-64

[mh+gcc at glandium dot org]

------- Comment #10 from mh+gcc at glandium dot org  2010-09-16 07:43 -------
(In reply to comment #9)
> Created an attachment (id=21806)
 --> (http://gcc.gnu.org/bugzilla/attachment.cgi?id=21806&action=view) [edit]
> testcase
> 
> Here you go. This passes at -O0 but fails at -O2. Note that the testcase
> requires >= 7 args to the test function, to force the last arg to spill onto
> the stack; also an inner (non-inlined) function call, to force that single
> stack arg to be zero-extended to word size and overwrite the flags.

Another way to achieve the test without relying on the compiler optimization
would be to use int or size_t arguments, but pass bools through ffi_call. This
may not work on all architectures, though.

Also, interestingly, the original patch doesn't trigger any testsuite failure.
Maybe the various cls tests should be extended to get a first dummy argument.


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=45677

[Bug libffi/45677] Bad stack allocation for ffi function calls on x86-64

[dwitte at gmail dot com]

------- Comment #9 from dwitte at gmail dot com  2010-09-15 21:01 -------
Created an attachment (id=21806)
 --> (http://gcc.gnu.org/bugzilla/attachment.cgi?id=21806&action=view)
testcase

Here you go. This passes at -O0 but fails at -O2. Note that the testcase
requires >= 7 args to the test function, to force the last arg to spill onto
the stack; also an inner (non-inlined) function call, to force that single
stack arg to be zero-extended to word size and overwrite the flags.


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=45677

[Bug libffi/45677] Bad stack allocation for ffi function calls on x86-64

[jakub at gcc dot gnu dot org]

------- Comment #8 from jakub at gcc dot gnu dot org  2010-09-15 19:21 -------
Yes, x86-64 requires 16 byte stack alignment, but alloca should ensure that.

Can you come up with a small testcase which was misbehaving before?  It would
be great to add it to the libffi testsuite.


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=45677

[Bug libffi/45677] Bad stack allocation for ffi function calls on x86-64

[mh+gcc at glandium dot org]

------- Comment #7 from mh+gcc at glandium dot org  2010-09-15 18:13 -------
Nice catch. BTW I read recently that gcc 4.5 assumes 16 bytes stack alignment,
is that x86 only or does that apply to x86-64 too ? (in which case we'd need to
be careful about that as well)


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=45677

[Bug libffi/45677] Bad stack allocation for ffi function calls on x86-64

[jakub at gcc dot gnu dot org]

------- Comment #6 from jakub at gcc dot gnu dot org  2010-09-15 17:51 -------
No, the 8 in the final ALIGN is because that's the stack word size, everything
smaller than that is passed as low bits of the whole 64-bit word.


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=45677

[Bug libffi/45677] Bad stack allocation for ffi function calls on x86-64

[dwitte at mozilla dot com]

------- Comment #5 from dwitte at mozilla dot com  2010-09-15 17:24 -------
Yeah, that sounds right to me. The final alignment really wants to be the
alignment of whatever comes next, right? Which happens to be cif->flags, so 8
is fine. I wonder if just assuming 8 is fragile, but since we'll only ever have
integers or pointers on the stack, it should be OK?


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=45677

[Bug libffi/45677] Bad stack allocation for ffi function calls on x86-64

[jakub at gcc dot gnu dot org]

------- Comment #4 from jakub at gcc dot gnu dot org  2010-09-15 17:07 -------
That patch looks wrong.  The padding for alignment must be done before the
arguments, not after them.
Say if bytes is 24 at the start of one iteration, cif->arg_types[i]->alignment
is 32 and cif->arg_types[i]->size is 40, currently it would correctly
first pad bytes to 32 and then increase it 72, while with your patch
it will be 64.
What we need is to make sure < 8 byte arguments have the whole word assigned
for them.  As alignment is always set to at least 8, it is enough to add this
final padding just at the end, so I think the right fix is:
--- libffi/src/x86/ffi64.c 2010-08-11 21:08:14.000000000 +0200
+++ libffi/src/x86/ffi64.c 2010-09-15 19:05:16.573717651 +0200
@@ -390,7 +390,7 @@ ffi_prep_cif_machdep (ffi_cif *cif)
   if (ssecount)
     flags |= 1 << 11;
   cif->flags = flags;
-  cif->bytes = bytes;
+  cif->bytes = ALIGN (bytes, 8);

   return FFI_OK;
 }


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=45677

[Bug libffi/45677] Bad stack allocation for ffi function calls on x86-64

[dwitte at mozilla dot com]

------- Comment #3 from dwitte at mozilla dot com  2010-09-15 16:18 -------
(Oh, and please include a description of your change in ChangeLog -- makes his
job easier.)


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=45677

[Bug libffi/45677] Bad stack allocation for ffi function calls on x86-64

[dwitte at mozilla dot com]

------- Comment #2 from dwitte at mozilla dot com  2010-09-15 16:17 -------
I'd recommend upstreaming things directly to the maintainer, Anthony Green
(that's what I do). If you'd like, close this out, and post the patch to
libffi-discuss@sourceware.org and CC green@redhat.com?


-- 

dwitte at mozilla dot com changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |dwitte at mozilla dot com


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=45677

[Bug libffi/45677] Bad stack allocation for ffi function calls on x86-64

[mh+gcc at glandium dot org]

------- Comment #1 from mh+gcc at glandium dot org  2010-09-15 12:21 -------
Created an attachment (id=21800)
 --> (http://gcc.gnu.org/bugzilla/attachment.cgi?id=21800&action=view)
Fix stack allocation for ffi function calls on x86-64


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=45677