feat(lint): abi0 register-args rule and go-asm width model

2026-08-29 13:36:28 +02:00
parent 6699ebd34f
commit d75e6bcae6
5 changed files with 244 additions and 21 deletions
@@ -25,18 +25,53 @@ func abiExpectedArgSize(doc string) (int64, bool) {
 	if sig == "" {
 		return 0, false
 	}
-	fset := token.NewFileSet()
-	f, err := parser.ParseFile(fset, "sig.go", "package p\n"+sig+" {}\n", 0)
-	if err != nil || len(f.Decls) == 0 {
-		return 0, false
-	}
-	fn, ok := f.Decls[0].(*ast.FuncDecl)
-	if !ok || fn.Type == nil {
+	fn, ok := parseSignature(sig)
+	if !ok || fn == nil {
 		return 0, false
 	}
 	return signatureSize(fn.Type.Params, fn.Type.Results)
 }

+// abiParamNames returns the parameter names declared by the `// func …`
+// signature in a doc comment, in declaration order.  Shared names
+// (`left, right []int32`) expand to one entry per name; a nameless parameter
+// (`[]int32`) contributes an empty placeholder so offsets stay aligned.
+func abiParamNames(doc string) ([]string, bool) {
+	sig := signatureLine(doc)
+	if sig == "" {
+		return nil, false
+	}
+	fn, ok := parseSignature(sig)
+	if !ok || fn == nil || fn.Type.Params == nil {
+		return nil, false
+	}
+	var names []string
+	for _, field := range fn.Type.Params.List {
+		if len(field.Names) == 0 {
+			names = append(names, "")
+			continue
+		}
+		for _, n := range field.Names {
+			names = append(names, n.Name)
+		}
+	}
+	return names, true
+}
+
+// parseSignature parses a `func …` line into a Go FuncDecl.
+func parseSignature(sig string) (*ast.FuncDecl, bool) {
+	fset := token.NewFileSet()
+	f, err := parser.ParseFile(fset, "sig.go", "package p\n"+sig+" {}\n", 0)
+	if err != nil || len(f.Decls) == 0 {
+		return nil, false
+	}
+	fn, ok := f.Decls[0].(*ast.FuncDecl)
+	if !ok || fn.Type == nil {
+		return nil, false
+	}
+	return fn, true
+}
+
 // signatureLine returns the first `func …` line from a doc comment, trimmed.
 func signatureLine(doc string) string {
 	for _, line := range strings.Split(doc, "\n") {
@@ -0,0 +1,92 @@
+// Copyright (c) 2026 Petr Balvín <opensource@petrbalvin.org> (https://petrbalvin.org)
+// SPDX-License-Identifier: BSD-3-Clause
+
+package lint
+
+import (
+	"fmt"
+	"strings"
+
+	"sourcedock.dev/petrbalvin/gasm-devkit/ast"
+)
+
+// checkABI0Args flags TEXT functions whose // func signature declares
+// parameters that are never read from the FP frame.  Under ABI0 every
+// argument lives on the stack and is addressed as name+offset(FP); a kernel
+// that never references a parameter's frame slot is almost certainly
+// consuming the caller's register contents instead, which produces
+// value-dependent garbage that only a direct-call differential test (not a
+// pipeline-level fuzz) can observe.
+//
+// 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 {
+	params, ok := abiParamNames(t.Doc)
+	if !ok || len(params) == 0 {
+		return nil
+	}
+	// Collect every FP frame slot the body references, by symbol name (e.g.
+	// "text" for text+0(FP)).  Frame reads and writes both count: writing a
+	// parameter slot is as wrong as ignoring it, but reads dominate, and the
+	// distinction is not worth the precision here.
+	frameRefs := make(map[string]bool)
+	for _, s := range t.Body {
+		in, ok := s.(*ast.Instr)
+		if !ok {
+			continue
+		}
+		for _, op := range in.Operands {
+			if op.Addr.Sym == nil || op.Addr.Sym.Pseudo != "FP" {
+				continue
+			}
+			frameRefs[strings.ToUpper(op.Addr.Sym.Name)] = true
+		}
+	}
+	if len(frameRefs) == 0 {
+		// No FP reference at all: the function ignores the frame entirely.
+		return []Diagnostic{{
+			Pos:      t.Keyword.Pos,
+			Severity: Warning,
+			Code:     CodeABI0RegisterArgs,
+			Message: fmt.Sprintf("function %q declares %d parameter(s) but never reads the FP frame; "+
+				"ABI0 passes arguments as name+offset(FP), not in registers", t.Name.Name, len(params)),
+		}}
+	}
+	var unread []string
+	for _, p := range params {
+		if p == "" {
+			continue
+		}
+		if !frameSlotCovers(frameRefs, p) {
+			unread = append(unread, p)
+		}
+	}
+	if len(unread) == 0 {
+		return nil
+	}
+	return []Diagnostic{{
+		Pos:      t.Keyword.Pos,
+		Severity: Warning,
+		Code:     CodeABI0RegisterArgs,
+		Message: fmt.Sprintf("function %q never reads parameter slot(s) %s from the FP frame; "+
+			"suspected register-args port bug (ABI0 arguments arrive as name+offset(FP))",
+			t.Name.Name, strings.Join(unread, ", ")),
+	}}
+}
+
+// frameSlotCovers reports whether any referenced FP slot belongs to the
+// parameter: the slot either spells the parameter name itself or the
+// compound form Go's ABI0 uses for multi-word types (swin_base, swin_len,
+// swin_cap for a parameter named swin).
+func frameSlotCovers(frameRefs map[string]bool, param string) bool {
+	p := strings.ToUpper(param)
+	if frameRefs[p] {
+		return true
+	}
+	for slot := range frameRefs {
+		if strings.HasPrefix(slot, p+"_") {
+			return true
+		}
+	}
+	return false
+}
@@ -77,6 +77,7 @@ const (
 	CodeInvalidFlag           = "invalid-textflag"
 	CodeStackImbalance        = "stack-imbalance"
 	CodeRegisterWidthMismatch = "register-width-mismatch"
+	CodeABI0RegisterArgs      = "abi0-register-args"
 )

 // knownTextFlags are the flags recognised by the Go assembler's textflag.h.
@@ -438,6 +439,16 @@ func lintText(t *ast.Text, tab *arch.Table, archKnown bool, cfg Config, macros m
 		}
 	}

+	// ABI0 argument-read check: every parameter the // func signature
+	// declares must be read from the frame (name+offset(FP)).  A kernel that
+	// declares parameters but never touches their frame slots is almost
+	// certainly reading its arguments from registers (AX, BX, …), which is
+	// the classic ABI0 port bug: Go pushes the arguments on the stack and
+	// the register content is whatever the caller left behind.
+	if cfg.Arch == arch.AMD64 && !cfg.Disable[CodeABI0RegisterArgs] && !hasMacro && instrCount > 0 {
+		out = append(out, checkABI0Args(t)...)
+	}
+
 	// FUNCDATA / PCDATA structural validation.
 	out = append(out, checkFuncdata(t, cfg)...)

@@ -730,9 +741,15 @@ func isSPReg(op *ast.Operand, a arch.Arch) bool {
 	return false
 }

-// checkRegisterWidth detects amd64 register-width mismatches: a Q-suffix
-// instruction (64-bit) using a 32-bit register, or an L/W/B-suffix
-// instruction using a 64-bit register.
+// 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
+// spellings in go tool asm), and AL–DH are the byte forms.  The width comes
+// from the opcode suffix, so an L/W operation over a canonical 64-bit name is
+// the normal, correct spelling — flagging it is pure noise on real kernels.
+// What remains worth flagging: a Q (64-bit) operation over a narrower spelled
+// 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 {
 	// Determine expected width from mnemonic suffix.
 	var expected int // 0=unknown, 8/4/2/1=bytes
@@ -757,28 +774,30 @@ func checkRegisterWidth(mnem string, ops []*ast.Operand) string {
 		if regWidth == 0 {
 			continue // not a register or unknown
 		}
-		if expected == 8 && regWidth == 4 {
-			return fmt.Sprintf("%s uses 32-bit register %s (expected 64-bit)", mnem, op.Addr.Sym.Name)
+		if expected == 8 && regWidth < 8 {
+			return fmt.Sprintf("%s uses %d-bit register %s (expected 64-bit)", mnem, regWidth*8, op.Addr.Sym.Name)
 		}
-		if expected == 4 && regWidth == 8 {
-			return fmt.Sprintf("%s uses 64-bit register %s (expected 32-bit)", mnem, op.Addr.Sym.Name)
+		if (expected == 4 || expected == 2) && regWidth == 1 {
+			return fmt.Sprintf("%s uses 8-bit register %s (expected a wider register)", mnem, op.Addr.Sym.Name)
 		}
 	}
 	return ""
 }

-// amd64RegWidth returns the width in bytes of an amd64 register name.
+// amd64RegWidth returns the width in bytes of an amd64 register name under
+// the Go assembler's naming model: the canonical word names (AX…SP, R8–R15)
+// are 64-bit, AL–DH are the 8-bit forms, and the R/E-prefixed spellings are
+// the gasm alias extension with their intuitive widths.
 func amd64RegWidth(name string) int {
 	switch name {
-	case "rax", "rbx", "rcx", "rdx", "rsi", "rdi", "rbp", "rsp",
-		"r8", "r9", "r10", "r11", "r12", "r13", "r14", "r15":
+	case "ax", "bx", "cx", "dx", "si", "di", "bp", "sp",
+		"r8", "r9", "r10", "r11", "r12", "r13", "r14", "r15",
+		"rax", "rbx", "rcx", "rdx", "rsi", "rdi", "rbp", "rsp":
 		return 8
-	case "eax", "ebx", "ecx", "edx", "esi", "edi", "ebp", "esp":
-		return 4
-	case "ax", "bx", "cx", "dx", "si", "di", "bp", "sp":
-		return 2
 	case "al", "bl", "cl", "dl", "ah", "bh", "ch", "dh":
 		return 1
+	case "eax", "ebx", "ecx", "edx", "esi", "edi", "ebp", "esp":
+		return 4
 	}
 	return 0
 }
@@ -335,3 +335,79 @@ TEXT ·f(SB), NOSPLIT, $0
 		t.Fatalf("correct width must not be flagged: %+v", diags)
 	}
 }
