From 7d806d741b3f05116a458bdae77161e5339c5e67 Mon Sep 17 00:00:00 2001 From: Li Jie Date: Fri, 22 May 2026 13:50:51 +0800 Subject: [PATCH 1/2] fix reflect makefunc goroutine liveness --- runtime/internal/clite/bdwgc/bdwgc.go | 5 ++ runtime/internal/lib/reflect/makefunc.go | 30 ++++++++---- runtime/internal/runtime/z_gc.go | 8 ++++ runtime/internal/runtime/z_gc_baremetal.go | 7 +++ runtime/internal/runtime/z_nogc.go | 8 ++++ ssa/goroutine.go | 8 +++- ssa/goroutine_patch_test.go | 14 ++++-- test/go/reflect_makefunc_goroutine_test.go | 56 ++++++++++++++++++++++ test/goroot/xfail.yaml | 30 ------------ 9 files changed, 120 insertions(+), 46 deletions(-) create mode 100644 test/go/reflect_makefunc_goroutine_test.go diff --git a/runtime/internal/clite/bdwgc/bdwgc.go b/runtime/internal/clite/bdwgc/bdwgc.go index e7d2a3e347..c21f206ecb 100644 --- a/runtime/internal/clite/bdwgc/bdwgc.go +++ b/runtime/internal/clite/bdwgc/bdwgc.go @@ -34,6 +34,11 @@ func Init() //go:linkname Malloc C.GC_malloc func Malloc(size uintptr) c.Pointer +// MallocUncollectable allocates scanned memory that is not reclaimed until Free. +// +//go:linkname MallocUncollectable C.GC_malloc_uncollectable +func MallocUncollectable(size uintptr) c.Pointer + //go:linkname Realloc C.GC_realloc func Realloc(ptr c.Pointer, size uintptr) c.Pointer diff --git a/runtime/internal/lib/reflect/makefunc.go b/runtime/internal/lib/reflect/makefunc.go index 66afbaac4f..e27130bca4 100644 --- a/runtime/internal/lib/reflect/makefunc.go +++ b/runtime/internal/lib/reflect/makefunc.go @@ -28,12 +28,20 @@ import ( "github.com/goplus/llgo/runtime/abi" c "github.com/goplus/llgo/runtime/internal/clite" "github.com/goplus/llgo/runtime/internal/ffi" + "github.com/goplus/llgo/runtime/internal/runtime" ) type funcData struct { - ftyp *funcType - fn func(args []Value) (results []Value) - nin int + closure *ffi.Closure + sig *ffi.Signature + ftyp *funcType + fn func(args []Value) (results []Value) + nin int +} + +type funcValue struct { + fn unsafe.Pointer + env unsafe.Pointer } func MakeFunc(typ Type, fn func(args []Value) (results []Value)) Value { @@ -48,6 +56,10 @@ func MakeFunc(typ Type, fn func(args []Value) (results []Value)) Value { panic(err) } closure := ffi.NewClosure() + // libffi keeps sig and userdata after Bind returns. Make them reachable + // through the returned function's closure env for later calls. + fd := (*funcData)(runtime.AllocU(unsafe.Sizeof(funcData{}))) + *fd = funcData{closure: closure, sig: sig, ftyp: ftyp, fn: fn, nin: len(ftyp.In)} switch len(ftyp.Out) { case 0: @@ -58,7 +70,7 @@ func MakeFunc(typ Type, fn func(args []Value) (results []Value)) Value { ins[i] = ffiToValue(ffi.Index(args, uintptr(i+1)), fd.ftyp.In[i]) } fd.fn(ins) - }, unsafe.Pointer(&funcData{ftyp: ftyp, fn: fn, nin: len(ftyp.In)})) + }, unsafe.Pointer(fd)) case 1: err = closure.Bind(sig, func(cif *ffi.Signature, ret unsafe.Pointer, args *unsafe.Pointer, userdata unsafe.Pointer) { fd := (*funcData)(userdata) @@ -72,7 +84,7 @@ func MakeFunc(typ Type, fn func(args []Value) (results []Value)) Value { } else { *(*unsafe.Pointer)(ret) = unsafe.Pointer(out[0].ptr) } - }, unsafe.Pointer(&funcData{ftyp: ftyp, fn: fn, nin: len(ftyp.In)})) + }, unsafe.Pointer(fd)) default: err = closure.Bind(sig, func(cif *ffi.Signature, ret unsafe.Pointer, args *unsafe.Pointer, userdata unsafe.Pointer) { fd := (*funcData)(userdata) @@ -90,16 +102,14 @@ func MakeFunc(typ Type, fn func(args []Value) (results []Value)) Value { } offset += fd.ftyp.Out[i].Size_ } - }, unsafe.Pointer(&funcData{ftyp: ftyp, fn: fn, nin: len(ftyp.In)})) + }, unsafe.Pointer(fd)) } if err != nil { panic("libffi error: " + err.Error()) } styp := closureOf(ftyp) - fv := &struct { - fn unsafe.Pointer - env unsafe.Pointer - }{closure.Fn, unsafe.Pointer(&fn)} + fv := (*funcValue)(runtime.AllocU(unsafe.Sizeof(funcValue{}))) + *fv = funcValue{closure.Fn, unsafe.Pointer(fd)} return Value{styp, unsafe.Pointer(fv), flagIndir | flag(Func)} } diff --git a/runtime/internal/runtime/z_gc.go b/runtime/internal/runtime/z_gc.go index 04764a106a..4dc3f83775 100644 --- a/runtime/internal/runtime/z_gc.go +++ b/runtime/internal/runtime/z_gc.go @@ -37,6 +37,14 @@ func AllocZ(size uintptr) unsafe.Pointer { return c.Memset(ret, 0, size) } +func AllocRoot(size uintptr) unsafe.Pointer { + return bdwgc.MallocUncollectable(size) +} + +func FreeRoot(ptr unsafe.Pointer) { + bdwgc.Free(ptr) +} + type entry struct { fn func() // cleanup func prev unsafe.Pointer // prev cleanup func ptr diff --git a/runtime/internal/runtime/z_gc_baremetal.go b/runtime/internal/runtime/z_gc_baremetal.go index afccff9705..1ed272c0c7 100644 --- a/runtime/internal/runtime/z_gc_baremetal.go +++ b/runtime/internal/runtime/z_gc_baremetal.go @@ -34,6 +34,13 @@ func AllocZ(size uintptr) unsafe.Pointer { return tinygogc.Alloc(size) } +func AllocRoot(size uintptr) unsafe.Pointer { + return tinygogc.Alloc(size) +} + +func FreeRoot(ptr unsafe.Pointer) { +} + // AddCleanupPtr is not implemented in baremetal builds because tinygogc // does not support finalizers. Cleanup functions will never be called. // diff --git a/runtime/internal/runtime/z_nogc.go b/runtime/internal/runtime/z_nogc.go index 7cbc30c2f0..5b0828ee37 100644 --- a/runtime/internal/runtime/z_nogc.go +++ b/runtime/internal/runtime/z_nogc.go @@ -36,6 +36,14 @@ func AllocZ(size uintptr) unsafe.Pointer { return c.Memset(ret, 0, size) } +func AllocRoot(size uintptr) unsafe.Pointer { + return c.Malloc(size) +} + +func FreeRoot(ptr unsafe.Pointer) { + c.Free(ptr) +} + // AddCleanupPtr is not implemented when GC is disabled. // Cleanup functions will never be called. func AddCleanupPtr(ptr unsafe.Pointer, cleanup func()) (cancel func()) { diff --git a/ssa/goroutine.go b/ssa/goroutine.go index 78a5d74f32..1f85866fbe 100644 --- a/ssa/goroutine.go +++ b/ssa/goroutine.go @@ -74,8 +74,11 @@ func (b Builder) Go(fn Expr, buildCall func(Builder, Expr, ...Expr) Expr, args . t := prog.Struct(typs...) voidPtr := prog.VoidPtr() // Goroutine startup data may carry Go pointers, including closure ctx. - // Keep it in GC-managed memory so it remains visible across the thread start. - data := Expr{b.aggregateAllocU(t, flds...), voidPtr} + // Keep the startup record in scanned, uncollectable GC memory until the + // new routine has finished its entry call. + dataPtr := b.Call(pkg.rtFunc("AllocRoot"), prog.IntVal(prog.SizeOf(t), prog.Uintptr())).impl + aggregateInit(b.impl, dataPtr, t.ll, flds...) + data := Expr{dataPtr, voidPtr} size := prog.SizeOf(voidPtr) pthd := b.Alloca(prog.IntVal(uint64(size), prog.Uintptr())) b.pthreadCreate(pthd, prog.Nil(voidPtr), pkg.routine(t, fn, buildCall, len(args)), data) @@ -102,6 +105,7 @@ func (p Package) routine(t Type, fn Expr, buildCall func(Builder, Expr, ...Expr) args[i] = b.getField(data, i+offset) } buildCall(b, fn, args...) + b.Call(p.rtFunc("FreeRoot"), param) b.Return(prog.Nil(prog.VoidPtr())) return routine.Expr } diff --git a/ssa/goroutine_patch_test.go b/ssa/goroutine_patch_test.go index 77e303fc24..d9013f88cc 100644 --- a/ssa/goroutine_patch_test.go +++ b/ssa/goroutine_patch_test.go @@ -41,9 +41,15 @@ func TestGoClosureStartupUsesGCManagedMemory(t *testing.T) { if strings.Contains(ir, "@free") { t.Fatalf("goroutine startup data should not use free:\n%s", ir) } - // The closure context and the goroutine startup record both must remain - // visible to the runtime GC until the new goroutine consumes them. - if got := strings.Count(ir, `"github.com/goplus/llgo/runtime/internal/runtime.AllocU"`); got < 2 { - t.Fatalf("expected closure ctx and goroutine startup data to use AllocU, got %d:\n%s", got, ir) + if !strings.Contains(ir, `"github.com/goplus/llgo/runtime/internal/runtime.AllocRoot"`) { + t.Fatalf("goroutine startup data should use scanned uncollectable memory:\n%s", ir) + } + if !strings.Contains(ir, `"github.com/goplus/llgo/runtime/internal/runtime.FreeRoot"`) { + t.Fatalf("goroutine startup data should be freed after the entry call returns:\n%s", ir) + } + // The closure context must remain visible to the runtime GC until the + // uncollectable startup record is initialized. + if got := strings.Count(ir, `"github.com/goplus/llgo/runtime/internal/runtime.AllocU"`); got < 1 { + t.Fatalf("expected closure ctx to use AllocU, got %d:\n%s", got, ir) } } diff --git a/test/go/reflect_makefunc_goroutine_test.go b/test/go/reflect_makefunc_goroutine_test.go new file mode 100644 index 0000000000..0f4e4da273 --- /dev/null +++ b/test/go/reflect_makefunc_goroutine_test.go @@ -0,0 +1,56 @@ +package gotest + +import ( + "reflect" + "runtime" + "testing" +) + +func TestReflectMakeFuncGoroutineStartup(t *testing.T) { + oldProcs := runtime.GOMAXPROCS(1) + defer runtime.GOMAXPROCS(oldProcs) + + stopGC := make(chan struct{}) + gcDone := make(chan struct{}) + go func() { + defer close(gcDone) + for { + select { + case <-stopGC: + return + default: + runtime.GC() + } + } + }() + defer func() { + close(stopGC) + <-gcDone + }() + + const n = 100 + done := make(chan struct{}, n*2) + for i := 0; i < n; i++ { + f := reflect.MakeFunc(reflect.TypeOf((func(*int))(nil)), func(args []reflect.Value) []reflect.Value { + if len(args) != 1 || !args[0].IsNil() { + panic("bad reflect MakeFunc pointer argument") + } + done <- struct{}{} + return nil + }).Interface().(func(*int)) + go f(nil) + + g := reflect.MakeFunc(reflect.TypeOf((func())(nil)), func(args []reflect.Value) []reflect.Value { + if len(args) != 0 { + panic("bad reflect MakeFunc zero-argument call") + } + done <- struct{}{} + return nil + }).Interface().(func()) + go g() + } + + for i := 0; i < n*2; i++ { + <-done + } +} diff --git a/test/goroot/xfail.yaml b/test/goroot/xfail.yaml index a5a66d10b4..4f5290c9c8 100644 --- a/test/goroot/xfail.yaml +++ b/test/goroot/xfail.yaml @@ -2626,16 +2626,6 @@ xfails: directive: run case: fixedbugs/issue24491b.go reason: go1.25 goroot run failure on linux/amd64 - - version: go1.25 - platform: darwin/arm64 - directive: run - case: fixedbugs/issue25897a.go - reason: go1.25 goroot run failure on darwin/arm64 - - version: go1.25 - platform: linux/amd64 - directive: run - case: fixedbugs/issue25897a.go - reason: go1.25 goroot run failure on linux/amd64 - version: go1.25 platform: linux/amd64 directive: run @@ -3072,16 +3062,6 @@ xfails: directive: run case: fixedbugs/issue23837.go reason: go1.24 goroot run failure on linux/amd64 - - version: go1.24 - platform: darwin/arm64 - directive: run - case: fixedbugs/issue25897a.go - reason: go1.24 goroot run failure on darwin/arm64 - - version: go1.24 - platform: linux/amd64 - directive: run - case: fixedbugs/issue25897a.go - reason: go1.24 goroot run failure on linux/amd64 - version: go1.24 platform: linux/amd64 directive: run @@ -3322,16 +3302,6 @@ xfails: directive: run case: fixedbugs/issue23837.go reason: go1.26 goroot run failure on linux/amd64 - - version: go1.26 - platform: darwin/arm64 - directive: run - case: fixedbugs/issue25897a.go - reason: go1.26 goroot run failure on darwin/arm64 - - version: go1.26 - platform: linux/amd64 - directive: run - case: fixedbugs/issue25897a.go - reason: go1.26 goroot run failure on linux/amd64 - version: go1.26 platform: linux/amd64 directive: run From 1d04791cb280439c315bd568b2b20896152a3eb3 Mon Sep 17 00:00:00 2001 From: Li Jie Date: Sat, 23 May 2026 01:44:46 +0800 Subject: [PATCH 2/2] test: update goroutine root allocation checks --- cl/_testgo/goroutine/in.go | 6 ++++-- cl/_testgo/selects/in.go | 2 +- 2 files changed, 5 insertions(+), 3 deletions(-) diff --git a/cl/_testgo/goroutine/in.go b/cl/_testgo/goroutine/in.go index c5c6ddd435..795a9b42e2 100644 --- a/cl/_testgo/goroutine/in.go +++ b/cl/_testgo/goroutine/in.go @@ -5,14 +5,14 @@ package main func main() { // CHECK: call ptr @"{{.*}}AllocZ"(i64 1) // CHECK: store i1 false, ptr %0, align 1 - // CHECK: call ptr @"{{.*}}AllocU"(i64 16) + // CHECK: call ptr @"{{.*}}AllocRoot"(i64 16) // CHECK: call i32 @"{{.*}}CreateThread"(ptr %3, ptr null, ptr @"{{.*}}goroutine._llgo_routine$1", ptr %1) done := false go println("hello") go func(s string) { // CHECK: call ptr @"{{.*}}AllocU"(i64 8) // CHECK: { ptr @"{{.*}}goroutine.main$1", ptr undef } - // CHECK: call ptr @"{{.*}}AllocU"(i64 32) + // CHECK: call ptr @"{{.*}}AllocRoot"(i64 32) // CHECK: call i32 @"{{.*}}CreateThread"(ptr %11, ptr null, ptr @"{{.*}}goroutine._llgo_routine$2", ptr %8) // CHECK: call void @"{{.*}}PrintString"(%"{{.*}}String" { ptr @2, i64 1 }) // CHECK: ret void @@ -38,6 +38,7 @@ func main() { // CHECK-NEXT: %2 = extractvalue { %"{{.*}}String" } %1, 0 // CHECK-NEXT: call void @"{{.*}}PrintString"(%"{{.*}}String" %2) // CHECK-NEXT: call void @"{{.*}}PrintByte"(i8 10) +// CHECK-NEXT: call void @"{{.*}}FreeRoot"(ptr %0) // CHECK-NEXT: ret ptr null // CHECK-LABEL: define ptr @"{{.*}}goroutine._llgo_routine$2"(ptr %0){{.*}} { @@ -48,4 +49,5 @@ func main() { // CHECK-NEXT: %4 = extractvalue { ptr, ptr } %2, 1 // CHECK-NEXT: %5 = extractvalue { ptr, ptr } %2, 0 // CHECK-NEXT: call void %5(ptr %4, %"{{.*}}String" %3) +// CHECK-NEXT: call void @"{{.*}}FreeRoot"(ptr %0) // CHECK-NEXT: ret ptr null diff --git a/cl/_testgo/selects/in.go b/cl/_testgo/selects/in.go index f72ba639a3..5ef394eeb0 100644 --- a/cl/_testgo/selects/in.go +++ b/cl/_testgo/selects/in.go @@ -21,7 +21,7 @@ package main // CHECK-NEXT: %10 = getelementptr inbounds { ptr, ptr, ptr }, ptr %7, i32 0, i32 2 // CHECK-NEXT: store ptr %4, ptr %10, align 8 // CHECK-NEXT: %11 = insertvalue { ptr, ptr } { ptr @"{{.*}}/cl/_testgo/selects.main$1", ptr undef }, ptr %7, 1 -// CHECK-NEXT: %12 = call ptr @"{{.*}}/runtime/internal/runtime.AllocU"(i64 16) +// CHECK-NEXT: %12 = call ptr @"{{.*}}/runtime/internal/runtime.AllocRoot"(i64 16) // CHECK-NEXT: %13 = getelementptr inbounds { { ptr, ptr } }, ptr %12, i32 0, i32 0 // CHECK-NEXT: store { ptr, ptr } %11, ptr %13, align 8 // CHECK-NEXT: %14 = alloca i8, i64 8, align 1