Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

iter: Pull is too restrictive for Cgo use #67499

Open
eliasnaur opened this issue May 18, 2024 · 3 comments
Open

iter: Pull is too restrictive for Cgo use #67499

eliasnaur opened this issue May 18, 2024 · 3 comments
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone

Comments

@eliasnaur
Copy link
Contributor

eliasnaur commented May 18, 2024

Go version

go version devel go1.23-105ac94486 Fri May 17 21:53:50 2024 +0000 darwin/arm64

Output of go env in your module/workspace:

GO111MODULE=''
GOARCH='arm64'
GOBIN=''
GOCACHE='/Users/a/Library/Caches/go-build'
GOENV='/Users/a/Library/Application Support/go/env'
GOEXE=''
GOEXPERIMENT=''
GOFLAGS=''
GOHOSTARCH='arm64'
GOHOSTOS='darwin'
GOINSECURE=''
GOMODCACHE='/Users/a/go/pkg/mod'
GONOPROXY=''
GONOSUMDB=''
GOOS='darwin'
GOPATH='/Users/a/go'
GOPRIVATE=''
GOPROXY='https://proxy.golang.org,direct'
GOROOT='/Users/a/proj/goroot'
GOSUMDB='sum.golang.org'
GOTMPDIR=''
GOTOOLCHAIN='auto'
GOTOOLDIR='/Users/a/proj/goroot/pkg/tool/darwin_arm64'
GOVCS=''
GOVERSION='devel go1.23-105ac94486 Fri May 17 21:53:50 2024 +0000'
GODEBUG=''
GOTELEMETRY='local'
GOTELEMETRYDIR='/Users/a/Library/Application Support/go/telemetry'
GCCGO='gccgo'
GOARM64='v8.0'
AR='ar'
CC='clang'
CXX='clang++'
CGO_ENABLED='1'
GOMOD='/Users/a/proj/gio/go.mod'
GOWORK='/Users/a/proj/gio/go.work'
CGO_CFLAGS='-O2 -g'
CGO_CPPFLAGS=''
CGO_CXXFLAGS='-O2 -g'
CGO_FFLAGS='-O2 -g'
CGO_LDFLAGS='-O2 -g'
PKG_CONFIG='pkg-config'
GOGCCFLAGS='-fPIC -arch arm64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -ffile-prefix-map=/var/folders/9c/t7b8z6m93sxgr4m4m4kbvllw0000gn/T/go-build933130646=/tmp/go-build -gno-record-gcc-switches -fno-common'

What did you do?

Iterate values generated from Cgo with iter.Pull:

package main

/*

extern void go_yield(int);

static void native_iterate(void) {
  go_yield(5);
}
*/
import "C"
import "fmt"
import "iter"

var globalYield func(int) bool

//export go_yield
func go_yield(v C.int) {
	globalYield(int(v))
}

func main() {
	next, stop := iter.Pull(func(yield func(int) bool) {
		globalYield = yield
		C.native_iterate()
	})
	defer stop()
	for {
		v, ok := next()
		if !ok {
			break
		}
		fmt.Println("got from C: %d\n", v)
	}
}

What did you see happen?

$ go run .
coro: got thread 0x102a5f120, want 0x0
coro: got lock internal 1, want 0
coro: got lock external 0, want 0
fatal error: coro: OS thread locking must match locking at coroutine creation

runtime stack:
runtime.throw({0x10299dc8d?, 0x10293adf8?})
	/Users/a/proj/goroot/src/runtime/panic.go:1027 +0x38 fp=0x16d501de0 sp=0x16d501db0 pc=0x102961608
runtime.coroswitch_m(0x16d501f80?)
	/Users/a/proj/goroot/src/runtime/coro.go:125 +0x46c fp=0x16d501e60 sp=0x16d501de0 pc=0x1029004ec
runtime.mcall()
	/Users/a/proj/goroot/src/runtime/asm_arm64.s:193 +0x54 fp=0x16d501e70 sp=0x16d501e60 pc=0x1029649d4

