diff mbox

[libgo,arm64] Future of reflection

Message ID 542C6EB4.10106@redhat.com
State New
Headers show

Commit Message

Richard Henderson Oct. 1, 2014, 9:14 p.m. UTC 
I've been looking at reflection, and support for non-x86 targets.

In mainline, there's some support for using libffi, but I wasn't completely
confident that things are working correctly.  It doesn't help that the
testsuite still has conditionals like

 func TestMakeFunc(t *testing.T) {
        switch runtime.GOARCH {
        case "amd64", "386":
        default:
                t.Skip("MakeFunc not implemented for " + runtime.GOARCH)
        }

Anyway, I thought I'd see what happens if I enable use of libffi for x86 too.
Even there, just --with-libffi doesn't actually do anything, since we still
have tests like

        switch runtime.GOARCH {
        case "amd64", "386":
                // Indirect Go func value (dummy) to obtain actual
                // code address. (A Go func value is a pointer to a C
                // function pointer. http://golang.org/s/go11func.)
                dummy := makeFuncStub
                code = **(**uintptr)(unsafe.Pointer(&dummy))
        default:
                code, ffi = makeFuncFFI(ftyp, fn)
        }

So I decided to just rip out the non-libffi paths completely.  Honestly, the
result looks good from a maintenance standpoint:

	17 files changed, 31 insertions(+), 1290 deletions(-)

There's one really questionable portion in value.Pointer, where we really have
nothing useful to return.  There appears to be nothing that actually tests
this, so it's hard to tell, but I believe this to have been broken before my
patch when using libffi.  Now, at least, the randomness of the value is apparent.

One might be able to generate a similar value by Knowing Things about the
internals of libffi (and adjusting it as necessary to match our expectations).

(A thought occurs to me just now, which I haven't investigated: why don't we
compare the pointer of the receiver function, rather than the closure wrapper?)

I do question why use of libffi wasn't unconditional in the first place.  It
seems to put non-x86 as second-class citizens.  If a direct implementation is
that much better, then why don't we just write them for the other targets?

I've done this for arm64 (based on the 4.9 branch, since that's important
internally at the moment, and may be less invasive than backporting the libffi
code), and it at least passes the testsuite.

Thoughts?


r~
commit f1d42e628ed611297731b2a78bbee69d2f45f8e1
Author: Richard Henderson <rth@redhat.com>
Date:   Thu Sep 25 20:14:37 2014 -0700

    Always use libffi for libgo

	* Makefile.am (LIBFFI, LIBFFIINCS): Define directly
	(go_reflect_makefunc_file, go_reflect_makefunc_s_file): Remove.
	(reflect/makefunc.lo): Remove.
	* configure.ac (--with-libffi): Remove option.
	(LIBFFI, LIBFFIINCS): Remove subst.
	* Makefile.in, configure: Rebuild.
	* go/reflect/makefunc.go (MakeFunc): Don't check runtime.GOARCH.
	(makeValueMethod): Likewise.
	* go/reflect/makefunc_ffi_c.c: Don't check USE_LIBFFI.
	* runtime/go-ffi.c: Likewise.
	* runtime/go-ffi.h: Likewise.
	* runtime/go-reflect-call.c: Likewise.
	* go/reflect/value.go (makeFuncStubFn, makeFuncStubCode): Remove.
	(Value.call): Don't compare against makeFuncStubCode.
	(Value.Pointer): Return a dummy value for methods.
	* go/reflect/makefunc_386.S: Remove file.
	* go/reflect/makefunc_amd64.S: Remove file.
	* go/reflect/makefunc_dummy.c: Remove file.
	* go/reflect/makefuncgo_386.go: Remove file.
	* go/reflect/makefuncgo_amd64.go: Remove file.
commit daae00f8836408f1b4ce4f5e491ffc3695d86671
Author: Richard Henderson <rth@redhat.com>
Date:   Wed Oct 1 16:20:00 2014 -0400

    libgo: Add reflection for arm64

	* Makefile.am (go_reflect_makefunc_file, go_reflect_makefunc_s_file):
	Reorganize if chain; define for ARM64.
	* Makefile.in: Rebuild.
	* go/reflect/all_test.go (TestMakeFunc): Test arm64.
	(TestMakeFuncInterface, TestMethodValue, TestMethod5): Likewise.
	* go/reflect/makefunc.go (MakeFunc): Handle arm64.
	(makeMethodValue, makeValueMethod): Likewise.
	* go/reflect/makefunc_arm64.S: New file.
	* go/reflect/makefuncgo_arm64.go: New file.

diff --git a/libgo/Makefile.am b/libgo/Makefile.am
index 4f09bc3..81bb2fa 100644
--- a/libgo/Makefile.am
+++ b/libgo/Makefile.am
@@ -913,22 +913,22 @@ go_path_files = \
 	go/path/match.go \
 	go/path/path.go
 
+# Default
+go_reflect_makefunc_file :=
+go_reflect_makefunc_s_file := go/reflect/makefunc_dummy.c
+
+# Overrides; only one of these will be true
 if LIBGO_IS_X86_64
