fix(lint): trailing-label CFG guard and the goroutine alias

Assisted-by: GLM 5.3
2026-09-19 23:49:19 +02:00
parent a8bfd54ed2
commit eb8b0cd316
4 changed files with 176 additions and 15 deletions
@@ -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
@@ -12,6 +12,7 @@ import (
 	"fmt"
 	"strconv"
 	"strings"
+	"unicode/utf8"

 	"sourcedock.dev/petrbalvin/gasm-devkit/arch"
 	"sourcedock.dev/petrbalvin/gasm-devkit/asm"
@@ -367,7 +368,8 @@ func lintText(t *ast.Text, tab *arch.Table, archKnown bool, cfg Config, macros m
 			if _, ok := referenced[name]; !ok {
 				out = append(out, Diagnostic{
 					Pos: pos,
-					End:      token.Position{Line: pos.Line, Column: pos.Column + len(name)},
+					// 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),
@@ -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)
@@ -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")
+	}
 }