From 9e29217c73f36802de616c05bee9bf7f9090f722 Mon Sep 17 00:00:00 2001 From: Damien George Date: Wed, 14 Apr 2021 15:37:33 +1000 Subject: [PATCH] unix/modffi: Use a union for passing/returning FFI values. This fixes a bug where double arguments on a 32-bit architecture would not be passed correctly because they only had 4 bytes of storage (not 8). It also fixes a compiler warning/error in return_ffi_value on certian architectures: array subscript 'double[0]' is partly outside array bounds of 'ffi_arg[1]' {aka 'long unsigned int[1]'}. Fixes issue #7064. Signed-off-by: Damien George --- ports/unix/modffi.c | 62 +++++++++++++++---------------------- tests/unix/ffi_float.py | 13 ++++++-- tests/unix/ffi_float.py.exp | 20 ++++++++---- 3 files changed, 50 insertions(+), 45 deletions(-) diff --git a/ports/unix/modffi.c b/ports/unix/modffi.c index 598a28cd5..5c2595ec5 100644 --- a/ports/unix/modffi.c +++ b/ports/unix/modffi.c @@ -56,6 +56,13 @@ * but may be later. */ +// This union is large enough to hold any supported argument/return value. +typedef union _ffi_union_t { + ffi_arg ffi; + float flt; + double dbl; +} ffi_union_t; + typedef struct _mp_obj_opaque_t { mp_obj_base_t base; void *val; @@ -151,10 +158,10 @@ STATIC ffi_type *get_ffi_type(mp_obj_t o_in) { mp_raise_TypeError(MP_ERROR_TEXT("unknown type")); } -STATIC mp_obj_t return_ffi_value(ffi_arg val, char type) { +STATIC mp_obj_t return_ffi_value(ffi_union_t *val, char type) { switch (type) { case 's': { - const char *s = (const char *)(intptr_t)val; + const char *s = (const char *)(intptr_t)val->ffi; if (!s) { return mp_const_none; } @@ -164,20 +171,16 @@ STATIC mp_obj_t return_ffi_value(ffi_arg val, char type) { return mp_const_none; #if MICROPY_PY_BUILTINS_FLOAT case 'f': { - union { ffi_arg ffi; - float flt; - } val_union = { .ffi = val }; - return mp_obj_new_float_from_f(val_union.flt); + return mp_obj_new_float_from_f(val->flt); } case 'd': { - double *p = (double *)&val; - return mp_obj_new_float_from_d(*p); + return mp_obj_new_float_from_d(val->dbl); } #endif case 'O': - return (mp_obj_t)(intptr_t)val; + return (mp_obj_t)(intptr_t)val->ffi; default: - return mp_obj_new_int(val); + return mp_obj_new_int(val->ffi); } } @@ -368,28 +371,26 @@ STATIC mp_obj_t ffifunc_call(mp_obj_t self_in, size_t n_args, size_t n_kw, const assert(n_kw == 0); assert(n_args == self->cif.nargs); - ffi_arg values[n_args]; + ffi_union_t values[n_args]; void *valueptrs[n_args]; const char *argtype = self->argtypes; for (uint i = 0; i < n_args; i++, argtype++) { mp_obj_t a = args[i]; if (*argtype == 'O') { - values[i] = (ffi_arg)(intptr_t)a; + values[i].ffi = (ffi_arg)(intptr_t)a; #if MICROPY_PY_BUILTINS_FLOAT } else if (*argtype == 'f') { - float *p = (float *)&values[i]; - *p = mp_obj_get_float_to_f(a); + values[i].flt = mp_obj_get_float_to_f(a); } else if (*argtype == 'd') { - double *p = (double *)&values[i]; - *p = mp_obj_get_float_to_d(a); + values[i].dbl = mp_obj_get_float_to_d(a); #endif } else if (a == mp_const_none) { - values[i] = 0; + values[i].ffi = 0; } else if (mp_obj_is_int(a)) { - values[i] = mp_obj_int_get_truncated(a); + values[i].ffi = mp_obj_int_get_truncated(a); } else if (mp_obj_is_str(a)) { const char *s = mp_obj_str_get_str(a); - values[i] = (ffi_arg)(intptr_t)s; + values[i].ffi = (ffi_arg)(intptr_t)s; } else if (((mp_obj_base_t *)MP_OBJ_TO_PTR(a))->type->buffer_p.get_buffer != NULL) { mp_obj_base_t *o = (mp_obj_base_t *)MP_OBJ_TO_PTR(a); mp_buffer_info_t bufinfo; @@ -397,32 +398,19 @@ STATIC mp_obj_t ffifunc_call(mp_obj_t self_in, size_t n_args, size_t n_kw, const if (ret != 0) { goto error; } - values[i] = (ffi_arg)(intptr_t)bufinfo.buf; + values[i].ffi = (ffi_arg)(intptr_t)bufinfo.buf; } else if (mp_obj_is_type(a, &fficallback_type)) { mp_obj_fficallback_t *p = MP_OBJ_TO_PTR(a); - values[i] = (ffi_arg)(intptr_t)p->func; + values[i].ffi = (ffi_arg)(intptr_t)p->func; } else { goto error; } valueptrs[i] = &values[i]; } - // If ffi_arg is not big enough to hold a double, then we must pass along a - // pointer to a memory location of the correct size. - // TODO check if this needs to be done for other types which don't fit into - // ffi_arg. - #if MICROPY_PY_BUILTINS_FLOAT - if (sizeof(ffi_arg) == 4 && self->rettype == 'd') { - double retval; - ffi_call(&self->cif, self->func, &retval, valueptrs); - return mp_obj_new_float_from_d(retval); - } else - #endif - { - ffi_arg retval; - ffi_call(&self->cif, self->func, &retval, valueptrs); - return return_ffi_value(retval, self->rettype); - } + ffi_union_t retval; + ffi_call(&self->cif, self->func, &retval, valueptrs); + return return_ffi_value(&retval, self->rettype); error: mp_raise_TypeError(MP_ERROR_TEXT("don't know how to pass object to native function")); diff --git a/tests/unix/ffi_float.py b/tests/unix/ffi_float.py index d03939896..03bd9f7f1 100644 --- a/tests/unix/ffi_float.py +++ b/tests/unix/ffi_float.py @@ -35,6 +35,15 @@ print("%.6f" % strtod("1.23", None)) # test passing double and float args libm = ffi_open(("libm.so", "libm.so.6", "libc.so.0", "libc.so.6", "libc.dylib")) tgamma = libm.func("d", "tgamma", "d") -for fun in (tgamma,): +for fun_name in ("tgamma",): + fun = globals()[fun_name] for val in (0.5, 1, 1.0, 1.5, 4, 4.0): - print("%.6f" % fun(val)) + print(fun_name, "%.5f" % fun(val)) + +# test passing 2x float/double args +powf = libm.func("f", "powf", "ff") +pow = libm.func("d", "pow", "dd") +for fun_name in ("powf", "pow"): + fun = globals()[fun_name] + for args in ((0, 1), (1, 0), (2, 0.5), (3, 4)): + print(fun_name, "%.5f" % fun(*args)) diff --git a/tests/unix/ffi_float.py.exp b/tests/unix/ffi_float.py.exp index b9d7da2bd..3d9091431 100644 --- a/tests/unix/ffi_float.py.exp +++ b/tests/unix/ffi_float.py.exp @@ -1,8 +1,16 @@ 1.230000 1.230000 -1.772454 -1.000000 -1.000000 -0.886227 -6.000000 -6.000000 +tgamma 1.77245 +tgamma 1.00000 +tgamma 1.00000 +tgamma 0.88623 +tgamma 6.00000 +tgamma 6.00000 +powf 0.00000 +powf 1.00000 +powf 1.41421 +powf 81.00000 +pow 0.00000 +pow 1.00000 +pow 1.41421 +pow 81.00000