Conversation
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
e92e918 to
b55d1a1
Compare
6803f47 to
519f0f0
Compare
There was a problem hiding this comment.
Review summary
Large PR (~175k insertions across 518 files). The bulk is generated discovery testdata (testdata/discovery/ledger/**) and highly repetitive AMD64/ARM instruction-lowering tables, both of which I skimmed rather than reviewed line-by-line. I focused on the high-signal changes: the new discovery/scan tooling (cmd/plan9asmcorpus/discovery.go, cmd/plan9asmdiscover, cmd/plan9asmll, cmd/plan9asmscan), internal/discoverymeta, and the core translate/parser changes.
Overall this is solid, defensively-written code: file handles are consistently closed on error paths, zip extraction blocks zip-slip (proxy-prefix + path.Clean + filepath.Rel + O_EXCL), external commands use fixed argv with escaped module paths, and remote fetches are size-bounded. Doc/AGENTS.md claims (flags, scripts, committed manifest counts, test names) were verified accurate.
One inline correctness/consistency finding below, plus a few non-blocking notes.
Non-blocking notes:
-
Exponential build-tag search —
cmd/plan9asmcorpus/discovery.go(findDiscoveryBuildTags). The search iterates1 << len(customTags)masks, each doing filesystem-backedctx.MatchFilecalls, and the outerwantedCountloop re-walks the mask space ~len(customTags)+1times. With the 16-tag cap this is bounded but can reach millions ofMatchFilecalls for a package near the limit. Usually fine (tags are typically 0–2); consider iterating masks once (prioritizing by popcount) if large-tag modules become common. -
Symlink escape in local include expansion —
cmd/plan9asmll/main.go(pathWithinRoot/expandAsmIncludes). Containment is checked lexically viafilepath.Rel; a#includetarget that is a symlink pointing outside the source root would pass the check and be read (nofilepath.EvalSymlinks). Blast radius is contained (read-only, inlined into a discarded IR artifact, ephemeral CI against public modules; the zip extractor already rejects non-regular files), so this is defense-in-depth only. Resolving symlinks before the containment check would close the gap. -
Readability —
cmd/plan9asmll/main.go:988(and:1023).op == "JMP" || op == "B" || op == "RET" && len(ins.Args) == 1relies on&&binding tighter than||. The current behavior is correct, but explicit parentheses around(op == "RET" && len(ins.Args) == 1)would guard against a future||term silently changing the grouping.
Summary
386,amd64,arm,arm64, andwasm) using Go 1.27 tables, with positive/negative operand-form tests and LLVM 22 object checks.Discovery funnel
The committed checkpoint is one
manifest.jsonplus 256sha256(module)[0]JSONL record shards. Records are deduplicated and sorted by module path, Go semantic version, and record kind. No run directories, gzip files, or ZIPs are committed.module@versionrecordsThe invariant is
838 = 683 + 155 + 0; the aggregate verifier also proves every ledger match occurs once, in its correct shard, with the exact saved assembly inventory. The direct pass rate is 81.5036%; complete pass-or-evidence classification is 100%.Public-index scope
A separate fixed-cutoff census counted 55,128,672 default-index
module@versionentries and 2,856,345 unique module paths.This checkpoint covers the contiguous index interval from
2026-09-13T14:59:13.837766Zthrough2026-09-15T16:56:51.924668Z. Historical backfill to 2019 is not complete, so neither percentage is presented as whole-ecosystem completion.Independently rediscovered issue libraries
These were selected by the generic ledger path, independently of the curated manifest:
github.com/coder/websocket@v1.8.15github.com/klauspost/compress@v1.20.0github.com/tmthrgd/go-hex@v0.0.0-20190904060850-447a3041c3bcThe separate 28-library curated corpus also passes its complete 11-target matrix.
Official Go instruction coverage
Two different ratios are reported deliberately:
Across the non-context rows, current lowerability is
7,151 / 9,170 = 77.98%. Relative to current upstreammain, this PR adds 5,574 supported official form rows (1,577 → 7,151) and reduces unsupported rows by 5,593 (7,612 → 2,019). All five current reports have zero parse errors.The low 386/wasm observation ratios demonstrate why the Go standard library and compiler tests alone cannot be treated as the full encoder specification; external discovery and direct encoder-table/form tests remain necessary.
The ARM64 x/arch decoder corpus has 1,108 forms: 716 supported, 63 context, 329 decoder-only/unsupported, and zero parse errors. For the required families that the Go 1.27 assembler itself accepts, the gate is complete: 184 forms = 146 supported + 38 context + 0 unsupported.
Major completed instruction families include x86 scalar/vector arithmetic, moves, shifts, comparisons, shuffle/permute/blend/broadcast, conversion/rounding, x87, AES/CLMUL, AMD SVM system-management (VMRUN/VMMCALL/VMLOAD/VMSAVE/STGI/CLGI/SKINIT/INVLPGA), strings/port I/O and carry arithmetic; ARM scalar integer/floating arithmetic, conversion, shifts/div/mod, comparisons and transfers; ARM64 scalar/vector floating, conditional, widening multiply, matrix multiply, test/logical, divide/remainder, FP16 multiply and ABI/tail-call families; and wasm ABI/global/metadata plus F32/F64 unary lowering.
No xfail was added. The refreshed baselines record only reviewed positive coverage movement: compared with the prior branch baseline, 386 resolved 2 unsupported forms, amd64 158, ARM64 22, and the ARM64 x/arch corpus resolved 33.
Discovery mechanism
history_beforeandincremental_sinceare derived from exact contiguous index ranges, so gaps and overlaps fail validation.go vet -asmdeclevidence where relevant, translation, and LLVM 22 compilation. N/A cannot hide an unsupported form.asm_filesand replay only eligible exact versions without rereading the module index or revisiting no-assembly records.Validation
Validation on the current head:
go test ./... -count=1on Go 1.27, plus both nested modulesgit diff --checklinux/amd64CI runner; the localdarwin/arm64driver fails explicitly instead of skippingGitHub Actions is green (93/93 jobs), including 32/32 discovery shards and aggregate ledger verification. Codecov patch coverage is green at 87.62% against an 87.59% target. This PR remains Draft until review approval is complete.