From eb8b0cd31683ec9d6cb3eb528d4f4f5154fd7d9d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Petr=20Balv=C3=ADn?= Date: Sat, 19 Sep 2026 23:49:19 +0200 Subject: [PATCH] fix(lint): trailing-label CFG guard and the goroutine alias Assisted-by: GLM 5.3 --- lint/analysis.go | 24 ++++++--- lint/lint.go | 6 ++- lint/liveness.go | 40 ++++++++++++-- lint/liveness_test.go | 121 +++++++++++++++++++++++++++++++++++++++++- 4 files changed, 176 insertions(+), 15 deletions(-) diff --git a/lint/analysis.go b/lint/analysis.go index 9553bda..37e938b 100644 --- a/lint/analysis.go +++ b/lint/analysis.go @@ -44,7 +44,7 @@ func checkFunCDATA(in *ast.Instr, cfg Config) []Diagnostic { Message: fmt.Sprintf("FUNCDATA expects 2 operands (index, symbol), got %d", len(in.Operands)), }} } - out = append(out, checkIndex(in.Operands[0], "FUNCDATA")...) + out = append(out, checkIndex(in.Operands[0], "FUNCDATA", maxFuncdataIndex)...) if sym := in.Operands[1].Addr.Sym; in.Operands[1].Kind != ast.OpAddr || sym == nil { out = append(out, Diagnostic{ Pos: in.Operands[1].Pos, Severity: Warning, Code: CodeFuncdata, @@ -66,7 +66,7 @@ func checkPCDATA(in *ast.Instr, cfg Config) []Diagnostic { }} } var out []Diagnostic - out = append(out, checkIndex(in.Operands[0], "PCDATA")...) + out = append(out, checkIndex(in.Operands[0], "PCDATA", maxPcdataIndex)...) if in.Operands[1].Kind != ast.OpImmediate { out = append(out, Diagnostic{ Pos: in.Operands[1].Pos, Severity: Warning, Code: CodeFuncdata, @@ -76,20 +76,30 @@ func checkPCDATA(in *ast.Instr, cfg Config) []Diagnostic { return out } -// checkIndex validates an immediate index operand. A literal index must lie in -// the small range the runtime uses; a named constant (e.g. $PCDATA_StackMapIndex) +// The largest literal FUNCDATA and PCDATA indices the Go 1.27 runtime +// defines (internal/abi/symtab.go, which pkg/include/funcdata.h mirrors): +// FUNCDATA_WrapInfo is 7 and PCDATA_PanicBounds is 4. A literal above them +// addresses metadata no runtime reads, which in hand-written assembly is +// near-certainly a typo. +const ( + maxFuncdataIndex = 7 + maxPcdataIndex = 4 +) + +// checkIndex validates an immediate index operand. A literal index must lie +// in the range the runtime defines; a named constant (e.g. $PCDATA_StackMapIndex) // cannot be evaluated and is accepted without a range check. -func checkIndex(op *ast.Operand, directive string) []Diagnostic { +func checkIndex(op *ast.Operand, directive string, max int64) []Diagnostic { if op.Kind != ast.OpImmediate { return []Diagnostic{{ Pos: op.Pos, Severity: Warning, Code: CodeFuncdata, Message: directive + " index must be an immediate", }} } - if op.Imm.HasVal && (op.Imm.Val < 0 || op.Imm.Val > 10) { + if op.Imm.HasVal && (op.Imm.Val < 0 || op.Imm.Val > max) { return []Diagnostic{{ Pos: op.Pos, Severity: Warning, Code: CodeFuncdata, - Message: fmt.Sprintf("%s index %d is outside the valid range 0-10", directive, op.Imm.Val), + Message: fmt.Sprintf("%s index %d is outside the valid range 0-%d", directive, op.Imm.Val, max), }} } return nil diff --git a/lint/lint.go b/lint/lint.go index 8e415e3..ea79508 100644 --- a/lint/lint.go +++ b/lint/lint.go @@ -12,6 +12,7 @@ import ( "fmt" "strconv" "strings" + "unicode/utf8" "sourcedock.dev/petrbalvin/gasm-devkit/arch" "sourcedock.dev/petrbalvin/gasm-devkit/asm" @@ -366,8 +367,9 @@ func lintText(t *ast.Text, tab *arch.Table, archKnown bool, cfg Config, macros m for name, pos := range defined { if _, ok := referenced[name]; !ok { out = append(out, Diagnostic{ - Pos: pos, - End: token.Position{Line: pos.Line, Column: pos.Column + len(name)}, + Pos: pos, + // Columns are rune-based, so the end offset is too. + End: token.Position{Line: pos.Line, Column: pos.Column + utf8.RuneCountInString(name)}, Severity: Hint, Code: CodeUnusedLabel, Message: fmt.Sprintf("label %q is defined but never referenced", name), diff --git a/lint/liveness.go b/lint/liveness.go index d94747c..b395fd0 100644 --- a/lint/liveness.go +++ b/lint/liveness.go @@ -4,7 +4,7 @@ package lint import ( - "sort" + "slices" "strings" "sourcedock.dev/petrbalvin/gasm-devkit/arch" @@ -86,6 +86,15 @@ func (l *liveness) buildCFG(t *ast.Text) { labelToBlock[b.label] = i } } + // A trailing label whose block was never flushed (no instruction follows + // it) still holds the index the next block would have taken, which is one + // past the end. Drop those entries so a branch to such a label wires no + // edge instead of indexing past the live sets in the dataflow. + for name, v := range labelToBlock { + if v >= len(l.blocks) { + delete(labelToBlock, name) + } + } for i, b := range l.blocks { if len(b.instrs) == 0 { if i+1 < len(l.blocks) { @@ -210,9 +219,13 @@ func isUnconditionalBranchAny(m string) bool { return false } -// branchTarget returns the local-label target of a branch, if it is one. +// branchTarget returns the local-label target of a branch, if it is one. The +// last bare-symbol operand is taken because the Plan 9 branch target sits in +// the destination position, after any operand registers: CBZ R0, done or +// BEQ R1, R2, done. Scanning forward would pick R0 up as the target. func branchTarget(in *ast.Instr) (string, bool) { - for _, op := range in.Operands { + for _, op := range slices.Backward(in.Operands) { + if op.Kind == ast.OpAddr && op.Addr.Sym != nil && op.Addr.Sym.Pseudo == "" && op.Addr.Base == "" && op.Addr.Sym.Name != "" { return op.Addr.Sym.Name, true @@ -294,6 +307,20 @@ func isCompare(m string) bool { m == "FCMP" || m == "FCMPE" } +// goroutineAlias maps the assembler's architectural alias for the goroutine +// register to its numeric name, verified against go tool asm of the Go 1.27 +// toolchain: `MOVQ AX, g` encodes the same bytes as R14 on amd64, and on +// arm64, riscv64 and loong64 the alias is the ONLY spelling the toolchain +// accepts (a numeric R28, X27 or R22 operand is rejected), so a kernel can +// clobber the goroutine register through `g` alone. The name is +// case-sensitive: only lowercase `g` assembles. +var goroutineAlias = map[arch.Arch]string{ + arch.AMD64: "R14", + arch.ARM64: "R28", + arch.RISCV: "X27", + arch.LOONG64: "R22", +} + // gprName returns the canonical general-purpose register name of an operand, or // "" if the operand is not a bare GPR reference. func gprName(op *ast.Operand, a arch.Arch) string { @@ -307,6 +334,11 @@ func gprName(op *ast.Operand, a arch.Arch) string { if r, ok := arch.ForArch(a).Register(name); ok && (r.Class == arch.GPR || r.Class == arch.GPRSub) { return canonicalGPR(name) } + // The goroutine alias is not an arch-table register; resolve it so a + // clobber written through `g` is audited like the numeric register. + if n, ok := goroutineAlias[a]; ok && name == "g" { + return n + } return "" } @@ -445,7 +477,7 @@ func clobberedGoFixed(l *liveness, a arch.Arch, reachesRuntime bool) (always, ru out = append(out, r) } } - sort.Strings(out) + slices.Sort(out) return out } always = clobbered(alwaysSet) diff --git a/lint/liveness_test.go b/lint/liveness_test.go index 8abace1..8e74299 100644 --- a/lint/liveness_test.go +++ b/lint/liveness_test.go @@ -3,7 +3,12 @@ package lint -import "testing" +import ( + "testing" + + "sourcedock.dev/petrbalvin/gasm-devkit/arch" + "sourcedock.dev/petrbalvin/gasm-devkit/parser" +) // TestRegisterClobber checks the register-clobber audit is calibrated to the // Go ABI (cmd/compile/abi-internal.md), not the platform ABI: Go's @@ -134,6 +139,95 @@ func TestRegisterClobber(t *testing.T) { } } +// TestRegisterClobberGoroutineAlias checks the architectural alias `g` for +// the goroutine register. go tool asm accepts it on every architecture +// (lowercase only), and on arm64, riscv64 and loong64 it is the only +// spelling of the register at all, so a clobber written through the alias +// must be audited exactly like the numeric one. +func TestRegisterClobberGoroutineAlias(t *testing.T) { + // amd64: `g` is R14, a runtime-class register: the NOSPLIT-leaf pattern + // of the runtime's own assembly stays unflagged, a call makes it a + // hazard. + leaf := lintSrc(t, "#include \"textflag.h\"\n"+ + "TEXT ·f(SB), NOSPLIT, $0\n"+ + "\tMOVQ AX, g\n"+ + "\tRET\n") + if codes(leaf)[CodeRegisterClobber] != 0 { + t.Fatalf("amd64 g in a NOSPLIT leaf must not be flagged: %+v", leaf) + } + withCall := lintSrc(t, "#include \"textflag.h\"\n"+ + "TEXT ·f(SB), NOSPLIT, $0\n"+ + "\tMOVQ AX, g\n"+ + "\tCALL ·h(SB)\n"+ + "\tRET\n") + if codes(withCall)[CodeRegisterClobber] != 1 { + t.Fatalf("amd64 unsaved g with a call should be flagged: %+v", withCall) + } + + // arm64, riscv64, loong64: the goroutine register is always-fixed, so an + // unsaved write through the alias is flagged; a bare read is not. + for _, tc := range []struct{ filename, src string }{ + {"t_arm64.s", "#include \"textflag.h\"\n" + "TEXT ·f(SB), NOSPLIT, $0\n" + "\tMOVD R0, g\n" + "\tRET\n"}, + {"t_riscv64.s", "#include \"textflag.h\"\n" + "TEXT ·f(SB), NOSPLIT, $0\n" + "\tMOV X5, g\n" + "\tRET\n"}, + {"t_loong64.s", "#include \"textflag.h\"\n" + "TEXT ·f(SB), NOSPLIT, $0\n" + "\tMOVV R5, g\n" + "\tRET\n"}, + } { + if got := codes(lintSrcArch(t, tc.filename, tc.src))[CodeRegisterClobber]; got != 1 { + t.Fatalf("%s: unsaved write to g should be flagged once, got %d", tc.filename, got) + } + } + armRead := lintSrcArch(t, "t_arm64.s", "#include \"textflag.h\"\n"+ + "TEXT ·f(SB), NOSPLIT, $0\n"+ + "\tMOVD g, R1\n"+ + "\tRET\n") + if codes(armRead)[CodeRegisterClobber] != 0 { + t.Fatalf("reading g must not be flagged: %+v", armRead) + } +} + +// TestBranchToTrailingLabel pins the CFG guard for a label that ends a +// function body: no block is flushed for it, and the liveness dataflow must +// not index past the block list on the edge a branch to it would carry. +func TestBranchToTrailingLabel(t *testing.T) { + diags := lintSrc(t, "#include \"textflag.h\"\n"+ + "TEXT ·f(SB), NOSPLIT, $0\n"+ + "\tJMP done\n"+ + "done:\n") + if codes(diags)[CodeRegisterClobber] != 0 { + t.Fatalf("branch to a trailing label must not be flagged: %+v", diags) + } +} + +// TestConditionalBranchToTrailingLabel exercises a multi-operand conditional +// branch (CBZ R0, done): the branch target is the last bare-symbol operand, +// so the taken edge is wired to the trailing label and the guard of +// TestBranchToTrailingLabel is what keeps the dataflow in range. The run +// doubles as the termination check for the fixed-point loop. +func TestConditionalBranchToTrailingLabel(t *testing.T) { + diags := lintSrcArch(t, "t_arm64.s", "#include \"textflag.h\"\n"+ + "TEXT ·f(SB), NOSPLIT, $0\n"+ + "\tCBZ R0, done\n"+ + "\tRET\n"+ + "done:\n") + if codes(diags)[CodeRegisterClobber] != 0 { + t.Fatalf("conditional branch to a trailing label must not be flagged: %+v", diags) + } +} + +// TestMalformedTextNoSymbol checks that a TEXT line without a symbol name, +// which the parser keeps in the tree under a "?" placeholder, lints without +// a panic and still reports the ordinary findings. +func TestMalformedTextNoSymbol(t *testing.T) { + f, _ := parser.Parse("test_amd64.s", "// func f(a int) int\nTEXT $0\n\tMOVQ AX, BX\n") + diags := File(f, Config{Arch: arch.AMD64}) + c := codes(diags) + if c[CodeMissingRet] != 1 { + t.Fatalf("malformed TEXT should report missing-ret, got %+v", diags) + } + if c[CodeABI0RegisterArgs] != 1 { + t.Fatalf("malformed TEXT should report abi0-register-args, got %+v", diags) + } +} + // TestFuncdata validates the FUNCDATA/PCDATA structural checks. func TestFuncdata(t *testing.T) { // Well formed: no findings. @@ -164,7 +258,8 @@ func TestFuncdata(t *testing.T) { t.Fatal("PCDATA with a register value should be flagged") } - // FUNCDATA index out of range. + // FUNCDATA index out of range: the Go 1.27 runtime defines 0-7 + // (FUNCDATA_WrapInfo) and PCDATA 0-4 (PCDATA_PanicBounds). bad3 := lintSrc(t, "#include \"textflag.h\"\n"+ "TEXT ·f(SB), NOSPLIT, $0\n"+ "\tFUNCDATA $99, gclocals·abc(SB)\n"+ @@ -172,4 +267,26 @@ func TestFuncdata(t *testing.T) { if codes(bad3)[CodeFuncdata] == 0 { t.Fatal("out-of-range FUNCDATA index should be flagged") } + fdHigh := lintSrc(t, "#include \"textflag.h\"\n"+ + "TEXT ·f(SB), NOSPLIT, $0\n"+ + "\tFUNCDATA $7, gclocals·abc(SB)\n"+ + "\tPCDATA $4, $0\n"+ + "\tRET\n") + if codes(fdHigh)[CodeFuncdata] != 0 { + t.Fatalf("the highest defined FUNCDATA/PCDATA indices must pass: %+v", fdHigh) + } + fdOver := lintSrc(t, "#include \"textflag.h\"\n"+ + "TEXT ·f(SB), NOSPLIT, $0\n"+ + "\tFUNCDATA $8, gclocals·abc(SB)\n"+ + "\tRET\n") + if codes(fdOver)[CodeFuncdata] != 1 { + t.Fatal("FUNCDATA index above FUNCDATA_WrapInfo should be flagged") + } + pcOver := lintSrc(t, "#include \"textflag.h\"\n"+ + "TEXT ·f(SB), NOSPLIT, $0\n"+ + "\tPCDATA $5, $0\n"+ + "\tRET\n") + if codes(pcOver)[CodeFuncdata] != 1 { + t.Fatal("PCDATA index above PCDATA_PanicBounds should be flagged") + } }