plan9asm: support generated Go asm data and RET targets - #22
Conversation
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
FennoAI Review
Solid, well-tested PR. RET sym(SB) tail-call lowering is added uniformly across amd64/arm64/arm with matching multi-operand rejection, dataStmtPayload cleanly unifies string/integer payloads and is shared by both lowering paths, and the go_asm.h macro expansion correctly reuses the authoritative go/types sizing. Good test coverage in retjmp_test.go and parser_data_globl_test.go.
A few non-blocking findings below (2 inline). Additional minor notes that lack a reliable diff-line anchor:
- [P3] Unbounded GLOBL size in the text-emit path (
translate.go,emitDataGlobals):GLOBL x(SB),0,$<huge>flows intomake([]byte, sd.size)with no cap on this path, whiletranslate_module_direct.go:597caps at1<<31. Same class as the inline DATA-width note. Consider applying a shared cap. - [P3]
RET sym(SB)missing-signature handling differs by arch: amd64/arm64 fall back to the caller signature (csig = c.sig), but arm returns a hard error (arm tailcall missing signature). The same input can translate on two arches and fail on the third. Worth aligning or documenting. - [P3] Stale comment at
parser.go:168(accept "RET x" as generic for now):RET sym(SB)is now specifically modeled as a tail call, so the "generic for now" wording is outdated. - [P3] Codecov label at
.github/workflows/go-ci.yml:294still readsname: ubuntu-go1.25after the job's Go version was bumped to1.26.7. Cosmetic label mismatch. - [P3] Pinned patch vs
.x: single-version jobs pin exact1.26.7while matrices mix1.26.7with1.2x.xranges — slightly inconsistent.
|
Addressed the remaining review notes in 51c97bd and 541c2ab: ARM now uses the same missing-tail-signature fallback as AMD64/ARM64, the RET parser comment is current, all latest-version jobs consistently select Go 1.26.x, and the Codecov upload name is ubuntu-go1.26. Local validation passed full tests, race, and the Linux/Darwin amd64/arm64 stdlib corpus; measured changed-statement coverage is 97.30% (180/185). |
Summary
go tool compile -asmhdrfrom target-awarego/typesdataDATAstring payload bytes and cmd/asm-compatible zero paddingDATA/GLOBL, so their initialized globals are not silently droppedRET symbol(SB)as a tail call on ARM, ARM64, and AMD64 while preserving register-form returns such as Go 1.25 ARM64RET (R27)This was exposed by Go 1.26 GOROOT tests while validating LLGo Windows support:
test/asmhdr.gouses generated constants, struct layout macros, and a data-only assembly file;test/retjmp.gouses a symbol-targeted RET. The initial RET implementation also exposed an older Go 1.25 ARM64 register-return form, now covered by a regression test.Validation
go test ./...go test -coverprofile=/tmp/plan9asm-cover.out ./...asmhdr.goandretjmp.goexecution on Windows/ARM64; the same focused set also passes on macOSThe LLGo integration branch temporarily pins this fork commit for native Windows ARM64 GOROOT validation; it can return to an upstream version after this change is merged and released.