From 69dcbec8effa1736dcdc4ba93744bdf6e8f20f2e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Petr=20Balv=C3=ADn?= Date: Fri, 2 Oct 2026 00:40:54 +0200 Subject: [PATCH] feat(lint): eleven new rules over directives, data and addressing Assisted-by: GLM 5.3 --- docs/CLI.md | 17 ++- docs/man/gasm-lint.1 | 70 ++++++++- lint/abi.go | 52 +++++++ lint/addressing.go | 63 ++++++++ lint/addressing_test.go | 117 +++++++++++++++ lint/data.go | 172 +++++++++++++++++++++ lint/data_test.go | 204 +++++++++++++++++++++++++ lint/directives.go | 113 ++++++++++++++ lint/directives_test.go | 324 ++++++++++++++++++++++++++++++++++++++++ lint/lint.go | 153 +++++++++++++++++-- lint/vector.go | 70 +++++++++ lint/vector_test.go | 95 ++++++++++++ 12 files changed, 1431 insertions(+), 19 deletions(-) create mode 100644 lint/addressing.go create mode 100644 lint/addressing_test.go create mode 100644 lint/data.go create mode 100644 lint/data_test.go create mode 100644 lint/directives.go create mode 100644 lint/directives_test.go create mode 100644 lint/vector.go create mode 100644 lint/vector_test.go diff --git a/docs/CLI.md b/docs/CLI.md index 93ff3a7..cc55edc 100644 --- a/docs/CLI.md +++ b/docs/CLI.md @@ -131,8 +131,11 @@ Rules: `unknown-instruction`, `operand-count`, `undefined-label`, `abi-argsize`, `unreachable-code`, `register-clobber`, `funcdata-pcdata`, `unused-label`, `invalid-textflag`, `stack-imbalance`, `register-width-mismatch`, `abi0-register-args`, -`nonportable-register-name`, `unencodable-instruction` and -`reserved-register-write`. +`nonportable-register-name`, `unencodable-instruction`, +`reserved-register-write`, `missing-argsize`, `noframe-frame-size`, +`unnamed-fp-reference`, `hardware-sp-addressing`, `vex-sse-mixing`, +`unnamed-result`, `data-width`, `data-value-overflow`, +`data-string-width`, `data-without-globl` and `data-exceeds-globl`. ```sh gasm lint kernel_amd64.s @@ -455,14 +458,18 @@ Provides: completion, hover, document symbols, push and pull diagnostics, semantic tokens, go-to-definition, find references, rename, document formatting, inlay hints, code actions, signature help, document highlights, workspace symbol search, #include document links, and folding ranges for -function bodies. Definition, references and rename work across every open +function bodies. Completion offers the architecture's registers and +instructions, the TEXT/DATA/GLOBL directives, the annotation and padding +pseudo-ops (PCALIGN, FUNCDATA, PCDATA, the BYTE family) and the textflag +constants; hover documents mnemonics, registers, pseudo-registers and the +directives. Definition, references and rename work across every open document and the wider workspace on disk: the server indexes the `.s` files under the workspace root that the editor has never opened, an open buffer always shadows its disk copy, and watched-file events together with a per-query freshness check keep the index current. The quick fixes add the missing `#include "textflag.h"`, set the TEXT argument area to the size the -`// func` signature implies, add a missing `RET`, and remove an unused -label. +`// func` signature implies, declare that area when it is omitted entirely, +add a missing `RET`, and remove an unused label. ## version diff --git a/docs/man/gasm-lint.1 b/docs/man/gasm-lint.1 index f5f59e9..b6adde5 100644 --- a/docs/man/gasm-lint.1 +++ b/docs/man/gasm-lint.1 @@ -1,4 +1,4 @@ -.TH GASM-LINT 1 "2026-09-19" "gasm" "User Commands" +.TH GASM-LINT 1 "2026-10-01" "gasm" "User Commands" .SH NAME gasm-lint \- run the static checks over assembly files .SH SYNOPSIS @@ -27,7 +27,11 @@ A jump target names no label in the function. Two labels in one function share a name. .TP .B missing-ret -The function can fall off its end without a terminator. +The function has no RET, or has one but can still fall off its end: the +last instruction is neither RET nor an unconditional branch, so execution +continues into the next TEXT. Trailing labels, GC annotations and +padding are skipped, and traps (INT on amd64, BRK on arm64) count as +terminators. .TP .B missing-textflag-include TEXT flags are used without including textflag.h. @@ -56,8 +60,9 @@ FUNCDATA and PCDATA indices are malformed. A label no jump reaches. .TP .B invalid-textflag -An unknown TEXT or GLOBL flag, reported one flag at a time; numeric flags -are accepted as textflag.h constants. +An unknown TEXT or GLOBL flag, or a known one misplaced (NOSPLIT on +GLOBL, RODATA on TEXT), reported one flag at a time; numeric flags are +accepted as textflag.h constants. .TP .B stack-imbalance The function does not restore the stack pointer on every path. @@ -85,6 +90,63 @@ yet (amd64). .TP .B reserved-register-write A write to the register the runtime reserves (arm64 R18). +.TP +.B missing-argsize +A TEXT without NOSPLIT omits the argument area its +.B //\ function +signature implies; the object then records ArgsSizeUnknown and go vet +skips the size check. +.TP +.B noframe-frame-size +The NOFRAME flag against a positive frame size; NOFRAME is the +zero-frame spelling, and the negative frames of the arm64 BSD syscall +stubs are exempt. +.TP +.B unnamed-fp-reference +A numeric offset against FP, written +.IR 0(FP) ; +the assembler requires +.IR name+offset(FP) . +.TP +.B hardware-sp-addressing +A negative numeric offset against +.IR (SP) , +which addresses the hardware stack pointer rather than the named +virtual frame; the runtime's deliberate hardware-SP references all +carry positive offsets. +.TP +.B vex-sse-mixing +VEX-encoded instructions and legacy SSE on X registers in one function; +each switch between the encodings pays a transition penalty +(informational). +.TP +.B unnamed-result +A result slot addressed with the generic +.I ret +spelling although the +.B //\ function +signature names the result (a hint). +.TP +.B data-width +A DATA width its value cannot take: 1, 2, 4 or 8 for an integer, 4 or 8 +for a float, and the suffix itself is never optional; a string accepts +any width. +.TP +.B data-value-overflow +An integer initialiser wider than its DATA field; both assemblers +truncate it silently to the low bytes. +.TP +.B data-string-width +A string initialiser longer than its DATA field; both assemblers reject +it. +.TP +.B data-without-globl +A DATA initialiser whose symbol has no GLOBL declaration in the file; +gasm asm requires the pairing, go tool asm sizes the symbol implicitly. +.TP +.B data-exceeds-globl +A DATA initialiser writing past the GLOBL-declared size; go tool asm +grows the symbol silently, gasm asm rejects the file. .SH OPTIONS .TP .B \-disable \fIcodes\fR diff --git a/lint/abi.go b/lint/abi.go index bf517aa..1cfaa6a 100644 --- a/lint/abi.go +++ b/lint/abi.go @@ -67,6 +67,58 @@ func abiParamNames(doc string) ([]string, bool) { return names, true } +// resultSlot is one named result of a // func signature and the byte range +// its ABI0 stack slot occupies, relative to FP. +type resultSlot struct { + name string + off int64 + size int64 +} + +// abiResultSlots lays out the named result slots of the `// func …` +// signature in a doc comment: parameters first, then the result area on a +// word (8-byte) boundary, matching the ABI0 stack layout signatureSize +// computes. A nameless result field contributes no slot; it is the field +// the generic ret spelling documents. It returns ok=false when there is no +// parseable signature or a type size cannot be determined. +func abiResultSlots(doc string) ([]resultSlot, bool) { + sig := signatureLine(doc) + if sig == "" { + return nil, false + } + fn, ok := parseSignature(sig) + if !ok || fn == nil { + return nil, false + } + paramsSize, _, pok := fieldsSizeAlign(fn.Type.Params) + if !pok { + return nil, false + } + results := fn.Type.Results + if results == nil || len(results.List) == 0 { + return nil, false + } + off := int64(alignUp(paramsSize, 8)) + var slots []resultSlot + for _, field := range results.List { + es, ea, fieldOK := typeSizeAlign(field.Type) + if !fieldOK { + return nil, false + } + n := len(field.Names) + if n == 0 { + off = int64(alignUp(int(off), ea)) + int64(es) + continue + } + for _, nm := range field.Names { + off = int64(alignUp(int(off), ea)) + slots = append(slots, resultSlot{name: nm.Name, off: off, size: int64(es)}) + off += int64(es) + } + } + return slots, true +} + // parseSignature parses a `func …` line into a Go FuncDecl. func parseSignature(sig string) (*ast.FuncDecl, bool) { fset := token.NewFileSet() diff --git a/lint/addressing.go b/lint/addressing.go new file mode 100644 index 0000000..eafc41c --- /dev/null +++ b/lint/addressing.go @@ -0,0 +1,63 @@ +// Copyright (c) 2026 Petr Balvín (https://petrbalvin.org) +// SPDX-License-Identifier: BSD-3-Clause + +package lint + +import ( + "fmt" + "strings" + + "sourcedock.dev/petrbalvin/gasm-sdk/ast" +) + +// scanAddressing flags the two pseudo-register spellings the language +// reference (docs/asm/OPERANDS.md) calls its sharpest edge: a numeric offset +// against FP names no symbol, and a negative numeric offset against SP +// addresses the hardware stack pointer rather than the virtual frame. +func scanAddressing(t *ast.Text, cfg Config) []Diagnostic { + var out []Diagnostic + for _, s := range t.Body { + in, ok := s.(*ast.Instr) + if !ok { + continue + } + for _, op := range in.Operands { + if op.Kind != ast.OpAddr { + continue + } + // FP is a pseudo-register, never a base register of a memory + // operand; a Base of FP only arises from the numeric form + // 0(FP), which the parser routes through memory parsing. The + // toolchain rejects it with "cannot reference FP without a + // symbol" and gasm with "unknown base register FP". + if strings.EqualFold(op.Addr.Base, "FP") && !cfg.Disable[CodeUnnamedFPRef] { + out = append(out, Diagnostic{ + Pos: op.Pos, + Severity: Error, + Code: CodeUnnamedFPRef, + Message: "FP is referenced without a symbol; the assembler requires name+offset(FP)", + }) + continue + } + // A numeric offset against (SP) addresses the hardware stack + // pointer. Positive offsets are the deliberate runtime idiom + // for reading caller frames, but a negative offset is exactly + // the local-frame spelling with the name missing: x-8(SP) and + // -8(SP) are one character apart and mean different registers. + // Only the negative form is flagged, which keeps the rule + // silent across GOROOT's 140-odd deliberate hardware-SP + // references (all positive) while catching the mistyped local. + if strings.EqualFold(op.Addr.Base, "SP") && op.Addr.Sym == nil && + op.Addr.HasOff && op.Addr.Offset < 0 && !cfg.Disable[CodeHardwareSP] { + out = append(out, Diagnostic{ + Pos: op.Pos, + Severity: Warning, + Code: CodeHardwareSP, + Message: fmt.Sprintf("%d(SP) addresses the hardware stack pointer, not the virtual frame; "+ + "a frame local is spelled x%d(SP)", op.Addr.Offset, op.Addr.Offset), + }) + } + } + } + return out +} diff --git a/lint/addressing_test.go b/lint/addressing_test.go new file mode 100644 index 0000000..9a7fb15 --- /dev/null +++ b/lint/addressing_test.go @@ -0,0 +1,117 @@ +// Copyright (c) 2026 Petr Balvín (https://petrbalvin.org) +// SPDX-License-Identifier: BSD-3-Clause + +package lint + +import ( + "testing" + + "sourcedock.dev/petrbalvin/gasm-sdk/arch" + "sourcedock.dev/petrbalvin/gasm-sdk/parser" +) + +// TestUnnamedFPReference pins the toolchain contract "cannot reference FP +// without a symbol": a numeric offset against FP parses as a memory operand +// with FP in the base, and both assemblers reject it at assembly time. +func TestUnnamedFPReference(t *testing.T) { + diags := lintSrc(t, ` +#include "textflag.h" +TEXT ·f(SB), NOSPLIT, $0-8 + MOVQ 0(FP), AX + RET +`) + if codes(diags)[CodeUnnamedFPRef] != 1 { + t.Fatalf("want one unnamed-fp-reference, got %+v", diags) + } + for _, d := range diags { + if d.Code == CodeUnnamedFPRef && d.Severity != Error { + t.Fatalf("unnamed FP reference must be error severity: %+v", d) + } + } + + // The named spelling is the language's requirement. + diags = lintSrc(t, ` +#include "textflag.h" +TEXT ·f(SB), NOSPLIT, $0-8 + MOVQ x+0(FP), AX + RET +`) + if codes(diags)[CodeUnnamedFPRef] != 0 { + t.Fatalf("named FP reference must not be flagged: %+v", diags) + } + + // -disable silences the rule. + f, _ := parser.Parse("t_amd64.s", "#include \"textflag.h\"\nTEXT ·f(SB), NOSPLIT, $0-8\n\tMOVQ 0(FP), AX\n\tRET\n") + diags = File(f, Config{Arch: arch.AMD64, Disable: map[string]bool{CodeUnnamedFPRef: true}}) + if codes(diags)[CodeUnnamedFPRef] != 0 { + t.Fatalf("disabled rule must stay silent: %+v", diags) + } +} + +// TestHardwareSPAddressing pins the one-character edge between the virtual +// frame and the real stack pointer: a negative unnamed offset against (SP) +// is the local-frame spelling with the name missing, while the runtime's +// deliberate hardware-SP references all carry positive offsets. +func TestHardwareSPAddressing(t *testing.T) { + diags := lintSrc(t, ` +#include "textflag.h" +TEXT ·f(SB), NOSPLIT, $16 + MOVQ $1, -8(SP) + RET +`) + if codes(diags)[CodeHardwareSP] != 1 { + t.Fatalf("want one hardware-sp-addressing, got %+v", diags) + } + + // The named local in the virtual frame, the intended spelling. + diags = lintSrc(t, ` +#include "textflag.h" +TEXT ·f(SB), NOSPLIT, $16 + MOVQ $1, x-8(SP) + RET +`) + if codes(diags)[CodeHardwareSP] != 0 { + t.Fatalf("named SP local must not be flagged: %+v", diags) + } + + // Positive hardware-SP offsets are the runtime's caller-frame idiom. + diags = lintSrc(t, ` +#include "textflag.h" +TEXT ·f(SB), NOSPLIT, $0 + MOVQ 8(SP), AX + RET +`) + if codes(diags)[CodeHardwareSP] != 0 { + t.Fatalf("positive hardware-SP offset must not be flagged: %+v", diags) + } + + // The bare register is not a memory reference. + diags = lintSrc(t, ` +#include "textflag.h" +TEXT ·f(SB), NOSPLIT, $0 + MOVQ SP, AX + RET +`) + if codes(diags)[CodeHardwareSP] != 0 { + t.Fatalf("bare SP register must not be flagged: %+v", diags) + } + + // The rule is architecture-neutral: riscv64 numeric (SP) parses the + // same way (the parser documents 0(SP) as a RISC-V shape). + diags = lintSrcArch(t, "f_riscv64.s", ` +#include "textflag.h" +TEXT ·f(SB), NOSPLIT, $16 + MOV $1, -8(SP) + RET +`) + if codes(diags)[CodeHardwareSP] != 1 { + t.Fatalf("want one hardware-sp-addressing on riscv64, got %+v", diags) + } + + // -disable silences the rule. + f, _ := parser.Parse("t_amd64.s", "#include \"textflag.h\"\nTEXT ·f(SB), NOSPLIT, $16\n\tMOVQ $1, -8(SP)\n\tRET\n") + diags = File(f, Config{Arch: arch.AMD64, Disable: map[string]bool{CodeHardwareSP: true}}) + if codes(diags)[CodeHardwareSP] != 0 { + t.Fatalf("disabled rule must stay silent: %+v", diags) + } +} diff --git a/lint/data.go b/lint/data.go new file mode 100644 index 0000000..8df3bcc --- /dev/null +++ b/lint/data.go @@ -0,0 +1,172 @@ +// Copyright (c) 2026 Petr Balvín (https://petrbalvin.org) +// SPDX-License-Identifier: BSD-3-Clause + +package lint + +import ( + "fmt" + "strconv" + + "sourcedock.dev/petrbalvin/gasm-sdk/ast" +) + +// checkDataDecls validates the DATA and GLOBL structure of a file: the +// initialiser widths the language allows, values that actually fit them, and +// the GLOBL declaration that sizes each initialised symbol. Every check is +// calibrated against both assemblers: what go tool asm and gasm asm each +// reject or silently accept decides the severity, and the differences are +// spelled out in the message. +func checkDataDecls(f *ast.File, cfg Config) []Diagnostic { + // The GLOBL registry: every declared symbol keyed by package, name and + // static marker. Both assemblers pair DATA to GLOBL by name regardless + // of order, so the registry is built before any DATA is checked. + type globlDecl struct { + size int64 + hasSize bool + } + globls := map[string]globlDecl{} + for _, d := range f.Decls { + g, ok := d.(*ast.Globl) + if !ok || g.Name == nil || g.Name.Pseudo != "SB" { + continue + } + gd := globlDecl{} + if g.Size != nil && g.Size.Imm.HasVal { + gd.size, gd.hasSize = g.Size.Imm.Val, true + } + globls[dataSymKey(g.Name)] = gd + } + + var out []Diagnostic + for _, d := range f.Decls { + dd, ok := d.(*ast.Data) + if !ok || dd.Name == nil || dd.Name.Pseudo != "SB" { + continue + } + name := dd.Name.Name + if dd.Name.Static { + name += "<>" + } + w := int64(dd.Width) + + // Width. An integer initialiser allows exactly 1, 2, 4 or 8, a + // float needs its full 4 or 8 IEEE-754 bytes, and a string allows + // any width (its bytes are zero-padded to the field, which GOROOT's + // own messages rely on, /84 and all). The suffix itself is never + // optional. Both assemblers reject the rest (go tool asm: "bad int + // size for DATA argument", "bad float size", "expect /size"). + isFloat := dd.Value != nil && dd.Value.Imm.Float != "" && !dd.Value.Imm.HasVal + isStr := dd.Value != nil && dd.Value.Imm.Str != "" && !dd.Value.Imm.HasVal + if !cfg.Disable[CodeDataWidth] { + var msg string + switch { + case w == 0: + msg = "DATA requires a /width suffix" + case isFloat && w != 4 && w != 8: + msg = fmt.Sprintf("DATA width %d cannot hold a floating-point value, want 4 or 8", w) + case !isFloat && !isStr && w != 1 && w != 2 && w != 4 && w != 8: + msg = fmt.Sprintf("DATA width must be 1, 2, 4 or 8 for an integer value, got %d", w) + } + if msg != "" { + out = append(out, Diagnostic{ + Pos: dd.Keyword.Pos, + End: dd.Keyword.End, + Severity: Error, + Code: CodeDataWidth, + Message: msg, + }) + continue + } + } + + // Value fit. An integer wider than the field is silently truncated + // by both assemblers, and a string longer than the field is rejected + // by both (go tool asm: "bad string size"; a shorter one is + // zero-padded, which is the documented layout, not a defect). + if v := dd.Value; v != nil { + if v.Imm.Str != "" && !v.Imm.HasVal && !cfg.Disable[CodeDataStringWidth] { + if text, err := strconv.Unquote(v.Imm.Str); err == nil && int64(len(text)) > w { + out = append(out, Diagnostic{ + Pos: dd.Keyword.Pos, + End: dd.Keyword.End, + Severity: Error, + Code: CodeDataStringWidth, + Message: fmt.Sprintf("string of %d bytes does not fit DATA width %d", len(text), w), + }) + } + } + if v.Imm.HasVal && !cfg.Disable[CodeDataValueOverflow] { + val := v.Imm.Val + if v.Imm.Neg { + val = -val + } + if lo, hi := dataValueRange(w); val < lo || val > hi { + out = append(out, Diagnostic{ + Pos: dd.Keyword.Pos, + End: dd.Keyword.End, + Severity: Warning, + Code: CodeDataValueOverflow, + Message: fmt.Sprintf("DATA value %d does not fit width %d and is silently truncated: "+ + "both assemblers keep only the low %d bytes", val, w, w), + }) + } + } + } + + // The GLOBL pairing: gasm requires a matching GLOBL and bounds every + // initialiser by its size, while go tool asm sizes an unpaired symbol + // implicitly and grows a too-small one silently, so the declared size + // stops matching the data. + g, declared := globls[dataSymKey(dd.Name)] + if !declared { + if !cfg.Disable[CodeDataNoGlobl] { + out = append(out, Diagnostic{ + Pos: dd.Keyword.Pos, + End: dd.Keyword.End, + Severity: Warning, + Code: CodeDataNoGlobl, + Message: fmt.Sprintf("DATA initialiser for %q has no GLOBL declaration in this file; "+ + "gasm asm requires one, go tool asm sizes the symbol implicitly", name), + }) + } + continue + } + if g.hasSize && !cfg.Disable[CodeDataExceedsGlobl] { + if end := dd.Name.Offset + w; dd.Name.Offset < 0 || end > g.size { + out = append(out, Diagnostic{ + Pos: dd.Keyword.Pos, + End: dd.Keyword.End, + Severity: Warning, + Code: CodeDataExceedsGlobl, + Message: fmt.Sprintf("DATA %s+%d/%d exceeds the GLOBL size %d; "+ + "go tool asm grows the symbol silently, gasm asm rejects the file", + name, dd.Name.Offset, w, g.size), + }) + } + } + } + return out +} + +// dataSymKey identifies a data symbol within a file: package prefix, base +// name and the file-local <> marker, the same identity both assemblers pair +// DATA and GLOBL by. +func dataSymKey(sym *ast.Symbol) string { + key := sym.Pkg + "\x00" + sym.Name + if sym.Static { + key += "\x00static" + } + return key +} + +// dataValueRange returns the closed range of integers an initialiser of +// width w bytes round-trips: the low half of the unsigned range is read back +// as negative, so -2^(8w-1) through 2^(8w)-1 all survive the truncation. At +// width 8 every int64 is in range, and the unsigned bound is not +// representable, so the signed extremes stand in. +func dataValueRange(w int64) (lo, hi int64) { + if w == 8 { + return -1 << 63, 1<<63 - 1 + } + return -(1 << (8*w - 1)), (1 << (8 * w)) - 1 +} diff --git a/lint/data_test.go b/lint/data_test.go new file mode 100644 index 0000000..b29a29a --- /dev/null +++ b/lint/data_test.go @@ -0,0 +1,204 @@ +// Copyright (c) 2026 Petr Balvín (https://petrbalvin.org) +// SPDX-License-Identifier: BSD-3-Clause + +package lint + +import ( + "testing" + + "sourcedock.dev/petrbalvin/gasm-sdk/arch" + "sourcedock.dev/petrbalvin/gasm-sdk/parser" +) + +func TestDataWidth(t *testing.T) { + // Both assemblers reject the integer widths beyond 1, 2, 4 and 8 (go + // tool asm: "bad int size for DATA argument: 3"). + diags := lintSrc(t, ` +#include "textflag.h" +DATA ·tab+0(SB)/3, $0x010203 +GLOBL ·tab(SB), RODATA, $8 +`) + if codes(diags)[CodeDataWidth] != 1 { + t.Fatalf("want one data-width, got %+v", diags) + } + for _, d := range diags { + if d.Code == CodeDataWidth && d.Severity != Error { + t.Fatalf("bad width must be error severity: %+v", d) + } + } + + // The missing suffix is not a default ("expect /size"). + diags = lintSrc(t, ` +#include "textflag.h" +DATA ·tab+0(SB), $7 +GLOBL ·tab(SB), RODATA, $8 +`) + if codes(diags)[CodeDataWidth] != 1 { + t.Fatalf("missing width must be flagged: %+v", diags) + } + + // A float needs its full 4 or 8 IEEE-754 bytes ("bad float size"). + diags = lintSrc(t, ` +#include "textflag.h" +DATA ·f+0(SB)/2, $1.5 +GLOBL ·f(SB), RODATA, $8 +`) + if codes(diags)[CodeDataWidth] != 1 { + t.Fatalf("float in a narrow width must be flagged: %+v", diags) + } + + // A string accepts any width: GOROOT's own messages use /84 and the + // bytes are zero-padded. + diags = lintSrc(t, ` +#include "textflag.h" +DATA ·s+0(SB)/3, $"hi" +GLOBL ·s(SB), RODATA, $3 +`) + if codes(diags)[CodeDataWidth] != 0 { + t.Fatalf("string data in width 3 must not be flagged: %+v", diags) + } + + // The valid integer widths and float widths are clean. + diags = lintSrc(t, ` +#include "textflag.h" +DATA ·tab+0(SB)/1, $1 +DATA ·tab+1(SB)/2, $2 +DATA ·tab+4(SB)/4, $4 +DATA ·f+0(SB)/4, $1.5 +GLOBL ·tab(SB), RODATA, $8 +GLOBL ·f(SB), RODATA, $4 +`) + if codes(diags)[CodeDataWidth] != 0 { + t.Fatalf("valid widths must not be flagged: %+v", diags) + } +} + +func TestDataValueOverflow(t *testing.T) { + // $0x1ff into one byte: both assemblers keep 0xff silently. + diags := lintSrc(t, ` +#include "textflag.h" +DATA ·tab+0(SB)/1, $0x1ff +GLOBL ·tab(SB), RODATA, $8 +`) + if codes(diags)[CodeDataValueOverflow] != 1 { + t.Fatalf("want one data-value-overflow, got %+v", diags) + } + + // The full signed and unsigned range of the width round-trips. + diags = lintSrc(t, ` +#include "textflag.h" +DATA ·tab+0(SB)/1, $255 +DATA ·tab+1(SB)/1, $-128 +DATA ·tab+2(SB)/2, $65535 +GLOBL ·tab(SB), RODATA, $8 +`) + if codes(diags)[CodeDataValueOverflow] != 0 { + t.Fatalf("in-range values must not be flagged: %+v", diags) + } + + // Width 8 holds every int64 (the regression where the range itself + // overflowed and flagged 0). + diags = lintSrc(t, ` +#include "textflag.h" +DATA ·tab+0(SB)/8, $0 +DATA ·tab+8(SB)/8, $16 +GLOBL ·tab(SB), RODATA, $16 +`) + if codes(diags)[CodeDataValueOverflow] != 0 { + t.Fatalf("width 8 must hold every int64: %+v", diags) + } + + // -disable silences the rule. + f, _ := parser.Parse("t_amd64.s", "#include \"textflag.h\"\nDATA ·tab+0(SB)/1, $0x1ff\nGLOBL ·tab(SB), RODATA, $8\n") + diags = File(f, Config{Arch: arch.AMD64, Disable: map[string]bool{CodeDataValueOverflow: true}}) + if codes(diags)[CodeDataValueOverflow] != 0 { + t.Fatalf("disabled rule must stay silent: %+v", diags) + } +} + +func TestDataStringWidth(t *testing.T) { + // Both assemblers reject a string longer than the field. + diags := lintSrc(t, ` +#include "textflag.h" +DATA ·msg+0(SB)/2, $"hello" +GLOBL ·msg(SB), RODATA, $5 +`) + if codes(diags)[CodeDataStringWidth] != 1 { + t.Fatalf("want one data-string-width, got %+v", diags) + } + + // A shorter string is zero-padded to the width, the documented layout. + diags = lintSrc(t, ` +#include "textflag.h" +DATA ·msg+0(SB)/8, $"hi" +GLOBL ·msg(SB), RODATA, $8 +`) + if codes(diags)[CodeDataStringWidth] != 0 { + t.Fatalf("padded string must not be flagged: %+v", diags) + } +} + +func TestDataWithoutGlobl(t *testing.T) { + // gasm asm rejects an unpaired initialiser ("no matching GLOBL"); go + // tool asm sizes the symbol implicitly. + diags := lintSrc(t, ` +#include "textflag.h" +DATA ·tab+0(SB)/8, $0x0102030405060708 +`) + if codes(diags)[CodeDataNoGlobl] != 1 { + t.Fatalf("want one data-without-globl, got %+v", diags) + } + + // The pairing is order-independent: both assemblers resolve by name. + diags = lintSrc(t, ` +#include "textflag.h" +GLOBL ·tab(SB), RODATA, $8 +DATA ·tab+0(SB)/8, $0x0102030405060708 +`) + if codes(diags)[CodeDataNoGlobl] != 0 { + t.Fatalf("GLOBL before DATA must not be flagged: %+v", diags) + } + diags = lintSrc(t, ` +#include "textflag.h" +DATA ·tab+0(SB)/8, $0x0102030405060708 +GLOBL ·tab(SB), RODATA, $8 +`) + if codes(diags)[CodeDataNoGlobl] != 0 { + t.Fatalf("DATA before GLOBL must not be flagged: %+v", diags) + } + + // A static symbol pairs with its own static GLOBL. + diags = lintSrc(t, ` +#include "textflag.h" +DATA mask<>+0(SB)/4, $0x80020100 +GLOBL mask<>(SB), RODATA, $4 +`) + if codes(diags)[CodeDataNoGlobl] != 0 { + t.Fatalf("static pairing must not be flagged: %+v", diags) + } +} + +func TestDataExceedsGlobl(t *testing.T) { + // gasm asm bounds every initialiser by the declared size; go tool asm + // grows the symbol silently, so the declared size stops matching the + // data. + diags := lintSrc(t, ` +#include "textflag.h" +GLOBL ·tab(SB), RODATA, $4 +DATA ·tab+0(SB)/8, $0x0102030405060708 +`) + if codes(diags)[CodeDataExceedsGlobl] != 1 { + t.Fatalf("want one data-exceeds-globl, got %+v", diags) + } + + // An initialiser that fits the declaration is clean, at either order. + diags = lintSrc(t, ` +#include "textflag.h" +DATA ·tab+0(SB)/4, $1 +DATA ·tab+4(SB)/4, $2 +GLOBL ·tab(SB), RODATA, $8 +`) + if codes(diags)[CodeDataExceedsGlobl] != 0 { + t.Fatalf("fitting initialisers must not be flagged: %+v", diags) + } +} diff --git a/lint/directives.go b/lint/directives.go new file mode 100644 index 0000000..b6a3f82 --- /dev/null +++ b/lint/directives.go @@ -0,0 +1,113 @@ +// Copyright (c) 2026 Petr Balvín (https://petrbalvin.org) +// SPDX-License-Identifier: BSD-3-Clause + +package lint + +import ( + "fmt" + "strings" + + "sourcedock.dev/petrbalvin/gasm-sdk/ast" +) + +// checkTextDirectives holds the two directive-hygiene checks that read the +// TEXT line against the layer it lives in: the NOFRAME flag against the +// declared frame, and a missing argument area where the // func signature +// implies one. +func checkTextDirectives(t *ast.Text, cfg Config) []Diagnostic { + var out []Diagnostic + if t.Frame == nil || !t.Frame.Imm.HasVal { + return out + } + + // NOFRAME is only valid with a frame size of zero (textflag.h); the + // toolchain accepts a non-zero combination silently and still allocates + // the frame, so the flag states an arrangement the directive does not + // describe. Negative frames are the ABI-wrapper spelling and are left + // alone: the arm64 BSD syscall stubs deliberately pair NOFRAME with $-8. + if !cfg.Disable[CodeNoFrameFrameSize] && hasTextFlag(t, "NOFRAME") && t.Frame.Imm.Val > 0 { + out = append(out, Diagnostic{ + Pos: t.Keyword.Pos, + Severity: Warning, + Code: CodeNoFrameFrameSize, + Message: fmt.Sprintf("NOFRAME suppresses frame setup and is only valid with a zero frame size, "+ + "but this TEXT declares $%d", t.Frame.Imm.Val), + }) + } + + // Without NOSPLIT the stack-growth preamble runs, and the argument area + // the wrapper laid out should be stated explicitly: an omitted area + // records ArgsSizeUnknown (funcdata.h) in the object and go vet then + // skips the size check against the prototype. Only functions whose + // // func signature implies a non-zero area are flagged; with no + // signature or a zero area, $frame alone is the normal spelling. + if !cfg.Disable[CodeMissingArgSize] && t.Args == nil && !hasTextFlag(t, "NOSPLIT") { + if want, ok := abiExpectedArgSize(t.Doc); ok && want > 0 { + out = append(out, Diagnostic{ + Pos: t.Keyword.Pos, + Severity: Warning, + Code: CodeMissingArgSize, + Message: fmt.Sprintf("TEXT omits the argument area; the // func signature implies %d bytes, "+ + "so the frame should read $%d-%d (an omitted area records ArgsSizeUnknown)", want, t.Frame.Imm.Val, want), + }) + } + } + return out +} + +// checkResultNaming flags result slots addressed with the generic ret +// spelling although the // func signature names its results. The name is +// documentation the toolchain enforces for FP references, and go vet checks +// it against the prototype, so a signature that names a result and a body +// that writes ret+N(FP) disagree about what the bytes mean. +func checkResultNaming(t *ast.Text) []Diagnostic { + results, ok := abiResultSlots(t.Doc) + if !ok || len(results) == 0 { + return nil + } + // A parameter named ret makes the spelling a parameter reference, not + // the generic result name; the rule cannot tell them apart and stays + // silent. + params, pok := abiParamNames(t.Doc) + if pok { + for _, p := range params { + if strings.EqualFold(p, "ret") { + return nil + } + } + } + var out []Diagnostic + for _, s := range t.Body { + in, ok := s.(*ast.Instr) + if !ok { + continue + } + for _, op := range in.Operands { + sym := op.Addr.Sym + if op.Kind != ast.OpAddr || sym == nil || sym.Pseudo != "FP" { + continue + } + if !strings.EqualFold(sym.Name, "ret") || !sym.HasOff { + continue + } + for _, r := range results { + // A result the signature itself names ret coincides with + // the generic spelling; there is no better name to suggest. + if strings.EqualFold(r.name, "ret") { + break + } + if sym.Offset >= r.off && sym.Offset < r.off+r.size { + out = append(out, Diagnostic{ + Pos: op.Pos, + Severity: Hint, + Code: CodeUnnamedResult, + Message: fmt.Sprintf("the // func signature names this result %q; "+ + "reference it as %q+%d(FP) rather than ret+%d(FP)", r.name, r.name, r.off, sym.Offset), + }) + break + } + } + } + } + return out +} diff --git a/lint/directives_test.go b/lint/directives_test.go new file mode 100644 index 0000000..11d2158 --- /dev/null +++ b/lint/directives_test.go @@ -0,0 +1,324 @@ +// Copyright (c) 2026 Petr Balvín (https://petrbalvin.org) +// SPDX-License-Identifier: BSD-3-Clause + +package lint + +import ( + "testing" + + "sourcedock.dev/petrbalvin/gasm-sdk/arch" + "sourcedock.dev/petrbalvin/gasm-sdk/parser" +) + +func TestNoFrameFrameSize(t *testing.T) { + // NOFRAME against a positive frame: the flag states an arrangement the + // directive does not describe. + diags := lintSrc(t, ` +#include "textflag.h" +TEXT ·f(SB), NOSPLIT|NOFRAME, $8 + MOVQ $1, AX + RET +`) + if codes(diags)[CodeNoFrameFrameSize] != 1 { + t.Fatalf("want one noframe-frame-size, got %+v", diags) + } + + // The valid spelling: NOFRAME with a zero frame. + diags = lintSrc(t, ` +#include "textflag.h" +TEXT ·f(SB), NOSPLIT|NOFRAME, $0 + MOVQ $1, AX + RET +`) + if codes(diags)[CodeNoFrameFrameSize] != 0 { + t.Fatalf("NOFRAME with $0 must not be flagged: %+v", diags) + } + + // A negative frame with NOFRAME is the arm64 BSD syscall-stub pattern + // (GOROOT's sys_netbsd_arm64.s runtime·open), not a defect. + diags = lintSrcArch(t, "f_arm64.s", ` +#include "textflag.h" +TEXT ·open(SB),NOSPLIT|NOFRAME,$-8 + MOVD name+0(FP), R0 + RET +`) + if codes(diags)[CodeNoFrameFrameSize] != 0 { + t.Fatalf("NOFRAME with a negative frame must not be flagged: %+v", diags) + } + + // A frame without the flag is the ordinary spelling. + diags = lintSrc(t, ` +#include "textflag.h" +TEXT ·f(SB), NOSPLIT, $8 + MOVQ $1, AX + RET +`) + if codes(diags)[CodeNoFrameFrameSize] != 0 { + t.Fatalf("a frame without NOFRAME must not be flagged: %+v", diags) + } + + // -disable silences the rule. + f, _ := parser.Parse("t_amd64.s", "#include \"textflag.h\"\nTEXT ·f(SB), NOSPLIT|NOFRAME, $8\n\tRET\n") + diags = File(f, Config{Arch: arch.AMD64, Disable: map[string]bool{CodeNoFrameFrameSize: true}}) + if codes(diags)[CodeNoFrameFrameSize] != 0 { + t.Fatalf("disabled rule must stay silent: %+v", diags) + } +} + +func TestMissingArgSize(t *testing.T) { + // Without NOSPLIT and without an argument area, the object records + // ArgsSizeUnknown even though the signature states the size. + diags := lintSrc(t, ` +// func f(a int) int +TEXT ·f(SB), $0 + MOVQ a+0(FP), AX + MOVQ AX, ret+8(FP) + RET +`) + if codes(diags)[CodeMissingArgSize] != 1 { + t.Fatalf("want one missing-argsize, got %+v", diags) + } + + // The declared area matching the signature is clean. + diags = lintSrc(t, ` +// func f(a int) int +TEXT ·f(SB), $0-16 + MOVQ a+0(FP), AX + MOVQ AX, ret+8(FP) + RET +`) + if codes(diags)[CodeMissingArgSize] != 0 { + t.Fatalf("a declared argument area must not be flagged: %+v", diags) + } + + // NOSPLIT functions omit the area throughout GOROOT; not flagged. + diags = lintSrc(t, ` +// func f(a int) int +TEXT ·f(SB), NOSPLIT, $0 + MOVQ a+0(FP), AX + MOVQ AX, ret+8(FP) + RET +`) + if codes(diags)[CodeMissingArgSize] != 0 { + t.Fatalf("NOSPLIT without an area must not be flagged: %+v", diags) + } + + // No signature, no opinion. + diags = lintSrc(t, ` +#include "textflag.h" +TEXT ·f(SB), $0 + MOVQ $1, AX + RET +`) + if codes(diags)[CodeMissingArgSize] != 0 { + t.Fatalf("signature-less function must not be flagged: %+v", diags) + } + + // A signature whose argument area is empty implies nothing to declare. + diags = lintSrc(t, ` +// func f() +TEXT ·f(SB), $0 + RET +`) + if codes(diags)[CodeMissingArgSize] != 0 { + t.Fatalf("zero-size signature must not be flagged: %+v", diags) + } +} + +// TestMissingRetFallthroughEnd pins the second shape of missing-ret: the +// body knows how to return, but its tail can run off the end, and the +// toolchain appends nothing, so execution continues into the next TEXT. +func TestMissingRetFallthroughEnd(t *testing.T) { + diags := lintSrc(t, ` +#include "textflag.h" +TEXT ·f(SB), NOSPLIT, $0 + CMPQ AX, $0 + JNE done + RET +done: + MOVQ $2, BX +`) + if codes(diags)[CodeMissingRet] != 1 { + t.Fatalf("want one missing-ret for the fall-through tail, got %+v", diags) + } + + // Ending in RET is clean, trailing labels included. + diags = lintSrc(t, ` +#include "textflag.h" +TEXT ·f(SB), NOSPLIT, $0 + CMPQ AX, $0 + JNE done + MOVQ $1, AX +done: + RET +`) + if codes(diags)[CodeMissingRet] != 0 { + t.Fatalf("RET last must not be flagged: %+v", diags) + } + + // Ending in an unconditional branch is a tail call, not a defect. + diags = lintSrc(t, ` +#include "textflag.h" +TEXT ·f(SB), NOSPLIT, $0 + MOVQ $1, AX + JMP ·g(SB) +`) + if codes(diags)[CodeMissingRet] != 0 { + t.Fatalf("a tail call must not be flagged: %+v", diags) + } + + // A conditional branch last, with a RET earlier, still falls through. + diags = lintSrc(t, ` +#include "textflag.h" +TEXT ·f(SB), NOSPLIT, $0 + JE done + RET +done: + CMPQ AX, $0 + JNE done +`) + if codes(diags)[CodeMissingRet] != 1 { + t.Fatalf("conditional-branch tail must be flagged: %+v", diags) + } + + // GC annotations after the terminator carry no control flow. + diags = lintSrc(t, ` +#include "textflag.h" +TEXT ·f(SB), NOSPLIT, $0 + RET + FUNCDATA $1, ·sm(SB) +`) + if codes(diags)[CodeMissingRet] != 0 { + t.Fatalf("FUNCDATA after RET must not be flagged: %+v", diags) + } + + // Trapping tails: the runtime's abort paths end in INT (amd64) or BRK + // (arm64); they never return, so no RET is needed. + diags = lintSrc(t, ` +#include "textflag.h" +TEXT ·abort(SB), NOSPLIT, $0 + INT $3 +`) + if codes(diags)[CodeMissingRet] != 0 { + t.Fatalf("INT tail must not be flagged: %+v", diags) + } + diags = lintSrcArch(t, "f_arm64.s", ` +#include "textflag.h" +TEXT ·abort(SB), NOSPLIT, $0 + BRK +`) + if codes(diags)[CodeMissingRet] != 0 { + t.Fatalf("BRK tail must not be flagged: %+v", diags) + } + + // A file that includes non-textflag headers may hide the terminator in + // a macro, so the heuristic stays silent there. + diags = lintSrcArch(t, "f_amd64.s", ` +#include "go_tls.h" +TEXT ·f(SB), NOSPLIT, $0 + MOVQ $1, AX +`) + if codes(diags)[CodeMissingRet] != 0 { + t.Fatalf("macro-using files must not be flagged: %+v", diags) + } +} + +func TestInvalidFlagPlacement(t *testing.T) { + // NOSPLIT is a TEXT flag; on GLOBL it does nothing. + diags := lintSrc(t, ` +#include "textflag.h" +GLOBL ·tab(SB), NOSPLIT, $8 +`) + if codes(diags)[CodeInvalidFlag] != 1 { + t.Fatalf("want one invalid-textflag for the misplaced NOSPLIT, got %+v", diags) + } + + // RODATA belongs to data declarations, not to TEXT. + diags = lintSrc(t, ` +#include "textflag.h" +TEXT ·f(SB), RODATA, $0 + RET +`) + if codes(diags)[CodeInvalidFlag] != 1 { + t.Fatalf("want one invalid-textflag for RODATA on TEXT, got %+v", diags) + } + + // Correct placements stay silent, on both directives. + diags = lintSrc(t, ` +#include "textflag.h" +TEXT ·f(SB), NOSPLIT|DUPOK, $0 + RET +GLOBL ·tab(SB), RODATA|NOPTR, $8 +DATA ·tab+0(SB)/8, $0 +`) + if codes(diags)[CodeInvalidFlag] != 0 { + t.Fatalf("correct flag placements must not be flagged: %+v", diags) + } +} + +func TestUnnamedResult(t *testing.T) { + // The signature names its results, the body uses the generic spelling. + diags := lintSrc(t, ` +// func kevent(kq int, ch unsafe.Pointer) (n int, err error) +TEXT ·kevent(SB), NOSPLIT, $0-40 + MOVL kq+0(FP), AX + MOVQ ch+8(FP), BX + MOVL AX, ret+16(FP) + MOVL $0, err+24(FP) + RET +`) + if codes(diags)[CodeUnnamedResult] != 1 { + t.Fatalf("want one unnamed-result hint, got %+v", diags) + } + + // Referencing the result by its declared name is the clean spelling. + diags = lintSrc(t, ` +// func kevent(kq int, ch unsafe.Pointer) (n int, err error) +TEXT ·kevent(SB), NOSPLIT, $0-40 + MOVL kq+0(FP), AX + MOVQ ch+8(FP), BX + MOVL AX, n+16(FP) + MOVL $0, err+24(FP) + RET +`) + if codes(diags)[CodeUnnamedResult] != 0 { + t.Fatalf("named result reference must not be flagged: %+v", diags) + } + + // An unnamed result is exactly what the ret spelling documents. + diags = lintSrc(t, ` +// func f(a int) int +TEXT ·f(SB), NOSPLIT, $0-16 + MOVQ a+0(FP), AX + MOVQ AX, ret+8(FP) + RET +`) + if codes(diags)[CodeUnnamedResult] != 0 { + t.Fatalf("unnamed result via ret must not be flagged: %+v", diags) + } + + // A result the signature itself names ret coincides with the generic + // spelling; there is nothing better to suggest. + diags = lintSrc(t, ` +// func f(a int) (ret int) +TEXT ·f(SB), NOSPLIT, $0-16 + MOVQ a+0(FP), AX + MOVQ AX, ret+8(FP) + RET +`) + if codes(diags)[CodeUnnamedResult] != 0 { + t.Fatalf("a result literally named ret must not be flagged: %+v", diags) + } + + // A parameter named ret turns the spelling into a parameter reference. + diags = lintSrc(t, ` +// func f(ret int) (n int) +TEXT ·f(SB), NOSPLIT, $0-16 + MOVQ ret+0(FP), AX + MOVQ AX, n+8(FP) + RET +`) + if codes(diags)[CodeUnnamedResult] != 0 { + t.Fatalf("a parameter named ret must not be flagged: %+v", diags) + } +} diff --git a/lint/lint.go b/lint/lint.go index 7c5edb0..2a2992d 100644 --- a/lint/lint.go +++ b/lint/lint.go @@ -10,6 +10,7 @@ package lint import ( "fmt" + "slices" "strconv" "strings" "unicode/utf8" @@ -84,6 +85,17 @@ const ( CodeNonportableRegister = "nonportable-register-name" CodeUnencodable = "unencodable-instruction" CodeReservedRegister = "reserved-register-write" + CodeMissingArgSize = "missing-argsize" + CodeNoFrameFrameSize = "noframe-frame-size" + CodeUnnamedFPRef = "unnamed-fp-reference" + CodeHardwareSP = "hardware-sp-addressing" + CodeVEXSSEMixing = "vex-sse-mixing" + CodeUnnamedResult = "unnamed-result" + CodeDataWidth = "data-width" + CodeDataValueOverflow = "data-value-overflow" + CodeDataStringWidth = "data-string-width" + CodeDataNoGlobl = "data-without-globl" + CodeDataExceedsGlobl = "data-exceeds-globl" ) // knownTextFlags are the flags recognised by runtime/textflag.h, plus the @@ -95,6 +107,19 @@ var knownTextFlags = map[string]bool{ "TOPFRAME": true, "ABIWRAPPER": true, } +// textOnlyFlags and dataOnlyFlags carry the placement half of the textflag.h +// table: the names that exist on one directive but not the other. NOPROF and +// DUPOK apply to both, and every name outside these two sets is either +// shared or already reported by the unknown-flag check. +var textOnlyFlags = map[string]bool{ + "NOSPLIT": true, "WRAPPER": true, "NEEDCTXT": true, "NOFRAME": true, + "REFLECTED": true, "REFLECTMETHOD": true, "TOPFRAME": true, "ABIWRAPPER": true, +} + +var dataOnlyFlags = map[string]bool{ + "RODATA": true, "NOPTR": true, "TLSBSS": true, +} + // pseudoOps are assembler pseudo-operations that are valid instruction-position // tokens but are not machine instructions and so absent from the arch tables. var pseudoOps = map[string]bool{ @@ -165,12 +190,15 @@ func File(f *ast.File, cfg Config) []Diagnostic { for _, d := range f.Decls { var flags []string var pos token.Position + var directive string switch dd := d.(type) { case *ast.Text: flags = dd.Flags pos = dd.Keyword.Pos + directive = "TEXT" case *ast.Globl: flags = dd.Flags + directive = "GLOBL" if dd.Name != nil { pos = dd.Name.Pos } @@ -188,11 +216,28 @@ func File(f *ast.File, cfg Config) []Diagnostic { Code: CodeInvalidFlag, Message: fmt.Sprintf("unknown TEXT/GLOBL flag %q", fl), }) + continue + } + // Placement: the textflag.h table binds half the names to + // one directive. Both assemblers accept a misplaced name + // silently, so the flag simply does nothing. + if (directive == "TEXT" && dataOnlyFlags[fl]) || + (directive == "GLOBL" && textOnlyFlags[fl]) { + out = append(out, Diagnostic{ + Pos: pos, + Severity: Warning, + Code: CodeInvalidFlag, + Message: fmt.Sprintf("flag %q does not apply to %s", fl, directive), + }) } } } } + // DATA and GLOBL structure: widths, values that fit them, and the + // GLOBL declaration that sizes each initialised symbol. + out = append(out, checkDataDecls(f, cfg)...) + sortDiagnostics(out) return out } @@ -376,17 +421,33 @@ func lintText(t *ast.Text, tab *arch.Table, archKnown bool, cfg Config, macros m } } - // Missing RET heuristic. Functions that invoke a macro are skipped: the - // macro body (opaque to us) may supply the RET. A TEXT whose symbol is - // missing (already reported by the parser) is skipped too. + // Missing RET. Two shapes of the same defect, a function that can run + // off the end of its body: one with no RET anywhere, and one that has a + // RET (or a terminator) somewhere but whose last instruction is not a + // terminator, so the tail falls through into whatever follows the TEXT. + // Functions that invoke a macro are skipped: the macro body (opaque to + // us) may supply the RET. A TEXT whose symbol is missing (already + // reported by the parser) is skipped too. if doLabelChecks && !cfg.Disable[CodeMissingRet] && t.Name != nil && - instrCount > 0 && !hasRet && !lastTerminal && !hasMacro { - out = append(out, Diagnostic{ - Pos: t.Keyword.Pos, - Severity: Warning, - Code: CodeMissingRet, - Message: fmt.Sprintf("function %q has no RET", t.Name.Name), - }) + instrCount > 0 && !hasMacro { + if !trailingTerminal(t, cfg.Arch) { + if !hasRet && !lastTerminal { + out = append(out, Diagnostic{ + Pos: t.Keyword.Pos, + Severity: Warning, + Code: CodeMissingRet, + Message: fmt.Sprintf("function %q has no RET", t.Name.Name), + }) + } else { + out = append(out, Diagnostic{ + Pos: t.Keyword.Pos, + Severity: Warning, + Code: CodeMissingRet, + Message: fmt.Sprintf("function %q can fall off its end: the last instruction is neither "+ + "RET nor an unconditional branch, so execution continues into the next TEXT", t.Name.Name), + }) + } + } } // Stack imbalance: track SP changes and flag if the net delta at RET @@ -485,9 +546,81 @@ func lintText(t *ast.Text, tab *arch.Table, archKnown bool, cfg Config, macros m // FUNCDATA / PCDATA structural validation. out = append(out, checkFuncdata(t, cfg)...) + // TEXT directive hygiene: the NOFRAME flag against a non-zero frame, and + // a missing argument area where the // func signature implies one. + out = append(out, checkTextDirectives(t, cfg)...) + + // Addressing edges: unnamed FP references and hardware stack pointer + // offsets, the one-character spellings OPERANDS.md calls the sharpest + // edge in the language. + out = append(out, scanAddressing(t, cfg)...) + + // Encoding mixing: VEX and legacy SSE in one kernel pay a transition + // penalty on every switch (amd64 only). + if cfg.Arch == arch.AMD64 && !hasMacro && !cfg.Disable[CodeVEXSSEMixing] { + out = append(out, checkVectorEncoding(t, tab, macros)...) + } + + // Result naming: the // func signature names its results, but the body + // addresses a result slot with the generic ret spelling. + if !cfg.Disable[CodeUnnamedResult] { + out = append(out, checkResultNaming(t)...) + } + return out } +// trailingTerminal reports whether the function's last real instruction ends +// control flow: RET, UNDEF, an arch-conditional unconditional branch, or an +// architectural trap. Trailing labels, GC annotations and code/data padding +// pseudo-ops are skipped, so a terminator followed by padding (the goexit +// traceback pattern) still counts. A body of nothing but skipped statements +// counts as terminal: there is no fall-through instruction to report. +func trailingTerminal(t *ast.Text, a arch.Arch) bool { + for _, s := range slices.Backward(t.Body) { + in, ok := s.(*ast.Instr) + if !ok { + continue // a trailing label + } + upper := strings.ToUpper(in.Mnemonic.Text) + switch upper { + case "FUNCDATA", "PCDATA", "GO_ARGS", "PCALIGN", + "BYTE", "WORD", "LONG", "QUAD", "FLOAT": + continue // annotations and padding carry no control flow + } + return upper == "RET" || upper == "UNDEF" || + isUnconditionalJump(a, upper) || isTrap(a, upper) + } + return true +} + +// isTrap reports whether the mnemonic is an architectural trap: an +// instruction whose execution cannot continue (a breakpoint or an +// unconditional fault). The runtime ends its abort paths with these. +func isTrap(a arch.Arch, upper string) bool { + switch a { + case arch.AMD64: + return upper == "INT" + case arch.ARM64: + return upper == "BRK" + case arch.RISCV: + return upper == "EBREAK" + case arch.LOONG64: + return upper == "BREAK" + } + return false +} + +// hasTextFlag reports whether the TEXT carries the named flag. +func hasTextFlag(t *ast.Text, name string) bool { + for _, f := range t.Flags { + if strings.EqualFold(f, name) { + return true + } + } + return false +} + // reachesRuntime reports whether a function can reach the Go runtime: it is // not NOSPLIT (so the stack-split and traceback machinery runs) or it makes a // CALL. Goroutine-pointer registers must survive such functions; a NOSPLIT diff --git a/lint/vector.go b/lint/vector.go new file mode 100644 index 0000000..95e4401 --- /dev/null +++ b/lint/vector.go @@ -0,0 +1,70 @@ +// Copyright (c) 2026 Petr Balvín (https://petrbalvin.org) +// SPDX-License-Identifier: BSD-3-Clause + +package lint + +import ( + "strings" + + "sourcedock.dev/petrbalvin/gasm-sdk/arch" + "sourcedock.dev/petrbalvin/gasm-sdk/ast" +) + +// checkVectorEncoding reports a kernel that mixes VEX-encoded instructions +// with legacy SSE operating on X registers. Every switch between the two +// encodings pays a transition penalty (the upper YMM halves must be saved +// and restored by the processor), so a kernel is written in one encoding or +// the other; the runtime's memmove mixes them deliberately behind feature +// dispatch, which is why the severity is informational. +func checkVectorEncoding(t *ast.Text, tab *arch.Table, macros map[string]bool) []Diagnostic { + vex, legacy := false, false + for _, s := range t.Body { + in, ok := s.(*ast.Instr) + if !ok { + continue + } + upper := strings.ToUpper(in.Mnemonic.Text) + if isMacroInvocation(in.Mnemonic.Text, macros) { + continue + } + // VZEROUPPER and VZEROALL are the boundary management: their purpose + // is to clean the upper halves before legacy SSE code, so they count + // as neither side of the mixing. + if strings.HasPrefix(upper, "V") && upper != "VZEROUPPER" && upper != "VZEROALL" { + vex = true + continue + } + if !strings.HasPrefix(upper, "V") && hasXMMOperand(in, tab) { + legacy = true + } + } + if !vex || !legacy { + return nil + } + return []Diagnostic{{ + Pos: t.Keyword.Pos, + Severity: Information, + Code: CodeVEXSSEMixing, + Message: "mixes VEX-encoded instructions with legacy SSE on X registers; each switch " + + "between the encodings pays a transition penalty, so keep one encoding per kernel", + }} +} + +// hasXMMOperand reports whether an instruction addresses an X (128-bit SSE) +// register. Symbol names that collide with the spelling (a bare X86, say) +// are excluded by the register table lookup. +func hasXMMOperand(in *ast.Instr, tab *arch.Table) bool { + for _, op := range in.Operands { + if op.Kind != ast.OpAddr || op.Addr.Sym == nil { + continue + } + name := op.Addr.Sym.Name + if op.Addr.Sym.Pseudo != "" || op.Addr.Base != "" || op.Addr.Index != "" || name == "" { + continue + } + if r, ok := tab.Register(name); ok && r.Class == arch.Vector && strings.HasPrefix(strings.ToUpper(name), "X") { + return true + } + } + return false +} diff --git a/lint/vector_test.go b/lint/vector_test.go new file mode 100644 index 0000000..b9b86f4 --- /dev/null +++ b/lint/vector_test.go @@ -0,0 +1,95 @@ +// Copyright (c) 2026 Petr Balvín (https://petrbalvin.org) +// SPDX-License-Identifier: BSD-3-Clause + +package lint + +import ( + "testing" + + "sourcedock.dev/petrbalvin/gasm-sdk/arch" + "sourcedock.dev/petrbalvin/gasm-sdk/parser" +) + +// TestVEXSSEMixing checks the encoding-mixing advisory: VEX and legacy SSE +// in one kernel pay a transition penalty on every switch, so the finding is +// informational, not a defect. +func TestVEXSSEMixing(t *testing.T) { + // GOROOT's memmove shape: VMOVDQU on Y registers beside MOVOU on X. + diags := lintSrc(t, ` +#include "textflag.h" +TEXT ·copy(SB), NOSPLIT, $0 + VMOVDQU (SI), Y4 + MOVOU (SI), X0 + RET +`) + if codes(diags)[CodeVEXSSEMixing] != 1 { + t.Fatalf("want one vex-sse-mixing, got %+v", diags) + } + + // One encoding throughout is the recommendation. + diags = lintSrc(t, ` +#include "textflag.h" +TEXT ·copy(SB), NOSPLIT, $0 + VMOVDQU (SI), Y4 + VPSUBD Y1, Y2, Y3 + RET +`) + if codes(diags)[CodeVEXSSEMixing] != 0 { + t.Fatalf("pure VEX kernel must not be flagged: %+v", diags) + } + diags = lintSrc(t, ` +#include "textflag.h" +TEXT ·copy(SB), NOSPLIT, $0 + MOVOU (SI), X0 + PADDB X1, X2 + RET +`) + if codes(diags)[CodeVEXSSEMixing] != 0 { + t.Fatalf("pure legacy kernel must not be flagged: %+v", diags) + } + + // VZEROUPPER is the boundary management, not a side of the mixing: the + // idiomatic epilogue after legacy SSE stays clean. + diags = lintSrc(t, ` +#include "textflag.h" +TEXT ·copy(SB), NOSPLIT, $0 + MOVOU (SI), X0 + PADDB X1, X2 + VZEROUPPER + RET +`) + if codes(diags)[CodeVEXSSEMixing] != 0 { + t.Fatalf("VZEROUPPER epilogue must not be flagged: %+v", diags) + } + + // A symbol whose name collides with the register spelling (the runtime's + // internal∕cpu·X86 feature flags) is not a register. + diags = lintSrc(t, ` +#include "textflag.h" +TEXT ·probe(SB), NOSPLIT, $0 + CMPB internal∕cpu·X86+const_offsetX86HasERMS(SB), $1 + VMOVDQU (SI), Y4 + RET +`) + if codes(diags)[CodeVEXSSEMixing] != 0 { + t.Fatalf("a symbol named X86 must not count as legacy SSE: %+v", diags) + } + + // The rule is amd64-only: arm64 V registers are not SSE at all. + diags = lintSrcArch(t, "f_arm64.s", ` +#include "textflag.h" +TEXT ·f(SB), NOSPLIT, $0 + VLD1 (R0), [V1.B16] + RET +`) + if codes(diags)[CodeVEXSSEMixing] != 0 { + t.Fatalf("arm64 must not be flagged: %+v", diags) + } + + // -disable silences the rule. + f, _ := parser.Parse("t_amd64.s", "#include \"textflag.h\"\nTEXT ·copy(SB), NOSPLIT, $0\n\tVMOVDQU (SI), Y4\n\tMOVOU (SI), X0\n\tRET\n") + diags = File(f, Config{Arch: arch.AMD64, Disable: map[string]bool{CodeVEXSSEMixing: true}}) + if codes(diags)[CodeVEXSSEMixing] != 0 { + t.Fatalf("disabled rule must stay silent: %+v", diags) + } +}