Repository navigation
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.
68e5f23 to
11d9c12
Compare
|
Pushed the six-commit repair batch through Repairs:
Verification:
Native Windows verification remains pending. Remaining known failures include Windows feature/CPP path resolution, the full Windows root suite's deadline, unresolved SDK macros in the macOS standard-library scanner, and 21 real benchmark compilation failures involving runtime/reflect/ABI/storage contracts. The benchmark remains 164/185 in the completed old run; no failures were relabeled as passes or skipped. The Linux container race receipt is source/lifecycle verification, not native x86 ISA certification under Rosetta. |
Scope: translator and independent instruction tests
This Draft PR expands Go Plan 9 instruction/form coverage across
386,amd64,arm,arm64andwasm. It retains typed instruction-family lowering, guardedABI/source observations, official Go assembler/decoder tests, standard-library
compilation, independent semantic regressions and LLVM 22 object/runtime gates.
External discovery, ledgers and third-party compatibility tests have moved to
llgo-compat/plan9asm#1.
That PR owns the scan funnel, inventory and external coverage status. This PR
does not include those datasets or depend on external corpus jobs.
Split and preservation
CI to the independent repository. Preserve every transferred ledger byte,
cursor, retry and historical outcome; migration does not invent passes.
gotoolprofileimplementation for the translator CLI andindependent harness, without duplicating the translator.
benchmarks and cross-runtime tests.
gotoolprofile, preserving actualpackage/root inventories, disjoint partitions and mandatory aggregation.
integrated repair tree. Additional instruction repairs remain in this PR.
Latest CI repair batch
stream/file identity checks and the CMPXCHG fault observer to their actual
platform behavior. The observer rejects setup errors, ordinary nonzero exits,
timeouts and cleanup failures instead of accepting them as instruction faults.
schemas. Keep producer hashes and mutation rejection intact.
API reentry checks in owned, bounded child processes.
instruction. A reproduced Go 1.22 heap-sampling mismatch is fixed without
masking state differences; 50 repeated checks pass on that repair snapshot.
CLI suites in independent jobs so a root timeout cannot suppress either one.
Verification
LLVM 22 is required; missing tools are failures. Reports retain their exact
frozen revision and scope rather than relabeling old results as full-CI passes.
The Go test receipts below are from frozen code revision
0527ded5; the finalworkflow/documentation follow-up is checked separately.
go vetcmd/plan9asmcomplete Go 1.27 suitecmd/plan9asmllcomplete Go 1.27 suiteThe current five-architecture Go 1.27.2 observed-form classification gate
passes without baseline changes in this repair batch:
These are classifications, not complete operand-grammar, ABI or runtime
coverage. Context is not a compilation pass. The earlier decoder-baseline
failure listed in this PR body is no longer the current gate outcome.
Remaining failures — keep Draft
The completed pre-repair CI run
at
432796c7has 98 successful, 49 failed and 1 cancelled jobs, includingaggregates. The repair batch requires fresh CI; those historical failures
remain failures.
with 21 failures. These include runtime/reflect entry and caller-frame
contracts, TLS/hidden registers, mutable AES storage and a CGO source boundary.
No baseline or skip converts them to passes.
isolating the nested CLI jobs does not fix the root suite's timeout.
CI. Rosetta/container execution and object compilation do not replace them.
marking this contribution ready.
External assembly verification and LLGo compile/link/execute compatibility keep
their separate ownership and completion gates in the corresponding repositories.