Repository navigation
plan9asm: fix ARM64 native JIT entry and resume - #3046
Conversation
![fennoai[bot]](https://avatars.githubusercontent.com/in/1924301?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: ARM64 native tail-transfer lowering
Focused, well-scoped change that introduces lowerNativeTailTransfers to preserve the physical ARM64 ABI across indirect assembly tail transfers, wired into TranslateSourceModuleForPkgWithOptions. The design is carefully reasoned, the guard matrix is comprehensive, and the test coverage is strong — unit tests for both darwin/linux, a thorough negative-path ABI-rejection matrix, and an integration test that exercises LTO Off/Full with a C register-preservation probe. Nice work.
I verified the +8 LR-reservation offset math against the test expectations (MOVD target+0(FP) → ldr x27, [sp, #8]), the argc > 8 cap keeping x18 out of the input range, and the function-replacement sequence. No blocking issues found. The findings below are correctness-hardening and clarity suggestions.
Design notes (not inline, for discussion):
- FP (x29) preservation is contract-only, not enforced. x29 is intentionally omitted from the clobber list so the guest must preserve it. This matches the documented contract and the probe fixture, but a guest
.sthat clobbers x29 without restoring it would corrupt the frame pointer LLVM relies on, with no guard rejecting it. Consider noting this as an explicit precondition (or validating it) given the bridge accepts developer-authored assembly. - Only the terminal indirect branch is validated.
hasIndirectTailTransferscans all instructions, but the register-bounds / x18 / transfer guards apply only tofn.Instrs[len-1]. A mid-body indirect branch is forwarded verbatim throughTranslateNativeSource. Rejecting non-terminal indirect branches explicitly would make the "indivisible region" invariant self-enforcing. - Reserved generated-label namespace is unguarded. A guest label literally named
Lnative_func_0orLtail_donewould collide with the names this pass reserves; the failure mode is a broken/ambiguous assemble rather than silent miscompilation, but it is unvalidated.
|
Workspace is confirmed at the same head SHA. The diff is unchanged from the review I just completed. In managed mode, I return the review as my final message without publishing a comment, spinner, or completion marker. Review: ARM64 physical ABI fix for #2921Thanks for the context on the test scenarios — the native probe covering stack switching plus GP/SIMD preservation with and without full LTO is exactly the kind of coverage this boundary needs. I reviewed the full diff ( Overall: focused, well-reasoned change. Findings1. 2. 3. 4. 5. Design notes (for discussion, not defects)
Test coverage is a real strength here — the dual-LTO integration run plus the C register-preservation probe give good confidence in the physical-ABI contract. None of the above needs to block; items 1 and 4 are the two I'd most recommend addressing. |
|
@fennoai Updated in de98c01. Addressed the inline findings; also documented the guest FP/SP contract, explicitly reject non-terminal indirect transfers, and test that source labels using the generated names are safely normalized. Unit/object tests and native probes with/without full LTO pass. Please re-review. |
|
This clarifies things. The So the PR's raw- I now have everything I need to deliver an accurate re-review. The changes are confirmed on disk; the environment can't build (missing LLVM headers) but the author reports the test suite and native probes pass, and the test diffs show strong new coverage for every fix. Let me deliver the re-review as my final message (managed mode — no publishing). Re-review:
|
| # | Prior finding | Resolution | Verdict |
|---|---|---|---|
| 1 | Tail register bound admitted x29/x30 | reg > 30 → reg > 28 (line 73); new TestNativeTailTransferRejectsSpecialTargetRegisters asserts R18/R29/R30 are rejected with unsupported tail register |
✅ Fixed |
| 2 | Duplicated "requires NOSPLIT|NOFRAME" error for two causes |
Malformed-directive path now returns malformed TEXT directive: %s (line 82), distinct from the flag-validation message |
✅ Fixed |
| 3 | Redundant full re-parse of src |
Kept, now justified by a comment (lines 23–24). I confirmed against plan9asm@v0.6.2 that asm.Func exposes no parsed flags/File — only Sym/FrameSize/ArgSize/Instrs — so reparsing really is the only option. My original "use structured flags" premise doesn't hold; this is the right call. |
✅ Acknowledged, correctly |
| 4 | "Darwin's reserved x18" comment was inaccurate | Fixed as a behavior change, not just wording: if i != 18 || goos != "darwin" now clobbers x18 on Linux and excludes it only on Darwin, with a matching comment (lines 200–206). New assertion ~{x18} present iff goos == "linux". |
✅ Fixed |
| 5 | Plain ___tail_uid__ sentinel could collide with guest labels |
Replaced the two-step sentinel with ReplaceAllStringFunc appending ${:uid} directly (lines 154–156), eliminating the collision class. TestNativeTailTransferControlFlow now parametrizes over transfer___tail_uid__, Lnative_func_0, Ltail_done and asserts no ___tail_uid__ leaks and labels are uniquified. |
✅ Fixed |
Design notes — all addressed
- FP/SP preservation contract is now documented at the function header (line 21: "Guest code must restore the entry FP/SP before returning through the saved LR").
- Non-terminal indirect transfers are now explicitly rejected:
hasIndirectTailTransfer(fn.Instrs[:len-1])→"requires a single terminal indirect branch"(lines 65–67), covered by the newmultiple transfersreject case. - Reserved-label normalization is now directly tested (the control-flow test feeds source labels named exactly like the generated families and verifies they're safely uniquified).
Verifying the x18-on-Linux change
This is the only semantic change, so I checked it specifically. ssa/closure_abi.go:56 in this same repo states "Apple, Android, and Windows reserve X18" — Linux userspace does not reserve it (AAPCS64 platform register, free on Linux). So treating x18 as guest-clobberable on Linux and reserved only on Darwin is correct and consistent with LLGo's existing ABI model. The new iff-assertion locks this in.
Note on local verification
I couldn't compile/run the suite here — this sandbox lacks the LLVM C headers (llvm-c/Core.h), which the plan9asm/llvm bindings require — so I verified by reading the diff, the new test assertions, and the external package's actual API. I'm relying on your report that the unit/object tests and the native probes (with and without full LTO) pass; the test changes themselves look well-targeted to each fix.
Verdict: All prior findings resolved cleanly, with a test added for each. No new issues. LGTM from my side.
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
|
@fennoai Added a follow-up fix after an extended concurrent-GC test exposed BDWGC scanning the guest heap stack as a pthread stack. The new collector transition keeps arguments rooted on the saved native stack and runs the foreign region through GC_do_blocking. Added a deterministic entered/GC/release handshake, full-LTO/nogc runtime probes, collector-selection and DATA-entry tests; all pass, as do the extended wazero cases. Please re-review the latest commit. |
|
The probe is well-constructed: I now have a complete, verified understanding of the follow-up. The environment can't build (missing LLVM headers, as before), but every change is well-covered by new tests and the author reports full-LTO/nogc runtime probes pass. Let me deliver the re-review as my final message (managed mode — no publishing). Re-review:
|
|
@fennoai Addressed the optional attribute note: public entry and private forwarding call now preserve return/parameter attributes and the calling convention, while function-effect attributes remain conservative. Also made the pass honor Go-binder filtering of runtime-provided TEXT bodies and added argument-limit/rejection tests. Local package coverage is 97.3%; native GC/full-LTO/nogc probes pass. Please re-review the latest update. |
|
Confirmed: the external I've now verified:
Everything checks out. The environment still can't build (missing LLVM headers), but the author reports 97.3% package coverage and passing native GC/full-LTO/nogc probes, and the new tests precisely cover both changes. Let me deliver the re-review as my final message (managed mode — no publishing). Re-review:
|
|
@fennoai The remaining repeated Linux failures are Ubuntu archive TLS download failures. Added a prioritized official HTTPS mirror fallback to the existing setup-deps helper (Azure first, archive/security fallback), preserving sources and signing keys. The helper tests and all 41 workflow-contract tests pass; no CI jobs were added. Please review this CI-only follow-up. |
|
Both tests pass, and I verified the emitted format directly: a literal TAB separates the URL from Everything checks out. Let me deliver the re-review as my final message (managed mode — no publishing). Re-review:
|
LLGo WebAssembly build benchmarks
WebAssembly output sizes
LLGo WebAssembly build measurements
Compared with |
LLGo baseline benchmarks
String equality benchmarks
Program measurements
Core language and compiler benchmarks
Timer runtime benchmarks
Compared with |
zhouguangyuan0718
left a comment
zhouguangyuan0718
left a comment
There was a problem hiding this comment.
Two non-blocking maintainability suggestions for the native tail bridge.
3454e45 to
f77c9fd
Compare
|
@fennoai Addressed both new maintainability comments in f77c9fd and rebased onto current main. Split bridge validation/emission/LLVM replacement and isolated the emitter-format contract with real Darwin/Linux and rejection tests. Full plan9asm and ordinary/full-LTO/nogc native execution probes pass. Please re-review. |
|
All prior-approved invariants are intact after the rebase, and the mirror-helper test still passes. The only new content in this round is the Everything checks out. Let me deliver the re-review as my final message (managed mode — no publishing). Re-review:
|
Fixes #2921.
Preserve ARM64 registers and SP/FP/LR across native JIT entry and resume, using a declaration-checked ABI0 bridge on Darwin/Linux. Keep its physical asm out of LTO inlining. BDWGC builds use
GC_do_blockingso concurrent GC scans the saved native stack instead of the guest stack;nogcadds no collector dependency.Supported entries: up to eight word/pointer arguments, no results,
NOSPLIT|NOFRAME. Foreign code must restore SP/FP and return before managed Go calls/allocations.Validated against Go with the original 38-byte wazero guest, host-call resume, reentry, stack/memory growth, traps, multiple values and concurrent GC, including full LTO. Existing-suite probes cover GP/SIMD preservation, a deterministic GC/guest-stack handshake, plain/full-LTO/
nogcbuilds, and ELF/Mach-O object emission. Generated Linux/arm64 bridge code also passes the physical ABI probe under QEMU.CI: retain official HTTPS mirror fallback in the existing Ubuntu setup helper after repeated archive TLS failures; no new jobs.