goroutine 35 gp=0x140001028c0 m=0 mp=0x102a5f120 [running, locked to thread]:
runtime.coroswitch(0x0?)
	/Users/a/proj/goroot/src/runtime/coro.go:91 +0x4c fp=0x140001424d0 sp=0x140001424b0 pc=0x10295f26c
iter.Pull[...].func1.1()
	/Users/a/proj/goroot/src/iter/iter.go:71 +0x6c fp=0x14000142510 sp=0x140001424d0 pc=0x1029944cc
main.go_yield(...)
	/Users/a/proj/gio/callback.go:19
_cgoexp_fcbcb787d322_go_yield(0x14000142588?)
	_cgo_gotypes.go:56 +0x30 fp=0x14000142530 sp=0x14000142510 pc=0x102993f40
runtime.cgocallbackg1(0x102993f10, 0x16d501f60, 0x0)
	/Users/a/proj/goroot/src/runtime/cgocall.go:428 +0x218 fp=0x14000142600 sp=0x14000142530 pc=0x1028fe288
runtime.cgocallbackg(0x102993f10, 0x16d501f60, 0x0)
	/Users/a/proj/goroot/src/runtime/cgocall.go:347 +0xf4 fp=0x14000142650 sp=0x14000142600 pc=0x1028fdfe4
runtime.cgocallbackg(0x102993f10, 0x16d501f60, 0x0)
	<autogenerated>:1 +0x1c fp=0x14000142680 sp=0x14000142650 pc=0x102968b9c
runtime.cgocallback(0x140001426f8, 0x102993d90, 0x1029945b0)
	/Users/a/proj/goroot/src/runtime/asm_arm64.s:1131 +0xb0 fp=0x140001426b0 sp=0x14000142680 pc=0x102966e50
runtime.systemstack_switch()
	/Users/a/proj/goroot/src/runtime/asm_arm64.s:201 +0x8 fp=0x140001426c0 sp=0x140001426b0 pc=0x1029649e8
runtime.cgocall(0x1029945b0, 0x14000142738)
	/Users/a/proj/goroot/src/runtime/cgocall.go:176 +0x70 fp=0x14000142700 sp=0x140001426c0 pc=0x10295f030
main._Cfunc_native_iterate()
	_cgo_gotypes.go:45 +0x30 fp=0x14000142730 sp=0x14000142700 pc=0x102993d90
main.main.func1(0x0?)
	/Users/a/proj/gio/callback.go:25 +0x44 fp=0x14000142740 sp=0x14000142730 pc=0x102993ee4
iter.Pull[...].func1()
	/Users/a/proj/goroot/src/iter/iter.go:75 +0x104 fp=0x140001427a0 sp=0x14000142740 pc=0x102994414
runtime.corostart()
	/Users/a/proj/goroot/src/runtime/coro.go:71 +0x50 fp=0x140001427d0 sp=0x140001427a0 pc=0x102900010
runtime.goexit({})
	/Users/a/proj/goroot/src/runtime/asm_arm64.s:1223 +0x4 fp=0x140001427d0 sp=0x140001427d0 pc=0x102966f44
created by iter.Pull[...] in goroutine 1
	/Users/a/proj/goroot/src/iter/iter.go:59 +0x110

goroutine 1 gp=0x140000021c0 m=nil [coroutine]:
runtime.coroswitch(0x14000088ea8?)
	/Users/a/proj/goroot/src/runtime/coro.go:91 +0x4c fp=0x14000088e80 sp=0x14000088e60 pc=0x10295f26c
iter.Pull[...].func2()
	/Users/a/proj/goroot/src/iter/iter.go:91 +0x7c fp=0x14000088ed0 sp=0x14000088e80 pc=0x1029942ac
main.main()
	/Users/a/proj/gio/callback.go:29 +0xb0 fp=0x14000088f40 sp=0x14000088ed0 pc=0x102993e60
