Skip to content

Commit

Permalink
pass: ensure frame pointer register is saved (#174)
Browse files Browse the repository at this point in the history
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
  • Loading branch information
mmcloughlin authored Apr 19, 2021
1 parent 4592e16 commit f295bde
Show file tree
Hide file tree
Showing 16 changed files with 154 additions and 28 deletions.
2 changes: 1 addition & 1 deletion examples/fnv1a/fnv1a.s
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
2 changes: 1 addition & 1 deletion examples/stadtx/stadtx.s
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
3 changes: 3 additions & 0 deletions gotypes/components.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
11 changes: 0 additions & 11 deletions internal/cmd/asmdecl/main.go

This file was deleted.

17 changes: 17 additions & 0 deletions internal/cmd/asmvet/main.go
Original file line number Diff line number Diff line change
@@ -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,
)
}
1 change: 1 addition & 0 deletions pass/pass.go
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ var Compile = Concat(
FunctionPass(AllocateRegisters),
FunctionPass(BindRegisters),
FunctionPass(VerifyAllocation),
FunctionPass(EnsureBasePointerCalleeSaved),
Func(IncludeTextFlagHeader),
FunctionPass(PruneSelfMoves),
FunctionPass(RequiredISAExtensions),
Expand Down
63 changes: 63 additions & 0 deletions pass/reg.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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
}
Expand All @@ -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
}
52 changes: 52 additions & 0 deletions pass/reg_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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")
}
}
1 change: 1 addition & 0 deletions reg/types.go
Original file line number Diff line number Diff line change
Expand Up @@ -145,6 +145,7 @@ type Info uint8
const (
None Info = 0
Restricted Info = 1 << iota
BasePointer
)

// Physical is a concrete register.
Expand Down
8 changes: 4 additions & 4 deletions reg/x86.go
Original file line number Diff line number Diff line change
Expand Up @@ -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")
Expand All @@ -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")
Expand All @@ -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")
Expand All @@ -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")
Expand Down
4 changes: 2 additions & 2 deletions script/bootstrap
Original file line number Diff line number Diff line change
@@ -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'
Expand Down
2 changes: 1 addition & 1 deletion script/lint
Original file line number Diff line number Diff line change
Expand Up @@ -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
2 changes: 1 addition & 1 deletion tests/alloc/gp8/gp8.s
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
2 changes: 1 addition & 1 deletion tests/alloc/masks/masks.s
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
2 changes: 1 addition & 1 deletion tests/alloc/upper32/upper32.s
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down
10 changes: 5 additions & 5 deletions tests/fixedbugs/issue100/allocfail/allocfail.s
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down

0 comments on commit f295bde

Please sign in to comment.