From f295bde84ca8cfc23fd2074649045da5bd8f1242 Mon Sep 17 00:00:00 2001 From: Michael McLoughlin Date: Sun, 18 Apr 2021 18:37:56 -0700 Subject: [PATCH] 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 | 2 +- examples/stadtx/stadtx.s | 2 +- gotypes/components.go | 3 + internal/cmd/asmdecl/main.go | 11 ---- internal/cmd/asmvet/main.go | 17 +++++ pass/pass.go | 1 + pass/reg.go | 63 +++++++++++++++++++ pass/reg_test.go | 52 +++++++++++++++ reg/types.go | 1 + reg/x86.go | 8 +-- script/bootstrap | 4 +- script/lint | 2 +- tests/alloc/gp8/gp8.s | 2 +- tests/alloc/masks/masks.s | 2 +- tests/alloc/upper32/upper32.s | 2 +- .../fixedbugs/issue100/allocfail/allocfail.s | 10 +-- 16 files changed, 154 insertions(+), 28 deletions(-) delete mode 100644 internal/cmd/asmdecl/main.go create mode 100644 internal/cmd/asmvet/main.go diff --git a/examples/fnv1a/fnv1a.s b/examples/fnv1a/fnv1a.s index 2999665..44b48cd 100644 --- a/examples/fnv1a/fnv1a.s +++ b/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 diff --git a/examples/stadtx/stadtx.s b/examples/stadtx/stadtx.s index 58fede0..2a52cc4 100644 --- a/examples/stadtx/stadtx.s +++ b/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 diff --git a/gotypes/components.go b/gotypes/components.go index 2206afa..e9cfdb1 100644 --- a/gotypes/components.go +++ b/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 diff --git a/internal/cmd/asmdecl/main.go b/internal/cmd/asmdecl/main.go deleted file mode 100644 index cb092d2..0000000 --- a/internal/cmd/asmdecl/main.go +++ /dev/null @@ -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) } diff --git a/internal/cmd/asmvet/main.go b/internal/cmd/asmvet/main.go new file mode 100644 index 0000000..92dcb41 --- /dev/null +++ b/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, + ) +} diff --git a/pass/pass.go b/pass/pass.go index 62f37b1..4f36b8b 100644 --- a/pass/pass.go +++ b/pass/pass.go @@ -21,6 +21,7 @@ var Compile = Concat( FunctionPass(AllocateRegisters), FunctionPass(BindRegisters), FunctionPass(VerifyAllocation), + FunctionPass(EnsureBasePointerCalleeSaved), Func(IncludeTextFlagHeader), FunctionPass(PruneSelfMoves), FunctionPass(RequiredISAExtensions), diff --git a/pass/reg.go b/pass/reg.go index 79147b0..5df7f0d 100644 --- a/pass/reg.go +++ b/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()®.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 +} diff --git a/pass/reg_test.go b/pass/reg_test.go index 1d91112..1bb332d 100644 --- a/pass/reg_test.go +++ b/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") + } +} diff --git a/reg/types.go b/reg/types.go index 9f69e91..9eacdf9 100644 --- a/reg/types.go +++ b/reg/types.go @@ -145,6 +145,7 @@ type Info uint8 const ( None Info = 0 Restricted Info = 1 << iota + BasePointer ) // Physical is a concrete register. diff --git a/reg/x86.go b/reg/x86.go index a1ec94c..a911671 100644 --- a/reg/x86.go +++ b/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") diff --git a/script/bootstrap b/script/bootstrap index 4f32870..6a7c7ab 100755 --- a/script/bootstrap +++ b/script/bootstrap @@ -1,7 +1,7 @@ #!/bin/bash -ex -# Standalone version of the asmdecl analysis tool. -go install ./internal/cmd/asmdecl +# Standalone version of the assembly checks in go vet. +go install ./internal/cmd/asmvet # Install golangci-lint golangci_lint_version='v1.23.6' diff --git a/script/lint b/script/lint index de47561..d681371 100755 --- a/script/lint +++ b/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 diff --git a/tests/alloc/gp8/gp8.s b/tests/alloc/gp8/gp8.s index ff48db8..b2f25fc 100644 --- a/tests/alloc/gp8/gp8.s +++ b/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 diff --git a/tests/alloc/masks/masks.s b/tests/alloc/masks/masks.s index 6de1045..0565dcc 100644 --- a/tests/alloc/masks/masks.s +++ b/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 diff --git a/tests/alloc/upper32/upper32.s b/tests/alloc/upper32/upper32.s index 21aaa36..d1ca59f 100644 --- a/tests/alloc/upper32/upper32.s +++ b/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 diff --git a/tests/fixedbugs/issue100/allocfail/allocfail.s b/tests/fixedbugs/issue100/allocfail/allocfail.s index 03fbc78..e168c9e 100644 --- a/tests/fixedbugs/issue100/allocfail/allocfail.s +++ b/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