-go_reflect_makefunc_file = \
-	go/reflect/makefuncgo_amd64.go
-go_reflect_makefunc_s_file = \
-	go/reflect/makefunc_amd64.S
-else
+go_reflect_makefunc_file := go/reflect/makefuncgo_amd64.go
+go_reflect_makefunc_s_file := go/reflect/makefunc_amd64.S
+endif
 if LIBGO_IS_386
-go_reflect_makefunc_file = \
-	go/reflect/makefuncgo_386.go
-go_reflect_makefunc_s_file = \
-	go/reflect/makefunc_386.S
-else
-go_reflect_makefunc_file =
-go_reflect_makefunc_s_file = \
-	go/reflect/makefunc_dummy.c
+go_reflect_makefunc_file := go/reflect/makefuncgo_386.go
+go_reflect_makefunc_s_file := go/reflect/makefunc_386.S
 endif
+if LIBGO_IS_ARM64
+go_reflect_makefunc_file := go/reflect/makefuncgo_arm64.go
+go_reflect_makefunc_s_file := go/reflect/makefunc_arm64.S
 endif
 
 go_reflect_files = \
diff --git a/libgo/go/reflect/all_test.go b/libgo/go/reflect/all_test.go
index f9700ce..27ee1bd 100644
--- a/libgo/go/reflect/all_test.go
+++ b/libgo/go/reflect/all_test.go
@@ -1476,7 +1476,7 @@ func TestCallWithStruct(t *testing.T) {
 
 func TestMakeFunc(t *testing.T) {
 	switch runtime.GOARCH {
-	case "amd64", "386":
+	case "amd64", "386", "arm64":
 	default:
 		t.Skip("MakeFunc not implemented for " + runtime.GOARCH)
 	}
@@ -1500,7 +1500,7 @@ func TestMakeFunc(t *testing.T) {
 
 func TestMakeFuncInterface(t *testing.T) {
 	switch runtime.GOARCH {
-	case "amd64", "386":
+	case "amd64", "386", "arm64":
 	default:
 		t.Skip("MakeFunc not implemented for " + runtime.GOARCH)
 	}
@@ -1633,7 +1633,7 @@ func TestMethod(t *testing.T) {
 
 func TestMethodValue(t *testing.T) {
 	switch runtime.GOARCH {
-	case "amd64", "386":
+	case "amd64", "386", "arm64":
 	default:
 		t.Skip("reflect method values not implemented for " + runtime.GOARCH)
 	}
@@ -1810,7 +1810,7 @@ func (t4 Tm4) M(x int, b byte) (byte, int) { return b, x + 40 }
 
 func TestMethod5(t *testing.T) {
 	switch runtime.GOARCH {
-	case "amd64", "386":
+	case "amd64", "386", "arm64":
 	default:
 		t.Skip("reflect method values not implemented for " + runtime.GOARCH)
 	}
diff --git a/libgo/go/reflect/makefunc.go b/libgo/go/reflect/makefunc.go
index 935a3d3..9c9e7ce 100644
--- a/libgo/go/reflect/makefunc.go
+++ b/libgo/go/reflect/makefunc.go
@@ -52,7 +52,7 @@ func MakeFunc(typ Type, fn func(args []Value) (results []Value)) Value {
 	}
 
 	switch runtime.GOARCH {
-	case "amd64", "386":
+	case "amd64", "386", "arm64":
 	default:
 		panic("reflect.MakeFunc not implemented for " + runtime.GOARCH)
 	}
@@ -91,7 +91,7 @@ func makeMethodValue(op string, v Value) Value {
 	}
 
 	switch runtime.GOARCH {
-	case "amd64", "386":
+	case "amd64", "386", "arm64":
 	default:
 		panic("reflect.makeMethodValue not implemented for " + runtime.GOARCH)
 	}
@@ -138,7 +138,7 @@ func makeValueMethod(v Value) Value {
 	}
 
 	switch runtime.GOARCH {
-	case "amd64", "386":
+	case "amd64", "386", "arm64":
 	default:
 		panic("reflect.makeValueMethod not implemented for " + runtime.GOARCH)
 	}
diff --git a/libgo/go/reflect/makefunc_arm64.S b/libgo/go/reflect/makefunc_arm64.S
new file mode 100644
index 0000000..fdb2a58
--- /dev/null
+++ b/libgo/go/reflect/makefunc_arm64.S
@@ -0,0 +1,72 @@
+# Copyright 2014 The Go Authors. All rights reserved.
+# Use of this source code is governed by a BSD-style
+# license that can be found in the LICENSE file.
+
+# MakeFunc arm64 assembly code.
+
+#include "config.h"
+
+	.global	reflect.makeFuncStub
+	.type	reflect.makeFuncStub,%function
+	.cfi_sections .eh_frame, .debug_frame
+
+reflect.makeFuncStub:
+	.cfi_startproc
+
+	# Store all the parameter registers in a struct that looks
+	# like:
+	# struct {
+	#   reg [8]uint64	// 0
+	#   freg [8]uint64	// 64
+	#   sp uint64		// 128 Pointer to arguments on stack.
+	# };
+
+	stp	x29, x30, [sp, #-160]!
+	.cfi_def_cfa_offset 160
+	.cfi_rel_offset x29, 0
+	.cfi_rel_offset x30, 8
+
+	mov	x29, sp
+	stp	x0, x1, [sp, 16*1]
+	stp	x2, x3, [sp, 16*2]
+	add	x1, sp, #160
+	stp	x4, x5, [sp, 16*3]
+	stp	x6, x7, [sp, 16*4]
+	stp	d0, d1, [sp, 16*5]
+	stp	d2, d3, [sp, 16*6]
+	stp	d4, d5, [sp, 16*7]
+	stp	d6, d7, [sp, 16*8]
+	stp	x8, x1, [sp, 16*9]
+
+	/* For MakeFunc functions that call recover.  */
+	mov	x0, x30
+	bl	__go_makefunc_can_recover
+
+	# Get function type.
+	bl	__go_get_closure
+
+	mov	x1, x0
+	add	x0, sp, #16
+	bl	reflect.MakeFuncStubGo
+
+	/* MakeFunc functions can no longer call recover.  */
+	bl	__go_makefunc_returning
+
+	# The structure will be updated with any return values.  Load
+	# all possible return registers before returning to the caller.
+	ldp	x0, x1, [sp, 16*1]
+	ldp	d0, d1, [sp, 16*5]
+	ldp	d2, d3, [sp, 16*6]
+
+	ldp	x29, x30, [sp], #160
+	.cfi_def_cfa_offset 0
+	.cfi_restore x29
+	.cfi_restore x30
+	ret
+
+	.cfi_endproc
+	.size	reflect.makeFuncStub, . - reflect.makeFuncStub
+
+	.section	.note.GNU-stack,"",@progbits
+	.section	.note.GNU-split-stack,"",@progbits
+	.section	.note.GNU-no-split-stack,"",@progbits
diff --git a/libgo/go/reflect/makefuncgo_arm64.go b/libgo/go/reflect/makefuncgo_arm64.go
new file mode 100644
index 0000000..1426d2c
--- /dev/null
+++ b/libgo/go/reflect/makefuncgo_arm64.go
@@ -0,0 +1,338 @@
+// Copyright 2014 The Go Authors.  All rights reserved.
+// Use of this source code is governed by a BSD-style
+// license that can be found in the LICENSE file.
+
+// MakeFunc arm64 implementation.
+
+package reflect
+
+import "unsafe"
+
+// The assembler stub will pass a pointer to this structure.
+// This will come in holding all the registers that might hold
+// function parameters.  On return we will set the registers that
+// might hold result values.
+type arm64Regs struct {
+	reg  [8]uint64
+	freg [8]uint64
+	x8   uint64
+	sp   uint64
+}
+
+// MakeFuncStubGo implements the arm64 calling convention for
+// MakeFunc.  This should not be called.  It is exported so that
+// assembly code can call it.
+
+func MakeFuncStubGo(regs *arm64Regs, c *makeFuncImpl) {
+	ftyp := c.typ
+	in := make([]Value, 0, len(ftyp.in))
+	var ngrn uintptr = 0
+	var nsrn uintptr = 0
+	nsaa := uintptr(regs.sp)
+
+	for _, rt := range ftyp.in {
+		var kind Kind = rt.Kind()
+		var nreg uintptr = 1
+
+		fl := flag(kind) << flagKindShift
+		size := rt.size
+		indirect := false
+
+		switch kind {
+		case Bool, Int, Int8, Int16, Int32, Int64,
+		     Uint, Uint8, Uint16, Uint32, Uint64,
+		     Uintptr, Chan, Func, Map, Ptr, UnsafePointer,
+		     Float32, Float64:
+			// Scalar types: variables already set
+
+		default:
+			// Composite types: check for HFAs
+			kind, nreg = arm64ClassifyComposite(rt)
+			// B.3
+			if kind == Struct && size > 16 {
+				indirect = true
+				nreg = 1
+			}
+		}
+
+		// We have to copy the argument onto the heap in case the
+		// function hangs onto the reflect.Value we pass it.
+		var src, dst unsafe.Pointer
+
+		switch kind {
+		case Float32:
+			// C.1
+			if nreg == 1 && nsrn < 8 {
+				dst = makeIword(regs.freg[nsrn], 4)
+				nsrn += 1
+				goto finish_value
+			}
+			// C.2
+			if nsrn + nreg <= 8 {
+				dst = unsafe_New(rt)
+				fl = fl | flagIndir
+				for i := uintptr(0); i < nreg; i++ {
+					d := uintptr(dst) + i*4
+					*(*uint32)(unsafe.Pointer(d)) =
+						uint32(regs.freg[nsrn])
+					nsrn++
+				}
+				goto finish_value
+			}
+			// C.3
+			nsrn = 8
+			// C.5
+			nreg = (nreg + 1) / 2
+			// C.6: on stack
+
+		case Float64:
+			// C.1
+			if nreg == 1 && nsrn < 8 && ptrSize == 8 {
+				dst = makeIword(regs.freg[nsrn], 8)
+				nsrn += 1
+				goto finish_value
+			}
+			// C.1 and C.2
+			if nsrn + nreg <= 8 {
+				src = unsafe.Pointer(&regs.freg[nsrn])
+				nsrn += nreg
+				goto finish_copy
+			}
+			// C.3
+			nsrn = 8
+			// C.6: on stack
+
+		default:
+			// C.8
+			if rt.align == 16 && !indirect {
+				ngrn = align(ngrn, 2)
+			}
+			// C.7
+			if nreg == 1 && ngrn < 8 && size <= ptrSize {
+				dst = makeIword(regs.reg[ngrn], size)
+				ngrn += 1
+				goto finish_value
+			}
+			// C.7 and C.10
+			if ngrn + nreg <= 8 {
+				if indirect {
+					src = unsafe.Pointer(uintptr(regs.reg[ngrn]))
+				} else {
+					// Given makeIword case above has
+					// handled all arguments < 64 bits,
+					// this is endianness safe.
+					src = unsafe.Pointer(&regs.reg[ngrn])
+				}
+				ngrn += nreg
+				goto finish_copy
+			}
+			// C.11
+			ngrn = 8
+			// C.12
+			nsaa = align(nsaa, 8)
+			nsaa = align(nsaa, uintptr(rt.align))
+			// C.13: on stack
+		}
+		src = unsafe.Pointer(nsaa)
+		nsaa += nreg * 8
+		switch {
+		case indirect:
+			src = *(*unsafe.Pointer)(src)
+		case size <= ptrSize:
+			dst = makeIword(*(*uint64)(src), size)
+			goto finish_value
+		}
+	finish_copy:
+		dst = unsafe_New(rt)
+		memmove(dst, src, size)
+		fl = fl | flagIndir
+	finish_value:
+		v := Value{ rt, dst, fl }
+		in = append(in, v)
+	}
+
+	// All the real arguments have been found and turned into Value's.
+	// Call the real function.
+	out := c.call(in)
+	nret := len(out)
+
+	if nret != len(ftyp.out) {
+		panic("reflect: wrong return count from function created by MakeFunc")
+	}
+	if nret == 0 {
+		return;
+	}
+
+	for i, typ := range ftyp.out {
+		v := out[i]
+		if v.typ != typ {
+			panic("reflect: function created by MakeFunc using " + funcName(c.fn) +
+				" returned wrong type: have " +
+				out[i].typ.String() + " for " + typ.String())
+		}
+		if v.flag&flagRO != 0 {
+			panic("reflect: function created by MakeFunc using " + funcName(c.fn) +
+				" returned value obtained from unexported field")
+		}
+	}
+
+	if nret == 1 {
+		kind, nreg := arm64ClassifyComposite(ftyp.out[0])
+		v := out[0]
+		p := uintptr(v.val);
+		if v.flag&flagIndir == 0 {
+			p = uintptr(unsafe.Pointer(&v.val))
+		}
+		switch kind {
+		case Float32:
+			for j := uintptr(0); j < nreg; j++ {
+				q := unsafe.Pointer(p + j*4)
+				regs.freg[j] = uint64(*(*uint32)(q))
+			}
+		case Float64:
+			for j := uintptr(0); j < nreg; j++ {
+				q := unsafe.Pointer(p + j*8)
+				regs.freg[j] = *(*uint64)(q)
+			}
+		case Bool:
+			if v.Bool() {
+				regs.reg[0] = 1
+			} else {
+				regs.reg[0] = 0
+			}
+		case Int, Int8, Int16, Int32, Int64:
+			regs.reg[0] = uint64(v.Int())
+		case Uint, Uint8, Uint16, Uint32, Uint64, Uintptr:
+			regs.reg[0] = v.Uint()
+		case Chan, Func, Map, Ptr, UnsafePointer:
+			regs.reg[0] = uint64(v.Pointer())
+		default:
+			size := v.typ.size
+			nsaa = uintptr(regs.x8)
+			if size <= 16 {
+				nsaa = uintptr(unsafe.Pointer(&regs.reg[0]))
+			}
+			memmove(unsafe.Pointer(nsaa), unsafe.Pointer(p), size)
+		}
+	} else {
+		// Multiple return types imply an unnamed structure.
+		// Lay out that struct.
+		var kind Kind = Invalid
+		var size uintptr = 0
+		for _, typ := range ftyp.out {
+			iter := arm64HomogeneousType(typ)
+			if kind == Invalid {
+				kind = iter
+			} else if kind != iter {
+				kind = Struct
+			}
+			size = align(size, uintptr(typ.align))
+			size += typ.size
+		}
+		kind, _ = arm64ClassifyCompositeInner(kind, size)
+
+		switch kind {
+		case Float32:
+			nsrn = 0
+			for i, typ := range ftyp.out {
+				v := out[i]
+				p := uintptr(v.val);
+				if v.flag&flagIndir == 0 {
+					p = uintptr(unsafe.Pointer(&v.val))
+				}
+				for j := uintptr(0); j < typ.size; j += 4 {
+					q := unsafe.Pointer(p + j)
+					regs.freg[nsrn] = uint64(*(*uint32)(q))
+					nsrn += 1
+				}
+			}
+
+		case Float64:
+			for i, typ := range ftyp.out {
+				v := out[i]
+				p := uintptr(v.val);
+				if v.flag&flagIndir == 0 {
+					p = uintptr(unsafe.Pointer(&v.val))
+				}
+				for j := uintptr(0); j < typ.size; j += 8 {
+					q := unsafe.Pointer(p + j)
+					regs.freg[nsrn] = *(*uint64)(q)
+					nsrn += 1
+				}
+			}
+
+		default:
+			nsaa = uintptr(regs.x8)
+			if size <= 16 {
+				nsaa = uintptr(unsafe.Pointer(&regs.reg[0]))
+			}
+			var off uintptr = 0
+			for i, typ := range ftyp.out {
+				s := typ.size
+				v := out[i]
+				p := v.val;
+				if v.flag&flagIndir == 0 {
+					p = unsafe.Pointer(&v.val)
+				}
+				off = align(off, uintptr(typ.align))
+				memmove(unsafe.Pointer(nsaa + off), p, s)
+				off += s
+			}
+		}
+	}
+}
+
+func makeIword(reg uint64, size uintptr) unsafe.Pointer {
+	ret := uintptr(reg)
+	if bigEndian && size < ptrSize {
+		ret = ret << ((ptrSize - size) * 8)
+	}
+	return unsafe.Pointer(ret)
+}
+
+func arm64HomogeneousType(typ *rtype) Kind {
+	k := typ.Kind()
+	switch k {
+	case Struct:
+		styp := (*structType)(unsafe.Pointer(typ))
+		var candidate Kind = Invalid
+
+		for _, field := range styp.fields {
+			iter := arm64HomogeneousType(field.typ)
+			if candidate == Invalid {
+				candidate = iter
+			} else if candidate != iter {
+				return Struct
+			}
+		}
+		if candidate != Invalid {
+			return candidate
+		}
+	case Array:
+		atyp := (*arrayType)(unsafe.Pointer(typ))
+		return atyp.rtype.Kind()
+	case Complex64:
+		return Float32
+	case Complex128:
+		return Float64
+	}
+	return k
+}
+
+func arm64ClassifyCompositeInner(htyp Kind, size uintptr) (Kind, uintptr) {
+	switch htyp {
+	case Float32:
+		if size >= 4 && size <= 4*4 {
+			return Float32, size / 4
+		}
+	case Float64:
+		if size >= 8 && size <= 8*4 {
+			return Float64, size / 8
+		}
+	}
+	return Struct, (size + 7) / 8
+}
+
+func arm64ClassifyComposite(typ *rtype) (Kind, uintptr) {
+	return arm64ClassifyCompositeInner(arm64HomogeneousType(typ), typ.size)
+}

Comments

Ian Lance Taylor Oct. 1, 2014, 10:08 p.m. UTC | #1 
On Wed, Oct 1, 2014 at 2:14 PM, Richard Henderson <rth@redhat.com> wrote:
>
> I've been looking at reflection, and support for non-x86 targets.
>
> In mainline, there's some support for using libffi, but I wasn't completely
> confident that things are working correctly.  It doesn't help that the
> testsuite still has conditionals like
>
>  func TestMakeFunc(t *testing.T) {
>         switch runtime.GOARCH {
>         case "amd64", "386":
>         default:
>                 t.Skip("MakeFunc not implemented for " + runtime.GOARCH)
>         }

Wait, what sources are you looking at?  I took that out on July 19.


> So I decided to just rip out the non-libffi paths completely.  Honestly, the
> result looks good from a maintenance standpoint:
>
>         17 files changed, 31 insertions(+), 1290 deletions(-)
>
> There's one really questionable portion in value.Pointer, where we really have
> nothing useful to return.  There appears to be nothing that actually tests
> this, so it's hard to tell, but I believe this to have been broken before my
> patch when using libffi.  Now, at least, the randomness of the value is apparent.

I think that value.Pointer is vaguely meaningful when applied to an
ordinary func value.  I agree that it is useless when applied to a
function created by MakeFunc.  I don't think there is any useful way
to use value.Pointer for a function value in any case.


> I do question why use of libffi wasn't unconditional in the first place.  It
> seems to put non-x86 as second-class citizens.  If a direct implementation is
> that much better, then why don't we just write them for the other targets?

Partly historical reasons.  I wrote the x86 support because I didn't
know that libffi had closures.  But then even using closures, the x86
support looks a lot nicer: it doesn't have to call mmap.  All the mmap
stuff is elegant and is necessary for C style function pointers, but
it's not necessary for Go func values since they carry their own
closures anyhow.  So, yes, I think we should write direct
implementations for other targets that we care about.

Ian
Richard Henderson Oct. 2, 2014, 3:45 p.m. UTC | #2 
On 10/01/2014 03:08 PM, Ian Lance Taylor wrote:
>>  func TestMakeFunc(t *testing.T) {
>>         switch runtime.GOARCH {
>>         case "amd64", "386":
>>         default:
>>                 t.Skip("MakeFunc not implemented for " + runtime.GOARCH)
>>         }
> 
> Wait, what sources are you looking at?  I took that out on July 19.

Err.. too many different ones, apparently.  Sorry for the confusion.

> Partly historical reasons.  I wrote the x86 support because I didn't
> know that libffi had closures.  But then even using closures, the x86
> support looks a lot nicer: it doesn't have to call mmap.  All the mmap
> stuff is elegant and is necessary for C style function pointers, but
> it's not necessary for Go func values since they carry their own
> closures anyhow.  So, yes, I think we should write direct
> implementations for other targets that we care about.

Ok.  I'd forgotten about the extra mmap overhead implied by libffi closures.


r~
diff mbox

Patch

diff --git a/libgo/Makefile.am b/libgo/Makefile.am
index 6d00bcc..06ce555 100644
--- a/libgo/Makefile.am
+++ b/libgo/Makefile.am
@@ -27,8 +27,8 @@  toolexecdir = $(glibgo_toolexecdir)
 toolexeclibdir = $(glibgo_toolexeclibdir)
 toolexeclibgodir = $(nover_glibgo_toolexeclibdir)/go/$(gcc_version)/$(target_alias)
 
-LIBFFI = @LIBFFI@
-LIBFFIINCS = @LIBFFIINCS@
+LIBFFI = ../libffi/libffi_convenience.la
+LIBFFIINCS = -I$(top_srcdir)/../libffi/include -I../libffi/include
 
 LIBATOMIC = @LIBATOMIC@
 
@@ -931,29 +931,10 @@  go_path_files = \
 	go/path/match.go \
 	go/path/path.go
 
-if LIBGO_IS_X86_64
-go_reflect_makefunc_file = \
-	go/reflect/makefuncgo_amd64.go
-go_reflect_makefunc_s_file = \
-	go/reflect/makefunc_amd64.S
-else
-if LIBGO_IS_386
-go_reflect_makefunc_file = \
-	go/reflect/makefuncgo_386.go
-go_reflect_makefunc_s_file = \
-	go/reflect/makefunc_386.S
-else
-go_reflect_makefunc_file =
-go_reflect_makefunc_s_file = \
-	go/reflect/makefunc_dummy.c
-endif
-endif
-
 go_reflect_files = \
 	go/reflect/deepequal.go \
 	go/reflect/makefunc.go \
 	go/reflect/makefunc_ffi.go \
-	$(go_reflect_makefunc_file) \
 	go/reflect/type.go \
 	go/reflect/value.go
 go_reflect_makefunc_c_file = \
@@ -1852,7 +1833,6 @@  libgo_go_objs = \
 	os.lo \
 	path.lo \
 	reflect-go.lo \
-	reflect/makefunc.lo \
 	reflect/makefunc_ffi_c.lo \
 	regexp.lo \
 	runtime-go.lo \
@@ -2254,9 +2234,6 @@  reflect-go.lo: $(go_reflect_files)
 	$(BUILDPACKAGE)
 reflect/check: $(CHECK_DEPS)
 	@$(CHECK)
-reflect/makefunc.lo: $(go_reflect_makefunc_s_file)
-	@$(MKDIR_P) reflect
-	$(LTCOMPILE) -c -o $@ $<
 reflect/makefunc_ffi_c.lo: $(go_reflect_makefunc_c_file)
 	@$(MKDIR_P) reflect
 	$(LTCOMPILE) -c -o $@ $<
diff --git a/libgo/config.h.in b/libgo/config.h.in
index 9e622c6..e243e11 100644
--- a/libgo/config.h.in
+++ b/libgo/config.h.in
@@ -374,9 +374,6 @@ 
 /* Define to 1 if you have the ANSI C header files. */
 #undef STDC_HEADERS
 
-/* Define if we're to use libffi. */
-#undef USE_LIBFFI
-
 /* Define if the compiler supports -fsplit-stack */
 #undef USING_SPLIT_STACK
 
diff --git a/libgo/configure.ac b/libgo/configure.ac
index 5e1e4d9..568188d 100644
--- a/libgo/configure.ac
+++ b/libgo/configure.ac
@@ -103,25 +103,6 @@  AC_SUBST(glibgo_toolexecdir)
 AC_SUBST(glibgo_toolexeclibdir)
 AC_SUBST(nover_glibgo_toolexeclibdir)
 
-# See if the user wants to configure without libffi.  Some
-# architectures don't support it.  FIXME: We should set a default
-# based on the host.
-AC_ARG_WITH(libffi,
-  AS_HELP_STRING([--without-libffi],
-                 [don't use libffi]),
-  [:],
-  [with_libffi=${with_libffi_default-yes}])
-
-LIBFFI=
-LIBFFIINCS=
-if test "$with_libffi" != no; then
-   AC_DEFINE(USE_LIBFFI, 1, [Define if we're to use libffi.])
-   LIBFFI=../libffi/libffi_convenience.la
-   LIBFFIINCS='-I$(top_srcdir)/../libffi/include -I../libffi/include'
-fi
-AC_SUBST(LIBFFI)
-AC_SUBST(LIBFFIINCS)
-
 # See if the user wants to configure without libatomic. This is useful if we are
 # on an architecture for which libgo does not need an atomic support library and
 # libatomic does not support our C compiler.
diff --git a/libgo/go/reflect/makefunc.go b/libgo/go/reflect/makefunc.go
index 736ac36..6549f33 100644
--- a/libgo/go/reflect/makefunc.go
+++ b/libgo/go/reflect/makefunc.go
@@ -7,7 +7,6 @@ 
 package reflect
 
 import (
-	"runtime"
 	"unsafe"
 )
 
@@ -58,18 +57,7 @@  func MakeFunc(typ Type, fn func(args []Value) (results []Value)) Value {
 	t := typ.common()
 	ftyp := (*funcType)(unsafe.Pointer(t))
 
-	var code uintptr
-	var ffi *ffiData
-	switch runtime.GOARCH {
-	case "amd64", "386":
-		// Indirect Go func value (dummy) to obtain actual
-		// code address. (A Go func value is a pointer to a C
-		// function pointer. http://golang.org/s/go11func.)
-		dummy := makeFuncStub
-		code = **(**uintptr)(unsafe.Pointer(&dummy))
-	default:
-		code, ffi = makeFuncFFI(ftyp, fn)
-	}
+	code, ffi := makeFuncFFI(ftyp, fn)
 
 	impl := &makeFuncImpl{
 		code:   code,
@@ -117,22 +105,11 @@  func makeMethodValue(op string, v Value) Value {
 	ftyp := (*funcType)(unsafe.Pointer(t))
 	method := int(v.flag) >> flagMethodShift
 
-	var code uintptr
-	var ffi *ffiData
-	switch runtime.GOARCH {
-	case "amd64", "386":
-		// Indirect Go func value (dummy) to obtain actual
-		// code address. (A Go func value is a pointer to a C
-		// function pointer. http://golang.org/s/go11func.)
-		dummy := makeFuncStub
-		code = **(**uintptr)(unsafe.Pointer(&dummy))
-	default:
-		code, ffi = makeFuncFFI(ftyp,
-			func(in []Value) []Value {
-				m := rcvr.Method(method)
-				return m.Call(in)
-			})
-	}
+	code, ffi := makeFuncFFI(ftyp,
+		func(in []Value) []Value {
+			m := rcvr.Method(method)
+			return m.Call(in)
+		})
 
 	fv := &makeFuncImpl{
 		code:   code,
@@ -160,21 +137,10 @@  func makeValueMethod(v Value) Value {
 	t := typ.common()
 	ftyp := (*funcType)(unsafe.Pointer(t))
 
-	var code uintptr
-	var ffi *ffiData
-	switch runtime.GOARCH {
-	case "amd64", "386":
-		// Indirect Go func value (dummy) to obtain actual
-		// code address. (A Go func value is a pointer to a C
-		// function pointer. http://golang.org/s/go11func.)
-		dummy := makeFuncStub
-		code = **(**uintptr)(unsafe.Pointer(&dummy))
-	default:
-		code, ffi = makeFuncFFI(ftyp,
-			func(in []Value) []Value {
-				return v.Call(in)
-			})
-	}
+	code, ffi := makeFuncFFI(ftyp,
+		func(in []Value) []Value {
+			return v.Call(in)
+		})
 
 	impl := &makeFuncImpl{
 		code:   code,
diff --git a/libgo/go/reflect/makefunc_ffi_c.c b/libgo/go/reflect/makefunc_ffi_c.c
index fba269d..d05f82b 100644
--- a/libgo/go/reflect/makefunc_ffi_c.c
+++ b/libgo/go/reflect/makefunc_ffi_c.c
@@ -5,17 +5,8 @@ 
 #include "runtime.h"
 #include "go-type.h"
 #include "go-panic.h"
-
-#ifdef USE_LIBFFI
-
 #include "go-ffi.h"
 
-#if FFI_CLOSURES
-#define USE_LIBFFI_CLOSURES
-#endif
-
-#endif /* defined(USE_LIBFFI) */
-
 /* Declare C functions with the names used to call from Go.  */
 
 struct ffi_ret {
@@ -30,8 +21,6 @@  struct ffi_ret ffi(const struct __go_func_type *ftyp, FuncVal *callback)
 void ffiFree(void *data)
   __asm__ (GOSYM_PREFIX "reflect.ffiFree");
 
-#ifdef USE_LIBFFI_CLOSURES
-
 /* The function that we pass to ffi_prep_closure_loc.  This calls the
    Go callback function (passed in user_data) with the pointer to the
    arguments and the results area.  */
@@ -116,20 +105,3 @@  ffiFree (void *data)
 {
   ffi_closure_free (data);
 }
-
-#else /* !defined(USE_LIBFFI_CLOSURES) */
-
-struct ffi_ret
-ffi(const struct __go_func_type *ftyp, FuncVal *callback)
-{
-  runtime_panicstring ("libgo built without FFI does not support "
-		       "reflect.MakeFunc");
-}
-
-void ffiFree(void *data)
-{
-  runtime_panicstring ("libgo built without FFI does not support "
-		       "reflect.MakeFunc");
-}
-
-#endif
diff --git a/libgo/go/reflect/value.go b/libgo/go/reflect/value.go
index c390b8e..0a4c5e1 100644
--- a/libgo/go/reflect/value.go
+++ b/libgo/go/reflect/value.go
@@ -427,9 +427,6 @@  func (v Value) CallSlice(in []Value) []Value {
 
 var callGC bool // for testing; see TestCallMethodJump
 
-var makeFuncStubFn = makeFuncStub
-var makeFuncStubCode = **(**uintptr)(unsafe.Pointer(&makeFuncStubFn))
-
 func (v Value) call(op string, in []Value) []Value {
 	// Get function pointer, type.
 	t := v.typ
@@ -507,17 +504,6 @@  func (v Value) call(op string, in []Value) []Value {
 	}
 	nout := t.NumOut()
 
-	// If target is makeFuncStub, short circuit the unpack onto stack /
-	// pack back into []Value for the args and return values.  Just do the
-	// call directly.
-	// We need to do this here because otherwise we have a situation where
-	// reflect.callXX calls makeFuncStub, neither of which knows the
-	// layout of the args.  That's bad for precise gc & stack copying.
-	x := (*makeFuncImpl)(fn)
-	if x.code == makeFuncStubCode {
-		return x.call(in)
-	}
-
 	if v.flag&flagMethod != 0 {
 		nin++
 	}
@@ -1313,12 +1299,14 @@  func (v Value) Pointer() uintptr {
 		if v.flag&flagMethod != 0 {
 			// As the doc comment says, the returned pointer is an
 			// underlying code pointer but not necessarily enough to
-			// identify a single function uniquely. All method expressions
-			// created via reflect have the same underlying code pointer,
-			// so their Pointers are equal. The function used here must
-			// match the one used in makeMethodValue.
-			f := makeFuncStub
-			return **(**uintptr)(unsafe.Pointer(&f))
+			// identify a single function uniquely.
+			// ??? Prior to the use of libffi, all method expressions
+			// created via reflect had the same underlying code pointer,
+			// so their Pointers were equal.  Since we now create ffi
+			// thunks, we have no way to remember the thunk value for
+			// any method, so we have no way at all to generate pointer
+			// equality.  The best we can do is some non-zero quantity.
+			return 0xdeadbeef
 		}
 		p := v.pointer()
 		// Non-nil func value points at data block.
diff --git a/libgo/runtime/go-ffi.c b/libgo/runtime/go-ffi.c
index 21879b9..42f8134 100644
--- a/libgo/runtime/go-ffi.c
+++ b/libgo/runtime/go-ffi.c
@@ -12,9 +12,6 @@ 
 #include "go-alloc.h"
 #include "go-assert.h"
 #include "go-type.h"
-
-#ifdef USE_LIBFFI
-
 #include "ffi.h"
 
 /* The functions in this file are only called from reflect_call and
@@ -334,5 +331,3 @@  __go_func_to_cif (const struct __go_func_type *func, _Bool is_interface,
   status = ffi_prep_cif (cif, FFI_DEFAULT_ABI, num_args, rettype, args);
   __go_assert (status == FFI_OK);
 }
-
-#endif /* defined(USE_LIBFFI) */
diff --git a/libgo/runtime/go-ffi.h b/libgo/runtime/go-ffi.h
index afae4b6..1b4f523 100644
--- a/libgo/runtime/go-ffi.h
+++ b/libgo/runtime/go-ffi.h
@@ -6,11 +6,6 @@ 
 
 #include "config.h"
 #include "go-type.h"
-
-#ifdef USE_LIBFFI
-
 #include "ffi.h"
 
 void __go_func_to_cif (const struct __go_func_type *, _Bool, _Bool, ffi_cif *);
-
-#endif
diff --git a/libgo/runtime/go-reflect-call.c b/libgo/runtime/go-reflect-call.c
index dfc703e..8018183 100644
--- a/libgo/runtime/go-reflect-call.c
+++ b/libgo/runtime/go-reflect-call.c
@@ -12,9 +12,6 @@ 
 #include "go-alloc.h"
 #include "go-assert.h"
 #include "go-type.h"
-
-#ifdef USE_LIBFFI
-
 #include "go-ffi.h"
 
 /* The functions in this file are only called from reflect_call.  As
@@ -232,20 +229,3 @@  reflect_call (const struct __go_func_type *func_type, FuncVal *func_val,
 
   free (call_result);
 }
-
-#else /* !defined(USE_LIBFFI) */
-
-void
-reflect_call (const struct __go_func_type *func_type __attribute__ ((unused)),
-	      FuncVal *func_val __attribute__ ((unused)),
-	      _Bool is_interface __attribute__ ((unused)),
-	      _Bool is_method __attribute__ ((unused)),
-	      void **params __attribute__ ((unused)),
-	      void **results __attribute__ ((unused)))
-{
-  /* Without FFI there is nothing we can do.  */
-  runtime_throw("libgo built without FFI does not support "
-		"reflect.Call or runtime.SetFinalizer");
-}
-
-#endif /* !defined(USE_LIBFFI) */