From 7604a9443f81964f16971130b6692234410a0a6d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Petr=20Balv=C3=ADn?= Date: Sat, 19 Sep 2026 23:49:19 +0200 Subject: [PATCH] fix(cmd): usage exit codes, asm output file and cross-arch ground truth Assisted-by: GLM 5.3 --- cmd/gasm/audit.go | 13 ++++- cmd/gasm/main.go | 127 +++++++++++++++++++++++++----------------- cmd/gasm/main_test.go | 92 ++++++++++++++++++++++++++++++ cmd/gasm/scaffold.go | 2 +- 4 files changed, 179 insertions(+), 55 deletions(-) diff --git a/cmd/gasm/audit.go b/cmd/gasm/audit.go index 6c25e55..958cb7a 100644 --- a/cmd/gasm/audit.go +++ b/cmd/gasm/audit.go @@ -63,7 +63,7 @@ the encodability backlog by frequency rather than by table order. archName := "amd64" switch n := len(fs.Args()); { case n > 1: - return fmt.Errorf("audit-instructions takes at most one architecture argument") + return &usageError{fmt.Errorf("audit-instructions takes at most one architecture argument")} case n == 1: archName = strings.ToLower(fs.Arg(0)) } @@ -136,7 +136,7 @@ func auditArch(name string) (arch.Arch, error) { case "loong64", "loong": return arch.LOONG64, nil } - return arch.Unknown, fmt.Errorf("unknown architecture %q: want amd64, arm64, riscv64 or loong64", name) + return arch.Unknown, &usageError{fmt.Errorf("unknown architecture %q: want amd64, arm64, riscv64 or loong64", name)} } // goarchName maps an arch identifier onto its GOARCH spelling. @@ -217,6 +217,13 @@ func probeGoAsm(goarch string, names []string) (map[string]bool, error) { cmd := exec.Command(asmBin, "-p", "probe", "-o", filepath.Join(dir, "probe.o"), probePath) cmd.Env = append(os.Environ(), "GOARCH="+goarch, "GOOS="+runtime.GOOS) out, _ := cmd.CombinedOutput() + // The expected failure mode is a non-zero exit with compiler diagnostics + // on stdout; empty output means the probe broke at the exec level (a + // killed child, a tool that would not start), and seeding every name as + // recognized on that silence would fake a clean audit. + if len(out) == 0 { + return nil, fmt.Errorf("go tool asm probe for GOARCH=%s produced no output", goarch) + } result := map[string]bool{} for _, name := range names { @@ -344,7 +351,7 @@ func (t *corpusTally) fail(path, reason string) { // cmdAuditCorpus implements audit-instructions --corpus. func cmdAuditCorpus(args []string) error { if len(args) > 1 { - return fmt.Errorf("audit-instructions --corpus takes at most one directory argument") + return &usageError{fmt.Errorf("audit-instructions --corpus takes at most one directory argument")} } root := "" if len(args) == 1 { diff --git a/cmd/gasm/main.go b/cmd/gasm/main.go index 5ff4f40..cffc814 100644 --- a/cmd/gasm/main.go +++ b/cmd/gasm/main.go @@ -10,6 +10,7 @@ package main import ( "bytes" "encoding/json" + "errors" "flag" "fmt" "io" @@ -48,6 +49,22 @@ func version() string { return bi.Main.Version } +// usageError marks an error the caller's arguments caused, which exits 2 +// instead of the 1 a runtime failure gets. +type usageError struct{ err error } + +func (e *usageError) Error() string { return e.err.Error() } +func (e *usageError) Unwrap() error { return e.err } + +// exitCodeFor maps an error onto the process exit status: 2 for a usage +// error, 1 for anything else. +func exitCodeFor(err error) int { + if _, ok := errors.AsType[*usageError](err); ok { + return 2 + } + return 1 +} + func main() { if len(os.Args) < 2 { usage(os.Stderr) @@ -77,12 +94,12 @@ func main() { case "audit-instructions": if err := cmdAuditInstructions(os.Args[2:]); err != nil { fmt.Fprintln(os.Stderr, err) - os.Exit(1) + os.Exit(exitCodeFor(err)) } case "scaffold": if err := cmdScaffold(os.Args[2:]); err != nil { fmt.Fprintln(os.Stderr, err) - os.Exit(1) + os.Exit(exitCodeFor(err)) } case "lsp": os.Exit(cmdLSP(os.Args[2:])) @@ -104,11 +121,11 @@ func cmdVersion() int { // ANSI colour helpers for terminal output. const ( - colorReset = "\033[0m" - colorBold = "\033[1m" - colorCyan = "\033[36m" - colorYellow = "\033[33m" - colorGray = "\033[90m" + colourReset = "\033[0m" + colourBold = "\033[1m" + colourCyan = "\033[36m" + colourYellow = "\033[33m" + colourGrey = "\033[90m" ) // isTTY reports whether the writer is a terminal (for colour output). @@ -122,9 +139,9 @@ func isTTY(w io.Writer) bool { func usage(w io.Writer) { useColor := isTTY(w) - bold, cyan, yellow, gray, reset := "", "", "", "", "" + bold, cyan, yellow, grey, reset := "", "", "", "", "" if useColor { - bold, cyan, yellow, gray, reset = colorBold, colorCyan, colorYellow, colorGray, colorReset + bold, cyan, yellow, grey, reset = colourBold, colourCyan, colourYellow, colourGrey, colourReset } fmt.Fprintf(w, "%sgasm %s%s: developer tooling for Go's Plan 9 assembler (GAsm)%s\n\n", bold, version(), reset, reset) @@ -153,12 +170,12 @@ func usage(w io.Writer) { {"version", "print the version (same as --version)"}, } for _, c := range commands { - fmt.Fprintf(w, " %s%-10s%s %s%s%s\n", cyan, c.name, reset, gray, c.desc, reset) + fmt.Fprintf(w, " %s%-10s%s %s%s%s\n", cyan, c.name, reset, grey, c.desc, reset) } fmt.Fprintf(w, "\n%sFlags:%s\n", yellow, reset) - fmt.Fprintf(w, " %s-h, --help%s %sshow this help%s\n", cyan, reset, gray, reset) - fmt.Fprintf(w, " %s-V, --version%s %sprint the version%s\n", cyan, reset, gray, reset) + fmt.Fprintf(w, " %s-h, --help%s %sshow this help%s\n", cyan, reset, grey, reset) + fmt.Fprintf(w, " %s-V, --version%s %sprint the version%s\n", cyan, reset, grey, reset) fmt.Fprintf(w, "\nRun \"gasm -h\" for a command's usage and flags.\n\n") @@ -172,7 +189,7 @@ func usage(w io.Writer) { } for _, e := range examples { if e.desc != "" { - fmt.Fprintf(w, " %s%s%s %s%s%s\n", cyan, e.cmd, reset, gray, e.desc, reset) + fmt.Fprintf(w, " %s%s%s %s%s%s\n", cyan, e.cmd, reset, grey, e.desc, reset) } else { fmt.Fprintf(w, " %s%s%s\n", cyan, e.cmd, reset) } @@ -483,6 +500,14 @@ requires -p, the package path, and the installed Go toolchain). fmt.Fprintln(os.Stderr, "usage: gasm asm [--format raw|elf|goobj] [-p pkg] [-GOARCH arch] [-o out] ") return 2 } + // The format is validated before anything else, so a bogus value exits 2 + // with or without -o instead of silently dumping the hex of a raw image. + switch *format { + case "raw", "elf", "goobj": + default: + fmt.Fprintf(os.Stderr, "gasm asm: unknown format %q (want raw, elf or goobj)\n", *format) + return 2 + } path := fs.Arg(0) targetArch := arch.FromFilename(path) if *archName != "" { @@ -517,38 +542,42 @@ requires -p, the package path, and the installed Go toolchain). fmt.Fprintln(os.Stderr, "gasm asm: no assemblable TEXT functions or GLOBL data found") return 1 } - for _, fn := range img.Funcs { - code := img.Code[fn.Offset : fn.Offset+fn.Size] - fmt.Printf("%s: %d bytes\n", fn.Name, fn.Size) - for i := 0; i < len(code); i += 16 { - end := min(i+16, len(code)) - fmt.Printf(" %04x:", i) - for _, b := range code[i:end] { - fmt.Printf(" %02x", b) + // Without -o the hex dump on stdout is the output; with -o the file is, + // and the dump is skipped, as the -o help text promises. + if *out == "" { + for _, fn := range img.Funcs { + code := img.Code[fn.Offset : fn.Offset+fn.Size] + fmt.Printf("%s: %d bytes\n", fn.Name, fn.Size) + for i := 0; i < len(code); i += 16 { + end := min(i+16, len(code)) + fmt.Printf(" %04x:", i) + for _, b := range code[i:end] { + fmt.Printf(" %02x", b) + } + fmt.Println() } - fmt.Println() } - } - if len(img.Data) > 0 { - fmt.Printf("data: %d bytes at 0x%x\n", len(img.Data), len(img.Code)) - for _, d := range f.Decls { - g, ok := d.(*ast.Globl) - if !ok || g.Name == nil || g.Name.Pseudo != "SB" { - continue + if len(img.Data) > 0 { + fmt.Printf("data: %d bytes at 0x%x\n", len(img.Data), len(img.Code)) + for _, d := range f.Decls { + g, ok := d.(*ast.Globl) + if !ok || g.Name == nil || g.Name.Pseudo != "SB" { + continue + } + size := 0 + if g.Size != nil && g.Size.Imm.HasVal { + size = int(g.Size.Imm.Val) + } + fmt.Printf(" %s: %d bytes at 0x%x\n", g.Name.Name, size, img.Symbols[g.Name.Name]) } - size := 0 - if g.Size != nil && g.Size.Imm.HasVal { - size = int(g.Size.Imm.Val) + for i := 0; i < len(img.Data); i += 16 { + end := min(i+16, len(img.Data)) + fmt.Printf(" %04x:", len(img.Code)+i) + for _, b := range img.Data[i:end] { + fmt.Printf(" %02x", b) + } + fmt.Println() } - fmt.Printf(" %s: %d bytes at 0x%x\n", g.Name.Name, size, img.Symbols[g.Name.Name]) - } - for i := 0; i < len(img.Data); i += 16 { - end := min(i+16, len(img.Data)) - fmt.Printf(" %04x:", len(img.Code)+i) - for _, b := range img.Data[i:end] { - fmt.Printf(" %02x", b) - } - fmt.Println() } } if *out != "" { @@ -586,9 +615,6 @@ requires -p, the package path, and the installed Go toolchain). obj, err = img.GOObject(*pkg, path) } kind = "Go object" - default: - fmt.Fprintf(os.Stderr, "gasm asm: unknown format %q (want raw, elf or goobj)\n", *format) - return 2 } if err != nil { fmt.Fprintln(os.Stderr, "gasm asm:", err) @@ -1012,15 +1038,14 @@ each entry reproduces. // under the available loong64 emulators), so those kernels take the // toolchain-comparison path. if targetArch != hostArch() || targetArch == arch.LOONG64 { - // No JIT on this host: ground truth and profile remain available. - // (loong64 is ground-truth-only everywhere for now: its trampoline - // is implemented but not yet validated against real hardware.) + // No JIT on this host: ground truth and profile remain available for + // every architecture, because cmdVerifyNonJIT assembles and compares + // against the toolchain without executing anything. (loong64 is + // ground-truth-only everywhere for now: its trampoline is implemented + // but not yet validated against real hardware.) switch targetArch { - case arch.RISCV, arch.LOONG64, arch.ARM64: + case arch.AMD64, arch.RISCV, arch.LOONG64, arch.ARM64: return cmdVerifyNonJIT(path, targetArch, *groundTruth, *profile) - case arch.AMD64: - fmt.Fprintln(os.Stderr, "gasm verify: JIT-based checks need an amd64 host; use --ground-truth here") - return 1 default: fmt.Fprintln(os.Stderr, "gasm verify: unsupported architecture") return 1 diff --git a/cmd/gasm/main_test.go b/cmd/gasm/main_test.go index f806f8e..318633f 100644 --- a/cmd/gasm/main_test.go +++ b/cmd/gasm/main_test.go @@ -13,6 +13,8 @@ import ( "strings" "syscall" "testing" + + "sourcedock.dev/petrbalvin/gasm-devkit/arch" ) const clean = "#include \"textflag.h\"\n" + @@ -242,6 +244,96 @@ func TestCmdArgErrors(t *testing.T) { } } +// TestUsageExitCodes pins the exit-code contract for the commands whose main +// dispatches on a returned error: a wrong argument set exits 2, the same as +// the commands that count their arguments themselves, while a runtime +// failure (an unreadable file) keeps exit 1. +func TestUsageExitCodes(t *testing.T) { + for name, err := range map[string]error{ + "audit-instructions extra argument": cmdAuditInstructions([]string{"amd64", "extra"}), + "audit-instructions unknown arch": cmdAuditInstructions([]string{"mips"}), + "audit-instructions corpus extra": cmdAuditInstructions([]string{"--corpus", "a", "b"}), + "scaffold no arguments": cmdScaffold(nil), + "scaffold extra arguments": cmdScaffold([]string{"differential", "a.s", "b.s"}), + } { + if err == nil { + t.Errorf("%s: expected an error", name) + continue + } + if code := exitCodeFor(err); code != 2 { + t.Errorf("%s: exit code = %d, want 2 (err: %v)", name, code, err) + } + } + if err := cmdScaffold([]string{"differential", "/nonexistent/file.s"}); err == nil { + t.Error("scaffold on a missing file should fail") + } else if code := exitCodeFor(err); code != 1 { + t.Errorf("scaffold on a missing file: exit code = %d, want 1", code) + } +} + +// TestCmdAsmFormatValidation checks that an unknown --format exits 2 with +// and without -o, instead of assembling and silently dumping a raw image. +func TestCmdAsmFormatValidation(t *testing.T) { + path := writeTemp(t, "f_amd64.s", clean) + out := filepath.Join(t.TempDir(), "f.bin") + if _, _, code := capture(func() int { return cmdAsm([]string{"--format", "bogus", path}) }); code != 2 { + t.Errorf("asm --format bogus without -o: code = %d, want 2", code) + } + if _, _, code := capture(func() int { return cmdAsm([]string{"--format", "bogus", "-o", out, path}) }); code != 2 { + t.Errorf("asm --format bogus with -o: code = %d, want 2", code) + } +} + +// TestCmdAsmOutputFile pins the documented -o behaviour: the output goes to +// the file and stdout carries no hex dump; without -o the dump is the output. +func TestCmdAsmOutputFile(t *testing.T) { + path := writeTemp(t, "f_amd64.s", clean) + out := filepath.Join(t.TempDir(), "f.bin") + stdout, _, code := capture(func() int { return cmdAsm([]string{"-o", out, path}) }) + if code != 0 { + t.Fatalf("code = %d", code) + } + if strings.Contains(stdout, "0000:") { + t.Errorf("stdout carries a hex dump despite -o:\n%s", stdout) + } + if !strings.Contains(stdout, "wrote ") { + t.Errorf("stdout misses the wrote line:\n%s", stdout) + } + b, err := os.ReadFile(out) + if err != nil { + t.Fatal(err) + } + if len(b) == 0 { + t.Error("the output file is empty") + } + + stdout, _, code = capture(func() int { return cmdAsm([]string{path}) }) + if code != 0 { + t.Fatalf("without -o: code = %d", code) + } + if !strings.Contains(stdout, "0000:") { + t.Errorf("without -o the hex dump is missing:\n%s", stdout) + } +} + +// TestVerifyNonJITAMD64GroundTruth drives the cross-architecture +// ground-truth path for an amd64 kernel: the path a host of any other +// architecture takes, which must compare against the toolchain rather than +// refuse to run. +func TestVerifyNonJITAMD64GroundTruth(t *testing.T) { + if testing.Short() { + t.Skip("runs go tool asm") + } + path := writeTemp(t, "f_amd64.s", clean) + out, _, code := capture(func() int { return cmdVerifyNonJIT(path, arch.AMD64, true, false) }) + if code != 0 { + t.Fatalf("code = %d (%s)", code, out) + } + if !strings.Contains(out, "1/1 matched") { + t.Errorf("output misses the matched report:\n%s", out) + } +} + // TestVerifySmokeCrashIsolation checks that a function faulting on its // zeroed smoke arguments is reported as CRASH by a child process instead of // killing `gasm verify` itself. diff --git a/cmd/gasm/scaffold.go b/cmd/gasm/scaffold.go index 351aa3b..4e94ac1 100644 --- a/cmd/gasm/scaffold.go +++ b/cmd/gasm/scaffold.go @@ -44,7 +44,7 @@ bodies, place the file in the kernel's package, and run it in CI. rest = rest[1:] } if len(rest) != 1 { - return fmt.Errorf("usage: gasm scaffold differential ") + return &usageError{fmt.Errorf("usage: gasm scaffold differential ")} } path := rest[0] src, err := os.ReadFile(path)