feat: add hidden closure environment ABI - #2255
Conversation
LLGo baseline benchmarks
Program measurements
Core language and compiler benchmarks
Compared only with the latest matching platform in the main series. |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
![fennoai[bot]](https://avatars.githubusercontent.com/in/1924301?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review summary
Reviewed the hidden closure-environment ABI change across the compiler (ssa/, cl/), the C libffi bridge, and the reflect runtime. The design is careful and well-commented; the env-vs-no-env distinction, the zero-sized sentinel, and the reentrancy handling in llgo_ffi_call_with_env (save/restore of previous) all check out for the normal synchronous paths. The docs (doc/closure.md) match the implementation.
A few concrete findings are left as inline comments. Cross-cutting notes that don't map to a single line:
Performance (worth a benchmark vs. the base commit):
runtime/internal/clite/ffi/_wrap/libffi.c— theffi_call_gofast path is only weakly linked and is disabled on Apple targets (LLGO_FFI_GO_ABI_MATCHESexcludes__APPLE__). Per the code's own comment, common system libffi builds omit that symbol, so on typical Linux and all macOS the slow trampoline is the default path for every env-bearing FFI call. That path spills/reloads the whole argument register file, marshals arguments twice, and adds an extra out-of-line call. Sincereflect.Value.callnow routes throughffi.CallWithEnv, this is on the reflect hot path — worth measuring the regression.runtime/internal/lib/reflect/makefunc.gomakeMethodValue— each method value now allocates a libffi closure (ffi_closure_alloc, mmap-backed executable memory) plus a heap funcval and an extra indirection per call, versus the previous direct funcval. If method values are created in loops this is a real allocation/perf cost; consider caching per(type, method).
Memory-safety (low severity, documentation-only asks):
runtime/internal/clite/ffi/_wrap/libffi.c— the aarch64 Apple/Android and arm32 trampolines stash callee-saved registers/return address in the single_Thread_localcontext and restore them only after the real target returns normally (matching thepreviousrestore inllgo_ffi_call_with_env). A non-local exit that unwinds across theffi_callframe (longjmp-style) would skip both restores, leaving callee-saved registers clobbered and the TLS record stale. Synchronous nesting and balanced signal-handler nesting are safe. Recommend a comment asserting the "target must return normally / every entry goes through the balanced save/restore" invariant next to the struct definition.runtime/internal/lib/reflect/makefunc.go:74—keepAlivestores the interior pointer&closure.Fnto retain the whole*ffi.Closure. This relies on interior-pointer retention in the current GC; storingclosuredirectly would be more robust. No live bug.
a009e26 to
5b379e0
Compare
|
Review follow-up is pushed on top of current
I also folded the independently reproduced The review performance concern already has same-machine A/B data in the PR body. Caching reflect method-value closures is a separate allocation/lifetime design change and is intentionally not included here. |
| } | ||
|
|
||
| func makeFunc(typ Type, method bool, fn func(args []Value) (results []Value)) Value { | ||
| func makeFunc(typ Type, fn func(args []Value) (results []Value)) Value { |
There was a problem hiding this comment.
这里不再需要 makeFunc 函数，可以合并到 MakeFunc 函数。
There was a problem hiding this comment.
已在后续提交中将 makeFunc 内联到 MakeFunc；rebase 后该修复仍保留在 fbce776。
b407f0e to
572a29f
Compare
2 participants
Summary
llssa.Functionnestorswiftselfon validated native targets; Wasm and portable fallbacks use exact typed env/no-env edges__llgo_stubcall layersret, callback-wrapper, and C ABI loweringCallWithEnvbridge; no libffi rebuild is required{fn, env}with nil meaning no physical env, while eliding provably zero-sized lexical environmentsBenefits
Platform selection
nestin ECX / R10ffi_call_gowhenFFI_GO_CLOSURESis available; otherwise use publicffi_call+ TLS trampolinenestin t2/x7ffi_call_go; libffi Go closures are requirednestin X18ffi_call_go; libffi Go closures are requiredswiftselfin X20ffi_call+ TLS trampoline because libffi's Go ABI uses X18swiftselfin R10ffi_call_go+ register bridge from libffi R12 to R10nestffi_call_gowhen available; LLGo does not yet support WindowsswiftselfSelection is based on the physical LLVM target and libffi's
FFI_GO_CLOSUREScapability, rather than a hard-coded package GOOS/GOARCH pair. The build uses
pkg-config libffiplus-lffi; no Homebrew-specific libffi path or libffirebuild is required.
Review follow-up
//llgo:envand// llgo:envthrough the shared directive parser0x48as the next available opcode; no reserved gap is retainednestt2/x7 transport and its match with the libffi bridge; ESP32-C3 exercises the riscv32 pathValidation
3d73ab8dad1794f9b352a6a3c9758b74199129ca4e1aa4fb):go test ./ssago test ./internal/cabigo test ./internal/buildgit diff --check xgo-dev/main...HEADffi_call+ TLS fallback under Rosetta, verifying that the target receives env in R10sretSupersedes #2248, whose closed head cannot be reopened after the fork branch was force-pushed.
Closes #2170