Patchwork libgo patch committed: Implement reflect.MakeFunc for 386

login
register
mail settings
Submitter Ian Taylor
Date Sept. 27, 2013, 9:34 p.m.
Message ID <mcrvc1mx822.fsf@iant-glaptop.roam.corp.google.com>
Download mbox | patch
Permalink /patch/278686/
State New
Headers show

Comments

Ian Taylor - Sept. 27, 2013, 9:34 p.m.
Following up on my earlier patch, this patch implements the
reflect.MakeFunc function for 386.

Tom Tromey pointed out to me that the libffi closure support can
probably be used for this.  I was not aware of that support.  It
supports a lot more processors, and I should probably start using it.
The approach I am using does have a couple of advantages: it's more
efficient, and it doesn't require any type of writable executable
memory.  I can get away with that because indirect calls in Go always
pass a closure value.  So even when and if I do change to using libffi,
I might still keep this code for amd64 and 386.

Anyhow, for this patch, bootstrapped and ran Go testsuite on
x86_64-unknown-linux-gnu.  Committed to mainline and 4.8 branch.

Ian
Rainer Orth - Sept. 30, 2013, 1:07 p.m.
Ian Lance Taylor <iant@google.com> writes:

> Following up on my earlier patch, this patch implements the
> reflect.MakeFunc function for 386.
>
> Tom Tromey pointed out to me that the libffi closure support can
> probably be used for this.  I was not aware of that support.  It
> supports a lot more processors, and I should probably start using it.
> The approach I am using does have a couple of advantages: it's more
> efficient, and it doesn't require any type of writable executable
> memory.  I can get away with that because indirect calls in Go always
> pass a closure value.  So even when and if I do change to using libffi,
> I might still keep this code for amd64 and 386.

Unfortunately, this patch (and undoubtedly the corresponding amd64 one)
break Solaris/x86 libgo bootstrap with native as:

Assembler: 
	"/var/tmp//cctly9hk.s", line 8 : Illegal mnemonic
	Near line: " .cfi_startproc"
	"/var/tmp//cctly9hk.s", line 8 : Syntax error
	Near line: " .cfi_startproc"
	"/var/tmp//cctly9hk.s", line 21 : Illegal mnemonic
	Near line: " .cfi_def_cfa_offset 8"
	"/var/tmp//cctly9hk.s", line 21 : Syntax error
	Near line: " .cfi_def_cfa_offset 8"
	"/var/tmp//cctly9hk.s", line 22 : Illegal mnemonic
	Near line: " .cfi_offset %ebp, -8"
	"/var/tmp//cctly9hk.s", line 22 : Syntax error
	Near line: " .cfi_offset %ebp, -8"
	"/var/tmp//cctly9hk.s", line 24 : Illegal mnemonic
	Near line: " .cfi_def_cfa_register %ebp"
	"/var/tmp//cctly9hk.s", line 24 : Syntax error
	Near line: " .cfi_def_cfa_register %ebp"
	"/var/tmp//cctly9hk.s", line 27 : Illegal mnemonic
	Near line: " .cfi_offset %ebx, -12"
	"/var/tmp//cctly9hk.s", line 27 : Syntax error
	Near line: " .cfi_offset %ebx, -12"
	"/var/tmp//cctly9hk.s", line 45 : Illegal mnemonic
	Near line: " .cfi_restore %ebx"
	"/var/tmp//cctly9hk.s", line 45 : Syntax error
	Near line: " .cfi_restore %ebx"
	"/var/tmp//cctly9hk.s", line 47 : Illegal mnemonic
	Near line: " .cfi_restore %ebp"
	"/var/tmp//cctly9hk.s", line 47 : Syntax error
	Near line: " .cfi_restore %ebp"
	"/var/tmp//cctly9hk.s", line 48 : Illegal mnemonic
	Near line: " .cfi_def_cfa %esp, 4"
	"/var/tmp//cctly9hk.s", line 48 : Syntax error
	Near line: " .cfi_def_cfa %esp, 4"
	"/var/tmp//cctly9hk.s", line 50 : Illegal mnemonic
	Near line: " .cfi_endproc"
	"/var/tmp//cctly9hk.s", line 50 : Syntax error
	Near line: " .cfi_endproc"
	"/var/tmp//cctly9hk.s", line 52 : Invalid section attribute
	"/var/tmp//cctly9hk.s", line 52 : Syntax error
	Near line: " .section .text.__x86.get_pc_thunk.bx,"axG",@progbits,__x86.get_pc_thunk.bx,comdat"
	"/var/tmp//cctly9hk.s", line 57 : Illegal mnemonic
	Near line: " .cfi_startproc"
	"/var/tmp//cctly9hk.s", line 57 : Syntax error
	Near line: " .cfi_startproc"
	"/var/tmp//cctly9hk.s", line 60 : Illegal mnemonic
	Near line: " .cfi_endproc"
	"/var/tmp//cctly9hk.s", line 60 : Syntax error
	Near line: " .cfi_endproc"
	"/var/tmp//cctly9hk.s", line 62 : Syntax error
	Near line: " .section .note.GNU-stack,"",@progbits"
	"/var/tmp//cctly9hk.s", line 63 : Syntax error
	Near line: " .section .note.GNU-split-stack,"",@progbits"
	"/var/tmp//cctly9hk.s", line 64 : Syntax error
	Near line: " .section .note.GNU-no-split-stack,"",@progbits"
