From 4356e7e85fcd8f59de6bc1fd1db6e4f01a92f19e Mon Sep 17 00:00:00 2001
From: Michael Anthony Knyszek <mknyszek@google.com>
Date: Fri, 21 May 2021 20:30:02 +0000
Subject: [PATCH] runtime: account for spill slots in Windows callback
 compilation

The Go ABI, as it stands, requires spill space to be reserved for
register arguments. syscall.NewCallback (because of compileCallback)
does not actually reserve this space, leading to issues if the Go code
it invokes actually makes use of it.

Fixes #46301.

Change-Id: Idbc3578accaaaa29e4ba32291ef08d464da0b7b0
Reviewed-on: https://go-review.googlesource.com/c/go/+/322029
Trust: Michael Knyszek <mknyszek@google.com>
Run-TryBot: Michael Knyszek <mknyszek@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Egon Elbre <egonelbre@gmail.com>
Reviewed-by: Cherry Mui <cherryyz@google.com>
---
 src/runtime/syscall_windows.go      | 23 ++++++++++++++++++++---
 src/runtime/syscall_windows_test.go | 15 +++++++++++++++
 2 files changed, 35 insertions(+), 3 deletions(-)

diff --git a/src/runtime/syscall_windows.go b/src/runtime/syscall_windows.go
index 6b9195bcd5..4763a440e7 100644
--- a/src/runtime/syscall_windows.go
+++ b/src/runtime/syscall_windows.go
@@ -64,6 +64,7 @@ type abiDesc struct {
 
 	srcStackSize uintptr // stdcall/fastcall stack space tracking
 	dstStackSize uintptr // Go stack space used
+	dstSpill     uintptr // Extra stack space for argument spill slots
 	dstRegisters int     // Go ABI int argument registers used
 
 	// retOffset is the offset of the uintptr-sized result in the Go
@@ -110,7 +111,14 @@ func (p *abiDesc) assignArg(t *_type) {
 	// arguments. The same is true on arm.
 
 	oldParts := p.parts
-	if !p.tryRegAssignArg(t, 0) {
+	if p.tryRegAssignArg(t, 0) {
+		// Account for spill space.
+		//
+		// TODO(mknyszek): Remove this when we no longer have
+		// caller reserved spill space.
+		p.dstSpill = alignUp(p.dstSpill, uintptr(t.align))
+		p.dstSpill += t.size
+	} else {
 		// Register assignment failed.
 		// Undo the work and stack assign.
 		p.parts = oldParts
@@ -277,7 +285,11 @@ func compileCallback(fn eface, cdecl bool) (code uintptr) {
 		abiMap.dstStackSize += sys.PtrSize
 	}
 
-	if abiMap.dstStackSize > callbackMaxFrame {
+	// TODO(mknyszek): Remove dstSpill from this calculation when we no longer have
+	// caller reserved spill space.
+	frameSize := alignUp(abiMap.dstStackSize, sys.PtrSize)
+	frameSize += abiMap.dstSpill
+	if frameSize > callbackMaxFrame {
 		panic("compileCallback: function argument frame too large")
 	}
 
@@ -356,9 +368,14 @@ func callbackWrap(a *callbackArgs) {
 		}
 	}
 
+	// TODO(mknyszek): Remove this when we no longer have
+	// caller reserved spill space.
+	frameSize := alignUp(c.abiMap.dstStackSize, sys.PtrSize)
+	frameSize += c.abiMap.dstSpill
+
 	// 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(goArgs), uint32(c.abiMap.dstStackSize), uint32(c.abiMap.retOffset), uint32(c.abiMap.dstStackSize), &regs)
+	reflectcall(nil, unsafe.Pointer(c.fn), noescape(goArgs), uint32(c.abiMap.dstStackSize), uint32(c.abiMap.retOffset), uint32(frameSize), &regs)
 
 	// Extract the result.
 	//
diff --git a/src/runtime/syscall_windows_test.go b/src/runtime/syscall_windows_test.go
index 5e9694d444..e3f772ac4b 100644
--- a/src/runtime/syscall_windows_test.go
+++ b/src/runtime/syscall_windows_test.go
@@ -389,6 +389,10 @@ var cbFuncs = []cbFunc{
 	{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)
 	}},
+	{func(i1, i2, i3, i4, i5, i6, i7, i8, i9 uint32) uintptr {
+		runtime.GC()
+		return uintptr(i1 + i2 + i3 + i4 + i5 + i6 + i7 + i8 + i9)
+	}},
 }
 
 //go:registerparams
@@ -461,6 +465,16 @@ func sum5andPair(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)
 }
 
+// This test forces a GC. The idea is to have enough arguments
+// that insufficient spill slots allocated (according to the ABI)
+// may cause compiler-generated spills to clobber the return PC.
+// Then, the GC stack scanning will catch that.
+//go:registerparams
+func sum9andGC(i1, i2, i3, i4, i5, i6, i7, i8, i9 uint32) uintptr {
+	runtime.GC()
+	return uintptr(i1 + i2 + i3 + i4 + i5 + i6 + i7 + i8 + i9)
+}
+
 // TODO(register args): Remove this once we switch to using the register
 // calling convention by default, since this is redundant with the existing
 // tests.
@@ -479,6 +493,7 @@ var cbFuncsRegABI = []cbFunc{
 	{sum9int8},
 	{sum5mix},
 	{sum5andPair},
+	{sum9andGC},
 }
 
 func getCallbackTestFuncs() []cbFunc {