-
Notifications
You must be signed in to change notification settings - Fork 22
fix unsafe uintptr usage to be GC-safe on go1.25 #78
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
Conversation
28f87e8 to
130a4a7
Compare
|
@danieljoos PTAL; this fixes a bug that's exposed in go1.25 where the GC purges the structure during write; see; Regression-test is ran on this PR with go1.25; |
|
A colleague (@akerouanton) suggested to try if just the |
Gave that a try in thaJeztah#1, but doesn't look to be enough; still failing if we only add that. Looks to be documented indeed that that's the case; https://pkg.go.dev/unsafe#Pointer
|
akerouanton
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very nice, thank you for this change!
Would you mind rebasing or merge master into this such that the tests for stable and oldstable run once with your new test case?
Replace uintptr fields with typed pointers so the GC considers them alive. relates to changes in go1.25: https://go.dev/doc/go1.25#faster-slices > Faster slices > > The compiler can now allocate the backing store for slices on the stack > in more situations, which improves performance. This change has the potential > to amplify the effects of incorrect `unsafe.Pointer` usage, see for example > [issue 73199]. In order to track down these problems, the [bisect tool] can > be used to find the allocation causing trouble using the `-compile=variablemake` > flag. All such new stack allocations can also be turned off using > `-gcflags=all=-d=variablemakehash=n`. [issue 73199]: https://go.dev/issue/73199 [bisect tool]: https://pkg.go.dev/golang.org/x/tools/cmd/bisect Assisted-by: OpenAI ChatGPT Signed-off-by: Sebastiaan van Stijn <[email protected]>
|
For sure; rebased! Also apologies if you got pinged on your Slack; I happened to be chatting with a colleague of you when I noticed you're GitHub staff, so thought I'd abuse the opportunity (I know I always overlook my personal projects during the week, so thought it's worth a try) ❤️ Definitely owe you a 🍻 or other beverage if we happen to meet in person! |
|
No problem at all. Didn't have a look into my private repos for a while and probably wouldn't have noticed in time. Thank you for this very valuable change! |
|
Whoop, thank you! |
Replace uintptr fields with typed pointers so the GC considers them alive.
relates to changes in go1.25: https://go.dev/doc/go1.25#faster-slices
Assisted-by: OpenAI ChatGPT