make[4]: *** [reflect/makefunc.lo] Error 1

AFAICS, this is just the .cfi_* directives and empty section flags.

	Rainer
Ian Taylor - Sept. 30, 2013, 1:44 p.m.
On Mon, Sep 30, 2013 at 6:07 AM, Rainer Orth
<ro@cebitec.uni-bielefeld.de> wrote:
> Ian Lance Taylor <iant@google.com> writes:
>
>> Following up on my earlier patch, this patch implements the
>> reflect.MakeFunc function for 386.
>>
>> Tom Tromey pointed out to me that the libffi closure support can
>> probably be used for this.  I was not aware of that support.  It
>> supports a lot more processors, and I should probably start using it.
>> The approach I am using does have a couple of advantages: it's more
>> efficient, and it doesn't require any type of writable executable
>> memory.  I can get away with that because indirect calls in Go always
>> pass a closure value.  So even when and if I do change to using libffi,
>> I might still keep this code for amd64 and 386.
>
> Unfortunately, this patch (and undoubtedly the corresponding amd64 one)
> break Solaris/x86 libgo bootstrap with native as:

Unfortunately I think I'll have to somehow disable this functionality
on systems with assemblers that do not understand the .cfi directives,
as otherwise calling panic in a function created with MakeFunc will
not work.

Ian
Rainer Orth - Sept. 30, 2013, 2:07 p.m.
Ian Lance Taylor <iant@google.com> writes:

> On Mon, Sep 30, 2013 at 6:07 AM, Rainer Orth
> <ro@cebitec.uni-bielefeld.de> wrote:
>> Ian Lance Taylor <iant@google.com> writes:
>>
>>> Following up on my earlier patch, this patch implements the
>>> reflect.MakeFunc function for 386.
>>>
>>> Tom Tromey pointed out to me that the libffi closure support can
>>> probably be used for this.  I was not aware of that support.  It
>>> supports a lot more processors, and I should probably start using it.
>>> The approach I am using does have a couple of advantages: it's more
>>> efficient, and it doesn't require any type of writable executable
>>> memory.  I can get away with that because indirect calls in Go always
>>> pass a closure value.  So even when and if I do change to using libffi,
>>> I might still keep this code for amd64 and 386.
>>
>> Unfortunately, this patch (and undoubtedly the corresponding amd64 one)
>> break Solaris/x86 libgo bootstrap with native as:
>
> Unfortunately I think I'll have to somehow disable this functionality
> on systems with assemblers that do not understand the .cfi directives,
> as otherwise calling panic in a function created with MakeFunc will
> not work.

Alternatively, one could hand-craft the .eh_frame section for such
systems along the lines of libffi/src/x86/sysv.S: ugly, but doable.

	Rainer
Ian Taylor - Sept. 30, 2013, 3:44 p.m.
On Mon, Sep 30, 2013 at 7:07 AM, Rainer Orth
<ro@cebitec.uni-bielefeld.de> wrote:
> Ian Lance Taylor <iant@google.com> writes:
>
>> On Mon, Sep 30, 2013 at 6:07 AM, Rainer Orth
>> <ro@cebitec.uni-bielefeld.de> wrote:
>>> Ian Lance Taylor <iant@google.com> writes:
>>>
>>>> Following up on my earlier patch, this patch implements the
>>>> reflect.MakeFunc function for 386.
>>>>
>>>> Tom Tromey pointed out to me that the libffi closure support can
>>>> probably be used for this.  I was not aware of that support.  It
>>>> supports a lot more processors, and I should probably start using it.
>>>> The approach I am using does have a couple of advantages: it's more
>>>> efficient, and it doesn't require any type of writable executable
>>>> memory.  I can get away with that because indirect calls in Go always
>>>> pass a closure value.  So even when and if I do change to using libffi,
>>>> I might still keep this code for amd64 and 386.
>>>
>>> Unfortunately, this patch (and undoubtedly the corresponding amd64 one)
>>> break Solaris/x86 libgo bootstrap with native as:
>>
>> Unfortunately I think I'll have to somehow disable this functionality
>> on systems with assemblers that do not understand the .cfi directives,
>> as otherwise calling panic in a function created with MakeFunc will
>> not work.
>
> Alternatively, one could hand-craft the .eh_frame section for such
> systems along the lines of libffi/src/x86/sysv.S: ugly, but doable.

Yeah.  I'm not going to do that myself.  But I would be happy to
approve a patch for that if somebody else wants to write it.

Ian

Patch

diff -r 14a32fb52e24 libgo/Makefile.am
--- a/libgo/Makefile.am	Fri Sep 27 10:46:09 2013 -0700
+++ b/libgo/Makefile.am	Fri Sep 27 14:30:27 2013 -0700
@@ -901,10 +901,17 @@ 
 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 \
diff -r 14a32fb52e24 libgo/go/reflect/all_test.go
--- a/libgo/go/reflect/all_test.go	Fri Sep 27 10:46:09 2013 -0700
+++ b/libgo/go/reflect/all_test.go	Fri Sep 27 14:30:27 2013 -0700
@@ -1432,7 +1432,7 @@ 
 
 func TestMakeFunc(t *testing.T) {
 	switch runtime.GOARCH {
-	case "amd64":
+	case "amd64", "386":
 	default:
 		t.Skip("MakeFunc not implemented for " + runtime.GOARCH)
 	}
diff -r 14a32fb52e24 libgo/go/reflect/makefunc.go
--- a/libgo/go/reflect/makefunc.go	Fri Sep 27 10:46:09 2013 -0700
+++ b/libgo/go/reflect/makefunc.go	Fri Sep 27 14:30:27 2013 -0700
@@ -47,7 +47,7 @@ 
 	}
 
 	switch runtime.GOARCH {
-	case "amd64":
+	case "amd64", "386":
 	default:
 		panic("reflect.MakeFunc not implemented for " + runtime.GOARCH)
 	}
diff -r 14a32fb52e24 libgo/go/reflect/makefunc_386.S
--- /dev/null	Thu Jan 01 00:00:00 1970 +0000
+++ b/libgo/go/reflect/makefunc_386.S	Fri Sep 27 14:30:27 2013 -0700
@@ -0,0 +1,111 @@ 
+# Copyright 2013 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 386 assembly code.
+
+	.global reflect.makeFuncStub
+
+#ifdef __ELF__
+	.type reflect.makeFuncStub,@function
+#endif
+
+reflect.makeFuncStub:
+	.cfi_startproc
+
+	# Go does not provide any equivalent to the regparm function
+	# attribute, so on Go we do not need to worry about passing
+	# parameters in registers.  We just pass a pointer to the
+	# arguments on the stack.
+	#
+	# We do need to pick up the return values, though, so we pass
+	# a pointer to a struct that looks like this.
+	# struct {
+	#   esp uint32		// 0x0
+	#   eax uint32		// 0x4
+	#   st0 uint64		// 0x8
+	# }
+
+	pushl	%ebp
+	.cfi_def_cfa_offset 8
+	.cfi_offset %ebp, -8
+	movl	%esp, %ebp
+	.cfi_def_cfa_register %ebp
+
+	pushl	%ebx		# In case this is PIC.
+
+	subl	$36, %esp	# Enough for args and to align stack.
+	.cfi_offset %ebx, -12
+
+#ifdef __PIC__
+	call	__x86.get_pc_thunk.bx
+	addl	$_GLOBAL_OFFSET_TABLE_, %ebx
+#endif
+
+	leal	8(%ebp), %eax	# Set esp field in struct.
+	movl	%eax, -24(%ebp)
+
+#ifdef __PIC__
+	call	__go_get_closure@PLT
+#else
+	call	__go_get_closure
+#endif
+
+	movl	%eax, 4(%esp)
+
+	leal	-24(%ebp), %eax
+	movl	%eax, (%esp)
+
+#ifdef __PIC__
+	call	reflect.MakeFuncStubGo@PLT
+#else
+	call	reflect.MakeFuncStubGo
+#endif
+
+	# Set return registers.
+
+	movl	-20(%ebp), %eax
+	fldl	-16(%ebp)
+
+#ifdef __SSE2__
+	# In case we are compiling with -msseregparm.  This won't work
+	# correctly if only SSE1 is supported, but that seems unlikely.
+	movsd	-16(%ebp), %xmm0
+#endif
+
+	addl	$36, %esp
+	popl	%ebx
+	.cfi_restore %ebx
+	popl	%ebp
+	.cfi_restore %ebp
+	.cfi_def_cfa %esp, 4
+
+	ret
+	.cfi_endproc
+
+#ifdef __ELF__
+	.size	reflect.makeFuncStub, . - reflect.makeFuncStub
+#endif
+
+#ifdef __PIC__
+	.section	.text.__x86.get_pc_thunk.bx,"axG",@progbits,__x86.get_pc_thunk.bx,comdat
+	.globl	__x86.get_pc_thunk.bx
+	.hidden	__x86.get_pc_thunk.bx
+#ifdef __ELF__
+	.type	__x86.get_pc_thunk.bx, @function
+#endif
+__x86.get_pc_thunk.bx:
+	.cfi_startproc
+	movl	(%esp), %ebx
+	ret
+	.cfi_endproc
+#ifdef __ELF__
+	.size	__x86.get_pc_thunk.bx, . - __x86.get_pc_thunk.bx
+#endif
+#endif
+
+#ifdef __ELF__
+	.section	.note.GNU-stack,"",@progbits
+	.section	.note.GNU-split-stack,"",@progbits
+	.section	.note.GNU-no-split-stack,"",@progbits
+#endif
diff -r 14a32fb52e24 libgo/go/reflect/makefuncgo_386.go
--- /dev/null	Thu Jan 01 00:00:00 1970 +0000
+++ b/libgo/go/reflect/makefuncgo_386.go	Fri Sep 27 14:30:27 2013 -0700
@@ -0,0 +1,135 @@ 
+// Copyright 2013 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 386 implementation.
+
+package reflect
+
+import "unsafe"
+
+// The assembler stub will pass a pointer to this structure.  We
+// assume that no parameters are passed in registers--that is, we do
+// not support the -mregparm option.  On return we will set the
+// registers that might hold result values.
+type i386Regs struct {
+	esp uint32
+	eax uint32 // Value to return in %eax.
+	st0 uint64 // Value to return in %st(0).
+}
+
+// MakeFuncStubGo implements the 386 calling convention for MakeFunc.
+// This should not be called.  It is exported so that assembly code
+// can call it.
+
+func MakeFuncStubGo(regs *i386Regs, c *makeFuncImpl) {
+	ftyp := c.typ
+
+	// See if the result requires a struct.  If it does, the first
+	// parameter is a pointer to the struct.
+	retStruct := false
+	retEmpty := false
+	switch len(ftyp.out) {
+	case 0:
+		retEmpty = true
+	case 1:
+		if ftyp.out[0].size == 0 {
+			retEmpty = true
+		} else {
+			switch ftyp.out[0].Kind() {
+			case Complex64, Complex128, Array, Interface, Slice, String, Struct:
+				retStruct = true
+			}
+		}
+	default:
+		size := uintptr(0)
+		for _, typ := range ftyp.out {
+			size += typ.size
+		}
+		if size == 0 {
+			retEmpty = true
+		} else {
+			retStruct = true
+		}
+	}
+
+	in := make([]Value, 0, len(ftyp.in))
+	ap := uintptr(regs.esp)
+
+	var retPtr unsafe.Pointer
+	if retStruct {
+		retPtr = *(*unsafe.Pointer)(unsafe.Pointer(ap))
+		ap += ptrSize
+	}
+
+	for _, rt := range ftyp.in {
+		ap = align(ap, ptrSize)
+
+		// We have to copy the argument onto the heap in case
+		// the function hangs on the reflect.Value we pass it.
+		p := unsafe_New(rt)
+		memmove(p, unsafe.Pointer(ap), rt.size)
+
+		v := Value{rt, p, flag(rt.Kind()<<flagKindShift) | flagIndir}
+		in = append(in, v)
+		ap += rt.size
+	}
+
+	// Call the real function.
+
+	out := c.fn(in)
+
+	if len(out) != len(ftyp.out) {
+		panic("reflect: wrong return count from function created by MakeFunc")
+	}
+
+	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 retEmpty {
+		return
+	}
+
+	if retStruct {
+		off := uintptr(0)
+		for i, typ := range ftyp.out {
+			v := out[i]
+			off = align(off, uintptr(typ.fieldAlign))
+			addr := unsafe.Pointer(uintptr(retPtr) + off)
+			if v.flag&flagIndir == 0 && (v.kind() == Ptr || v.kind() == UnsafePointer) {
+				storeIword(addr, iword(v.val), typ.size)
+			} else {
+				memmove(addr, v.val, typ.size)
+			}
+			off += typ.size
+		}
+		regs.eax = uint32(uintptr(retPtr))
+		return
+	}
+
+	if len(ftyp.out) != 1 {
+		panic("inconsistency")
+	}
+
+	v := out[0]
+	w := v.iword()
+	if v.Kind() != Ptr && v.Kind() != UnsafePointer {
+		w = loadIword(unsafe.Pointer(w), v.typ.size)
+	}
+	switch v.Kind() {
+	case Float32, Float64:
+		regs.st0 = uint64(uintptr(w))
+	default:
+		regs.eax = uint32(uintptr(w))
+	}
+}