-
Notifications
You must be signed in to change notification settings - Fork 17.7k
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
proposal: runtime: explicitly allow pinning of strings and slices #65286
Comments
For golang#65286 Change-Id: I1a04f151ac78c8c09a37d2cb7f782b2b61ad3290
Change https://go.dev/cl/558578 mentions this issue: |
For golang#65286 Change-Id: Iaece5ba8b7fcc6618e8bc6bc2b8ee4f51118b733 GitHub-Last-Rev: 294dd97 GitHub-Pull-Request: golang#65295
Fixes golang#65286 Change-Id: Ic8749b359bd56ecc43873bf3f3598e398e18847b
For golang#65286 Change-Id: Iaece5ba8b7fcc6618e8bc6bc2b8ee4f51118b733 GitHub-Last-Rev: 4ddf24e GitHub-Pull-Request: golang#65295
Fixes golang#65286 Change-Id: Ic8749b359bd56ecc43873bf3f3598e398e18847b
Change https://go.dev/cl/558715 mentions this issue: |
I wrote the implementation for that. |
For golang#65286 Change-Id: I58299625c202a8252d8d0fa90dbe41fc33f0975f
Change https://go.dev/cl/559015 mentions this issue: |
I'm currently dealing with passing strings without doing a copy from Go to C to optimize FrankenPHP (Go and PHP use a similar data structure to store strings, so there is no need to copy the Go string to a C string to finally copy it to a PHP string). An important point is that pinning string only works if the memory storing the string is allocated on the heap. This panics ( package main
import (
"runtime"
"strings"
"unsafe"
)
func main() {
s := "s"
p := runtime.Pinner{}
p.Pin(unsafe.StringData(s))
} Forcing allocation on the heap makes this snippet work: package main
import (
"runtime"
"strings"
"unsafe"
)
func main() {
s := strings.Clone("s")
p := runtime.Pinner{}
p.Pin(unsafe.StringData(s))
} This should probably be mentioned in the docs, the documentation change introduced in 916e6cd is misleading. |
@dunglas This'll be fixed in Go 1.22. It really should silently ignore "non-Go" pointers in general (in this case, some pointer to read-only memory in the binary). I agree it is a usability issue. The decision in #63768 (comment) was to not backport this behavior since it's not technically a regression and there's a workaround. We could consider backporting it anyway, I suppose; the fix is pretty safe and the benefit is that the workarounds aren't necessary anymore. |
Awesome! @mknyszek could you point me to the workaround, I can't find it and I have an immediate use case for it 😅 |
Ah, sorry, by workaround I meant
|
Thanks for the clarification! I'll use the |
The cgo pointer passing rules (and previously the
Pinner
documentation) states something like:This wasn't really accurate in practice and also placed restrictions on the API that made it harder to allow new cases in the future (see #62356 and #63768).
Even still, the
Pinner
API explicitly rejects non-pointer types today, but there are ways to work around that for some common use-cases like strings and slices:&s[0]
works.unsafe.StringData(s)
works.While strings require unsafe, it's particularly surprising that slice elements just work given the documentation. Also, given that Go strings can already be passed as
_GoString_
to C functions, it makes sense that they should be included as well.I propose allowing both strings and slices as valid arguments to
Pinner
. The semantics are clear: pin the underlying memory for the string or the slice backing store. Callers are also able to pin any pointers inside the slice backing store (butPinner
will not do this for them automatically). We should make clear that C code is only allowed to access and manipulate the part of the slice backing store that has been pinned; going out-of-bounds on the slice passed to C is not OK.The text was updated successfully, but these errors were encountered: