fix(lint): exempt shift counts, SETcc and ABIInternal from false positives

Assisted-by: GLM 5.3 Flash
2026-09-20 11:40:39 +02:00
parent c66a47973a
commit cc6e416c59
4 changed files with 348 additions and 8 deletions
@@ -21,6 +21,12 @@ import (
 // The check requires a parseable signature; functions without one, and
 // functions whose parameters are all covered by frame reads, stay silent.
 func checkABI0Args(t *ast.Text) []Diagnostic {
+	// An explicit <ABIInternal> TEXT reads its arguments from the register
+	// file by declaration (runtime·memmove<ABIInternal> is the canonical
+	// example), so the ABI0 frame contract does not apply to it.
+	if t.Name != nil && t.Name.ABI != "" {
+		return nil
+	}
 	params, ok := abiParamNames(t.Doc)
 	if !ok || len(params) == 0 {
 		return nil
@@ -82,6 +82,23 @@ func TestABIArgSizeSkipsRegisterABI(t *testing.T) {
 	}
 }

+// TestABI0ArgsSkipsABIInternal verifies the frame-read check does not fire for
+// a TEXT declared <ABIInternal>: runtime·memmove<ABIInternal> and friends read
+// their arguments from the register file by declaration, which is the correct
+// spelling there, not the register-args port bug the rule hunts.
+func TestABI0ArgsSkipsABIInternal(t *testing.T) {
+	diags := lintSrc(t, "#include \"textflag.h\"\n"+
+		"// func memmove(to, from unsafe.Pointer, n uintptr)\n"+
+		"TEXT ·memmove<ABIInternal>(SB), NOSPLIT, $0-24\n"+
+		"\tMOVQ AX, DI\n"+
+		"\tMOVQ BX, SI\n"+
+		"\tMOVQ CX, BX\n"+
+		"\tRET\n")
+	if codes(diags)[CodeABI0RegisterArgs] != 0 {
+		t.Fatalf("ABIInternal TEXT must not be checked against the FP frame: %+v", diags)
+	}
+}
+
 // TestUnreachableCode exercises the dead-code detection and its guard rails.
 func TestUnreachableCode(t *testing.T) {
 	// Code after a RET is unreachable.
@@ -338,10 +338,8 @@ func lintText(t *ast.Text, tab *arch.Table, archKnown bool, cfg Config, macros m
 			}

 			if isJump(cfg.Arch, upper) {
-				for _, op := range st.Operands {
-					if name, pos, ok := localLabelRef(op); ok && !tab.IsRegister(name) && !arch.IsPseudoReg(name) {
-						referenced[name] = pos
-					}
+				if name, pos, ok := branchTargetRef(cfg.Arch, upper, st.Operands, tab); ok {
+					referenced[name] = pos
 				}
 			}
 		}
@@ -649,17 +647,65 @@ func localLabelRef(op *ast.Operand) (string, token.Position, bool) {
 	return sym.Name, op.Pos, true
 }

+// branchTargetRef returns the local label a branch transfers control to: the
+// bare symbol in the destination position, the last operand, since that is
+// where the Plan 9 branch target sits.  A register-named target is a
+// register-indirect branch (JMP AX, arm64 BR R5, riscv64 JALR X6, loong64
+// JIRL R1) and yields no reference, unless the encoder reads the target
+// positionally (positionalBranchTarget): there a label may legitimately
+// collide with a register alias, riscv64 ZERO being the ABI name of X0, and
+// a label named zero is ordinary code.
+func branchTargetRef(a arch.Arch, upper string, ops []*ast.Operand, tab *arch.Table) (string, token.Position, bool) {
+	if len(ops) == 0 {
+		return "", token.Position{}, false
+	}
+	name, pos, ok := localLabelRef(ops[len(ops)-1])
+	if !ok {
+		return "", token.Position{}, false
+	}
+	if !positionalBranchTarget(a, upper) && (tab.IsRegister(name) || arch.IsPseudoReg(name)) {
+		return "", token.Position{}, false
+	}
+	return name, pos, true
+}
+
+// positionalBranchTarget reports whether the encoder reads a bare-symbol
+// operand of the branch as its label target from a fixed position, without
+// consulting the register file.  The riscv64 branch, JMP and JAL encoders do
+// (labelFromOperand in asm/riscv_assemble.go), as do the loong64 branch,
+// BFPT/BFPF and jump encoders (l64Label in asm/loong64_assemble.go).  amd64
+// never does, because a bare register operand to JMP/CALL/Jcc is a
+// register-indirect branch; nor do the register-indirect forms of the RISC
+// families (arm64 BR/BLR, riscv64 JALR/JR, loong64 JIRL).
+func positionalBranchTarget(a arch.Arch, upper string) bool {
+	switch a {
+	case arch.RISCV:
+		return riscvBranches[upper] || upper == "JMP" || upper == "JAL"
+	case arch.LOONG64:
+		return loong64Branches[upper] || upper == "JMP" || upper == "B" ||
+			upper == "JAL" || upper == "BL"
+	}
+	return false
+}
+
 // riscvBranches and loong64Branches are the conditional-branch mnemonics; they
 // are listed explicitly rather than matched by a "B" prefix so that bit-manip
-// instructions (BCLR, BSET, …) are never mistaken for branches.
+// instructions (BCLR, BSET, …) are never mistaken for branches.  The sets
+// mirror the encoder's own branch cases: the B-type table entries
+// (riscv_encode.go), the branch-zero pseudos and the reversed branches
+// BGT/BGTU/BLE/BLEU (riscv_assemble.go), and for loong64 the 16-bit branch
+// table plus the single-register forms of l64branch21Table (BEQZ/BNEZ and the
+// floating-point branches BFPT/BFPF).
 var riscvBranches = map[string]bool{
 	"BEQ": true, "BNE": true, "BLT": true, "BGE": true, "BLTU": true, "BGEU": true,
 	"BEQZ": true, "BNEZ": true, "BLEZ": true, "BGEZ": true, "BLTZ": true, "BGTZ": true,
+	"BGT": true, "BGTU": true, "BLE": true, "BLEU": true,
 }

 var loong64Branches = map[string]bool{
 	"BEQ": true, "BNE": true, "BLT": true, "BGE": true, "BLTU": true, "BGEU": true,
 	"BLEZ": true, "BLTZ": true, "BGEZ": true, "BGTZ": true,
+	"BEQZ": true, "BNEZ": true, "BFPT": true, "BFPF": true,
 }

 // isJump reports whether the mnemonic is any branch.
@@ -676,7 +722,8 @@ func isJump(a arch.Arch, upper string) bool {
 			upper == "JR" || upper == "BR"
 	case arch.LOONG64:
 		return upper == "CALL" || loong64Branches[upper] ||
-			upper == "JIRL" || upper == "JMP" || upper == "BR"
+			upper == "JIRL" || upper == "JMP" || upper == "BR" ||
+			upper == "B" || upper == "JAL" || upper == "BL"
 	default: // amd64
 		return upper == "CALL" || strings.HasPrefix(upper, "J")
 	}
@@ -692,7 +739,8 @@ func isUnconditionalJump(a arch.Arch, upper string) bool {
 		return upper == "JMP" || upper == "J" || upper == "JAL" ||
 			upper == "JALR" || upper == "JR" || upper == "BR"
 	case arch.LOONG64:
-		return upper == "JMP" || upper == "JIRL" || upper == "BR"
+		return upper == "JMP" || upper == "JIRL" || upper == "BR" || upper == "B" ||
+			upper == "JAL" || upper == "BL"
 	default:
 		return upper == "JMP"
 	}
@@ -792,6 +840,43 @@ func isSPReg(op *ast.Operand, a arch.Arch) bool {
 	return false
 }

+// shiftRotateBases are the shift and rotate mnemonics without their width
+// suffix.  These are the instructions whose encoder path (encodeShift) reads
+// the count from the first operand.
+var shiftRotateBases = map[string]bool{
+	"SHL": true, "SHR": true, "SAR": true, "SAL": true,
+	"ROL": true, "ROR": true, "RCL": true, "RCR": true,
+}
+
+// isShiftCountOperand reports whether operand i of mnem is the shift count.
+// The ISA fixes the shift/rotate count register at CL: the D2/D3 group (and
+// C0/C1 for immediates) encode the count outside the ModRM register field,
+// so the count operand is 8-bit by definition no matter how wide the data is.
+// The count arrives as the first of the two operands; the one-operand form
+// does not exist.
+func isShiftCountOperand(mnem string, i, nops int) bool {
+	if nops != 2 || i != 0 {
+		return false
+	}
+	if shiftRotateBases[mnem] {
+		return true
+	}
+	if len(mnem) > 1 {
+		switch mnem[len(mnem)-1] {
+		case 'Q', 'L', 'W', 'B':
+			return shiftRotateBases[mnem[:len(mnem)-1]]
+		}
+	}
+	return false
+}
+
+// isSetcc reports whether the mnemonic is a SETcc: SET plus a condition code.
+// The membership test is the encoder's own SET dispatch, which asm.Encodable
+// mirrors.
+func isSetcc(mnem string) bool {
+	return strings.HasPrefix(mnem, "SET") && asm.Encodable(mnem)
+}
+
 // checkRegisterWidth detects amd64 register-width mismatches.  The naming
 // truth of the Go assembler governs: AX, BX, CX, DX, SI, DI, BP, SP and
 // R8-R15 ARE the 64-bit register names (there are no separate EAX/RAX
@@ -802,6 +887,14 @@ func isSPReg(op *ast.Operand, a arch.Arch) bool {
 // register (EAX under the gasm alias extension, or a byte form), and byte
 // registers in L/W operations.
 func checkRegisterWidth(mnem string, ops []*ast.Operand) string {
+	// A SETcc stores one byte: the destination is an 8-bit register or an
+	// 8-bit memory location by definition (0F 90+cc), whichever condition it
+	// tests.  The trailing letter of spellings like SETPL or SETEQ is part of
+	// the condition code, not an operand width, so the whole family is
+	// exempt from the suffix logic.
+	if isSetcc(mnem) {
+		return ""
+	}
 	// Determine expected width from mnemonic suffix.
 	var expected int // 0=unknown, 8/4/2/1=bytes
 	switch {
@@ -816,10 +909,20 @@ func checkRegisterWidth(mnem string, ops []*ast.Operand) string {
 	default:
 		return "" // no suffix, can't determine width
 	}
-	for _, op := range ops {
+	for i, op := range ops {
 		if op.Kind != ast.OpAddr || op.Addr.Sym == nil {
 			continue
 		}
+		// Only a bare register carries a width to compare: frame and static
+		// symbol references (ch+8(FP), foo(SB)) and memory operands are not
+		// registers even when their name collides with one.
+		if op.Addr.Sym.Pseudo != "" || op.Addr.Base != "" || op.Addr.Index != "" {
+			continue
+		}
+		// The shift/rotate count is exempt: fixed at 8 bits by the ISA.
+		if isShiftCountOperand(mnem, i, len(ops)) {
+			continue
+		}
 		name := strings.ToLower(op.Addr.Sym.Name)
 		regWidth := amd64RegWidth(name)
 		if regWidth == 0 {
@@ -8,6 +8,7 @@ import (
 	"testing"

 	"sourcedock.dev/petrbalvin/gasm-devkit/arch"
+	"sourcedock.dev/petrbalvin/gasm-devkit/asm"
 	"sourcedock.dev/petrbalvin/gasm-devkit/ast"
 	"sourcedock.dev/petrbalvin/gasm-devkit/parser"
 )
@@ -21,6 +22,21 @@ func lintSrc(t *testing.T, src string) []Diagnostic {
 	return File(f, Config{Arch: arch.AMD64})
 }

+// lintArchFile parses and lints src under a, then hands the same file to
+// assemble so the assertion is pinned against the encoder: a kernel the
+// linter reasons about must also be one the encoder accepts.
+func lintArchFile(t *testing.T, filename, src string, a arch.Arch, assemble func(*ast.File) (*asm.Image, error)) []Diagnostic {
+	t.Helper()
+	f, errs := parser.Parse(filename, src)
+	if len(errs) > 0 {
+		t.Fatalf("parse: %v", errs)
+	}
+	if _, err := assemble(f); err != nil {
+		t.Fatalf("encoder rejects the kernel: %v", err)
+	}
+	return File(f, Config{Arch: a})
+}
+
 // lintSrcArch lints src under the architecture inferred from filename.
 func lintSrcArch(t *testing.T, filename, src string) []Diagnostic {
 	t.Helper()
@@ -262,6 +278,138 @@ loop:
 	}
 }

+func TestRiscvBranchFamilyRegistersLabels(t *testing.T) {
+	// Every riscv64 pseudo-branch that references a label must register that
+	// reference: the reversed branches BGT/BGTU/BLE/BLEU (GOROOT's
+	// memmove_riscv64 branches with BGTU) and a label named like the ZERO
+	// register alias (GOROOT's memclr_riscv64 carries a label named zero;
+	// ZERO is the ABI name of X0) must not be reported unused.
+	diags := lintSrcArch(t, "f_riscv64.s", `
+#include "textflag.h"
+TEXT ·f(SB), NOSPLIT, $0
+	BGTU  X10, X11, backward
+	BGT   X10, X11, zero
+	BLE   X10, X11, one
+	BLEU  X10, X11, two
+	BEQZ  X10, zero
+	BNEZ  X10, one
+	JMP   two
+backward:
+	RET
+zero:
+	RET
+one:
+	RET
+two:
+	RET
+`)
+	if codes(diags)[CodeUnusedLabel] != 0 {
+		t.Fatalf("branch-referenced labels must not be flagged unused: %+v", diags)
+	}
+	if codes(diags)[CodeUndefinedLabel] != 0 {
+		t.Fatalf("defined labels must resolve: %+v", diags)
+	}
+
+	// A branch to a truly undefined label still reports.
+	diags = lintSrcArch(t, "f_riscv64.s", `
+#include "textflag.h"
+TEXT ·f(SB), NOSPLIT, $0
+	BGT X10, X11, nowhere
+	RET
+`)
+	if codes(diags)[CodeUndefinedLabel] != 1 {
+		t.Fatalf("undefined branch target must be flagged: %+v", diags)
+	}
+
+	// A register-indirect JALR is not a label reference.
+	diags = lintSrcArch(t, "f_riscv64.s", `
+#include "textflag.h"
+TEXT ·f(SB), NOSPLIT, $0
+	JALR X1
+	RET
+`)
+	if codes(diags)[CodeUndefinedLabel] != 0 {
+		t.Fatalf("register operand of JALR is not a label: %+v", diags)
+	}
+}
+
+func TestLoong64BranchFamilyRegistersLabels(t *testing.T) {
+	// The loong64 jumps and single-register branches (JAL, B, BL, BEQZ/BNEZ,
+	// BFPT/BFPF) all reference their label from the last operand; GOROOT's
+	// own basic kernels tail-call with JAL, so the reference must register.
+	diags := lintSrcArch(t, "f_loong64.s", `
+#include "textflag.h"
+TEXT ·f(SB), NOSPLIT, $0
+	BEQZ R4, fin
+	BNEZ R4, fin
+	BLTZ R4, fin
+	JAL  fin
+	BL   fin
+	B    fin
+	RET
+fin:
+	RET
+`)
+	if codes(diags)[CodeUnusedLabel] != 0 {
+		t.Fatalf("branch-referenced labels must not be flagged unused: %+v", diags)
+	}
+	if codes(diags)[CodeUndefinedLabel] != 0 {
+		t.Fatalf("defined labels must resolve: %+v", diags)
+	}
+}
+
+// TestBranchFamiliesAssemble pins the lint branch sets to the encoder: every
+// mnemonic the linter classifies as a riscv64 or loong64 label branch must be
+// a branch the encoder actually assembles, with the label in the last
+// operand.  If the encoder gains or renames a branch, this test fails and the
+// set follows it.
+func TestBranchFamiliesAssemble(t *testing.T) {
+	riscvForms := map[string]string{}
+	for m := range riscvBranches {
+		riscvForms[m] = m + " X10, X11, tgt"
+	}
+	for _, m := range []string{"BEQZ", "BNEZ", "BLTZ", "BGEZ", "BLEZ", "BGTZ"} {
+		riscvForms[m] = m + " X10, tgt"
+	}
+	riscvForms["JMP"] = "JMP tgt"
+	riscvForms["JAL"] = "JAL tgt"
+
+	loongForms := map[string]string{}
+	for _, m := range []string{"BEQ", "BNE", "BLT", "BGE", "BLTU", "BGEU"} {
+		loongForms[m] = m + " R4, R5, tgt"
+	}
+	for _, m := range []string{"BEQZ", "BNEZ", "BLTZ", "BGEZ", "BLEZ", "BGTZ", "BFPT", "BFPF"} {
+		loongForms[m] = m + " R4, tgt"
+	}
+	loongForms["JMP"] = "JMP tgt"
+	loongForms["B"] = "B tgt"
+	loongForms["JAL"] = "JAL tgt"
+	loongForms["BL"] = "BL tgt"
+
+	for m, form := range riscvForms {
+		src := "#include \"textflag.h\"\n" +
+			"TEXT ·f(SB), NOSPLIT, $0\n" +
+			"\t" + form + "\n" +
+			"tgt:\n" +
+			"\tRET\n"
+		diags := lintArchFile(t, "f_riscv64.s", src, arch.RISCV, asm.AssembleFileRISCV)
+		if codes(diags)[CodeUnusedLabel] != 0 || codes(diags)[CodeUndefinedLabel] != 0 {
+			t.Errorf("riscv64 %s: label reference not registered: %+v", m, diags)
+		}
+	}
+	for m, form := range loongForms {
+		src := "#include \"textflag.h\"\n" +
+			"TEXT ·f(SB), NOSPLIT, $0\n" +
+			"\t" + form + "\n" +
+			"tgt:\n" +
+			"\tRET\n"
+		diags := lintArchFile(t, "f_loong64.s", src, arch.LOONG64, asm.AssembleFileLOONG64)
+		if codes(diags)[CodeUnusedLabel] != 0 || codes(diags)[CodeUndefinedLabel] != 0 {
+			t.Errorf("loong64 %s: label reference not registered: %+v", m, diags)
+		}
+	}
+}
+
 func TestInvalidTextflag(t *testing.T) {
 	diags := lintSrc(t, `
 #include "textflag.h"
@@ -413,6 +561,72 @@ TEXT ·f(SB), NOSPLIT, $0
 	}
 }

+func TestRegisterWidthShiftCount(t *testing.T) {
+	// The shift and rotate count lives in CL by ISA definition (the D2/D3
+	// group encodes the count outside the ModRM register field), so the count
+	// operand is 8-bit no matter how wide the data is: SHLQ CL, AX is the
+	// normal spelling of a 64-bit shift.  The data operand keeps its check.
+	diags := lintSrc(t, `
+#include "textflag.h"
+TEXT ·f(SB), NOSPLIT, $0
+	SHLQ CL, AX
+	SHRL CL, BX
+	SARQ CL, CX
+	ROLL CL, DX
+	RORQ CL, R8
+	RCLL CL, R9
+	RCRQ CL, R10
+	MOVQ CL, R10
+	RET
+`)
+	if codes(diags)[CodeRegisterWidthMismatch] != 1 {
+		t.Fatalf("only the MOVQ CL data move must be flagged, got %+v", diags)
+	}
+}
+
+func TestRegisterWidthSetcc(t *testing.T) {
+	// A SETcc stores one byte whichever condition it tests (0F 90+cc), so
+	// SETNE AL is always right and the trailing letters of SETEQ, SETPL and
+	// SETLS are condition codes, not width suffixes.
+	diags := lintSrc(t, `
+#include "textflag.h"
+TEXT ·f(SB), NOSPLIT, $0
+	CMPQ  AX, BX
+	SETNE AL
+	SETEQ AL
+	SETPL AL
+	SETLS AL
+	SETCC (BX)
+	SETGE (R8)
+	RET
+`)
+	if codes(diags)[CodeRegisterWidthMismatch] != 0 {
+		t.Fatalf("SETcc destinations are 8-bit by definition: %+v", diags)
+	}
+	if codes(diags)[CodeUnknownInstr] != 0 {
+		t.Fatalf("every SETcc spelling must be known: %+v", diags)
+	}
+}
+
+func TestRegisterWidthFrameNames(t *testing.T) {
+	// GOROOT's BSD syscall stubs carry frame parameters whose names collide
+	// with byte register names (kevent's ch and nch): MOVQ ch+8(FP), SI is a
+	// frame reference, not the CH register.
+	diags := lintSrc(t, `
+#include "textflag.h"
+TEXT ·kevent(SB), NOSPLIT, $0-36
+	MOVL kq+0(FP), DI
+	MOVQ ch+8(FP), SI
+	MOVL nch+16(FP), DX
+	MOVQ ev+24(FP), R10
+	MOVQ AX, ret+32(FP)
+	RET
+`)
+	if codes(diags)[CodeRegisterWidthMismatch] != 0 {
+		t.Fatalf("frame and static symbol names are not registers: %+v", diags)
+	}
+}
+
 func TestNonportableRegisterName(t *testing.T) {
 	diags := lintSrc(t, `
 #include "textflag.h"