Conversation
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
f5906c0 to
e92e918
Compare
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.
| if len(ins.Args) != 2 || ins.Args[1].Kind != OpReg { | ||
| return true, false, fmt.Errorf("amd64 PSUBUSB expects Xsrc, Xdst: %q", ins.Raw) | ||
| } | ||
| src, err := c.loadXVecOperand(ins.Args[0]) |
There was a problem hiding this comment.
[P2] PSUBUSB/PABSD emit source IR before the loadX fall-through
loadXVecOperand(ins.Args[0]) emits IR into c.b before dst, err := c.loadX(ins.Args[1].Reg), whose error branch returns false, false, nil ("unhandled", fall through to other lowerers). If Args[1] is a non-X register, the already-emitted source-load IR is left orphaned in the buffer and a later lowerer may re-emit the same operand.
Contrast the PMINUB/min-max cases just below (line ~2446), which load dst first and are therefore safe. PTEST (line 2414) has the same ordering as here.
Suggest validating that Args[1].Reg is an X register before emitting any source IR (mirroring the min/max pattern), so the fall-through happens before IR is written. Impact is low in practice (the parser rarely yields a non-X second operand for these P-prefixed ops), but the ordering is inconsistent with the sibling cases.
Summary
386,amd64,arm,arm64, andwasm) from Go 1.27 tables, with positive/negative form tests and LLVM 22 object checks..spath for current and future platforms.@latestversions.xgo-dev, using LLVM 22 only.Discovery funnel
The committed checkpoint is one
manifest.jsonplus 256sha256(module)[0]JSONL record shards. Files are sorted by module path, Go semantic version, and record kind. There are no run directories, gzip files, or ZIPs in the final PR history.module@versionrecordsmodule@latestbefore its exact-version seen check.spaths in 1,758 module-version/source directoriesAll applicable discovered assembly passes translation and LLVM 22 compilation. N/A is accepted only with structured Go source/build/assembler/asmdecl evidence; it is not an unsupported-form escape hatch.
Scope of the public index
The separate census at the fixed 2026-09-13 cutoff counted 55,128,672 default-index
module@versionentries and 2,856,345 unique module paths. This PR's checkpoint covers:This measures the public default Go module index, not
include=all, private modules, or modules never served by the Go services.Future-platform replay
asm_filesis the durable fact source;architecturesis a derived query index. It includes all current Go architecture suffixes, not only architectures currently lowered by plan9asm. Unknown, unsuffixed, and custom-suffixed files are conservatively retained.For a newly supported platform,
PLAN9ASM_DISCOVERY_TARGETS=<goos>/<goarch>queries only the saved matched exact versions, derives candidate package directories from their saved paths, and then reapplies the current Go filename/build constraints before translation. It does not read the module index or revisit the 117,230 no-assembly inspected versions. A reallinux/arm64replay reduced 661 matched versions to 316 eligible candidates; shard 30 selected 11, passed 7, classified 4 with source evidence, and failed 0.Independently rediscovered issue libraries
These are selected by the generic ledger path, not by 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 curated corpus contains 28 libraries and also passes across its 11-target matrix.
github.com/phuslu/logis updated tov1.0.133after the final@latestcheck.Instruction and form coverage
Development followed red/green TDD: each external or official-corpus failure was reproduced first, then the complete Go 1.27 instruction family and legal/illegal operand rows were checked before lowering.
main(895 added after the earlier PR head, with no supported-op regression).main.xfailwas added. Corpus failures remain hard failures; only structured source/target N/A evidence is accepted.Major completed families include:
The exhaustive encoder reports still classify currently unimplemented table rows explicitly rather than skipping them; this PR claims completeness for the discovered external corpus and the Go standard-library matrix, not every decoder-only/unused optab row.
Validation\n\nFinal CI: run 34897723166 completed 91/91 jobs successfully with no failed or cancelled job.\n\n-
go test ./... -count=1, plus both nested command modulesgo test ./... -race -count=1go build,go vet, andgo mod verify386,amd64,arm,arm64, andwasmgit diff --check, ledger integrity/dedup/semver/hash tests, and no committed.gz/.zipcmd/plan9asm31.7%,cmd/plan9asmll51.4%; Codecov patch coverage 88.09% (target 87.59%)CI and Codecov now pass. This PR intentionally remains Draft until review passes.