Patchwork [go] Passing Complex64 and Complex128 values via reflect.Call (using libffi) introduces ABI mismatch

login
register
mail settings
Submitter Uros Bizjak
Date March 1, 2013, 12:57 p.m.
Message ID <CAFULd4arpzXGiSpEYz1uToPJ94homCko+gsf-zbXtweVNaiHpA@mail.gmail.com>
Download mbox | patch
Permalink /patch/224330/
State New
Headers show

Comments

Uros Bizjak - March 1, 2013, 12:57 p.m.
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.
Ian Taylor - March 1, 2013, 2:43 p.m.
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
Uros Bizjak - March 1, 2013, 2:50 p.m.
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.
Uros Bizjak - March 1, 2013, 2:53 p.m.
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.
Uros Bizjak - March 1, 2013, 5:34 p.m.
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.
Ian Taylor - March 1, 2013, 6:26 p.m.
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

Patch

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)