From 26c5008136ce9cc10616e808b6248182c9573fb7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Petr=20Balv=C3=ADn?= Date: Wed, 23 Sep 2026 21:03:16 +0200 Subject: [PATCH] fix(cmd): honour //go:build in the corpus audit Assisted-by: GLM 5.3 Flash --- CHANGELOG.md | 12 ++++ README.md | 11 ++-- cmd/gasm/asmhdr_test.go | 46 ++++++++++++++ cmd/gasm/audit.go | 136 +++++++++++++++++++++++++++++++++++----- cmd/gasm/audit_test.go | 60 ++++++++++++++++++ 5 files changed, 247 insertions(+), 18 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 1e449e2..8ca1868 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -21,6 +21,18 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 TEXT argument area to the size the `// func` signature implies, computed by the new `lint.ExpectedArgSize`. +### Changed + +- **The corpus audit assembles like the build.** A file's `//go:build` + constraint decides which target architectures attempt it: cpu_x86.s is + an x86 build alone, and the msan and goexperiment.runtimesecret trees + are compiled by no supported build, so they leave the measured set + instead of failing it. The headline now reads "assemble for every + applicable target": every real-code GOROOT assembly file, the tree + without testdata, assembles for all four architectures (250 of 250, + 100 %); over the whole tree including testdata the measure is 271 of + 322 (84.2 %). + ### Fixed - **Rename edits land in their own documents.** A rename collected the diff --git a/README.md b/README.md index 3855802..625ea30 100644 --- a/README.md +++ b/README.md @@ -129,10 +129,13 @@ can emit today is narrower, and a recognised but unencodable instruction is reported as an explicit error, never as a wrong byte. The same measurement runs over GOROOT's whole assembly corpus: -`gasm audit-instructions --corpus` reports 291 of 353 attemptable files -(82.4 %) assembling for every target architecture today (files named for -other Go ports are counted but never attempted), with the top failure -reasons per architecture; the number moves with every release. +`gasm audit-instructions --corpus` reports every real-code GOROOT assembly +file (the tree without testdata) assembling for every target its build +admits: 250 of 250, 100 %. Over the whole tree including testdata the +measure is 271 of 322 attemptable (84.2 %); files named for other Go ports +are counted but never attempted, and `//go:build` constraints decide which +targets attempt a file at all, exactly as the build does. The number moves +with every release. ### Validation status diff --git a/cmd/gasm/asmhdr_test.go b/cmd/gasm/asmhdr_test.go index 5691b89..fbfa653 100644 --- a/cmd/gasm/asmhdr_test.go +++ b/cmd/gasm/asmhdr_test.go @@ -393,6 +393,52 @@ func TestRunCorpusAuditGOOS(t *testing.T) { } } +// TestRunCorpusAuditBuildConstraint covers the //go:build classification end +// to end: a generic-named file whose constraint admits one target is +// attempted there alone (cpu_x86.s on amd64), and a file whose constraint +// admits none of the four targets is never attempted (the msan and +// goexperiment trees). +func TestRunCorpusAuditBuildConstraint(t *testing.T) { + dir := t.TempDir() + write := func(name, src string) { + t.Helper() + if err := os.WriteFile(filepath.Join(dir, name), []byte(src), 0o644); err != nil { + t.Fatal(err) + } + } + write("x86.s", "//go:build 386 || amd64\n\nTEXT \xc2\xb7f(SB), NOSPLIT, $0\n\tRET\n") + write("racey.s", "//go:build race\n\nTEXT \xc2\xb7r(SB), NOSPLIT, $0\n\tRET\n") + write("plain.s", "TEXT \xc2\xb7p(SB), NOSPLIT, $0\n\tRET\n") + + stats, err := runCorpusAudit(dir, nil) + if err != nil { + t.Fatalf("runCorpusAudit: %v", err) + } + tally := func(name string) *corpusTally { + for i, tg := range stats.targets { + if tg.name == name { + return stats.tallies[i] + } + } + t.Fatalf("no tally for %s", name) + return nil + } + if stats.narrowed != 1 || stats.excluded != 1 || stats.generic != 1 { + t.Errorf("buckets = narrowed %d, excluded %d, generic %d; want 1, 1, 1", stats.narrowed, stats.excluded, stats.generic) + } + if a := tally("amd64"); a.attempted != 2 || a.assembled != 2 { + t.Errorf("amd64 = %d/%d, want 2/2 (x86.s and plain.s)", a.assembled, a.attempted) + } + for _, name := range []string{"arm64", "riscv64", "loong64"} { + if a := tally(name); a.attempted != 1 || a.assembled != 1 { + t.Errorf("%s = %d/%d, want 1/1 (plain.s only)", name, a.assembled, a.attempted) + } + } + if stats.full != 2 { + t.Errorf("full = %d, want 2 (x86.s over its one target, plain.s over all four)", stats.full) + } +} + // TestGenerateGoAsmHeaderRuntime pins the generator against the real thing: // the runtime package of the ambient toolchain, whose header the toolchain's // own -asmhdr output was sampled from. Skipped in short mode: it type-checks diff --git a/cmd/gasm/audit.go b/cmd/gasm/audit.go index d8c9daf..62427e5 100644 --- a/cmd/gasm/audit.go +++ b/cmd/gasm/audit.go @@ -5,6 +5,7 @@ package main import ( "fmt" + "go/build/constraint" "maps" "os" "os/exec" @@ -476,8 +477,10 @@ type corpusStats struct { root string files int generic int // files attempted for all four architectures + narrowed int // files whose //go:build admits a proper subset of the four + excluded int // files whose //go:build admits none of the four: never compiled otherPort int // files named for another Go port: never attempted - full int // files that assembled for every target architecture + full int // files that assembled for every applicable target architecture targets []corpusTarget tallies []*corpusTally } @@ -573,6 +576,57 @@ func otherGOOSFile(path string) bool { return false } +// buildConstraint returns the file's leading //go:build expression, or nil +// when the file carries none. The constraint governs the same header block +// go/build reads: blank lines and comments may precede it, and the first +// line that is neither ends the block. A constraint that does not parse +// narrows nothing, so the file stays in the attempted set: the audit must +// never exclude a file the toolchain would compile. +func buildConstraint(src string) constraint.Expr { + for line := range strings.SplitSeq(src, "\n") { + t := strings.TrimSpace(line) + switch { + case t == "": + continue + case strings.HasPrefix(t, "//"): + if constraint.IsGoBuild(t) { + e, err := constraint.Parse(t) + if err != nil { + return nil + } + return e + } + continue + default: + return nil + } + } + return nil +} + +// unixOS is go/build's unixOS set: the GOOSes the unix build tag admits. +var unixOS = map[string]bool{ + "aix": true, "android": true, "darwin": true, "dragonfly": true, + "freebsd": true, "hurd": true, "illumos": true, "ios": true, + "linux": true, "netbsd": true, "openbsd": true, "solaris": true, +} + +// constraintTags answers the build tags a plain `go build` sets for a +// target: the GOOS and GOARCH, gc, and unix on the unix-like GOOSes. No +// experiment, sanitiser or cgo tag is ever true: the audit models the +// default build, and no GOROOT assembly file's constraint hinges on cgo. +func constraintTags(goarch, goos string) func(string) bool { + return func(tag string) bool { + switch tag { + case goarch, goos, "gc": + return true + case "unix": + return unixOS[goos] + } + return false + } +} + func runCorpusAudit(root string, dirs includeDirs) (*corpusStats, error) { files, err := asmFiles(root) if err != nil { @@ -590,8 +644,8 @@ func runCorpusAudit(root string, dirs includeDirs) (*corpusStats, error) { tallies[i] = &corpusTally{reasons: map[string]int{}, example: map[string]string{}} } // full is the north-star number: a file counts when every architecture - // its name allows assembles it. - full, generic, otherPort := 0, 0, 0 + // its build admits assembles it. + full, generic, otherPort, narrowedCount, excluded := 0, 0, 0, 0, 0 // Header generation is created on first use, so a corpus with no // go_asm.h includes never pays for a temp directory. @@ -614,28 +668,78 @@ func runCorpusAudit(root string, dirs includeDirs) (*corpusStats, error) { // invisible to a file-name rule). goos := goosFromFilename(path) + // The GOOS the header generation type-checks under follows the + // file's name when the name carries one; the ambient GOOS is the + // honest guess otherwise. + namedArch := arch.FromFilename(path) var wanted []int // indexes into targets - if a := arch.FromFilename(path); a != arch.Unknown { + other := false + switch { + case namedArch != arch.Unknown: for i, tg := range targets { - if tg.a == a { + if tg.a == namedArch { wanted = append(wanted, i) } } - } else if otherPortFile(path) { + case otherPortFile(path): // A file named for a Go port gasm does not support (arm, // 386, s390x, ...) or for another GOOS is compiled by no // supported-arch build, so it is neither generic nor a // per-arch attempt: counting it as generic would make the // headline unreachably low for reasons no supported target // can fix. + other = true otherPort++ - } else { - generic++ + default: for i := range targets { wanted = append(wanted, i) } } + // A //go:build constraint narrows the set of targets the file is + // assembled for, the way the go command compiles the file only for + // the targets the expression admits: cpu_x86.s belongs to the x86 + // build alone, and a file whose constraint admits none of the four + // targets (the goexperiment.runtimesecret and msan trees) is + // compiled by no supported build. The tags mirror what a plain + // `go build` sets: the GOOS and GOARCH, gc, and unix on the + // unix-like GOOSes; no experiment, sanitiser or cgo tag is ever + // true. The GOOS is the file's own when the name carries one, + // else the ambient one. + goosForEval := goos + if goosForEval == "" { + goosForEval = runtime.GOOS + } + narrowed := false + if len(wanted) > 0 { + if ce := buildConstraint(src); ce != nil { + kept := make([]int, 0, len(wanted)) + for _, i := range wanted { + tg := targets[i] + if ce.Eval(constraintTags(goarchName(tg.a), goosForEval)) { + kept = append(kept, i) + } + } + if len(kept) < len(wanted) { + narrowed = true + } + wanted = kept + } + } + + switch { + case other: + // already tallied above + case len(wanted) == 0: + excluded++ + case namedArch != arch.Unknown: + // a per-arch attempt over the constraint's subset + case narrowed: + narrowedCount++ + default: + generic++ + } + // A file that includes go_asm.h parses against a per-target header: // the defines differ per architecture (internal/cpu's layout, for // one) and per GOOS (sys_darwin_arm64.s's trampoline constants, @@ -719,6 +823,8 @@ func runCorpusAudit(root string, dirs includeDirs) (*corpusStats, error) { root: root, files: len(files), generic: generic, + narrowed: narrowedCount, + excluded: excluded, otherPort: otherPort, full: full, targets: targets, @@ -728,13 +834,15 @@ func runCorpusAudit(root string, dirs includeDirs) (*corpusStats, error) { // printCorpusStats renders the corpus audit report. func printCorpusStats(s *corpusStats, list bool) { - fmt.Printf("corpus %s: %d files (%d generic, attempted for all architectures; %d named for other Go ports, never attempted)\n", s.root, s.files, s.generic, s.otherPort) + fmt.Printf("corpus %s: %d files (%d generic, attempted for all architectures; %d narrowed by //go:build; %d excluded by //go:build; %d named for other Go ports, never attempted)\n", + s.root, s.files, s.generic, s.narrowed, s.excluded, s.otherPort) // The rate is over the files a supported build would attempt: the - // other ports' files sit in the count for completeness but can never - // assemble, so counting them in the denominator would report the gap - // of architectures gasm deliberately does not target. - attemptable := max(s.files-s.otherPort, 1) - fmt.Printf(" assemble for every target architecture: %d of %d attemptable (%.1f%%)\n", s.full, attemptable, 100*float64(s.full)/float64(attemptable)) + // other ports' files and the ones no supported target compiles sit in + // the count for completeness but can never assemble, so counting them + // in the denominator would report the gap of platforms gasm + // deliberately does not target. + attemptable := max(s.files-s.otherPort-s.excluded, 1) + fmt.Printf(" assemble for every applicable target: %d of %d attemptable (%.1f%%)\n", s.full, attemptable, 100*float64(s.full)/float64(attemptable)) for i, tg := range s.targets { t := s.tallies[i] fmt.Printf(" %s: %d/%d attempted\n", tg.name, t.assembled, t.attempted) diff --git a/cmd/gasm/audit_test.go b/cmd/gasm/audit_test.go index 99d1426..03f26a0 100644 --- a/cmd/gasm/audit_test.go +++ b/cmd/gasm/audit_test.go @@ -4,6 +4,7 @@ package main import ( + "runtime" "testing" "sourcedock.dev/petrbalvin/gasm-devkit/arch" @@ -64,3 +65,62 @@ func TestGasmEncodable(t *testing.T) { } } } + +// TestBuildConstraint pins the //go:build reader: the constraint governs the +// leading comment block, the first non-comment line ends it (a tag below a +// #include governs nothing, exactly as go/build drops it), and a file +// without one admits every target. +func TestBuildConstraint(t *testing.T) { + admits := func(src, goarch, goos string) bool { + t.Helper() + e := buildConstraint(src) + if e == nil { + return true + } + return e.Eval(constraintTags(goarch, goos)) + } + const ret = "TEXT \xc2\xb7f(SB), NOSPLIT, $0\n\tRET\n" + cases := []struct { + name string + src string + amd64, arm64 bool + }{ + {"no constraint", ret, true, true}, + {"x86 only", "//go:build 386 || amd64\n\n" + ret, true, false}, + {"arm64 and linux", "//go:build arm64 && linux\n\n" + ret, false, true}, + {"msan never", "//go:build msan\n\n" + ret, false, false}, + {"experiment never", "//go:build goexperiment.runtimesecret\n\n" + ret, false, false}, + {"below an include governs nothing", "#include \"textflag.h\"\n//go:build amd64\n" + ret, true, true}, + {"unparsable narrows nothing", "//go:build (amd64\n" + ret, true, true}, + } + for _, c := range cases { + t.Run(c.name, func(t *testing.T) { + if got := admits(c.src, "amd64", runtime.GOOS); got != c.amd64 { + t.Errorf("amd64 admission = %v, want %v", got, c.amd64) + } + if got := admits(c.src, "arm64", runtime.GOOS); got != c.arm64 { + t.Errorf("arm64 admission = %v, want %v", got, c.arm64) + } + }) + } +} + +// TestConstraintTags pins the tag set a plain `go build` sets: the GOOS and +// GOARCH, gc, unix on the unix-like GOOSes; nothing else is ever true. +func TestConstraintTags(t *testing.T) { + ok := constraintTags("amd64", "linux") + for _, tag := range []string{"amd64", "linux", "gc", "unix"} { + if !ok(tag) { + t.Errorf("tag %q = false, want true", tag) + } + } + for _, tag := range []string{"arm64", "freebsd", "darwin", "cgo", "race", "msan", "goexperiment.runtimesecret"} { + if ok(tag) { + t.Errorf("tag %q = true, want false", tag) + } + } + fb := constraintTags("arm64", "freebsd") + if !fb("unix") { + t.Error("unix on freebsd = false, want true") + } +}