pass: ensure frame pointer register is saved (#174)

Currently `avo` uses `BP` as a standard general-purpose register. However, `BP` is used for the frame pointer and should be callee-save. Under some circumstances, the Go assembler will do this automatically, but not always. At the moment `avo` can produce code that clobbers the `BP` register. Since Go 1.16 this code will also fail a new `go vet` check.

This PR provides a (currently sub-optimal) fix for the issue. It introduces an `EnsureBasePointerCalleeSaved` pass which will check if the base pointer is written to by a function, and if so will artificially ensure that the function has a non-zero frame size. This will trigger the Go assembler to automatically save and restore the BP register.

In addition, we update the `asmdecl` tool to `asmvet`, which includes the `framepointer` vet check.

Updates #156

examples/fnv1a/fnv1a.s

@@ -3,7 +3,7 @@
 #include "textflag.h"
 
 // func Hash64(data []byte) uint64
-TEXT ·Hash64(SB), NOSPLIT, $0-32
+TEXT ·Hash64(SB), NOSPLIT, $8-32
 	MOVQ data_base+0(FP), CX
 	MOVQ data_len+8(FP), BX
 	MOVQ $0xcbf29ce484222325, AX

examples/stadtx/stadtx.s

@@ -3,7 +3,7 @@
 #include "textflag.h"
 
 // func Hash(state *State, key []byte) uint64
-TEXT ·Hash(SB), NOSPLIT, $0-40
+TEXT ·Hash(SB), NOSPLIT, $8-40
 	MOVQ  state+0(FP), AX
 	MOVQ  key_base+8(FP), CX
 	MOVQ  key_len+16(FP), DX

gotypes/components.go

@@ -15,6 +15,9 @@ import (
 // Sizes provides type sizes used by the standard Go compiler on amd64.
 var Sizes = types.SizesFor("gc", "amd64")
 
+// PointerSize is the size of a pointer on amd64.
+var PointerSize = Sizes.Sizeof(types.Typ[types.UnsafePointer])
+
 // Basic represents a primitive/basic type at a given memory address.
 type Basic struct {
 	Addr operand.Mem

internal/cmd/asmdecl/main.go

@@ -1,11 +0,0 @@
-// Command asmdecl reports mismatches between assembly files and Go declarations.
-//
-// Standalone version of the static analyzer in go vet.
-package main
-
-import (
-	"golang.org/x/tools/go/analysis/passes/asmdecl"
-	"golang.org/x/tools/go/analysis/singlechecker"
-)
-
-func main() { singlechecker.Main(asmdecl.Analyzer) }

internal/cmd/asmvet/main.go

@@ -0,0 +1,17 @@
+// Command asmvet checks for correctness of Go assembly.
+//
+// Standalone version of the assembly checks in go vet.
+package main
+
+import (
+	"golang.org/x/tools/go/analysis/passes/asmdecl"
+	"golang.org/x/tools/go/analysis/passes/framepointer"
+	"golang.org/x/tools/go/analysis/unitchecker"
+)
+
+func main() {
+	unitchecker.Main(
+		asmdecl.Analyzer,
+		framepointer.Analyzer,
+	)
+}

pass/pass.go

@@ -21,6 +21,7 @@ var Compile = Concat(
 	FunctionPass(AllocateRegisters),
 	FunctionPass(BindRegisters),
 	FunctionPass(VerifyAllocation),
+	FunctionPass(EnsureBasePointerCalleeSaved),
 	Func(IncludeTextFlagHeader),
 	FunctionPass(PruneSelfMoves),
 	FunctionPass(RequiredISAExtensions),

pass/reg.go

@@ -3,6 +3,7 @@ package pass
 import (
 	"errors"
 
+	"github.com/mmcloughlin/avo/gotypes"
 	"github.com/mmcloughlin/avo/ir"
 	"github.com/mmcloughlin/avo/operand"
 	"github.com/mmcloughlin/avo/reg"
@@ -120,6 +121,12 @@ func BindRegisters(fn *ir.Function) error {
 		for idx := range i.Operands {
 			i.Operands[idx] = operand.ApplyAllocation(i.Operands[idx], fn.Allocation)
 		}
+		for idx := range i.Inputs {
+			i.Inputs[idx] = operand.ApplyAllocation(i.Inputs[idx], fn.Allocation)
+		}
+		for idx := range i.Outputs {
+			i.Outputs[idx] = operand.ApplyAllocation(i.Outputs[idx], fn.Allocation)
+		}
 	}
 	return nil
 }
@@ -137,3 +144,59 @@ func VerifyAllocation(fn *ir.Function) error {
 
 	return nil
 }
+
+// EnsureBasePointerCalleeSaved ensures that the base pointer register will be
+// saved and restored if it has been clobbered by the function.
+func EnsureBasePointerCalleeSaved(fn *ir.Function) error {
+	// Check to see if the base pointer is written to.
+	clobbered := false
+	for _, i := range fn.Instructions() {
+		for _, r := range i.OutputRegisters() {
+			if p := reg.ToPhysical(r); p != nil && (p.Info()&reg.BasePointer) != 0 {
+				clobbered = true
+			}
+		}
+	}
+
+	if !clobbered {
+		return nil
+	}
+
+	// This function clobbers the base pointer register so we need to ensure it
+	// will be saved and restored. The Go assembler will do this automatically,
+	// with a few exceptions detailed below. In summary, we can usually ensure
+	// this happens by ensuring the function is not frameless (apart from
+	// NOFRAME functions).
+	//
+	// Reference: https://github.com/golang/go/blob/3f4977bd5800beca059defb5de4dc64cd758cbb9/src/cmd/internal/obj/x86/obj6.go#L591-L609
+	//
+	//		var bpsize int
+	//		if ctxt.Arch.Family == sys.AMD64 &&
+	//			!p.From.Sym.NoFrame() && // (1) below
+	//			!(autoffset == 0 && p.From.Sym.NoSplit()) && // (2) below
+	//			!(autoffset == 0 && !hasCall) { // (3) below
+	//			// Make room to save a base pointer.
+	//			// There are 2 cases we must avoid:
+	//			// 1) If noframe is set (which we do for functions which tail call).
+	//			// 2) Scary runtime internals which would be all messed up by frame pointers.
+	//			//    We detect these using a heuristic: frameless nosplit functions.
+	//			//    TODO: Maybe someday we label them all with NOFRAME and get rid of this heuristic.
+	//			// For performance, we also want to avoid:
+	//			// 3) Frameless leaf functions
+	//			bpsize = ctxt.Arch.PtrSize
+	//			autoffset += int32(bpsize)
+	//			p.To.Offset += int64(bpsize)
+	//		} else {
+	//			bpsize = 0
+	//		}
+	//
+	if fn.Attributes.NOFRAME() {
+		return errors.New("NOFRAME function clobbers base pointer register")
+	}
+
+	if fn.LocalSize == 0 {
+		fn.AllocLocal(int(gotypes.PointerSize))
+	}
+
+	return nil
+}

pass/reg_test.go

@@ -3,6 +3,7 @@ package pass_test
 import (
 	"testing"
 
+	"github.com/mmcloughlin/avo/attr"
 	"github.com/mmcloughlin/avo/build"
 	"github.com/mmcloughlin/avo/ir"
 	"github.com/mmcloughlin/avo/operand"
@@ -104,3 +105,54 @@ func AssertRegistersMatchSet(t *testing.T, rs []reg.Register, s reg.MaskSet) {
 func ConstructLiveness(t *testing.T, ctx *build.Context) *ir.Function {
 	return BuildFunction(t, ctx, pass.LabelTarget, pass.CFG, pass.Liveness)
 }
+
+func TestEnsureBasePointerCalleeSavedFrameless(t *testing.T) {
+	// Construct a function that writes to the base pointer.
+	ctx := build.NewContext()
+	ctx.Function("clobber")
+	ctx.ADDQ(reg.RAX, reg.RBP)
+
+	// Build the function with the EnsureBasePointerCalleeSaved pass.
+	fn := BuildFunction(t, ctx, pass.EnsureBasePointerCalleeSaved)
+
+	// Since the function was frameless, expect that the pass would have
+	expect := 8
+	if fn.LocalSize != expect {
+		t.Fatalf("expected frame size %d; got %d", expect, fn.LocalSize)
+	}
+}
+
+func TestEnsureBasePointerCalleeSavedWithFrame(t *testing.T) {
+	// Construct a function that writes to the base pointer, but already has a
+	// stack frame.
+	expect := 64
+
+	ctx := build.NewContext()
+	ctx.Function("clobber")
+	ctx.AllocLocal(expect)
+	ctx.ADDQ(reg.RAX, reg.RBP)
+
+	// Build the function with the EnsureBasePointerCalleeSaved pass.
+	fn := BuildFunction(t, ctx, pass.EnsureBasePointerCalleeSaved)
+
+	// Expect that since the function already has a stack frame, there's no need to increase its size.
+	if fn.LocalSize != expect {
+		t.Fatalf("expected frame size %d; got %d", expect, fn.LocalSize)
+	}
+}
+
+func TestEnsureBasePointerCalleeSavedNOFRAME(t *testing.T) {
+	// Construct a NOFRAME function that writes to base pointer.
+	ctx := build.NewContext()
+	ctx.Function("clobber")
+	ctx.Attributes(attr.NOFRAME)
+	ctx.ADDQ(reg.RAX, reg.RBP)
+
+	// Build the function.
+	fn := BuildFunction(t, ctx)
+
+	// Expect the pass to fail due to NOFRAME exception.
+	if err := pass.EnsureBasePointerCalleeSaved(fn); err == nil {
+		t.Fatal("expected error from NOFRAME function that clobbers base pointer")
+	}
+}

reg/types.go

@@ -145,6 +145,7 @@ type Info uint8
 const (
 	None       Info = 0
 	Restricted Info = 1 << iota
+	BasePointer
 )
 
 // Physical is a concrete register.

reg/x86.go

@@ -111,7 +111,7 @@ var (
 
 	// 8-bit
 	SPB  = gp(S8, 4, "SP", Restricted)
-	BPB  = gp(S8, 5, "BP")
+	BPB  = gp(S8, 5, "BP", BasePointer)
 	SIB  = gp(S8, 6, "SI")
 	DIB  = gp(S8, 7, "DI")
 	R8B  = gp(S8, 8, "R8")
@@ -129,7 +129,7 @@ var (
 	DX   = gp(S16, 2, "DX")
 	BX   = gp(S16, 3, "BX")
 	SP   = gp(S16, 4, "SP", Restricted)
-	BP   = gp(S16, 5, "BP")
+	BP   = gp(S16, 5, "BP", BasePointer)
 	SI   = gp(S16, 6, "SI")
 	DI   = gp(S16, 7, "DI")
 	R8W  = gp(S16, 8, "R8")
@@ -147,7 +147,7 @@ var (
 	EDX  = gp(S32, 2, "DX")
 	EBX  = gp(S32, 3, "BX")
 	ESP  = gp(S32, 4, "SP", Restricted)
-	EBP  = gp(S32, 5, "BP")
+	EBP  = gp(S32, 5, "BP", BasePointer)
 	ESI  = gp(S32, 6, "SI")
 	EDI  = gp(S32, 7, "DI")
 	R8L  = gp(S32, 8, "R8")
@@ -165,7 +165,7 @@ var (
 	RDX = gp(S64, 2, "DX")
 	RBX = gp(S64, 3, "BX")
 	RSP = gp(S64, 4, "SP", Restricted)
-	RBP = gp(S64, 5, "BP")
+	RBP = gp(S64, 5, "BP", BasePointer)
 	RSI = gp(S64, 6, "SI")
 	RDI = gp(S64, 7, "DI")
 	R8  = gp(S64, 8, "R8")

script/bootstrap

@@ -1,7 +1,7 @@
 #!/bin/bash -ex
 
-# Standalone version of the asmdecl analysis tool.
+# Standalone version of the assembly checks in go vet.
-go install ./internal/cmd/asmdecl
+go install ./internal/cmd/asmvet
 
 # Install golangci-lint
 golangci_lint_version='v1.23.6'

script/lint

@@ -18,7 +18,7 @@ test -z "$(git status --porcelain)"
 golangci-lint run ./... ./examples/...
 
 # Check asm declarations.
-asmdecl ./...
+go vet -vettool=$(which asmvet) ./...
 
 # Custom linters.
 ./script/linter/pkgdoc

tests/alloc/gp8/gp8.s

@@ -3,7 +3,7 @@
 #include "textflag.h"
 
 // func GP8() uint8
-TEXT ·GP8(SB), NOSPLIT, $0-1
+TEXT ·GP8(SB), NOSPLIT, $8-1
 	MOVB $0x01, AL
 	MOVB $0x02, CL
 	MOVB $0x03, DL

tests/alloc/masks/masks.s

@@ -3,7 +3,7 @@
 #include "textflag.h"
 
 // func Masks() (uint16, uint64)
-TEXT ·Masks(SB), NOSPLIT, $0-16
+TEXT ·Masks(SB), NOSPLIT, $8-16
 	MOVQ $0x0001002a, AX
 	MOVQ $0x0002002a, CX
 	MOVQ $0x0003002a, DX

tests/alloc/upper32/upper32.s

@@ -3,7 +3,7 @@
 #include "textflag.h"
 
 // func Upper32() uint64
-TEXT ·Upper32(SB), NOSPLIT, $0-8
+TEXT ·Upper32(SB), NOSPLIT, $8-8
 	// Initialize sum.
 	XORQ AX, AX
 

tests/fixedbugs/issue100/allocfail/allocfail.s

@@ -8978,7 +8978,7 @@ emit_literal_skip_emit_remainder_encodeBlockAsm12BAvx:
 
 // func emitLiteral(dst []byte, lit []byte) int
 // Requires: SSE2
-TEXT ·emitLiteral(SB), NOSPLIT, $0-56
+TEXT ·emitLiteral(SB), NOSPLIT, $8-56
 	MOVQ dst_base+0(FP), AX
 	MOVQ lit_base+24(FP), CX
 	MOVQ lit_len+32(FP), DX
@@ -9212,7 +9212,7 @@ emit_literal_end_standalone:
 
 // func emitLiteralAvx(dst []byte, lit []byte) int
 // Requires: AVX, SSE2
-TEXT ·emitLiteralAvx(SB), NOSPLIT, $0-56
+TEXT ·emitLiteralAvx(SB), NOSPLIT, $8-56
 	MOVQ dst_base+0(FP), AX
 	MOVQ lit_base+24(FP), CX
 	MOVQ lit_len+32(FP), DX
@@ -9492,7 +9492,7 @@ emit_literal_end_avx_standalone:
 	RET
 
 // func emitRepeat(dst []byte, offset int, length int) int
-TEXT ·emitRepeat(SB), NOSPLIT, $0-48
+TEXT ·emitRepeat(SB), NOSPLIT, $8-48
 	XORQ BX, BX
 	MOVQ dst_base+0(FP), AX
 	MOVQ offset+24(FP), CX
@@ -9574,7 +9574,7 @@ gen_emit_repeat_end:
 	RET
 
 // func emitCopy(dst []byte, offset int, length int) int
-TEXT ·emitCopy(SB), NOSPLIT, $0-48
+TEXT ·emitCopy(SB), NOSPLIT, $8-48
 	XORQ BX, BX
 	MOVQ dst_base+0(FP), AX
 	MOVQ offset+24(FP), CX
@@ -9784,7 +9784,7 @@ gen_emit_copy_end:
 	RET
 
 // func matchLen(a []byte, b []byte) int
-TEXT ·matchLen(SB), NOSPLIT, $0-56
+TEXT ·matchLen(SB), NOSPLIT, $8-56
 	MOVQ a_base+0(FP), AX
 	MOVQ b_base+24(FP), CX
 	MOVQ a_len+8(FP), DX