| Message ID | CAFULd4arpzXGiSpEYz1uToPJ94homCko+gsf-zbXtweVNaiHpA@mail.gmail.com |
|---|---|
| State | New |
| Headers | show |
On Fri, Mar 1, 2013 at 4:57 AM, Uros Bizjak <ubizjak@gmail.com> wrote: > > Due to the fact that libFFI does not handle C99 _Complex arguments > correctly [1], libgo passes Complex64 and Complex128 arguments via a > temporary structure. However, passing parts of complex number in a > structure is not the same as passing true C99 _Complex value, so this > workaround introduces ABI mismatch between caller and callee. This > mismatch results in wrong passed values of complex types. > > Fortunately all x86 ABIs tolerate this mismatch, but other targets > (i.e. alpha) don't have this privilege. Is there a PR open against libffi? Do we have any idea which targets pass complex in a manner different than a struct of two float/doubles? Your patch assumes that only x86 targets work, but I would expect that many targets work that way. Ian
On Fri, Mar 1, 2013 at 3:43 PM, Ian Lance Taylor <iant@google.com> wrote: > On Fri, Mar 1, 2013 at 4:57 AM, Uros Bizjak <ubizjak@gmail.com> wrote: >> >> Due to the fact that libFFI does not handle C99 _Complex arguments >> correctly [1], libgo passes Complex64 and Complex128 arguments via a >> temporary structure. However, passing parts of complex number in a >> structure is not the same as passing true C99 _Complex value, so this >> workaround introduces ABI mismatch between caller and callee. This >> mismatch results in wrong passed values of complex types. >> >> Fortunately all x86 ABIs tolerate this mismatch, but other targets >> (i.e. alpha) don't have this privilege. > > Is there a PR open against libffi? Not that I know of, but I can open one if requested. > Do we have any idea which targets pass complex in a manner different > than a struct of two float/doubles? Your patch assumes that only x86 > targets work, but I would expect that many targets work that way. Maybe a test could be added to reflect package that calls foreign function with _Complex arguments/return value? The function should do some basic arithmetic on complex values and the test could check if it returns expected values. This test would reliably catch affected targets. Uros.
On Fri, Mar 1, 2013 at 3:50 PM, Uros Bizjak <ubizjak@gmail.com> wrote: >>> Due to the fact that libFFI does not handle C99 _Complex arguments >>> correctly [1], libgo passes Complex64 and Complex128 arguments via a >>> temporary structure. However, passing parts of complex number in a >>> structure is not the same as passing true C99 _Complex value, so this >>> workaround introduces ABI mismatch between caller and callee. This >>> mismatch results in wrong passed values of complex types. >>> >>> Fortunately all x86 ABIs tolerate this mismatch, but other targets >>> (i.e. alpha) don't have this privilege. >> >> Is there a PR open against libffi? > > Not that I know of, but I can open one if requested. FYI, python is also interested [1] in this enhancement. [1] http://bugs.python.org/issue16899 Uros.
On Fri, Mar 1, 2013 at 3:43 PM, Ian Lance Taylor <iant@google.com> wrote: > On Fri, Mar 1, 2013 at 4:57 AM, Uros Bizjak <ubizjak@gmail.com> wrote: >> >> Due to the fact that libFFI does not handle C99 _Complex arguments >> correctly [1], libgo passes Complex64 and Complex128 arguments via a >> temporary structure. However, passing parts of complex number in a >> structure is not the same as passing true C99 _Complex value, so this >> workaround introduces ABI mismatch between caller and callee. This >> mismatch results in wrong passed values of complex types. >> >> Fortunately all x86 ABIs tolerate this mismatch, but other targets >> (i.e. alpha) don't have this privilege. > > Is there a PR open against libffi? > > Do we have any idea which targets pass complex in a manner different > than a struct of two float/doubles? Your patch assumes that only x86 > targets work, but I would expect that many targets work that way. $ grep -R "define TARGET_SPLIT_COMPLEX_ARG" config config/rs6000/rs6000.c:#define TARGET_SPLIT_COMPLEX_ARG hook_bool_const_tree_true config/xtensa/xtensa.c:#define TARGET_SPLIT_COMPLEX_ARG hook_bool_const_tree_true config/alpha/alpha.c:#define TARGET_SPLIT_COMPLEX_ARG alpha_split_complex_arg We can probably define Complex64 type as "struct { float re, im; }" and Complex128 as "struct { double re, im; }" for these targets. What do you think about this approach? Uros.
On Fri, Mar 1, 2013 at 9:34 AM, Uros Bizjak <ubizjak@gmail.com> wrote: > On Fri, Mar 1, 2013 at 3:43 PM, Ian Lance Taylor <iant@google.com> wrote: >> On Fri, Mar 1, 2013 at 4:57 AM, Uros Bizjak <ubizjak@gmail.com> wrote: >>> >>> Due to the fact that libFFI does not handle C99 _Complex arguments >>> correctly [1], libgo passes Complex64 and Complex128 arguments via a >>> temporary structure. However, passing parts of complex number in a >>> structure is not the same as passing true C99 _Complex value, so this >>> workaround introduces ABI mismatch between caller and callee. This >>> mismatch results in wrong passed values of complex types. >>> >>> Fortunately all x86 ABIs tolerate this mismatch, but other targets >>> (i.e. alpha) don't have this privilege. >> >> Is there a PR open against libffi? >> >> Do we have any idea which targets pass complex in a manner different >> than a struct of two float/doubles? Your patch assumes that only x86 >> targets work, but I would expect that many targets work that way. > > $ grep -R "define TARGET_SPLIT_COMPLEX_ARG" config > config/rs6000/rs6000.c:#define TARGET_SPLIT_COMPLEX_ARG > hook_bool_const_tree_true > config/xtensa/xtensa.c:#define TARGET_SPLIT_COMPLEX_ARG > hook_bool_const_tree_true > config/alpha/alpha.c:#define TARGET_SPLIT_COMPLEX_ARG alpha_split_complex_arg > > We can probably define Complex64 type as "struct { float re, im; }" > and Complex128 as "struct { double re, im; }" for these targets. What > do you think about this approach? The odd thing is that those are exactly the targets I would expect to work with the current code. I don't see why those should be the targets that fail. Right now the reflect.Call implementation is passing Go complex64 and complex128 types as though they were implemented as struct { float32; float32 } and struct { float64; float64 } respectively. The code assumes that a function that takes a complex64 argument can be called as though it took a struct argument instead. Apparently that does not work on Alpha. So the case that fails is when the calling convention for a complex number differs from the calling convention for passing a struct. In the Alpha calling convention, how is a complex number passed? How is a struct of two floats/doubles passed? I think your suggestion of implementing complex as a struct would be somewhat painful to implement, because it would require generating complicated code for all complex arithmetic. I'm not strongly opposed to your original patch, I just think it overreaches in assuming that only x86 works. What if we flip it around and assume that only Alpha falis? Ian
Index: runtime/go-reflect-call.c =================================================================== --- runtime/go-reflect-call.c (revision 196368) +++ runtime/go-reflect-call.c (working copy) @@ -30,7 +30,7 @@ static ffi_type *go_string_to_ffi (void) __attribute__ ((no_split_stack)); static ffi_type *go_interface_to_ffi (void) __attribute__ ((no_split_stack)); static ffi_type *go_complex_to_ffi (ffi_type *) - __attribute__ ((no_split_stack)); + __attribute__ ((no_split_stack,unused)); static ffi_type *go_type_to_ffi (const struct __go_type_descriptor *) __attribute__ ((no_split_stack)); static ffi_type *go_func_return_ffi (const struct __go_func_type *) @@ -185,13 +185,23 @@ return &ffi_type_double; abort (); case GO_COMPLEX64: +#if defined (__i386__) || defined (__x86_64__) if (sizeof (float) == 4) return go_complex_to_ffi (&ffi_type_float); abort (); +#else + runtime_throw("the ABI does not support Complex64 type with " + "reflect.Call or runtime.SetFinalizer"); +#endif case GO_COMPLEX128: +#if defined (__i386__) || defined (__x86_64__) if (sizeof (double) == 8) return go_complex_to_ffi (&ffi_type_double); abort (); +#else + runtime_throw("the ABI does not support Complex128 type with " + "reflect.Call or runtime.SetFinalizer"); +#endif case GO_INT16: return &ffi_type_sint16; case GO_INT32: Index: go/testing/quick/quick_test.go =================================================================== --- go/testing/quick/quick_test.go (revision 196368) +++ go/testing/quick/quick_test.go (working copy) @@ -7,6 +7,7 @@ import ( "math/rand" "reflect" + "runtime" "testing" ) @@ -72,8 +73,10 @@ reportError("fBool", CheckEqual(fBool, fBool, nil), t) reportError("fFloat32", CheckEqual(fFloat32, fFloat32, nil), t) reportError("fFloat64", CheckEqual(fFloat64, fFloat64, nil), t) - reportError("fComplex64", CheckEqual(fComplex64, fComplex64, nil), t) - reportError("fComplex128", CheckEqual(fComplex128, fComplex128, nil), t) + if runtime.GOARCH == "386" || runtime.GOARCH == "amd64" { + reportError("fComplex64", CheckEqual(fComplex64, fComplex64, nil), t) + reportError("fComplex128", CheckEqual(fComplex128, fComplex128, nil), t) + } reportError("fInt16", CheckEqual(fInt16, fInt16, nil), t) reportError("fInt32", CheckEqual(fInt32, fInt32, nil), t) reportError("fInt64", CheckEqual(fInt64, fInt64, nil), t)
Hello! Due to the fact that libFFI does not handle C99 _Complex arguments correctly [1], libgo passes Complex64 and Complex128 arguments via a temporary structure. However, passing parts of complex number in a structure is not the same as passing true C99 _Complex value, so this workaround introduces ABI mismatch between caller and callee. This mismatch results in wrong passed values of complex types. Fortunately all x86 ABIs tolerate this mismatch, but other targets (i.e. alpha) don't have this privilege. Attached patch disables passing of C99 _Complex arguments via FFI on all targets, other than x86 (to be on the safe side w.r.t. other targets). Hopefully, someday libffi will be extended to handle _Complex arguments in correct way. Patch was tested on x86_64-pc-linux-gnu {,-m32} and alphaev68-pc-linux-gnu. [1] http://sourceware.org/ml/libffi-discuss/2007/msg00010.html Uros.