* PPC bug when handling double
@ 2015-08-01 15:59 Aaron R
2015-08-03 5:31 ` Alan Modra
0 siblings, 1 reply; 4+ messages in thread
From: Aaron R @ 2015-08-01 15:59 UTC (permalink / raw)
To: libffi-discuss
[-- Attachment #1: Type: text/plain, Size: 1682 bytes --]
Hi,
I am not very familiar with libffi but I believe I may have stumbled upon a bug.
If you build the attached test code, you will get two test programs -
namely, ex1 and ex2. The only difference between the two is
'-D__USE_F6__' , which passes the last parameter of test_func1() as a
float instead of a double. I have tried these test programs on both
X86 and PPC (32-bit in both cases) using libffi 3.2.1. I did not use
any special configure flags when I built libffi 3.2.1 for both
platforms.
This is what I get on X86:
$ ./ex1
args_idx = 12
tst_func1 : 0xa 0xb 11.100000 12.1 13.100000 14.1 15.100000 16.1
17.100000 18.1 19.100000 20.100000
rc = 1 - success? Y
$ ./ex2
args_idx = 12
tst_func1 : 0xa 0xb 11.100000 12.1 13.100000 14.1 15.100000 16.1
17.100000 18.1 19.100000 20.1
rc = 1 - success? Y
That looks fine. The last parameter is always 20.1.
If I run the same code on PPC I get:
$ uname -a
Linux tstsrv 3.0.101-0.47.52-ppc64 #1 SMP Thu Mar 26 10:55:49 UTC 2015
(0e3c7c8) ppc64 ppc64 ppc64 GNU/Linux
$ file ./ppc/libffi_3.2.1/lib/libffi.so.6.0.4
./ppc/libffi_3.2.1/lib/libffi.so.6.0.4: ELF 32-bit MSB shared object,
PowerPC or cisco 4500, version 1 (SYSV), dynamically linked, not
stripped
$ gcc -dumpversion
4.3
$ ./ex1
args_idx = 12
tst_func1 : 0xa 0xb 11.100000 12.1 13.100000 14.1 15.100000 16.1
17.100000 18.1 19.100000 20.100000
rc = 1 - success? Y
$ ./ex2
args_idx = 12
tst_func1 : 0xa 0xb 11.100000 12.1 13.100000 14.1 15.100000 16.1
17.100000 18.1 19.100000 -2.35344e-185
rc = 0 - success? N
You can see above that the last parameter printed by tst_func1() is
incorrect in the case of ex2 (i.e., when passed as a double).
Any idea?
Thanks in advance
[-- Attachment #2: ex.c --]
[-- Type: text/x-csrc, Size: 3794 bytes --]
#include <stdint.h>
#include <stdio.h>
#include <ffi.h>
#include "trg.h"
#define NUM_TST_ARGS 12
#define FFI_FN(f) ((void (*)(void))f)
static int test1(void) {
uint32_t sValues[2 * NUM_TST_ARGS];
size_t sValues_idx = 0;
ffi_cif cif;
ffi_type *args[NUM_TST_ARGS];
size_t args_idx = 0;
void *values[NUM_TST_ARGS];
int rc = -1;
/* Initialize the argument info vectors */
args[args_idx] = &ffi_type_pointer;
sValues[sValues_idx] = (uint32_t) 0xa;
values[args_idx] = &(sValues[sValues_idx]);
++args_idx;
++sValues_idx;
args[args_idx] = &ffi_type_pointer;
sValues[sValues_idx] = (uint32_t) 0xb;
values[args_idx] = &(sValues[sValues_idx]);
++args_idx;
++sValues_idx;
args[args_idx] = &ffi_type_float;
{
const float tf = 11.1;
const uint32_t * const tp = (uint32_t *) &tf;
sValues[sValues_idx] = *tp;
}
values[args_idx] = &(sValues[sValues_idx]);
++args_idx;
++sValues_idx;
args[args_idx] = &ffi_type_double;
{
const double tf = 12.1;
const uint32_t * const tp = (uint32_t *) &tf;
sValues[sValues_idx] = *tp;
++sValues_idx;
sValues[sValues_idx] = *(tp + 1);
}
values[args_idx] = &(sValues[sValues_idx - 1]);
++args_idx;
++sValues_idx;
args[args_idx] = &ffi_type_float;
{
const float tf = 13.1;
const uint32_t * const tp = (uint32_t *) &tf;
sValues[sValues_idx] = *tp;
}
values[args_idx] = &(sValues[sValues_idx]);
++args_idx;
++sValues_idx;
args[args_idx] = &ffi_type_double;
{
const double tf = 14.1;
const uint32_t * const tp = (uint32_t *) &tf;
sValues[sValues_idx] = *tp;
++sValues_idx;
sValues[sValues_idx] = *(tp + 1);
}
values[args_idx] = &(sValues[sValues_idx - 1]);
++args_idx;
++sValues_idx;
args[args_idx] = &ffi_type_float;
{
const float tf = 15.1;
const uint32_t * const tp = (uint32_t *) &tf;
sValues[sValues_idx] = *tp;
}
values[args_idx] = &(sValues[sValues_idx]);
++args_idx;
++sValues_idx;
args[args_idx] = &ffi_type_double;
{
const double tf = 16.1;
const uint32_t * const tp = (uint32_t *) &tf;
sValues[sValues_idx] = *tp;
++sValues_idx;
sValues[sValues_idx] = *(tp + 1);
}
values[args_idx] = &(sValues[sValues_idx - 1]);
++args_idx;
++sValues_idx;
args[args_idx] = &ffi_type_float;
{
const float tf = 17.1;
const uint32_t * const tp = (uint32_t *) &tf;
sValues[sValues_idx] = *tp;
}
values[args_idx] = &(sValues[sValues_idx]);
++args_idx;
++sValues_idx;
args[args_idx] = &ffi_type_double;
{
const double tf = 18.1;
const uint32_t * const tp = (uint32_t *) &tf;
sValues[sValues_idx] = *tp;
++sValues_idx;
sValues[sValues_idx] = *(tp + 1);
}
values[args_idx] = &(sValues[sValues_idx - 1]);
++args_idx;
++sValues_idx;
args[args_idx] = &ffi_type_float;
{
const float tf = 19.1;
const uint32_t * const tp = (uint32_t *) &tf;
sValues[sValues_idx] = *tp;
}
values[args_idx] = &(sValues[sValues_idx]);
++args_idx;
++sValues_idx;
#ifdef __USE_F6__
args[args_idx] = &ffi_type_float;
{
const float tf = 20.1;
const uint32_t * const tp = (uint32_t *) &tf;
sValues[sValues_idx] = *tp;
}
values[args_idx] = &(sValues[sValues_idx]);
++args_idx;
++sValues_idx;
#else /* __USE_F6__ */
args[args_idx] = &ffi_type_double;
{
const double tf = 20.1;
const uint32_t * const tp = (uint32_t *) &tf;
sValues[sValues_idx] = *tp;
++sValues_idx;
sValues[sValues_idx] = *(tp + 1);
}
values[args_idx] = &(sValues[sValues_idx - 1]);
++args_idx;
++sValues_idx;
#endif /* __USE_F6__ */
printf("args_idx = %d\n", args_idx);
/* Initialize the cif and issue ffi call */
if (ffi_prep_cif(&cif, FFI_DEFAULT_ABI, NUM_TST_ARGS, &ffi_type_uint, args) == FFI_OK) {
ffi_call(&cif, FFI_FN(tst_func1), &rc, values);
printf("rc = %d - success? %c\n", rc, (1 == rc) ? 'Y' : 'N');
}
return rc;
}
int main() {
return test1();
}
[-- Attachment #3: Makefile --]
[-- Type: application/octet-stream, Size: 710 bytes --]
all: x86
x86: x86_ex1 x86_ex2
ppc: ppc_ex1 ppc_ex2
trg1:
gcc -D__USE_F6__ -g -m32 -c trg.c -o trg1.o
x86_ex1: trg1
gcc -D__USE_F6__ -g -m32 -I./x86/libffi_3.2.1/lib/libffi-3.2.1/include/ -L./x86/libffi_3.2.1/lib/ ex.c -o ex1 trg1.o -lffi
ppc_ex1: trg1
gcc -D__USE_F6__ -g -m32 -I./ppc/libffi_3.2.1/lib/libffi-3.2.1/include/ -L./ppc/libffi_3.2.1/lib/ ex.c -o ex1 trg1.o -lffi
trg2:
gcc -g -m32 -c trg.c -o trg2.o
x86_ex2: trg2
gcc -g -m32 -I./x86/libffi_3.2.1/lib/libffi-3.2.1/include/ -L./x86/libffi_3.2.1/lib/ ex.c -o ex2 trg2.o -lffi
ppc_ex2: trg2
gcc -g -m32 -I./ppc/libffi_3.2.1/lib/libffi-3.2.1/include/ -L./ppc/libffi_3.2.1/lib/ ex.c -o ex2 trg2.o -lffi
clean:
rm -f *.o ex1 ex2
[-- Attachment #4: trg.c --]
[-- Type: text/x-csrc, Size: 1246 bytes --]
#include <stdio.h>
int tst_func1(const char * p1, const char * p2, float f1, double d1, float f2, double d2,
#ifdef __USE_F6__
float f3, double d3, float f4, double d4, float f5, float f6)
#else /* __USE_F6__ */
float f3, double d3, float f4, double d4, float f5, double d5)
#endif /* __USE_F6__ */
{
const int success =
((const char *) 0xa == p1) &&
((const char *) 0xb == p2) &&
((float) 11.1 == f1) &&
((double) 12.1 == d1) &&
((float) 13.1 == f2) &&
((double) 14.1 == d2) &&
((float) 15.1 == f3) &&
((double) 16.1 == d3) &&
((float) 17.1 == f4) &&
((double) 18.1 == d4) &&
((float) 19.1 == f5) &&
#ifdef __USE_F6__
((float) 20.1 == f6);
#else /* __USE_F6__ */
((double) 20.1 == d5);
#endif /* __USE_F6__ */
#ifdef __USE_F6__
printf("%s : %p %p %f %g %f %g %f %g %f %g %f %f\n",
#else /* __USE_F6__ */
printf("%s : %p %p %f %g %f %g %f %g %f %g %f %g\n",
#endif /* __USE_F6__ */
__func__,
p1,
p2,
f1,
d1,
f2,
d2,
f3,
d3,
f4,
d4,
f5,
#ifdef __USE_F6__
f6);
#else /* __USE_F6__ */
d5);
#endif /* __USE_F6__ */
return success;
}
[-- Attachment #5: trg.h --]
[-- Type: text/x-chdr, Size: 347 bytes --]
#ifndef __TRG_H__
#define __TRG_H__
int tst_func1(const char * p1, const char * p2, float f1, double d1, float f2, double d2,
#ifdef __USE_F6__
float f3, double d3, float f4, double d4, float f5, float f6);
#else /* __USE_F6__ */
float f3, double d3, float f4, double d4, float f5, double d5);
#endif /* __USE_F6__ */
#endif /* __TRG_H__ */
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: PPC bug when handling double 2015-08-01 15:59 PPC bug when handling double Aaron R @ 2015-08-03 5:31 ` Alan Modra 2015-08-03 15:10 ` Alan Modra 0 siblings, 1 reply; 4+ messages in thread From: Alan Modra @ 2015-08-03 5:31 UTC (permalink / raw) To: Aaron R; +Cc: libffi-discuss On Sat, Aug 01, 2015 at 11:59:00AM -0400, Aaron R wrote: > I am not very familiar with libffi but I believe I may have stumbled upon a bug. I agree, you have found a bug. ffi_sysv.c:ffi_prep_args_SYSV does not correctly align doubles passed on the stack. -- Alan Modra Australia Development Lab, IBM ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: PPC bug when handling double 2015-08-03 5:31 ` Alan Modra @ 2015-08-03 15:10 ` Alan Modra 2015-08-04 11:46 ` Aaron R 0 siblings, 1 reply; 4+ messages in thread From: Alan Modra @ 2015-08-03 15:10 UTC (permalink / raw) To: Aaron R; +Cc: libffi-discuss On Mon, Aug 03, 2015 at 03:00:55PM +0930, Alan Modra wrote: > On Sat, Aug 01, 2015 at 11:59:00AM -0400, Aaron R wrote: > > I am not very familiar with libffi but I believe I may have stumbled upon a bug. > > I agree, you have found a bug. ffi_sysv.c:ffi_prep_args_SYSV does not > correctly align doubles passed on the stack. It's more than just alignment. The following is available in github.com/amodra/libffi.git, and I've created a pull request. Regression tested powerpc-linux. commit 43fc5bca48715a522b34c1124589575063362a90 Author: Alan Modra <amodra@gmail.com> Date: Mon Aug 3 23:34:05 2015 +0930 Correct powerpc sysv stack argument accounting ppc32 starts using the stack for integer arg passing when we run out of integer arg passing registers. Similarly, we start using the stack for floating point args when we run out of floating point registers. The decision on where an integer arg goes does not depend on number of floating point args, nor does the decision on where a floating point arg goes depend on number of integer args. Alignment of stack args also simply depends on number of stack args. This patch untangles the horrible mess we had, with intarg_count being wrongly used to count both integer args and stack words. * src/powerpc/ffi_sysv.c (ffi_prep_cif_sysv_core): Count fprs, gprs, and stack words separately. (ffi_prep_args_SYSV): Similarly. diff --git a/src/powerpc/ffi_sysv.c b/src/powerpc/ffi_sysv.c index 646c340..4078e75 100644 --- a/src/powerpc/ffi_sysv.c +++ b/src/powerpc/ffi_sysv.c @@ -93,7 +93,7 @@ ffi_prep_cif_sysv_core (ffi_cif *cif) { ffi_type **ptr; unsigned bytes; - unsigned i, fparg_count = 0, intarg_count = 0; + unsigned i, fpr_count = 0, gpr_count = 0, stack_count = 0; unsigned flags = cif->flags; unsigned struct_copy_size = 0; unsigned type = cif->rtype->type; @@ -155,7 +155,7 @@ ffi_prep_cif_sysv_core (ffi_cif *cif) flags |= FLAG_RETURNS_SMST; break; } - intarg_count++; + gpr_count++; flags |= FLAG_RETVAL_REFERENCE; /* Fall through. */ case FFI_TYPE_VOID: @@ -182,24 +182,41 @@ ffi_prep_cif_sysv_core (ffi_cif *cif) { #if FFI_TYPE_LONGDOUBLE != FFI_TYPE_DOUBLE case FFI_TYPE_LONGDOUBLE: - fparg_count++; - /* Fall thru */ + if (fpr_count >= NUM_FPR_ARG_REGISTERS - 1) + { + fpr_count = NUM_FPR_ARG_REGISTERS; + /* 8-byte align long doubles. */ + stack_count += stack_count & 1; + stack_count += 4; + } + else + fpr_count += 2; +#ifdef __NO_FPRS__ + return FFI_BAD_ABI; #endif + break; +#endif + case FFI_TYPE_DOUBLE: - fparg_count++; - /* If this FP arg is going on the stack, it must be - 8-byte-aligned. */ - if (fparg_count > NUM_FPR_ARG_REGISTERS - && intarg_count >= NUM_GPR_ARG_REGISTERS - && intarg_count % 2 != 0) - intarg_count++; + if (fpr_count >= NUM_FPR_ARG_REGISTERS) + { + /* 8-byte align doubles. */ + stack_count += stack_count & 1; + stack_count += 2; + } + else + fpr_count += 1; #ifdef __NO_FPRS__ return FFI_BAD_ABI; #endif break; case FFI_TYPE_FLOAT: - fparg_count++; + if (fpr_count >= NUM_FPR_ARG_REGISTERS) + /* Yes, we don't follow the ABI, but neither does gcc. */ + stack_count += 1; + else + fpr_count += 1; #ifdef __NO_FPRS__ return FFI_BAD_ABI; #endif @@ -208,11 +225,13 @@ ffi_prep_cif_sysv_core (ffi_cif *cif) case FFI_TYPE_UINT128: /* A long double in FFI_LINUX_SOFT_FLOAT can use only a set of four consecutive gprs. If we do not have enough, we - have to adjust the intarg_count value. */ - if (intarg_count >= NUM_GPR_ARG_REGISTERS - 3 - && intarg_count < NUM_GPR_ARG_REGISTERS) - intarg_count = NUM_GPR_ARG_REGISTERS; - intarg_count += 4; + have to adjust the gpr_count value. */ + if (gpr_count >= NUM_GPR_ARG_REGISTERS - 3) + gpr_count = NUM_GPR_ARG_REGISTERS; + if (gpr_count >= NUM_GPR_ARG_REGISTERS) + stack_count += 4; + else + gpr_count += 4; break; case FFI_TYPE_UINT64: @@ -225,10 +244,14 @@ ffi_prep_cif_sysv_core (ffi_cif *cif) Also, only certain register pairs can be used for passing long long int -- specifically (r3,r4), (r5,r6), (r7,r8), (r9,r10). */ - if (intarg_count == NUM_GPR_ARG_REGISTERS-1 - || intarg_count % 2 != 0) - intarg_count++; - intarg_count += 2; + gpr_count += gpr_count & 1; + if (gpr_count >= NUM_GPR_ARG_REGISTERS) + { + stack_count += stack_count & 1; + stack_count += 2; + } + else + gpr_count += 2; break; case FFI_TYPE_STRUCT: @@ -249,7 +272,10 @@ ffi_prep_cif_sysv_core (ffi_cif *cif) case FFI_TYPE_SINT8: /* Everything else is passed as a 4-byte word in a GPR, either the object itself or a pointer to it. */ - intarg_count++; + if (gpr_count >= NUM_GPR_ARG_REGISTERS) + stack_count += 1; + else + gpr_count += 1; break; default: @@ -257,22 +283,19 @@ ffi_prep_cif_sysv_core (ffi_cif *cif) } } - if (fparg_count != 0) + if (fpr_count != 0) flags |= FLAG_FP_ARGUMENTS; - if (intarg_count > 4) + if (gpr_count > 4) flags |= FLAG_4_GPR_ARGUMENTS; if (struct_copy_size != 0) flags |= FLAG_ARG_NEEDS_COPY; /* Space for the FPR registers, if needed. */ - if (fparg_count != 0) + if (fpr_count != 0) bytes += NUM_FPR_ARG_REGISTERS * sizeof (double); /* Stack space. */ - if (intarg_count > NUM_GPR_ARG_REGISTERS) - bytes += (intarg_count - NUM_GPR_ARG_REGISTERS) * sizeof (int); - if (fparg_count > NUM_FPR_ARG_REGISTERS) - bytes += (fparg_count - NUM_FPR_ARG_REGISTERS) * sizeof (double); + bytes += stack_count * sizeof (int); /* The stack space allocated needs to be a multiple of 16 bytes. */ bytes = (bytes + 15) & ~0xF; @@ -367,13 +390,13 @@ ffi_prep_args_SYSV (extended_cif *ecif, unsigned *const stack) /* 'gpr_base' points at the space for gpr3, and grows upwards as we use GPR registers. */ valp gpr_base; - int intarg_count; + valp gpr_end; #ifndef __NO_FPRS__ /* 'fpr_base' points at the space for fpr1, and grows upwards as we use FPR registers. */ valp fpr_base; - int fparg_count; + valp fpr_end; #endif /* 'copy_space' grows down as we put structures in it. It should @@ -405,11 +428,11 @@ ffi_prep_args_SYSV (extended_cif *ecif, unsigned *const stack) unsigned gprvalue; stacktop.c = (char *) stack + bytes; - gpr_base.u = stacktop.u - ASM_NEEDS_REGISTERS - NUM_GPR_ARG_REGISTERS; - intarg_count = 0; + gpr_end.u = stacktop.u - ASM_NEEDS_REGISTERS; + gpr_base.u = gpr_end.u - NUM_GPR_ARG_REGISTERS; #ifndef __NO_FPRS__ - fpr_base.d = gpr_base.d - NUM_FPR_ARG_REGISTERS; - fparg_count = 0; + fpr_end.d = gpr_base.d; + fpr_base.d = fpr_end.d - NUM_FPR_ARG_REGISTERS; copy_space.c = ((flags & FLAG_FP_ARGUMENTS) ? fpr_base.c : gpr_base.c); #else copy_space.c = gpr_base.c; @@ -425,10 +448,7 @@ ffi_prep_args_SYSV (extended_cif *ecif, unsigned *const stack) /* Deal with return values that are actually pass-by-reference. */ if (flags & FLAG_RETVAL_REFERENCE) - { - *gpr_base.u++ = (unsigned long) (char *) ecif->rvalue; - intarg_count++; - } + *gpr_base.u++ = (unsigned) (char *) ecif->rvalue; /* Now for the arguments. */ p_argv.v = ecif->avalue; @@ -448,14 +468,11 @@ ffi_prep_args_SYSV (extended_cif *ecif, unsigned *const stack) case FFI_TYPE_LONGDOUBLE: double_tmp = (*p_argv.d)[0]; - if (fparg_count >= NUM_FPR_ARG_REGISTERS - 1) + if (fpr_base.d >= fpr_end.d - 1) { - if (intarg_count >= NUM_GPR_ARG_REGISTERS - && intarg_count % 2 != 0) - { - intarg_count++; - next_arg.u++; - } + fpr_base.d = fpr_end.d; + if (((next_arg.u - stack) & 1) != 0) + next_arg.u += 1; *next_arg.d = double_tmp; next_arg.u += 2; double_tmp = (*p_argv.d)[1]; @@ -468,42 +485,33 @@ ffi_prep_args_SYSV (extended_cif *ecif, unsigned *const stack) double_tmp = (*p_argv.d)[1]; *fpr_base.d++ = double_tmp; } - - fparg_count += 2; FFI_ASSERT (flags & FLAG_FP_ARGUMENTS); break; # endif case FFI_TYPE_DOUBLE: double_tmp = **p_argv.d; - if (fparg_count >= NUM_FPR_ARG_REGISTERS) + if (fpr_base.d >= fpr_end.d) { - if (intarg_count >= NUM_GPR_ARG_REGISTERS - && intarg_count % 2 != 0) - { - intarg_count++; - next_arg.u++; - } + if (((next_arg.u - stack) & 1) != 0) + next_arg.u += 1; *next_arg.d = double_tmp; next_arg.u += 2; } else *fpr_base.d++ = double_tmp; - fparg_count++; FFI_ASSERT (flags & FLAG_FP_ARGUMENTS); break; case FFI_TYPE_FLOAT: double_tmp = **p_argv.f; - if (fparg_count >= NUM_FPR_ARG_REGISTERS) + if (fpr_base.d >= fpr_end.d) { *next_arg.f = (float) double_tmp; next_arg.u += 1; - intarg_count++; } else *fpr_base.d++ = double_tmp; - fparg_count++; FFI_ASSERT (flags & FLAG_FP_ARGUMENTS); break; #endif /* have FPRs */ @@ -513,42 +521,34 @@ ffi_prep_args_SYSV (extended_cif *ecif, unsigned *const stack) is passed in four consecutive GPRs if available. A maximum of 2 long doubles can be passed in gprs. If we do not have 4 GPRs left, the long double is passed on the stack, 4-byte aligned. */ - { - unsigned int int_tmp; - unsigned int ii; - if (intarg_count >= NUM_GPR_ARG_REGISTERS - 3) - { - if (intarg_count < NUM_GPR_ARG_REGISTERS) - intarg_count = NUM_GPR_ARG_REGISTERS; - for (ii = 0; ii < 4; ii++) - { - int_tmp = (*p_argv.ui)[ii]; - *next_arg.u++ = int_tmp; - } - } - else - { - for (ii = 0; ii < 4; ii++) - { - int_tmp = (*p_argv.ui)[ii]; - *gpr_base.u++ = int_tmp; - } - } - intarg_count += 4; - break; - } + if (gpr_base.u >= gpr_end.u - 3) + { + unsigned int ii; + gpr_base.u = gpr_end.u; + for (ii = 0; ii < 4; ii++) + { + unsigned int int_tmp = (*p_argv.ui)[ii]; + *next_arg.u++ = int_tmp; + } + } + else + { + unsigned int ii; + for (ii = 0; ii < 4; ii++) + { + unsigned int int_tmp = (*p_argv.ui)[ii]; + *gpr_base.u++ = int_tmp; + } + } + break; case FFI_TYPE_UINT64: case FFI_TYPE_SINT64: - if (intarg_count == NUM_GPR_ARG_REGISTERS-1) - intarg_count++; - if (intarg_count >= NUM_GPR_ARG_REGISTERS) + if (gpr_base.u >= gpr_end.u - 1) { - if (intarg_count % 2 != 0) - { - intarg_count++; - next_arg.u++; - } + gpr_base.u = gpr_end.u; + if (((next_arg.u - stack) & 1) != 0) + next_arg.u++; *next_arg.ll = **p_argv.ll; next_arg.u += 2; } @@ -559,14 +559,10 @@ ffi_prep_args_SYSV (extended_cif *ecif, unsigned *const stack) (r5,r6), (r7,r8), (r9,r10). If next arg is long long but not correct starting register of pair then skip until the proper starting register. */ - if (intarg_count % 2 != 0) - { - intarg_count ++; - gpr_base.u++; - } + if (((gpr_end.u - gpr_base.u) & 1) != 0) + gpr_base.u++; *gpr_base.ll++ = **p_argv.ll; } - intarg_count += 2; break; case FFI_TYPE_STRUCT: @@ -601,29 +597,22 @@ ffi_prep_args_SYSV (extended_cif *ecif, unsigned *const stack) gprvalue = **p_argv.ui; putgpr: - if (intarg_count >= NUM_GPR_ARG_REGISTERS) + if (gpr_base.u >= gpr_end.u) *next_arg.u++ = gprvalue; else *gpr_base.u++ = gprvalue; - intarg_count++; break; } } /* Check that we didn't overrun the stack... */ FFI_ASSERT (copy_space.c >= next_arg.c); - FFI_ASSERT (gpr_base.u <= stacktop.u - ASM_NEEDS_REGISTERS); - /* The assert below is testing that the number of integer arguments agrees - with the number found in ffi_prep_cif_machdep(). However, intarg_count - is incremented whenever we place an FP arg on the stack, so account for - that before our assert test. */ + FFI_ASSERT (gpr_base.u <= gpr_end.u); #ifndef __NO_FPRS__ - if (fparg_count > NUM_FPR_ARG_REGISTERS) - intarg_count -= fparg_count - NUM_FPR_ARG_REGISTERS; - FFI_ASSERT (fpr_base.u - <= stacktop.u - ASM_NEEDS_REGISTERS - NUM_GPR_ARG_REGISTERS); + FFI_ASSERT (fpr_base.u <= fpr_end.u); #endif - FFI_ASSERT (flags & FLAG_4_GPR_ARGUMENTS || intarg_count <= 4); + FFI_ASSERT (((flags & FLAG_4_GPR_ARGUMENTS) != 0) + == (gpr_end.u - gpr_base.u < 4)); } #define MIN_CACHE_LINE_SIZE 8 -- Alan Modra Australia Development Lab, IBM ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: PPC bug when handling double 2015-08-03 15:10 ` Alan Modra @ 2015-08-04 11:46 ` Aaron R 0 siblings, 0 replies; 4+ messages in thread From: Aaron R @ 2015-08-04 11:46 UTC (permalink / raw) To: Alan Modra; +Cc: libffi-discuss Cool! I have applied your patch on top of libffi 3.2.1 and I have confirmed that it does indeed fix the problem. The test case passes now. Thanks for the help On Mon, Aug 3, 2015 at 11:09 AM, Alan Modra <amodra@gmail.com> wrote: > On Mon, Aug 03, 2015 at 03:00:55PM +0930, Alan Modra wrote: >> On Sat, Aug 01, 2015 at 11:59:00AM -0400, Aaron R wrote: >> > I am not very familiar with libffi but I believe I may have stumbled upon a bug. >> >> I agree, you have found a bug. ffi_sysv.c:ffi_prep_args_SYSV does not >> correctly align doubles passed on the stack. > > It's more than just alignment. The following is available in > github.com/amodra/libffi.git, and I've created a pull request. > Regression tested powerpc-linux. > > commit 43fc5bca48715a522b34c1124589575063362a90 > Author: Alan Modra <amodra@gmail.com> > Date: Mon Aug 3 23:34:05 2015 +0930 > > Correct powerpc sysv stack argument accounting > > ppc32 starts using the stack for integer arg passing when we run out > of integer arg passing registers. Similarly, we start using the stack > for floating point args when we run out of floating point registers. > The decision on where an integer arg goes does not depend on number of > floating point args, nor does the decision on where a floating point > arg goes depend on number of integer args. Alignment of stack args > also simply depends on number of stack args. > > This patch untangles the horrible mess we had, with intarg_count being > wrongly used to count both integer args and stack words. > > * src/powerpc/ffi_sysv.c (ffi_prep_cif_sysv_core): Count fprs, > gprs, and stack words separately. > (ffi_prep_args_SYSV): Similarly. > > diff --git a/src/powerpc/ffi_sysv.c b/src/powerpc/ffi_sysv.c > index 646c340..4078e75 100644 > --- a/src/powerpc/ffi_sysv.c > +++ b/src/powerpc/ffi_sysv.c > @@ -93,7 +93,7 @@ ffi_prep_cif_sysv_core (ffi_cif *cif) > { > ffi_type **ptr; > unsigned bytes; > - unsigned i, fparg_count = 0, intarg_count = 0; > + unsigned i, fpr_count = 0, gpr_count = 0, stack_count = 0; > unsigned flags = cif->flags; > unsigned struct_copy_size = 0; > unsigned type = cif->rtype->type; > @@ -155,7 +155,7 @@ ffi_prep_cif_sysv_core (ffi_cif *cif) > flags |= FLAG_RETURNS_SMST; > break; > } > - intarg_count++; > + gpr_count++; > flags |= FLAG_RETVAL_REFERENCE; > /* Fall through. */ > case FFI_TYPE_VOID: > @@ -182,24 +182,41 @@ ffi_prep_cif_sysv_core (ffi_cif *cif) > { > #if FFI_TYPE_LONGDOUBLE != FFI_TYPE_DOUBLE > case FFI_TYPE_LONGDOUBLE: > - fparg_count++; > - /* Fall thru */ > + if (fpr_count >= NUM_FPR_ARG_REGISTERS - 1) > + { > + fpr_count = NUM_FPR_ARG_REGISTERS; > + /* 8-byte align long doubles. */ > + stack_count += stack_count & 1; > + stack_count += 4; > + } > + else > + fpr_count += 2; > +#ifdef __NO_FPRS__ > + return FFI_BAD_ABI; > #endif > + break; > +#endif > + > case FFI_TYPE_DOUBLE: > - fparg_count++; > - /* If this FP arg is going on the stack, it must be > - 8-byte-aligned. */ > - if (fparg_count > NUM_FPR_ARG_REGISTERS > - && intarg_count >= NUM_GPR_ARG_REGISTERS > - && intarg_count % 2 != 0) > - intarg_count++; > + if (fpr_count >= NUM_FPR_ARG_REGISTERS) > + { > + /* 8-byte align doubles. */ > + stack_count += stack_count & 1; > + stack_count += 2; > + } > + else > + fpr_count += 1; > #ifdef __NO_FPRS__ > return FFI_BAD_ABI; > #endif > break; > > case FFI_TYPE_FLOAT: > - fparg_count++; > + if (fpr_count >= NUM_FPR_ARG_REGISTERS) > + /* Yes, we don't follow the ABI, but neither does gcc. */ > + stack_count += 1; > + else > + fpr_count += 1; > #ifdef __NO_FPRS__ > return FFI_BAD_ABI; > #endif > @@ -208,11 +225,13 @@ ffi_prep_cif_sysv_core (ffi_cif *cif) > case FFI_TYPE_UINT128: > /* A long double in FFI_LINUX_SOFT_FLOAT can use only a set > of four consecutive gprs. If we do not have enough, we > - have to adjust the intarg_count value. */ > - if (intarg_count >= NUM_GPR_ARG_REGISTERS - 3 > - && intarg_count < NUM_GPR_ARG_REGISTERS) > - intarg_count = NUM_GPR_ARG_REGISTERS; > - intarg_count += 4; > + have to adjust the gpr_count value. */ > + if (gpr_count >= NUM_GPR_ARG_REGISTERS - 3) > + gpr_count = NUM_GPR_ARG_REGISTERS; > + if (gpr_count >= NUM_GPR_ARG_REGISTERS) > + stack_count += 4; > + else > + gpr_count += 4; > break; > > case FFI_TYPE_UINT64: > @@ -225,10 +244,14 @@ ffi_prep_cif_sysv_core (ffi_cif *cif) > Also, only certain register pairs can be used for > passing long long int -- specifically (r3,r4), (r5,r6), > (r7,r8), (r9,r10). */ > - if (intarg_count == NUM_GPR_ARG_REGISTERS-1 > - || intarg_count % 2 != 0) > - intarg_count++; > - intarg_count += 2; > + gpr_count += gpr_count & 1; > + if (gpr_count >= NUM_GPR_ARG_REGISTERS) > + { > + stack_count += stack_count & 1; > + stack_count += 2; > + } > + else > + gpr_count += 2; > break; > > case FFI_TYPE_STRUCT: > @@ -249,7 +272,10 @@ ffi_prep_cif_sysv_core (ffi_cif *cif) > case FFI_TYPE_SINT8: > /* Everything else is passed as a 4-byte word in a GPR, either > the object itself or a pointer to it. */ > - intarg_count++; > + if (gpr_count >= NUM_GPR_ARG_REGISTERS) > + stack_count += 1; > + else > + gpr_count += 1; > break; > > default: > @@ -257,22 +283,19 @@ ffi_prep_cif_sysv_core (ffi_cif *cif) > } > } > > - if (fparg_count != 0) > + if (fpr_count != 0) > flags |= FLAG_FP_ARGUMENTS; > - if (intarg_count > 4) > + if (gpr_count > 4) > flags |= FLAG_4_GPR_ARGUMENTS; > if (struct_copy_size != 0) > flags |= FLAG_ARG_NEEDS_COPY; > > /* Space for the FPR registers, if needed. */ > - if (fparg_count != 0) > + if (fpr_count != 0) > bytes += NUM_FPR_ARG_REGISTERS * sizeof (double); > > /* Stack space. */ > - if (intarg_count > NUM_GPR_ARG_REGISTERS) > - bytes += (intarg_count - NUM_GPR_ARG_REGISTERS) * sizeof (int); > - if (fparg_count > NUM_FPR_ARG_REGISTERS) > - bytes += (fparg_count - NUM_FPR_ARG_REGISTERS) * sizeof (double); > + bytes += stack_count * sizeof (int); > > /* The stack space allocated needs to be a multiple of 16 bytes. */ > bytes = (bytes + 15) & ~0xF; > @@ -367,13 +390,13 @@ ffi_prep_args_SYSV (extended_cif *ecif, unsigned *const stack) > /* 'gpr_base' points at the space for gpr3, and grows upwards as > we use GPR registers. */ > valp gpr_base; > - int intarg_count; > + valp gpr_end; > > #ifndef __NO_FPRS__ > /* 'fpr_base' points at the space for fpr1, and grows upwards as > we use FPR registers. */ > valp fpr_base; > - int fparg_count; > + valp fpr_end; > #endif > > /* 'copy_space' grows down as we put structures in it. It should > @@ -405,11 +428,11 @@ ffi_prep_args_SYSV (extended_cif *ecif, unsigned *const stack) > unsigned gprvalue; > > stacktop.c = (char *) stack + bytes; > - gpr_base.u = stacktop.u - ASM_NEEDS_REGISTERS - NUM_GPR_ARG_REGISTERS; > - intarg_count = 0; > + gpr_end.u = stacktop.u - ASM_NEEDS_REGISTERS; > + gpr_base.u = gpr_end.u - NUM_GPR_ARG_REGISTERS; > #ifndef __NO_FPRS__ > - fpr_base.d = gpr_base.d - NUM_FPR_ARG_REGISTERS; > - fparg_count = 0; > + fpr_end.d = gpr_base.d; > + fpr_base.d = fpr_end.d - NUM_FPR_ARG_REGISTERS; > copy_space.c = ((flags & FLAG_FP_ARGUMENTS) ? fpr_base.c : gpr_base.c); > #else > copy_space.c = gpr_base.c; > @@ -425,10 +448,7 @@ ffi_prep_args_SYSV (extended_cif *ecif, unsigned *const stack) > > /* Deal with return values that are actually pass-by-reference. */ > if (flags & FLAG_RETVAL_REFERENCE) > - { > - *gpr_base.u++ = (unsigned long) (char *) ecif->rvalue; > - intarg_count++; > - } > + *gpr_base.u++ = (unsigned) (char *) ecif->rvalue; > > /* Now for the arguments. */ > p_argv.v = ecif->avalue; > @@ -448,14 +468,11 @@ ffi_prep_args_SYSV (extended_cif *ecif, unsigned *const stack) > case FFI_TYPE_LONGDOUBLE: > double_tmp = (*p_argv.d)[0]; > > - if (fparg_count >= NUM_FPR_ARG_REGISTERS - 1) > + if (fpr_base.d >= fpr_end.d - 1) > { > - if (intarg_count >= NUM_GPR_ARG_REGISTERS > - && intarg_count % 2 != 0) > - { > - intarg_count++; > - next_arg.u++; > - } > + fpr_base.d = fpr_end.d; > + if (((next_arg.u - stack) & 1) != 0) > + next_arg.u += 1; > *next_arg.d = double_tmp; > next_arg.u += 2; > double_tmp = (*p_argv.d)[1]; > @@ -468,42 +485,33 @@ ffi_prep_args_SYSV (extended_cif *ecif, unsigned *const stack) > double_tmp = (*p_argv.d)[1]; > *fpr_base.d++ = double_tmp; > } > - > - fparg_count += 2; > FFI_ASSERT (flags & FLAG_FP_ARGUMENTS); > break; > # endif > case FFI_TYPE_DOUBLE: > double_tmp = **p_argv.d; > > - if (fparg_count >= NUM_FPR_ARG_REGISTERS) > + if (fpr_base.d >= fpr_end.d) > { > - if (intarg_count >= NUM_GPR_ARG_REGISTERS > - && intarg_count % 2 != 0) > - { > - intarg_count++; > - next_arg.u++; > - } > + if (((next_arg.u - stack) & 1) != 0) > + next_arg.u += 1; > *next_arg.d = double_tmp; > next_arg.u += 2; > } > else > *fpr_base.d++ = double_tmp; > - fparg_count++; > FFI_ASSERT (flags & FLAG_FP_ARGUMENTS); > break; > > case FFI_TYPE_FLOAT: > double_tmp = **p_argv.f; > - if (fparg_count >= NUM_FPR_ARG_REGISTERS) > + if (fpr_base.d >= fpr_end.d) > { > *next_arg.f = (float) double_tmp; > next_arg.u += 1; > - intarg_count++; > } > else > *fpr_base.d++ = double_tmp; > - fparg_count++; > FFI_ASSERT (flags & FLAG_FP_ARGUMENTS); > break; > #endif /* have FPRs */ > @@ -513,42 +521,34 @@ ffi_prep_args_SYSV (extended_cif *ecif, unsigned *const stack) > is passed in four consecutive GPRs if available. A maximum of 2 > long doubles can be passed in gprs. If we do not have 4 GPRs > left, the long double is passed on the stack, 4-byte aligned. */ > - { > - unsigned int int_tmp; > - unsigned int ii; > - if (intarg_count >= NUM_GPR_ARG_REGISTERS - 3) > - { > - if (intarg_count < NUM_GPR_ARG_REGISTERS) > - intarg_count = NUM_GPR_ARG_REGISTERS; > - for (ii = 0; ii < 4; ii++) > - { > - int_tmp = (*p_argv.ui)[ii]; > - *next_arg.u++ = int_tmp; > - } > - } > - else > - { > - for (ii = 0; ii < 4; ii++) > - { > - int_tmp = (*p_argv.ui)[ii]; > - *gpr_base.u++ = int_tmp; > - } > - } > - intarg_count += 4; > - break; > - } > + if (gpr_base.u >= gpr_end.u - 3) > + { > + unsigned int ii; > + gpr_base.u = gpr_end.u; > + for (ii = 0; ii < 4; ii++) > + { > + unsigned int int_tmp = (*p_argv.ui)[ii]; > + *next_arg.u++ = int_tmp; > + } > + } > + else > + { > + unsigned int ii; > + for (ii = 0; ii < 4; ii++) > + { > + unsigned int int_tmp = (*p_argv.ui)[ii]; > + *gpr_base.u++ = int_tmp; > + } > + } > + break; > > case FFI_TYPE_UINT64: > case FFI_TYPE_SINT64: > - if (intarg_count == NUM_GPR_ARG_REGISTERS-1) > - intarg_count++; > - if (intarg_count >= NUM_GPR_ARG_REGISTERS) > + if (gpr_base.u >= gpr_end.u - 1) > { > - if (intarg_count % 2 != 0) > - { > - intarg_count++; > - next_arg.u++; > - } > + gpr_base.u = gpr_end.u; > + if (((next_arg.u - stack) & 1) != 0) > + next_arg.u++; > *next_arg.ll = **p_argv.ll; > next_arg.u += 2; > } > @@ -559,14 +559,10 @@ ffi_prep_args_SYSV (extended_cif *ecif, unsigned *const stack) > (r5,r6), (r7,r8), (r9,r10). If next arg is long long > but not correct starting register of pair then skip > until the proper starting register. */ > - if (intarg_count % 2 != 0) > - { > - intarg_count ++; > - gpr_base.u++; > - } > + if (((gpr_end.u - gpr_base.u) & 1) != 0) > + gpr_base.u++; > *gpr_base.ll++ = **p_argv.ll; > } > - intarg_count += 2; > break; > > case FFI_TYPE_STRUCT: > @@ -601,29 +597,22 @@ ffi_prep_args_SYSV (extended_cif *ecif, unsigned *const stack) > gprvalue = **p_argv.ui; > > putgpr: > - if (intarg_count >= NUM_GPR_ARG_REGISTERS) > + if (gpr_base.u >= gpr_end.u) > *next_arg.u++ = gprvalue; > else > *gpr_base.u++ = gprvalue; > - intarg_count++; > break; > } > } > > /* Check that we didn't overrun the stack... */ > FFI_ASSERT (copy_space.c >= next_arg.c); > - FFI_ASSERT (gpr_base.u <= stacktop.u - ASM_NEEDS_REGISTERS); > - /* The assert below is testing that the number of integer arguments agrees > - with the number found in ffi_prep_cif_machdep(). However, intarg_count > - is incremented whenever we place an FP arg on the stack, so account for > - that before our assert test. */ > + FFI_ASSERT (gpr_base.u <= gpr_end.u); > #ifndef __NO_FPRS__ > - if (fparg_count > NUM_FPR_ARG_REGISTERS) > - intarg_count -= fparg_count - NUM_FPR_ARG_REGISTERS; > - FFI_ASSERT (fpr_base.u > - <= stacktop.u - ASM_NEEDS_REGISTERS - NUM_GPR_ARG_REGISTERS); > + FFI_ASSERT (fpr_base.u <= fpr_end.u); > #endif > - FFI_ASSERT (flags & FLAG_4_GPR_ARGUMENTS || intarg_count <= 4); > + FFI_ASSERT (((flags & FLAG_4_GPR_ARGUMENTS) != 0) > + == (gpr_end.u - gpr_base.u < 4)); > } > > #define MIN_CACHE_LINE_SIZE 8 > > > -- > Alan Modra > Australia Development Lab, IBM ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2015-08-04 11:46 UTC | newest] Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2015-08-01 15:59 PPC bug when handling double Aaron R 2015-08-03 5:31 ` Alan Modra 2015-08-03 15:10 ` Alan Modra 2015-08-04 11:46 ` Aaron R
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).