Repository navigation
Conversation
![fennoai[bot]](https://avatars.githubusercontent.com/in/1924301?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: typed-wrapper lowering for runtime.SetFinalizer
This is a well-structured change. The compiler-side lowering is careful: it gates strictly on llssa.PathOf(...) == "runtime" (so a user-defined SetFinalizer is left alone — nicely covered by TestSetFinalizerLoweringTargetsRuntimeOnly), recovers concrete operands from the SSA-boxed MakeInterface nodes, handles the nil-clear case, and preserves Call/Defer/Go timing via LowerSetFinalizerCall. setFinalizerArgCompatible mirrors Go's SetFinalizer argument rules, and the m[key] == e guard in RunPointerFinalizers correctly avoids clobbering a newer registration.
The main concern is a runtime concurrency asymmetry in the new registry (see the P1 inline finding). Remaining items are minor. Inline comments have the specifics.
| func pointerFinalizerCallback(ptr unsafe.Pointer, cb unsafe.Pointer) { | ||
| e := (*pointerFinalizerEntry)(cb) | ||
| pointerFinalizers.mu.Lock() | ||
| e.arg = ptr | ||
| if pointerFinalizers.tail == nil { | ||
| pointerFinalizers.head = e | ||
| } else { | ||
| pointerFinalizers.tail.next = e | ||
| } | ||
| pointerFinalizers.tail = e | ||
| pointerFinalizers.mu.Unlock() | ||
| } |
There was a problem hiding this comment.
[P1] pointerFinalizerCallback takes a lock in BDWGC finalizer context
pointerFinalizerCallback calls pointerFinalizers.mu.Lock(), but the sibling freeCleanupSlot in this same file carries an explicit warning that BDWGC-invoked callbacks "must not allocate or acquire a lock: BDWGC may invoke another finalizer while Go code is in a collector allocation or map operation" — and it is deliberately implemented lock-free with CAS. With the default GC_register_finalizer (no GC_set_finalize_on_demand), BDWGC can run finalizers synchronously from within GC_malloc. If a thread is inside SetFinalizerPtr holding pointerFinalizers.mu and an allocation there (e.g. &pointerFinalizerEntry{} or map growth) drives BDWGC to invoke pointerFinalizerCallback on the same thread, it self-deadlocks on the non-recursive mutex. Consider making the callback lock-free like freeCleanupSlot, or confirm/document why these callbacks can never run while pointerFinalizers.mu is held. Independently flagged by three reviewers.
| func SetFinalizerPtr(obj unsafe.Pointer, finalizer func(unsafe.Pointer)) { | ||
| if obj == nil { | ||
| panic("runtime.SetFinalizerPtr: ptr is nil") | ||
| } | ||
| pointerFinalizers.once.Do(initPointerFinalizers) | ||
| key := ^uintptr(obj) | ||
|
|
||
| pointerFinalizers.mu.Lock() | ||
| old := pointerFinalizers.m[key] | ||
| delete(pointerFinalizers.m, key) | ||
| if old != nil { | ||
| var ignoredFn bdwgc.FinalizerFunc | ||
| var ignoredCb unsafe.Pointer | ||
| bdwgc.RegisterFinalizer(obj, old.prevFn, old.prevCb, &ignoredFn, &ignoredCb) | ||
| } | ||
| if finalizer == nil { | ||
| pointerFinalizers.mu.Unlock() | ||
| return | ||
| } | ||
|
|
||
| e := &pointerFinalizerEntry{fn: finalizer} | ||
| var ignoredFn bdwgc.FinalizerFunc | ||
| var ignoredCb unsafe.Pointer | ||
| bdwgc.RegisterFinalizer(obj, pointerFinalizerCallback, unsafe.Pointer(e), &ignoredFn, &ignoredCb) | ||
| e.prevFn = ignoredFn | ||
| e.prevCb = ignoredCb | ||
| pointerFinalizers.m[key] = e | ||
| pointerFinalizers.mu.Unlock() | ||
| } |
There was a problem hiding this comment.
[P2] Global mutex + allocation on every SetFinalizer registration
SetFinalizerPtr holds pointerFinalizers.mu across a heap allocation (&pointerFinalizerEntry{...}), a map insert (potential rehash), and two bdwgc.RegisterFinalizer C-calls. This serializes every SetFinalizer call site in the program through one global lock (vs. Go's sharded finalizer tables), and the allocation-under-lock is the concrete trigger for the re-entrancy hazard in the P1 finding. Reducing the work done under the lock — or moving allocation/registration outside the critical section — would address both.
| wrapperSig := types.NewSignatureType(nil, nil, nil, | ||
| types.NewTuple(types.NewVar(token.NoPos, nil, "ptr", unsafePtr)), | ||
| types.NewTuple(), false) | ||
| name := fmt.Sprintf("%s$setFinalizer.%d", b.Pkg.Path(), atomic.AddUint64(&setFinalizerWrapperSeq, 1)) | ||
| wrapper := b.Pkg.NewFunc(name, wrapperSig, InGo) | ||
| wb := wrapper.MakeBody(1) | ||
| converted := wb.Convert(b.Prog.rawType(ptrType), wb.Param(0)) | ||
| if !types.Identical(argType, ptrType) { | ||
| converted = wb.Convert(b.Prog.rawType(argType), converted) | ||
| } | ||
| wb.Call(args[1], converted) | ||
| wb.Return() | ||
| fn := wb.MakeClosure(wrapper.Expr, nil) | ||
| obj := b.Convert(b.Prog.VoidPtr(), args[0]) | ||
| return runtimeFn, []Expr{obj, fn}, true |
There was a problem hiding this comment.
[P3] Per-call-site wrapper function increases code size
Each lowered SetFinalizer call site emits a fresh named function (%s$setFinalizer.%d, global sequence counter) plus a MakeClosure(..., nil). Since lowering is already gated to vkFuncDecl finalizers, the wrapper captures nothing and is effectively a static thunk — yet a distinct symbol is generated even for structurally identical wrappers. Consider caching wrappers keyed on the conversion shape (ptrType, argType, finalizerFn) to bound code-size growth. Runtime cost is fine (nil closure env); this is purely output size.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
c9e249e to
9ec1e7e
Compare
visualfc
changed the title
visualfc
changed the title
There was a problem hiding this comment.
Review: typed SetFinalizer wrapper / noffi path
Solid, carefully-designed change. The SSA lowering restricts to *types.Pointer receivers, rejects closures/bound methods (vkFuncDecl check) so the generated wrapper is a safe no-env closure, handles the nil-clear path, and gates on SetFinalizerPtr being present so wasm/other targets fall back cleanly. finalizerConcreteArgs correctly unboxes the MakeInterface nodes. The lock-free finalizer registry keeps allocation out of the critical section and uses complemented uintptr keys so the registry map does not retain objects. The llgo_noffi restart is gated by four conjunctive conditions and bounded to a single re-run via noFFIRestart.
Findings below. The most important is the unreachable direct-method-value block in value.go.
Note (out of diff, worth confirming): the entire lock-free scheme (pointerFinalizers.pending LIFO and the cleanupSlots free list) depends on runtime/internal/sync/atomic genuinely lowering to fenced atomic hardware ops. Two reviewers independently flagged this as the load-bearing assumption; if atomic.Store(&e.next, head) is not at least release-ordered relative to the CAS, a concurrent drain could observe a stale e.next.
| delete(pointerFinalizers.m, key) | ||
| } | ||
| pointerFinalizers.mu.Unlock() | ||
| } |
There was a problem hiding this comment.
The drain loop acquires/releases pointerFinalizers.mu once per finalized entry. Since SetFinalizerPtr holds the same mutex across its BDWGC RegisterFinalizer calls, a large finalization batch produces O(N) lock round-trips that each contend against concurrent registrations. The queue is already detached and private to the draining goroutine, so the map deletes could be batched under a single Lock/Unlock after the callbacks run — O(1) acquisitions and a shorter registration-blocking window.
1b46348 to
1094c7d
Compare
visualfc
changed the title
1094c7d to
e718218
Compare
LLGo WebAssembly build benchmarks
WebAssembly output sizes
LLGo WebAssembly build measurements
Compared with |
LLGo baseline benchmarks
Program measurements
Core language and compiler benchmarks
Timer runtime benchmarks
Compared with |
bcc4634 to
02d2ba3
Compare
|
Pushed a follow-up for the Windows 386 Capturing closures stay on the boxed |
0e5243f to
3e76f87
Compare
cpunion
left a comment
cpunion
left a comment
There was a problem hiding this comment.
I verified that the size-reduction direction is real, but the current implementation is not safe to merge yet. The inline comments contain four deterministic correctness failures reproduced against this head and one material build-time issue. The current tests miss the named-interface-finalizer and higher-order MakeFunc paths. This branch is also 42 commits behind current main and overlaps the recent runtime cleanup/reentrancy work, so it needs a rebase and full retest after the correctness fixes. I recommend separating typed SetFinalizer lowering/runtime support from automatic noffi reflect/method-value selection if that makes the invariants and review surface easier to establish.
| wb := wrapper.MakeBody(1) | ||
| converted := wb.Convert(b.Prog.rawType(ptrType), wb.Param(0)) | ||
| if !types.Identical(argType, ptrType) { | ||
| converted = wb.Convert(b.Prog.rawType(argType), converted) |
There was a problem hiding this comment.
Blocking: setFinalizerArgCompatible explicitly accepts empty and implemented interfaces, but this is a pointer-to-interface conversion and Builder.Convert does not construct an interface value. A named finalizer such as func finalize(v interface{ Value() int }) passed to runtime.SetFinalizer(p, finalize) reliably aborts compilation with package job ... panicked: todo; named func(any) has the same problem. The existing assignability tests use capturing closures, so they remain on the boxed/libffi path and never exercise this wrapper. Please use the proper interface-boxing operation here and add named-function compile-and-run cases for both any and a non-empty implemented interface.
There was a problem hiding this comment.
Fixed. Interface arguments now go through MakeInterface instead of Convert. Named func(any) and func(interface{ Value() int }) compile-and-run cases are in TestRuntimeSetFinalizerNamedInterfaceFunctions.
| pkg := b.Pkg | ||
| if !pkg.NeedFFI { | ||
| switch fn.Name() { | ||
| case "reflect.Value.Call", "reflect.Value.CallSlice", "reflect.MakeFunc": |
There was a problem hiding this comment.
Blocking: this detection only observes direct vkFuncDecl calls. A higher-order call such as invokeMakeFunc(reflect.MakeFunc), where invokeMakeFunc accepts the function as a value and calls it, never reaches checkFFI because the vkClosure path returns earlier. With -lto=full, automatic selection then omits libffi and the resulting program panics with reflect.MakeFunc is unavailable without libffi. Any value/address escape of an FFI-sensitive symbol must conservatively set NeedFFI, or the decision must use real post-lowering reachability. Please add this exact higher-order case as a regression test.
There was a problem hiding this comment.
Fixed. checkFFI now also runs when an FFI-sensitive function escapes as a value (vkFuncDecl → closure in checkExpr, and MakeClosure). The higher-order case invokeMakeFunc(reflect.MakeFunc) is covered by TestMakeFuncValueRequiresFFI / TestCheckFFIMarksHigherOrderMakeFunc.
| fv := &struct { | ||
| fn unsafe.Pointer | ||
| env unsafe.Pointer | ||
| }{fn, unsafe.Pointer(env)} |
There was a problem hiding this comment.
Blocking: fn is the real method entry, which expects the receiver as its ordinary first ABI argument, while env is supplied as the hidden closure context. Calling reflect.ValueOf(&T{}).MethodByName("M").Interface().(func(int) int)(2) does not pass through reflect.Value.call to decode directMethodValueEnv; it jumps to fn with the wrong ABI. I reproduced a nil-pointer panic with automatic noffi, while the same program built with -check-libffi=false correctly prints 42. This representation needs an ABI-correct thunk that extracts the receiver from env and calls the method with it as the ordinary receiver argument.
There was a problem hiding this comment.
Fixed. Each method now gets an ABI-correct $methodvalue thunk that loads the receiver from the hidden env and calls the real method with it as the ordinary first argument. Thunk pointers live in a side array after the method table (abi.Method is unchanged). MethodByName("M").Interface().(func(int) int)(2) is covered by TestReflectValueMethodByNameInterfaceCall.
| } | ||
| var ignoredFn bdwgc.FinalizerFunc | ||
| var ignoredCb unsafe.Pointer | ||
| bdwgc.RegisterFinalizer(obj, old.prevFn, old.prevCb, &ignoredFn, &ignoredCb) |
There was a problem hiding this comment.
Blocking: this restores the callback snapshot captured when the typed finalizer was registered. If runtime.AddCleanup installs a newer callback chain afterward, restoring old.prevFn overwrites and discards that chain. For SetFinalizer(p, typedFinalizer); AddCleanup(p, cleanup, arg); SetFinalizer(p, nil), official Go reports cleanups 8 old 0, while this head consistently reports cleanups 0 old 0. Cancellation must preserve callbacks installed after the typed entry, and this ordering needs a runtime regression test.
There was a problem hiding this comment.
Fixed. Cancellation now probes the current BDWGC slot: if a later AddCleanup chain owns it, that chain is put back instead of restoring the registration-time snapshot. Stopped typed entries also forward prevFn so earlier cleanups are not dropped. Regression: TestRuntimeSetFinalizerNamedCancelAfterCleanup (SetFinalizer(named) → AddCleanup → SetFinalizer(nil)).
| restarted.Tags = appendBuildTag(restarted.Tags, "llgo_noffi") | ||
| restarted.Tags = appendBuildTag(restarted.Tags, "llgo_methodvalue_noffi") | ||
| restarted.noFFIRestart = true | ||
| return Build(Invocation{Args: inv.Args, Config: restarted, Dir: dir, |
There was a problem hiding this comment.
This recursively rebuilds the complete package graph after the first build has already finished. The benchmark attached to this PR shows full-LTO fmtprintf build-time regressions of +23.0% on Linux, +40.0% on macOS, and roughly +30% to +35% on Windows, for about 1.4% to 3.2% file-size reduction. Please select the mode before the expensive backend work or reuse the first-pass results; at minimum this cost should be documented explicitly and the default-on policy reconsidered.
There was a problem hiding this comment.
Fixed. CheckFFI now does a frontend-only scan of ordinary packages and, if libffi is not needed, reloads with llgo_noffi before any package LLVM backend or LTO. -lto=full no longer enables this by default; it is opt-in via -check-libffi=true.
3e76f87 to
6a37811
Compare
|
@cpunion Thanks for the review. The four correctness failures and the CheckFFI rebuild cost are addressed on the current head:
|
37e1856 to
cafdd47
Compare
visualfc
changed the title
Lower compatible runtime.SetFinalizer calls to SetFinalizerPtr with an internal typed wrapper. Keep the boxed path for capturing closures and for runtimes that omit SetFinalizerPtr. Preserve cleanup chains when a typed finalizer is canceled or replaced.
Split libffi-backed reflect.Call/MakeFunc from the noffi variants, add ABI-correct $methodvalue thunks for MethodByName().Interface(), and keep Windows NewCallback unavailable when llgo_noffi is set.
Export method-value thunks for deadcode drop, skip va_list shapes, and use Go64 storage on wasm32 so noffi method values keep the receiver in the ordinary argument slot.
When -check-libffi=true, scan the already-built Go SSA with RTA from init/main (or all functions for libraries and tests). Restart once with llgo_noffi if Call, MakeFunc, Windows NewCallback, or unlowered SetFinalizer are unreachable. LLVM backend and LTO still run once. The flag stays off by default, including with -lto=full.
WASI has no host libffi and uses typed reflection bridges. Restarting with llgo_noffi would drop Call/MakeFunc instead of selecting a libffi variant, so -check-libffi is ignored for wasip1.
llgo_noffi and llgo_methodvalue_noffi use ABI method-value thunks instead of llgo_reflect_bind*_js. Those ASYNCIFY_REMOVE patterns do not match, so omit them (and ffi_call_js) when compiling without libffi.
Wasm package IR is compiled with clang/emcc -c. Without -flto, unused $methodvalue thunks survive linking, so js/wasm -check-libffi binaries grew. Emit bitcode so -lto=full can DCE them.
Exercise the Codecov-missing branches: thunk-array DCE, wasm32 wide pointer array storage, named-pointer SetFinalizer wrappers, Windows NewCallback libffi detection, and the verbose -check-libffi restart. Analyze wasm initial packages through programUse.
a02491a to
5cc9cac
Compare
Summary
This PR adds compiler and runtime support for statically typed
runtime.SetFinalizercalls and a noffi reflect mode for builds that do not need libffi.Implementation
runtime.SetFinalizercalls toruntime.SetFinalizerPtr.unsafe.Pointerback to the finalizer's pointer argument, includinganyand implemented interface parameters viaMakeInterface.defer, andgocalls.runtime.SetFinalizer(obj, nil)to the pointer-based cancellation path.runtime.SetFinalizerpath for capturing closures and for runtimes that do not provideSetFinalizerPtr, including WASM and collector-free runtimes.-check-libffi=true, scan the already-built Go SSA with RTA frominit/main(or every function for libraries and tests). Restart once withllgo_noffiandllgo_methodvalue_noffiifreflect.Value.Call,CallSlice,MakeFunc, WindowsNewCallback, or an unloweredSetFinalizerare unreachable. LLVM backend and LTO still run once.wasip1,-target wasi/wasip1). WASI has no host libffi and uses typed reflection bridges.-fltothrough toclang/emcc -cso-lto=fullemits bitcode and can DCE unused$methodvaluethunks.llgo_reflect_bind*_js) so emcc does not warn about unmatched patterns.$methodvaluethunks for noffiMethodByName().Interface()values so the receiver is passed as an ordinary method argument.reflect.MakeFuncandreflect.Value.Callunavailable whenllgo_noffiis enabled.syscall.NewCallbackandsyscall.NewCallbackCDeclunavailable in noffi mode.Build Flag
llgo build -check-libffi=trueopts into automatic libffi selection. It is off by default, including with-lto=full. Eligible builds scan Go SSA reachability and compile once without libffi when the package graph does not need it. WASI Preview 1 ignores the flag.Comparison
Program:
cmd/_demo/hellow(fmt.Println("hello world")). Measured on macOS arm64 against currentxgo-dev/main(81a4d030f) and this PR (971590ba8plus local follow-ups).js/wasm32 rows use
-lto=fullon this PR. Currentmaindoes not pass-fltoto wasmclang -c, so those compiles do not complete here; the wasm comparison is this PR with libffi versus this PR with-check-libffi.libffimain-lto=full-lto=full -check-libffi-lto=full-lto=full -check-libffi