test: discover and pin third-party assembly corpus - #38
Conversation
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
Review summary
Large, well-executed PR extending amd64/arm64 opcode lowering, adding a reported-library corpus tool, and expanding conformance/coverage testing. The lowering additions are careful about LLVM shift UB, sign-vs-zero extension, and NZCV flag computation, and are backed by strong executable conformance tests validated against native Go oracles. I verified the specifically risky areas (SHLW/SHRW/SARW masking, BSFW/BSRW, arm64 ROR/REV/bitfield, CSEL family, setFlagsSub32, F/V register aliasing) and found them correct.
Findings below. Nothing here is blocking merge outright, but the CI injection and the PSRAL zero-fill are worth addressing.
Additional non-inline notes (lower confidence / design-level):
- Serial compile pipeline (performance).
cmd/plan9asmll/main.goruns targets serially, tasks serially, and one blockingllcsubprocess per file. This PR raisesdefaultMatrixTargets()from 8 to 11 targets and the new corpus CI runs the full matrix per library, so wall-clock issum(targets) x files x (translate + llc)with zero parallelism, leaving CI cores idle. A bounded worker pool over tasks would cut runtime roughly linearly with core count. - No subprocess timeouts (robustness).
cmd/plan9asmcorpus/main.go(go list -m @latest,go mod download) andcmd/plan9asmll(llc) useexec.Command(...).Run()/CombinedOutput()with nocontext.WithTimeout. Since the corpus tool intentionally fetches network modules, a hung proxy or stuckllcblocks until the job-level timeout. Considerexec.CommandContextwith per-command deadlines. @lateston manifest-controlled module paths (supply chain).runLibraryresolves and downloads whatever module the manifest names, validated only for non-emptiness/uniqueness. Not shell-injectable (passed as argv), but worth constraining to an allowlist of expected module prefixes or pinned versions with-check-latest=falsein CI.- Vector XMM/YMM branch duplication (maintainability).
VPAND,VPCMPEQB/VPCMPGTB,VPUNPCKLBW/VPUNPCKHBWeach carry two near-identical blocks differing only by width/parse/load/store; extracting a width-parameterized helper would halve the code and reduce drift risk.
Additional findings
README.md:29: [P2] README target list is stale and now contradicts the code: This PR changeddefaultMatrixTargets()incmd/plan9asmll/main.goto return 11 targets, addinglinux/arm,js/wasm, andwasip1/wasm. The README still lists only the original 8 and stateslinux/arm"is not part ofplan9asmll -all-targets" (line 29) — now false. The new testTestDefaultMatrixTargetsCoversEveryPlan9Architectureasserts all 11, so the README directly contradicts shipped behavior and the test. Update the target list and remove/reword thelinux/arm/wasm exclusion sentence.
ae942f5 to
77eb094
Compare
|
Addressed the review findings and LLVM toolchain alignment in the latest head:
The post-merge PSRAL coverage-baseline repair was also split into the minimal PR #39 so main can be repaired independently. Local validation used LLVM 22.1.8: root/nested Go tests, actionlint, shellcheck, official assembler coverage, the complete stdlib architecture-level/cross-OS matrix, and all reported-library suites across 11 targets passed. |
f73c65d to
0305b80
Compare
94392cd to
c62965b
Compare
Summary
PR #35 and the standalone main repair in PR #39 have merged. This PR is rebased onto current
xgo-dev/mainand adds a strict, persistent third-party assembly coverage layer.index.golang.orgfeed andproxy.golang.org/cached-onlymodule ZIPs without executing module codemodule@version, including versions with no assembly; repeatable-seen-reportinputs skip only those exact versions, while failures remain retryable.splaceholders, while retaining unreadable or unusually large assembly for corpus validation rather than silently dropping it@latest, filter packages by exact owning module path, and fail on package-load errors, inventory drift, translation failures, or LLVM object failuresEcosystem scan and corpus
The continuous local scan has processed 210,000 official index records from
2019-04-10T00:00:00Zthrough2019-09-14T10:28:12.476893Z. Its consolidated schema-2 state contains:module@versionpairs, whether or not they contain assemblyThe report state is incremental and resumable. A newly published version is not hidden by an older successful scan of the same module.
The pinned manifest now contains 24 modules and 239 target-specific assembly translations across Darwin, Linux, Windows,
js/wasm, andwasip1/wasm:coder/websocket,klauspost/compress, andtmthrgd/go-hexanacrolix/mmsg,cespare/xxhashv1,klauspost/cpuidv1,klauspost/reedsolomon,minio/highwayhash,modern-go/gls,tmthrgd/go-bitwise,tmthrgd/go-popcount, andzeebo/thisExact module ownership prevents a v1 suite such as
klauspost/cpuidfrom silently including its nested/v2module. Content inspection also removed false coverage: the Darwinx/net.splaceholders are comment-only, andmodern-go/reflect2contains empty placeholders, so neither is counted as executable assembly.TDD instruction-family fixes
Native Go cross-assembly and failing real-library suites were used as the oracle before changing plan9asm. In addition to the earlier Snappy, LZ4, and go-bits regressions, the expanded corpus first reproduced failures in HighwayHash, Reed-Solomon, and go-popcount and then drove family-level fixes:
SUBB;BSFQ/BSRQ/BSFL/BSRLandPOPCNTL/POPCNTQregister-or-memory sources;PMULULQ;PUNPCKLLQ/PUNPCKHLQ;VPBROADCASTQXMM/GPR/memory sources with XMM/YMM/ZMM destinations; and Go ABI aggregate call argumentsMOVH/MOVHUplusBPL/BVS/BVCMOVKWhalves, two- and three-operandMULW,MADDW/MSUBW,CCMP/CCMPW/CCMN/CCMNW, andBVS/BVCBroadcast positive and negative forms were checked directly with the Go 1.27 amd64 assembler, preventing the shared lowering path from accidentally accepting invalid XMM destinations for
VBROADCASTF32X2/VBROADCASTSDor immediate sources. Related spellings and memory/register variants are tested together.The official Go assembler coverage baselines for Go 1.20 through Go 1.27 were updated only for the 12 newly supported amd64 forms across
PMULULQ,POPCNTL/Q,PUNPCKLLQ/HLQ, andVPBROADCASTQ. On Go 1.27, amd64 reports 767 supported and 5,969 explicitly unsupported forms with zero parse errors. Every supported architecture is scanned; unsupported forms remain visible and are not reclassified as a non-target architecture or skipped.Candidates are admitted only when their complete selected inventory passes.
minio/sha256-simdremains outside the green manifest after its unpack instruction was fixed because it additionally requires an unsupported[16][]bytesignature and inlineLONGraw machine code.Validation
go test ./...go vet ./...actionlintandshellcheckscripts/check-go-asm-coverage.shscripts/check-arm64-plan9-corpus.sh