runtime.main()
	/Users/a/proj/goroot/src/runtime/proc.go:271 +0x288 fp=0x14000088fd0 sp=0x14000088f40 pc=0x102930b58
runtime.goexit({})
	/Users/a/proj/goroot/src/runtime/asm_arm64.s:1223 +0x4 fp=0x14000088fd0 sp=0x14000088fd0 pc=0x102966f44

goroutine 18 gp=0x140000aa380 m=nil [force gc (idle)]:
runtime.gopark(0x0?, 0x0?, 0x0?, 0x0?, 0x0?)
	/Users/a/proj/goroot/src/runtime/proc.go:402 +0xc8 fp=0x14000070790 sp=0x14000070770 pc=0x1029616e8
runtime.goparkunlock(...)
	/Users/a/proj/goroot/src/runtime/proc.go:408
runtime.forcegchelper()
	/Users/a/proj/goroot/src/runtime/proc.go:326 +0xb8 fp=0x140000707d0 sp=0x14000070790 pc=0x102930e08
runtime.goexit({})
	/Users/a/proj/goroot/src/runtime/asm_arm64.s:1223 +0x4 fp=0x140000707d0 sp=0x140000707d0 pc=0x102966f44
created by runtime.init.6 in goroutine 1
	/Users/a/proj/goroot/src/runtime/proc.go:314 +0x24

goroutine 19 gp=0x140000aa540 m=nil [GC sweep wait]:
runtime.gopark(0x0?, 0x0?, 0x0?, 0x0?, 0x0?)
	/Users/a/proj/goroot/src/runtime/proc.go:402 +0xc8 fp=0x14000070f60 sp=0x14000070f40 pc=0x1029616e8
runtime.goparkunlock(...)
	/Users/a/proj/goroot/src/runtime/proc.go:408
runtime.bgsweep(0x140000ba000)
	/Users/a/proj/goroot/src/runtime/mgcsweep.go:277 +0xa0 fp=0x14000070fb0 sp=0x14000070f60 pc=0x10291d260
runtime.gcenable.gowrap1()
	/Users/a/proj/goroot/src/runtime/mgc.go:203 +0x28 fp=0x14000070fd0 sp=0x14000070fb0 pc=0x1029114c8
runtime.goexit({})
	/Users/a/proj/goroot/src/runtime/asm_arm64.s:1223 +0x4 fp=0x14000070fd0 sp=0x14000070fd0 pc=0x102966f44
created by runtime.gcenable in goroutine 1
	/Users/a/proj/goroot/src/runtime/mgc.go:203 +0x6c

goroutine 20 gp=0x140000aa700 m=nil [GC scavenge wait]:
runtime.gopark(0x140000ba000?, 0x1029ba0a8?, 0x1?, 0x0?, 0x140000aa700?)
	/Users/a/proj/goroot/src/runtime/proc.go:402 +0xc8 fp=0x14000071760 sp=0x14000071740 pc=0x1029616e8
runtime.goparkunlock(...)
	/Users/a/proj/goroot/src/runtime/proc.go:408
runtime.(*scavengerState).park(0x102a5e9a0)
	/Users/a/proj/goroot/src/runtime/mgcscavenge.go:425 +0x5c fp=0x14000071790 sp=0x14000071760 pc=0x10291ac5c
runtime.bgscavenge(0x140000ba000)
	/Users/a/proj/goroot/src/runtime/mgcscavenge.go:653 +0x44 fp=0x140000717b0 sp=0x14000071790 pc=0x10291b1a4
runtime.gcenable.gowrap2()
	/Users/a/proj/goroot/src/runtime/mgc.go:204 +0x28 fp=0x140000717d0 sp=0x140000717b0 pc=0x102911468
runtime.goexit({})
	/Users/a/proj/goroot/src/runtime/asm_arm64.s:1223 +0x4 fp=0x140000717d0 sp=0x140000717d0 pc=0x102966f44
created by runtime.gcenable in goroutine 1
	/Users/a/proj/goroot/src/runtime/mgc.go:204 +0xac

