runtime: fix sub-uintptr-sized Windows callback arguments

The Windows callback support accepts Go functions with arguments that
are uintptr-sized or smaller. However, it doesn't implement smaller
arguments correctly. It assumes the Windows arguments layout is
equivalent to the Go argument layout. This is often true, but because
Windows C ABIs pad arguments to word size, while Go packs arguments,
the layout is different if there are multiple sub-word-size arguments
in a row. For example, a function with two uint16 arguments will have
a two-word C argument frame, but only a 4 byte Go argument frame.
There are also subtleties surrounding floating-point register
arguments that it doesn't handle correctly.

To fix this, when constructing a callback, we examine the Go
function's signature to construct a mapping between the C argument
frame and the Go argument frame. When the callback is invoked, we use
this mapping to build the Go argument frame and copy the result back.

This adds several test cases to TestStdcallAndCDeclCallbacks that
exercise more complex function signatures. These all fail with the
current code, but work with this CL.

In addition to fixing these callback types, this is also a step toward
the Go register ABI (#40724), which is going to make the ABI
translation more complex.

Change-Id: I19fb1681b659d9fd528ffd5e88912bebb95da052
Reviewed-on: https://go-review.googlesource.com/c/go/+/263271
Trust: Austin Clements <austin@google.com>
Trust: Alex Brainman <alex.brainman@gmail.com>
Run-TryBot: Austin Clements <austin@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Michael Knyszek <mknyszek@google.com>
Reviewed-by: Alex Brainman <alex.brainman@gmail.com>

src/runtime/syscall_windows.go

@@ -20,8 +20,31 @@ var cbs struct {
 // winCallback records information about a registered Go callback.
 type winCallback struct {
 	fn     *funcval // Go function
-	argsize uintptr  // Callback arguments size (in bytes)
+	retPop uintptr  // For 386 cdecl, how many bytes to pop on return
-	cdecl   bool     // C function uses cdecl calling convention
+
+	// abiMap specifies how to translate from a C frame to a Go
+	// frame. This does not specify how to translate back because
+	// the result is always a uintptr. If the C ABI is fastcall,
+	// this assumes the four fastcall registers were first spilled
+	// to the shadow space.
+	abiMap []abiPart
+	// retOffset is the offset of the uintptr-sized result in the Go
+	// frame.
+	retOffset uintptr
+}
+
+// abiPart encodes a step in translating between calling ABIs.
+type abiPart struct {
+	src, dst uintptr
+	len      uintptr
+}
+
+func (a *abiPart) tryMerge(b abiPart) bool {
+	if a.src+a.len == b.src && a.dst+a.len == b.dst {
+		a.len += b.len
+		return true
+	}
+	return false
 }
 
 type winCallbackKey struct {
@@ -55,7 +78,7 @@ func callbackasmAddr(i int) uintptr {
 	return funcPC(callbackasm) + uintptr(i*entrySize)
 }
 
-const callbackMaxArgs = 64
+const callbackMaxFrame = 64 * sys.PtrSize
 
 // compileCallback converts a Go function fn into a C function pointer
 // that can be passed to Windows APIs.
@@ -75,22 +98,81 @@ func compileCallback(fn eface, cdecl bool) (code uintptr) {
 		panic("compileCallback: expected function with one uintptr-sized result")
 	}
 	ft := (*functype)(unsafe.Pointer(fn._type))
+
+	// Check arguments and construct ABI translation.
+	var abiMap []abiPart
+	var src, dst uintptr
+	for _, t := range ft.in() {
+		if t.size > sys.PtrSize {
+			// We don't support this right now. In
+			// stdcall/cdecl, 64-bit ints and doubles are
+			// passed as two words (little endian); and
+			// structs are pushed on the stack. In
+			// fastcall, arguments larger than the word
+			// size are passed by reference.
+			panic("compileCallback: argument size is larger than uintptr")
+		}
+		if k := t.kind & kindMask; GOARCH == "amd64" && (k == kindFloat32 || k == kindFloat64) {
+			// In fastcall, floating-point arguments in
+			// the first four positions are passed in
+			// floating-point registers, which we don't
+			// currently spill.
+			panic("compileCallback: float arguments not supported")
+		}
+
+		// The Go ABI aligns arguments.
+		dst = alignUp(dst, uintptr(t.align))
+		// In the C ABI, we're already on a word boundary.
+		// Also, sub-word-sized fastcall register arguments
+		// are stored to the least-significant bytes of the
+		// argument word and all supported Windows
+		// architectures are little endian, so src is already
+		// pointing to the right place for smaller arguments.
+
+		// Copy just the size of the argument. Note that this
+		// could be a small by-value struct, but C and Go
+		// struct layouts are compatible, so we can copy these
+		// directly, too.
+		part := abiPart{src, dst, t.size}
+		// Add this step to the adapter.
+		if len(abiMap) == 0 || !abiMap[len(abiMap)-1].tryMerge(part) {
+			abiMap = append(abiMap, part)
+		}
+
+		// cdecl, stdcall, and fastcall pad arguments to word size.
+		src += sys.PtrSize
+		// The Go ABI packs arguments.
+		dst += t.size
+	}
+	// The Go ABI aligns the result to the word size. src is
+	// already aligned.
+	dst = alignUp(dst, sys.PtrSize)
+	retOffset := dst
+
 	if len(ft.out()) != 1 {
 		panic("compileCallback: expected function with one uintptr-sized result")
 	}
-	uintptrSize := unsafe.Sizeof(uintptr(0))
+	if ft.out()[0].size != sys.PtrSize {
-	if ft.out()[0].size != uintptrSize {
 		panic("compileCallback: expected function with one uintptr-sized result")
 	}
-	if len(ft.in()) > callbackMaxArgs {
+	if k := ft.out()[0].kind & kindMask; k == kindFloat32 || k == kindFloat64 {
-		panic("compileCallback: too many function arguments")
+		// In cdecl and stdcall, float results are returned in
+		// ST(0). In fastcall, they're returned in XMM0.
+		// Either way, it's not AX.
+		panic("compileCallback: float results not supported")
 	}
-	argsize := uintptr(0)
+	// Make room for the uintptr-sized result.
-	for _, t := range ft.in() {
+	dst += sys.PtrSize
-		if t.size > uintptrSize {
+
-			panic("compileCallback: argument size is larger than uintptr")
+	if dst > callbackMaxFrame {
+		panic("compileCallback: function argument frame too large")
 	}
-		argsize += uintptrSize
+
+	// For cdecl, the callee is responsible for popping its
+	// arguments from the C stack.
+	var retPop uintptr
+	if cdecl {
+		retPop = src
 	}
 
 	key := winCallbackKey{(*funcval)(fn.data), cdecl}
@@ -112,7 +194,7 @@ func compileCallback(fn eface, cdecl bool) (code uintptr) {
 		unlock(&cbs.lock)
 		throw("too many callback functions")
 	}
-	c := winCallback{key.fn, argsize, cdecl}
+	c := winCallback{key.fn, retPop, abiMap, retOffset}
 	cbs.ctxt[n] = c
 	cbs.index[key] = n
 	cbs.n++
@@ -123,7 +205,7 @@ func compileCallback(fn eface, cdecl bool) (code uintptr) {
 
 type callbackArgs struct {
 	index uintptr
-	args  *uintptr // Arguments in stdcall/cdecl convention, with registers spilled
+	args  unsafe.Pointer // Arguments in stdcall/cdecl convention, with registers spilled
 	// Below are out-args from callbackWrap
 	result uintptr
 	retPop uintptr // For 386 cdecl, how many bytes to pop on return
@@ -132,32 +214,21 @@ type callbackArgs struct {
 // callbackWrap is called by callbackasm to invoke a registered C callback.
 func callbackWrap(a *callbackArgs) {
 	c := cbs.ctxt[a.index]
-	if GOARCH == "386" {
+	a.retPop = c.retPop
-		if c.cdecl {
-			// In cdecl, the callee is responsible for
-			// popping its arguments.
-			a.retPop = c.argsize
-		} else {
-			a.retPop = 0
-		}
-	}
 
-	// Convert from stdcall to Go ABI. We assume the stack layout
+	// Convert from stdcall to Go ABI.
-	// is the same, and we just need to make room for the result.
+	var frame [callbackMaxFrame]byte
-	//
+	goArgs := unsafe.Pointer(&frame)
-	// TODO: This isn't a good assumption. For example, a function
+	for _, part := range c.abiMap {
-	// that takes two uint16 arguments will be laid out
+		memmove(add(goArgs, part.dst), add(a.args, part.src), part.len)
-	// differently by the stdcall and Go ABIs. We should implement
+	}
-	// proper ABI conversion.
-	var frame [callbackMaxArgs + 1]uintptr
-	memmove(unsafe.Pointer(&frame), unsafe.Pointer(a.args), c.argsize)
 
 	// Even though this is copying back results, we can pass a nil
 	// type because those results must not require write barriers.
-	reflectcall(nil, unsafe.Pointer(c.fn), noescape(unsafe.Pointer(&frame)), sys.PtrSize+uint32(c.argsize), uint32(c.argsize))
+	reflectcall(nil, unsafe.Pointer(c.fn), noescape(goArgs), uint32(c.retOffset)+sys.PtrSize, uint32(c.retOffset))
 
 	// Extract the result.
-	a.result = frame[c.argsize/sys.PtrSize]
+	a.result = *(*uintptr)(unsafe.Pointer(&frame[c.retOffset]))
 }
 
 const _LOAD_LIBRARY_SEARCH_SYSTEM32 = 0x00000800

src/runtime/syscall_windows_test.go

@@ -317,10 +317,14 @@ func (f cbFunc) cSrc(w io.Writer, cdecl bool) {
 	cArgs := make([]string, t.NumIn())
 	for i := range cTypes {
 		// We included stdint.h, so this works for all sized
-		// integer types.
+		// integer types, and uint8Pair_t.
 		cTypes[i] = t.In(i).Name() + "_t"
+		if t.In(i).Name() == "uint8Pair" {
+			cArgs[i] = fmt.Sprintf("(uint8Pair_t){%d,1}", i)
+		} else {
 			cArgs[i] = fmt.Sprintf("%d", i+1)
 		}
+	}
 	fmt.Fprintf(w, `
 typedef uintptr_t %s (*%s)(%s);
 uintptr_t %s(%s f) {
@@ -341,6 +345,8 @@ func (f cbFunc) testOne(t *testing.T, dll *syscall.DLL, cdecl bool, cb uintptr)
 	}
 }
 
+type uint8Pair struct{ x, y uint8 }
+
 var cbFuncs = []cbFunc{
 	{func(i1, i2 uintptr) uintptr {
 		return i1 + i2
@@ -366,6 +372,23 @@ var cbFuncs = []cbFunc{
 	{func(i1, i2, i3, i4, i5, i6, i7, i8, i9 uintptr) uintptr {
 		return i1 + i2 + i3 + i4 + i5 + i6 + i7 + i8 + i9
 	}},
+
+	// Non-uintptr parameters.
+	{func(i1, i2, i3, i4, i5, i6, i7, i8, i9 uint8) uintptr {
+		return uintptr(i1 + i2 + i3 + i4 + i5 + i6 + i7 + i8 + i9)
+	}},
+	{func(i1, i2, i3, i4, i5, i6, i7, i8, i9 uint16) uintptr {
+		return uintptr(i1 + i2 + i3 + i4 + i5 + i6 + i7 + i8 + i9)
+	}},
+	{func(i1, i2, i3, i4, i5, i6, i7, i8, i9 int8) uintptr {
+		return uintptr(i1 + i2 + i3 + i4 + i5 + i6 + i7 + i8 + i9)
+	}},
+	{func(i1 int8, i2 int16, i3 int32, i4, i5 uintptr) uintptr {
+		return uintptr(i1) + uintptr(i2) + uintptr(i3) + i4 + i5
+	}},
+	{func(i1, i2, i3, i4, i5 uint8Pair) uintptr {
+		return uintptr(i1.x + i1.y + i2.x + i2.y + i3.x + i3.y + i4.x + i4.y + i5.x + i5.y)
+	}},
 }
 
 type cbDLL struct {
@@ -380,7 +403,10 @@ func (d *cbDLL) makeSrc(t *testing.T, path string) {
 	}
 	defer f.Close()
 
-	fmt.Fprintf(f, "#include <stdint.h>\n\n")
+	fmt.Fprint(f, `
+#include <stdint.h>
+typedef struct { uint8_t x, y; } uint8Pair_t;
+`)
 	for _, cbf := range cbFuncs {
 		cbf.cSrc(f, false)
 		cbf.cSrc(f, true)