[PATCH] don't count return value space twice on x86

[PATCH] don't count return value space twice on x86

[Dan Witte]

[-- Attachment #1: Type: text/plain, Size: 385 bytes --]

Anthony,

There's a bug in stdcall x86 WIN32 closure code where stack space for the return value is counted twice -- which leads to overpopping and a spectacular crash. This adds an X86_ANY define for all x86 plats, which simplifies logic and means that stack computation is done consistently in the x86 prep_cif_machdep, rather than double-counted by prep_cif.

Look OK?

Cheers,
Dan.

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: libffi-patch-X86_ANY.diff --]
[-- Type: text/x-patch; name=libffi-patch-X86_ANY.diff, Size: 3175 bytes --]

diff --git a/ChangeLog b/ChangeLog
index 7ba744c..d5c1363 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,11 @@
+2010-08-04  Dan Witte  <dwitte@mozilla.com>
+
+        * src/x86/ffitarget.h: Add X86_ANY define for all x86/x86_64
+        platforms.
+        * src/x86/ffi.c: Remove redundant ifdef checks.
+	* src/prep_cif.c: Push stack space computation into src/x86/ffi.c
+        for X86_ANY so return value space doesn't get added twice.
+
 2010-08-03  Neil Rashbrooke <neil@httl.net>
 
 	* msvcc.sh: Don't pass -safeseh to ml64 because behavior is buggy.