goroutine 34 gp=0x14000102700 m=nil [finalizer wait]:
runtime.gopark(0x140000745b8?, 0x1029622c8?, 0x8?, 0x0?, 0x1029c6580?)
	/Users/a/proj/goroot/src/runtime/proc.go:402 +0xc8 fp=0x14000074580 sp=0x14000074560 pc=0x1029616e8
runtime.runfinq()
	/Users/a/proj/goroot/src/runtime/mfinal.go:193 +0x108 fp=0x140000747d0 sp=0x14000074580 pc=0x1029105b8
runtime.goexit({})
	/Users/a/proj/goroot/src/runtime/asm_arm64.s:1223 +0x4 fp=0x140000747d0 sp=0x140000747d0 pc=0x102966f44
created by runtime.createfing in goroutine 1
	/Users/a/proj/goroot/src/runtime/mfinal.go:163 +0x80
exit status 2

What did you expect to see?

The equivalent #65946 was closed as fixed, and the fix, https://go-review.googlesource.com/c/go/+/583675 does mention

Specifically, when iter.Pull is used in
conjunction with thread-locked goroutines, complex cases (passing next
between goroutines or passing yield between goroutines) are likely to
fail. Simple cases, where any number of iter.Pull iterators are used in
a straightforward way (nested, in series, etc.) from the same
goroutine, will work and will be guaranteed to be fast regardless of
thread-lock state.

Note that the error is at the very least confusing because there is no explicit thread locking involved (LockOSThread) in the above program. I realize using iter.Pull with Cgo may not be covered by "simple cases", in which case I worry about the "may" in the later mention

This is a compromise for the near-term and we may consider lifting the
restrictions imposed by this CL in the future.

In other words, I believe the above program should work in a future Go version, in which case this issue tracks the implementation.

@cagedmantis cagedmantis added compiler/runtime Issues related to the Go compiler and/or runtime. NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. labels May 21, 2024
@cagedmantis cagedmantis added this to the Go1.23 milestone May 21, 2024
@cagedmantis
Copy link
Contributor

cc @golang/compiler

@aclements
Copy link
Member

Thanks for filing this tracking issue. You're right that this is an unfortunate and rather hidden limitation of the current approach, though we hope that people won't run into this very often.

We do have a rough idea of how to fix this that would also probably improve the performance of cgo code. The key observation is that, while the C code needs to run on the same thread if you call back into C or return to C, the Go code itself does not. So, the idea is that we do a "half" OS thread lock on a C -> Go call. We don't actively kick the Go code off that thread, but we allow the scheduler to schedule it to other threads like usual. That way, if it very quickly returns or calls back into C, you don't pay the cost of moving threads. But if it blocks or runs for a long time in Go, the scheduler can move it around to make within-Go transitions cheap. When you transition back to C (either by returning or calling), at that point we make sure we're running on the original thread by switching back to it if we aren't already running on it.

@eliasnaur
Copy link
Contributor Author

eliasnaur commented May 29, 2024

You're right that this is an unfortunate and rather hidden limitation of the current approach, though we hope that people won't run into this very often.

FWIW, I ran into this :) As far as I can tell, both #64755 and #64777 can be closed in favor of an elegant range-func construction:

import "gioui.org/app" // Calls LockOSThread during init, as required by the platform.

func main() {
        w := new(app.Window)
        for _, e := range w.Events() {
             // Respond to e, usually by redrawing window content.
        }
}

even on the platforms where w.Events starts and run a native event loop. The caveat is that the user cannot exit the loop (e.g. break), because the native event loop is in general uncooperative: once started, it cannot be stopped nor paused[0].

The workaround is to introduce a singleton intermediate iter.Pull iterator to suspend the native main event loop and return control to the user. Without a fix for this issue, however, yield'ing from the native event loop to the intermediate iterator panics. The same happens if the user attempts to use their own iter.Pull with w.Events.

[0]: For example, the iOS UIApplicationMain function must be called from the main thread and blocks forever.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
Development

No branches or pull requests

4 participants