+
+func TestABI0RegisterArgs(t *testing.T) {
+	// A kernel with a // func signature whose parameters are never read from
+	// the FP frame: the classic register-args port bug.
+	diags := lintSrc(t, `
+// func kernel(text *byte, n int)
+TEXT ·kernel(SB), NOSPLIT, $0-16
+	MOVQ DI, R10
+	MOVQ R9, AX
+	RET
+`)
+	if codes(diags)[CodeABI0RegisterArgs] != 1 {
+		t.Fatalf("want one abi0-register-args, got %+v", diags)
+	}
+
+	// Reading every parameter from the frame is correct.
+	diags = lintSrc(t, `
+// func kernel(text *byte, n int)
+TEXT ·kernel(SB), NOSPLIT, $0-16
+	MOVQ text+0(FP), AX
+	MOVQ n+8(FP), BX
+	RET
+`)
+	if codes(diags)[CodeABI0RegisterArgs] != 0 {
+		t.Fatalf("frame-reading kernel must not be flagged: %+v", diags)
+	}
+
+	// Only a result write to FP, no parameter read: still flagged.
+	diags = lintSrc(t, `
+// func top(sa []int32) int
+TEXT ·top(SB), NOSPLIT, $0-24
+	MOVQ SI, R10
+	MOVQ R10, ret+16(FP)
+	RET
+`)
+	if codes(diags)[CodeABI0RegisterArgs] != 1 {
+		t.Fatalf("result-only FP write must still be flagged: %+v", diags)
+	}
+
+	// No signature: stay silent.
+	diags = lintSrc(t, `
+TEXT ·bare(SB), NOSPLIT, $0-16
+	MOVQ DI, AX
+	RET
+`)
+	if codes(diags)[CodeABI0RegisterArgs] != 0 {
+		t.Fatalf("signature-less function must not be flagged: %+v", diags)
+	}
+}
+
+func TestRegisterWidthCanonicalNames(t *testing.T) {
+	// Canonical Go asm names with an L operation: the correct spelling for a
+	// 32-bit operation, never a width mismatch.
+	diags := lintSrc(t, `
+#include "textflag.h"
+TEXT ·f(SB), NOSPLIT, $0
+	MOVL (BX)(R9*4), SI
+	XORL R9, R9
+	MOVL 128(BX)(R9*4), R12
+	RET
+`)
+	if codes(diags)[CodeRegisterWidthMismatch] != 0 {
+		t.Fatalf("canonical 64-bit names with L ops must not be flagged: %+v", diags)
+	}
+
+	// A byte register in an L operation stays a mismatch.
+	diags = lintSrc(t, `
+#include "textflag.h"
+TEXT ·f(SB), NOSPLIT, $0
+	MOVL AL, DX
+	RET
+`)
+	if codes(diags)[CodeRegisterWidthMismatch] != 1 {
+		t.Fatalf("byte register in L op must be flagged: %+v", diags)
+	}
+}
@@ -45,6 +45,7 @@ vec1done:
 // func decodeFixedO1AVX512(samples []int32, residual []int32)
 TEXT ·decodeFixedO1AVX512(SB), NOSPLIT, $0-48
 	MOVQ samples_base+0(FP), SI
+	MOVQ residual_base+16(FP), DI
 	VPBROADCASTD AX, Z15
 	VMOVDQU32 (DI)(AX*1), Z0
 	VALIGND   $15, Z9, Z0, Z1