diff --git a/src/prep_cif.c b/src/prep_cif.c
index 5d749ef..761abdc 100644
--- a/src/prep_cif.c
+++ b/src/prep_cif.c
@@ -110,7 +110,7 @@ ffi_status ffi_prep_cif(ffi_cif *cif, ffi_abi abi, unsigned int nargs,
   FFI_ASSERT_VALID_TYPE(cif->rtype);
 
   /* x86, x86-64 and s390 stack space allocation is handled in prep_machdep. */
-#if !defined M68K && !defined __i386__ && !defined __x86_64__ && !defined S390 && !defined PA
+#if !defined M68K && !defined X86_ANY && !defined S390 && !defined PA
   /* Make space for the return structure pointer */
   if (cif->rtype->type == FFI_TYPE_STRUCT
 #ifdef SPARC
@@ -131,7 +131,7 @@ ffi_status ffi_prep_cif(ffi_cif *cif, ffi_abi abi, unsigned int nargs,
 	 check after the initialization.  */
       FFI_ASSERT_VALID_TYPE(*ptr);
 
-#if !defined __i386__ && !defined __x86_64__ && !defined S390 && !defined PA
+#if !defined X86_ANY && !defined S390 && !defined PA
 #ifdef SPARC
       if (((*ptr)->type == FFI_TYPE_STRUCT
 	   && ((*ptr)->size > 16 || cif->abi != FFI_V9))
diff --git a/src/x86/ffi.c b/src/x86/ffi.c
index 69a4dcd..bc14ce5 100644
--- a/src/x86/ffi.c
+++ b/src/x86/ffi.c
@@ -155,12 +155,10 @@ ffi_status ffi_prep_cif_machdep(ffi_cif *cif)
   switch (cif->rtype->type)
     {
     case FFI_TYPE_VOID:
-#if defined(X86) || defined (X86_WIN32) || defined(X86_FREEBSD) || defined(X86_DARWIN) || defined(X86_WIN64)
     case FFI_TYPE_UINT8:
     case FFI_TYPE_UINT16:
     case FFI_TYPE_SINT8:
     case FFI_TYPE_SINT16:
-#endif
 #ifdef X86_WIN64
     case FFI_TYPE_UINT32:
     case FFI_TYPE_SINT32:
diff --git a/src/x86/ffitarget.h b/src/x86/ffitarget.h
index 2738875..542e6d4 100644
--- a/src/x86/ffitarget.h
+++ b/src/x86/ffitarget.h
@@ -31,6 +31,9 @@
 
 /* ---- System specific configurations ----------------------------------- */
 
+/* For code common to all platforms on x86 and x86_64. */
+#define X86_ANY
+
 #if defined (X86_64) && defined (__i386__)
 #undef X86_64
 #define X86
@@ -67,16 +70,14 @@ typedef enum ffi_abi {
   FFI_LAST_ABI,
   /* TODO: Add fastcall support for the sake of completeness */
   FFI_DEFAULT_ABI = FFI_SYSV
-#endif
 
-#ifdef X86_WIN64
+#elif defined(X86_WIN64)
   FFI_WIN64,
   FFI_LAST_ABI,
   FFI_DEFAULT_ABI = FFI_WIN64
-#else
 
+#else
   /* ---- Intel x86 and AMD x86-64 - */
-#if !defined(X86_WIN32) && (defined(__i386__) || defined(__x86_64__) || defined(__i386) || defined(__amd64))
   FFI_SYSV,
   FFI_UNIX64,   /* Unix variants all use the same ABI for x86-64  */
   FFI_LAST_ABI,
@@ -86,7 +87,6 @@ typedef enum ffi_abi {
   FFI_DEFAULT_ABI = FFI_UNIX64
 #endif
 #endif
-#endif /* X86_WIN64 */
 } ffi_abi;
 #endif
 

Re: [PATCH] don't count return value space twice on x86

[Timothy Wall]

I take it this is only under *some* circumstances, since I've used a number of different stdcall closures without issue.

On Aug 4, 2010, at 8:13 PM, Dan Witte wrote:

> Anthony,
> 
> There's a bug in stdcall x86 WIN32 closure code where stack space for the return value is counted twice -- which leads to overpopping and a spectacular crash. This adds an X86_ANY define for all x86 plats, which simplifies logic and means that stack computation is done consistently in the x86 prep_cif_machdep, rather than double-counted by prep_cif.
> 
> Look OK?
> 
> Cheers,
> Dan.
> <libffi-patch-X86_ANY.diff>

Re: [PATCH] don't count return value space twice on x86

[Dan Witte]

Nope, it's consistent for us. Closure returns an int32_t, building with MSVC on x86 WIN32.

If it's working for you, it's likely that you're tickling (or in this case, not tickling) the platform-#ifdefed code in prep_cif. Maybe you have some custom defines or flags (or we're missing them) that cause that?

If that code is getting compiled in your builds, then I'm stumped... you are building win32.S with ml, right?

Cheers,
Dan.

----- Original Message -----
> I take it this is only under *some* circumstances, since I've used a
> number of different stdcall closures without issue.
> 
> On Aug 4, 2010, at 8:13 PM, Dan Witte wrote:
> 
> > Anthony,
> >
> > There's a bug in stdcall x86 WIN32 closure code where stack space
> > for the return value is counted twice -- which leads to overpopping
> > and a spectacular crash. This adds an X86_ANY define for all x86
> > plats, which simplifies logic and means that stack computation is
> > done consistently in the x86 prep_cif_machdep, rather than
> > double-counted by prep_cif.
> >
> > Look OK?
> >
> > Cheers,
> > Dan.
> > <libffi-patch-X86_ANY.diff>

Re: [PATCH] don't count return value space twice on x86

[Timothy Wall]

On Aug 4, 2010, at 10:51 PM, Dan Witte wrote:

> Nope, it's consistent for us. Closure returns an int32_t, building with MSVC on x86 WIN32.
> 
> If it's working for you, it's likely that you're tickling (or in this case, not tickling) the platform-#ifdefed code in prep_cif. Maybe you have some custom defines or flags (or we're missing them) that cause that?
> 
> If that code is getting compiled in your builds, then I'm stumped... you are building win32.S with ml, right?
> 

That's a possible difference.  I've built and tested using cygwing/mingw32, and only built 64-bit windows with MS tools.


> Cheers,
> Dan.
> 
> ----- Original Message -----
>> I take it this is only under *some* circumstances, since I've used a
>> number of different stdcall closures without issue.
>> 
>> On Aug 4, 2010, at 8:13 PM, Dan Witte wrote:
>> 
>>> Anthony,
>>> 
>>> There's a bug in stdcall x86 WIN32 closure code where stack space
>>> for the return value is counted twice -- which leads to overpopping
>>> and a spectacular crash. This adds an X86_ANY define for all x86
>>> plats, which simplifies logic and means that stack computation is
>>> done consistently in the x86 prep_cif_machdep, rather than
>>> double-counted by prep_cif.
>>> 
>>> Look OK?
>>> 
>>> Cheers,
>>> Dan.
>>> <libffi-patch-X86_ANY.diff>

Re: [PATCH] don't count return value space twice on x86

[Anthony Green]

Dan Witte <dwitte@mozilla.com> writes:

> Anthony,
>
> There's a bug in stdcall x86 WIN32 closure code where stack space for the return value is counted twice -- which leads to overpopping and a spectacular crash. This adds an X86_ANY define for all x86 plats, which simplifies logic and means that stack computation is done consistently in the x86 prep_cif_machdep, rather than double-counted by prep_cif.
>
> Look OK?

Yes, thanks - applied.

AG

>
> Cheers,
> Dan.