diff --git a/runtime/internal/lib/runtime/weak_llgo.go b/runtime/internal/lib/runtime/weak_llgo.go index b97e26d199..b3eeac30b9 100644 --- a/runtime/internal/lib/runtime/weak_llgo.go +++ b/runtime/internal/lib/runtime/weak_llgo.go @@ -10,9 +10,18 @@ import ( psync "github.com/xgo-dev/llgo/runtime/internal/sync" ) +// weakHandle supplies the GC-dependent identity used by GOROOT's weak package. +// Go's collector removes weak registrations from spans during sweep. BDWGC +// invokes our cleanup during allocation, so registry removal must be deferred. +// Removing an entry releases the runtime's reference; a user-held weak.Pointer +// must keep its dead handle alive to preserve identity. type weakHandle struct { key uintptr live uint32 + // Written only by the producer before the head CAS publishes this handle; + // after the head Swap, only the drainer accesses the link. The head atomics + // order these ordinary accesses. Links retain handles, never referents. + next unsafe.Pointer } // BDWGC conservatively treats pointer-looking uintptr values as live roots. @@ -30,6 +39,7 @@ var weakState struct { once psync.Once mu psync.Mutex m map[uintptr]*weakHandle + dead unsafe.Pointer } func initWeakState() { @@ -37,6 +47,39 @@ func initWeakState() { weakState.m = make(map[uintptr]*weakHandle) } +// retireWeakHandle runs inside a GC finalizer. Even a map lookup can allocate +// and invoke another finalizer on the same thread, so this path must neither +// allocate nor take weakState.mu. Each handle is published exactly once. +func retireWeakHandle(h *weakHandle) { + latomic.StoreUint32(&h.live, 0) + for { + head := latomic.LoadPointer(&weakState.dead) + h.next = head + if latomic.CompareAndSwapPointer(&weakState.dead, head, unsafe.Pointer(h)) { + return + } + } +} + +// drainWeakHandles runs during registration with weakState.mu held. Detach only +// one batch so concurrent cleanup cannot keep a registration here indefinitely. +// This bounds the captured work, not its size: a batch of n handles takes O(n) +// time under the registry lock. Without another non-nil weak.Make, the last +// dead batch and its map entries remain retained; neither Value nor GC drains +// them. Reclaiming that idle batch requires a separately scheduled safe consumer. +func drainWeakHandles() { + for h := (*weakHandle)(latomic.SwapPointer(&weakState.dead, nil)); h != nil; { + next := (*weakHandle)(h.next) + // A user-held dead weak.Pointer must not retain the rest of this batch. + h.next = nil + // The address may already belong to a new object with a new handle. + if weakState.m[h.key] == h { + delete(weakState.m, h.key) + } + h = next + } +} + func llgoRegisterWeakPointer(p unsafe.Pointer) unsafe.Pointer { if p == nil { return nil @@ -45,7 +88,9 @@ func llgoRegisterWeakPointer(p unsafe.Pointer) unsafe.Pointer { key := encodeWeakPointer(p) weakState.mu.Lock() - if h := weakState.m[key]; h != nil { + drainWeakHandles() + // A cleanup may mark a handle dead before publishing it to the queue. + if h := weakState.m[key]; h != nil && latomic.LoadUint32(&h.live) != 0 { weakState.mu.Unlock() return unsafe.Pointer(h) } @@ -53,15 +98,9 @@ func llgoRegisterWeakPointer(p unsafe.Pointer) unsafe.Pointer { weakState.m[key] = h weakState.mu.Unlock() - // Keep the cleanup closure limited to encoded identities. Capturing p here - // would turn the cleanup itself into a strong reference to the referent. + // Capture only the handle with its encoded identity, never the referent p. llrt.AddCleanupPtr(p, func() { - latomic.StoreUint32(&h.live, 0) - weakState.mu.Lock() - if weakState.m[key] == h { - delete(weakState.m, key) - } - weakState.mu.Unlock() + retireWeakHandle(h) }) return unsafe.Pointer(h) } diff --git a/test/_stress/README.md b/test/_stress/README.md index 7900999695..19c9a7a591 100644 --- a/test/_stress/README.md +++ b/test/_stress/README.md @@ -33,6 +33,7 @@ go test -race -count=3 -timeout=20m ./runtime/timer /tmp/llgo-runtime-stress test -count=3 -timeout=30m ./runtime/signal /tmp/llgo-runtime-stress test -count=3 -timeout=30m ./runtime/cpuprof /tmp/llgo-runtime-stress test -count=3 -timeout=30m ./runtime/finalizer +/tmp/llgo-runtime-stress test -count=3 -timeout=5m ./runtime/weak ``` The signal suite is LLGo-only and targets Unix hosts. The CPU profile suite is @@ -66,3 +67,12 @@ regress fatal native-handler replacement windows. The finalizer suite repeatedly publishes large finalizer batches while many goroutines call `runtime.GC`, and checks that queued callbacks are neither corrupted nor delivered twice. + +The weak suite creates batches of weak pointers in a goroutine that exits, +then performs bounded collections while retaining only the weak handles. It +regresses cleanup callbacks recursively invoking another cleanup while +allocating under the weak registry lock. Each batch must expire more than half +its handles, and a separate helper process enforces a 60-second deadline even +if the tested runtime deadlocks. It uses only public Go APIs and also runs with +`go test` on Go 1.24 or newer. Use identical profiles before and after the fix; +neither sleeping nor reducing allocation pressure is part of the workload. diff --git a/test/_stress/runtime/weak/weak_stress_test.go b/test/_stress/runtime/weak/weak_stress_test.go new file mode 100644 index 0000000000..2bf0d9be63 --- /dev/null +++ b/test/_stress/runtime/weak/weak_stress_test.go @@ -0,0 +1,97 @@ +//go:build go1.24 && !baremetal && !nogc && !wasm + +package weakstress + +import ( + "context" + "os" + "os/exec" + "runtime" + "testing" + "time" + "weak" +) + +type referent struct { + id int + pad [256]byte +} + +func stressCount(t *testing.T, base int) int { + t.Helper() + switch profile := os.Getenv("LLGO_STRESS_PROFILE"); profile { + case "", "default": + return base + case "quick": + return base / 8 + case "heavy": + return base * 2 + default: + t.Fatalf("unknown LLGO_STRESS_PROFILE %q", profile) + return 0 + } +} + +func TestWeakCleanupReentrancy(t *testing.T) { + if os.Getenv("LLGO_STRESS_WEAK_CHILD") != t.Name() { + // A GC callback can deadlock the allocating thread, including testing's + // own timeout machinery. Keep the hard deadline in a separate process. + executable, err := os.Executable() + if err != nil { + t.Fatal(err) + } + ctx, cancel := context.WithTimeout(context.Background(), 60*time.Second) + defer cancel() + cmd := exec.CommandContext(ctx, executable, "-test.run=^"+t.Name()+"$", "-test.v", "-test.timeout=45s") + cmd.Env = append(os.Environ(), "LLGO_STRESS_WEAK_CHILD="+t.Name()) + output, err := cmd.CombinedOutput() + t.Logf("%s", output) + if ctx.Err() != nil { + t.Fatalf("weak cleanup helper did not exit within 60s: %v", ctx.Err()) + } + if err != nil { + t.Fatalf("weak cleanup helper failed: %v", err) + } + return + } + + rounds, objects := stressCount(t, 16), stressCount(t, 8192) + t.Logf("rounds=%d objects=%d GC_MARKERS=%q", rounds, objects, os.Getenv("GC_MARKERS")) + for round := 0; round < rounds; round++ { + handles := make(chan []weak.Pointer[referent], 1) + go func() { + live := make([]*referent, objects) + batch := make([]weak.Pointer[referent], objects) + for i := range live { + live[i] = &referent{id: i} + batch[i] = weak.Make(live[i]) + } + handles <- batch + runtime.KeepAlive(live) + // Exit this goroutine so conservative stack scanning does not keep + // the entire batch alive while another goroutine collects it. + }() + batch := <-handles + var expired int + for collection := 0; collection < 16; collection++ { + // Finite collections avoid mixing collector/allocator starvation + // from an unbounded GC loop into weak-callback reentrancy coverage. + runtime.GC() + expired = 0 + for _, h := range batch { + if h.Value() == nil { + expired++ + } + } + if expired > objects/2 { + break + } + runtime.Gosched() + } + if expired <= objects/2 { + t.Fatalf("round %d: only %d/%d weak handles expired", round, expired, objects) + } + t.Logf("round %d: expired %d/%d weak handles", round, expired, objects) + runtime.KeepAlive(batch) + } +} diff --git a/test/std/weak/weak_test.go b/test/std/weak/weak_test.go index d923f11d2c..4bbac5a841 100644 --- a/test/std/weak/weak_test.go +++ b/test/std/weak/weak_test.go @@ -128,3 +128,24 @@ func TestPointerNilInput(t *testing.T) { t.Errorf("Make(nil).Value() = %v, want nil", val) } } + +func TestPointerIdentity(t *testing.T) { + x, y := new([256]byte), new([256]byte) + x[0], y[0] = 1, 2 + xw, yw := weak.Make(x), weak.Make(y) + if xw != weak.Make(x) { + t.Fatal("repeated Make returned a different handle for the same object") + } + if xw == yw { + t.Fatal("Make returned the same handle for distinct objects") + } + runtime.GC() + if xw != weak.Make(x) || yw != weak.Make(y) { + t.Fatal("GC changed the identity of a live weak pointer") + } + if xw.Value() != x || yw.Value() != y { + t.Fatal("GC invalidated a live weak pointer") + } + runtime.KeepAlive(x) + runtime.KeepAlive(y) +}