diff --git a/lint/abi0.go b/lint/abi0.go index ac82eb1..2b09fb3 100644 --- a/lint/abi0.go +++ b/lint/abi0.go @@ -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 TEXT reads its arguments from the register + // file by declaration (runtime·memmove 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 diff --git a/lint/abi_test.go b/lint/abi_test.go index a3e0410..f1692d4 100644 --- a/lint/abi_test.go +++ b/lint/abi_test.go @@ -82,6 +82,23 @@ func TestABIArgSizeSkipsRegisterABI(t *testing.T) { } } +// TestABI0ArgsSkipsABIInternal verifies the frame-read check does not fire for +// a TEXT declared : runtime·memmove 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(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. diff --git a/lint/lint.go b/lint/lint.go index ea79508..b9dc456 100644 --- a/lint/lint.go +++ b/lint/lint.go @@ -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 { diff --git a/lint/lint_test.go b/lint/lint_test.go index 67175b7..b1bca60 100644 --- a/lint/lint_test.go +++ b/lint/lint_test.go @@ -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"