From d75e6bcae657519ed13f17535a585348063ebf63 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Petr=20Balv=C3=ADn?= Date: Sat, 29 Aug 2026 13:36:28 +0200 Subject: [PATCH] feat(lint): abi0 register-args rule and go-asm width model --- lint/abi.go | 49 ++++++++++++++++++---- lint/abi0.go | 92 +++++++++++++++++++++++++++++++++++++++++ lint/lint.go | 47 ++++++++++++++------- lint/lint_test.go | 76 ++++++++++++++++++++++++++++++++++ testdata/sample_amd64.s | 1 + 5 files changed, 244 insertions(+), 21 deletions(-) create mode 100644 lint/abi0.go diff --git a/lint/abi.go b/lint/abi.go index e2de396..6e65a56 100644 --- a/lint/abi.go +++ b/lint/abi.go @@ -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") { diff --git a/lint/abi0.go b/lint/abi0.go new file mode 100644 index 0000000..ac82eb1 --- /dev/null +++ b/lint/abi0.go @@ -0,0 +1,92 @@ +// Copyright (c) 2026 Petr Balvín (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 +} diff --git a/lint/lint.go b/lint/lint.go index aa5c9eb..6a8a014 100644 --- a/lint/lint.go +++ b/lint/lint.go @@ -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 } diff --git a/lint/lint_test.go b/lint/lint_test.go index 8eb7c50..3bfd70d 100644 --- a/lint/lint_test.go +++ b/lint/lint_test.go @@ -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) + } +} diff --git a/testdata/sample_amd64.s b/testdata/sample_amd64.s index a65294e..90e8a2c 100644 --- a/testdata/sample_amd64.s +++ b/testdata/sample_amd64.s @@ -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