Repository navigation
cl: preserve C variadic arguments in promoted method wrappers - #2840
Merged
cpunion merged 5 commits intoOct 18, 2026
Merged
Conversation
![fennoai[bot]](https://avatars.githubusercontent.com/in/1924301?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
This PR fixes #2838: promoted pointer-method wrappers that forward C variadic calls (e.g. w.Printf(...) through an embedded *CFmt) previously dropped the incoming ellipsis arguments. The fix emits a musttail variadic forwarding call (CallForwardVarargs) instead of a regular call, gated by forwardCVariadicWrapper.
Overall this is a well-crafted, carefully-scoped change. The gating predicate is conservative and its index accesses are safe (the len(call.Args) == 0 guard short-circuits before the call.Args[...] index, and HasNameValist + pointer-receiver guarantees fn.Params is non-empty). The musttail ABI invariant in CallForwardVarargs is validated correctly and fails closed (panic) rather than emitting unsafe IR. The nil-deref/AssertNilDeref receiver check is still emitted ahead of the forwarding call, and bypassing the recover-frame alias is justified because a musttail transfer removes the synthetic wrapper frame and the callee is always a C function.
Test coverage is strong: multi-target IR assertions, O0/O2 end-to-end runtime execution, varargs that overflow both GP and FP register sets after receiver adjustment, empty-varargs, nil-receiver panic, and negative cases (value receiver and external declaration) that lock in the gating boundaries.
The findings below are robustness/maintainability notes, not blocking defects. The main one is a latent panic path that is currently unreachable only because of the target gating.
Minor / non-blocking notes (no reliable inline anchor)
cl/instr.go:2829-2846— the 10-clause||guard inforwardCVariadicWrapperis dense; the safety of the two trailing index accesses depends on invariants established earlier in the same expression. Splitting into labeled early-return guards (with a one-line note that Recv + valist guaranteelen(fn.Params) >= 2) would make it more robust to reordering.ssa/expr.go:1497—"thunk"is a magic string also asserted verbatim by the unit test; a shared constant or a short comment on what the attribute signals to LLVM would reduce producer/test drift.cl/instr_variadic_method_test.go:46-47— matching the literal", ...)\n ret i32 "couples the test to LLVM's exact textual whitespace between the call andret; checking the two properties independently would be less brittle to future LLVM printer changes.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Collaborator
Author
|
Investigated the To check for an indirect compiler regression, I built compilers from base The native forwarding change explicitly excludes Wasm. I will retry the workers job after its workflow completes, without changing the runtime or relaxing its test deadlines. The separate Windows CRLF assertion failure is fixed in 9e18154 and is being verified by the new CI run. Update: the targeted workers rerun passed without further source changes or relaxed deadlines. All 12 workflows for |
LLGo baseline benchmarks
Program measurements
Core language and compiler benchmarks
Timer runtime benchmarks
Compared with |
LLGo WebAssembly build benchmarks
WebAssembly output sizes
LLGo WebAssembly build measurements
Compared with |
zhouguangyuan0718
left a comment
zhouguangyuan0718
left a comment
Collaborator
There was a problem hiding this comment.
Reviewed commit 9e18154. The thunk + musttail approach is appropriate for the supported pointer-receiver wrappers: forwarding the incoming ellipsis is required for correctness, including at O0. The prototype, calling-convention and result checks are useful safeguards. The focused cl/ssa tests and the O0/O2 execution regression passed locally on macOS arm64 with LLVM 22 and LLD 23.
One remaining correctness limitation should be tracked explicitly: promoted C variadic methods with a value receiver still lose their arguments when called through an interface. I reproduced this on this PR's head at O0: the example below should print 42 but prints -1.
main.go:
package main
import _ "unsafe"
const LLGoFiles = "_wrap/check.c"
type Receiver int32
//llgo:link Receiver.Check C.review_check
func (Receiver) Check(__llgo_va_list ...any) int32 { return 0 }
type Embedded struct{ Receiver }
type Checker interface {
Check(__llgo_va_list ...any) int32
}
//go:noinline
func check(v Checker) int32 {
return v.Check(int32(100), int32(200))
}
func main() {
e := Embedded{7}
println(check(&e))
}_wrap/check.c:
#include <stdarg.h>
#include <stdint.h>
int32_t review_check(int32_t receiver, ...) {
va_list args;
va_start(args, receiver);
int a = va_arg(args, int);
int b = va_arg(args, int);
va_end(args);
return receiver == 7 && a == 100 && b == 200 ? 42 : -1;
}Run the fixture with llgo run -O0 -ldflags=-w=true ./path/to/fixture.
This is an existing limitation deliberately excluded by forwardCVariadicWrapper, not a regression introduced by this PR, and it does not invalidate the fix for #2838. Please track the remaining unsupported forwarding forms in a follow-up issue and retain the narrow scope in the documentation. For calls whose incoming ellipsis cannot be forwarded correctly, a clear diagnostic would be preferable to silently generating an ordinary call that drops arguments. The negative ABI tests currently establish valid IR, not correct argument forwarding for these excluded cases.
I would keep this PR focused on the qualified ABI subset rather than expanding it to solve every remaining signature and target in the same change.
Collaborator
Author
|
@zhouguangyuan0718 Addressed in fedd13a.
Validation passed: go test ./cl ./ssa -run '^Test(CVariadic|IndirectCVariadic)' -count=1
go test ./internal/build -run '^TestCVariadicPromotedMethod$' -count=1The execution regression covers the original and mixed-argument fixtures at O0/O2. Correct forwarding or diagnostics for the excluded forms remains tracked by #2855. |
cpunion
force-pushed
the
codex/fix-c-variadic-interface-2838
branch
from
October 18, 2026 21:11
fedd13a to
a7e2843
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Calling a promoted C variadic method through an interface can corrupt its arguments at
-O0. The synthetic wrapper adjusts the receiver but drops the incoming ellipsis, so the existingvamethodexample prints garbage instead ofifmt (100,200).Emit LLVM
"thunk"wrappers withmusttailcalls to forward the incoming register and stack arguments unchanged. Check the fixed LLVM prototype, calling convention, and physical/logical result agreement before emitting the call, and omit recover-frame aliases around the tail transfer. This applies to hosted native pointer-receiver wrappers with scalar fixed parameters and at most one scalar result. Wasm, explicit-targetprofiles (including custom native profiles), value receivers, and aggregate/multiple-result signatures retain their existing lowering. These excluded forms are not fixed by this PR: their wrappers can still drop the incoming ellipsis. Follow-up #2855 tracks correct forwarding and diagnostics, including the reproduced value-receiver/interface failure.Add O0/O2 execution regressions for the original example and mixed integer/floating-point arguments that exceed the register sets, including nested promotion, value/pointer interface receivers, empty ellipses, and nil receivers. Add LLVM verification and object emission checks after C ABI lowering for 13 Linux, macOS, and Windows target configurations, plus external-declaration, fixed-pointer-parameter, non-variadic, incompatible-receiver, and aggregate/multiple-result checks. Separate SSA tests exercise mismatched fixed prototypes and result-conversion rejection on wasm32 independently of the compiler's target gate. The negative ABI tests establish valid IR and exclusion from the forwarding path; they do not establish correct argument forwarding for excluded forms.
Fixes #2838.
Validation
Passed locally on macOS arm64 with LLVM 22.1.8 and LLD 23.1.3:
go test ./cl -run '^Test(CVariadic|IndirectCVariadic)' -count=1go test ./internal/build -run '^TestCVariadicPromotedMethod$' -count=1go test ./cl -run '^TestRunAndTestFromTestrt$/^vamethod$' -count=1go test ./cl -skip '^Test(Run|BuildAndCheckSymbols)' -count=1 -timeout=10mgo test ./ssa -count=1go test ./internal/cabi -skip '^Test(Build|ABI)$' -count=1The new mixed-argument regression fails with the baseline compiler. Also verified the original example at O0 with DWARF enabled after temporarily applying #2836; that independent debug fix is not included here.
The full
internal/cabirun hit its five-minute timeout while building the ESP32 toolchain inTestBuild. Cross-target checks above verify and emit objects; execution was tested on macOS arm64.