Skip to content

Commit

Permalink
don't align arguments in stdcall
Browse files Browse the repository at this point in the history
  • Loading branch information
twall committed Jan 13, 2014
1 parent ed35e7b commit 4836f05
Show file tree
Hide file tree
Showing 2 changed files with 45 additions and 0 deletions.
3 changes: 3 additions & 0 deletions native/libffi/src/x86/ffi.c
Original file line number Diff line number Diff line change
Expand Up @@ -305,6 +305,9 @@ ffi_status ffi_prep_cif_machdep(ffi_cif *cif)

for (ptr = cif->arg_types, i = cif->nargs; i > 0; i--, ptr++)
{
#ifdef X86_WIN32
if (cif->abi != FFI_STDCALL)
#endif
if (((*ptr)->alignment - 1) & cif->bytes)
cif->bytes = ALIGN(cif->bytes, (*ptr)->alignment);
cif->bytes += ALIGN((*ptr)->size, FFI_SIZEOF_ARG);
Expand Down
42 changes: 42 additions & 0 deletions native/libffi/testsuite/libffi.call/align_stdcall.c
Original file line number Diff line number Diff line change
@@ -0,0 +1,42 @@
/* Area: ffi_call
Purpose: Check stdcall for argument alignment (always 4) on X86_WIN32 systems.
Limitations: none.
PR: none.
Originator: <twalljava@java.net> (from many_win32.c) */

/* { dg-do run { target i?86-*-cygwin* i?86-*-mingw* } } */

#include "ffitest.h"
#include <float.h>

static float __attribute__((stdcall)) stdcall_align(int i1,
double f2,
int i3,
double f4)
{
return i1+f2+i3+f4;
}

int main (void)
{
ffi_cif cif;
ffi_type *args[4] = {&ffi_type_int, &ffi_type_double, &ffi_type_int, &ffi_type_double};
float fa[2] = {1,2};
int ia[2] = {1,2};
void *values[4] = {&ia[0], &fa[0], &ia[1], &fa[1]};
float f, ff;

/* Initialize the cif */
CHECK(ffi_prep_cif(&cif, FFI_STDCALL, 4,
&ffi_type_float, args) == FFI_OK);

ff = stdcall_align(ia[0], fa[0], ia[1], fa[1]);

ffi_call(&cif, FFI_FN(stdcall_align), &f, values);

if (f - ff < FLT_EPSILON)
printf("stdcall many arg tests ok!\n");
else
CHECK(0);
exit(0);
}

5 comments on commit 4836f05

@joshtriplett
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about FFI_FASTCALL and FFI_THISCALL, which are just stdcall with one or two arguments passed in registers?

Also, this pull request interacts with the non-Windows stdcall support I've submitted patches for; the #ifdef should become #ifndef X86_WIN64, and the test should expand its target list to i?86-*-*.

@twall
Copy link
Contributor Author

@twall twall commented on 4836f05 Mar 20, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would guess FFI_FASTCALL and FFI_THISCALL would be the same? I'm not familiar with those.

I also wasn't aware there was any non-windows stdcall support. Are you referring to patches made to libffi git repo proper?

@joshtriplett
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would guess FFI_FASTCALL and FFI_THISCALL would be the same? I'm not familiar with those.

Given that thiscall and fastcall are simply stdcall with the first one or two (respectively) arguments passed in registers, I'd guess that the alignment requirements match.

Do you have any documentation for the alignment requirement you're changing? Good documentation on stdcall requirements is hard to come by.

I also wasn't aware there was any non-windows stdcall support. Are you referring to patches made to libffi git repo proper?

I added support for stdcall, thiscall, and fastcall for non-Windows x86 platforms; it's been merged into latest git.

Also, I expanded the testsuite to provide a general mechanism for testing all ABIs. If you change FFI_STDCALL to ABI_NUM and __attribute__((stdcall)) to ABI_ATTR, the testsuite will run your test against every ABI. (Please also generalize the names and comments to not mention stdcall specifically, other than saying this tests argument alignment.)

Finally, since that ABI testing mechanism works everywhere, you can drop the architecture specification, and allow this test to run on all architectures (including non-x86). I'd call it align_mixed.c or similar.

@twall
Copy link
Contributor Author

@twall twall commented on 4836f05 Apr 19, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Finally getting around to updating libffi and ran into conflicts around this one.

I don't have any documentation for the alignment requirement, only empirical evidence that without the patch, code crashes.

@twall
Copy link
Contributor Author

@twall twall commented on 4836f05 Apr 19, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@joshtriplett 's advice applied after update to libffi v3.2.1 in 4e8c935.

Please sign in to comment.