Skip to content

fix unsafe uintptr usage to be GC-safe on go1.25#78

Merged
danieljoos merged 1 commit intodanieljoos:masterfrom
thaJeztah:fix_go_1.25
Oct 2, 2025
Merged

fix unsafe uintptr usage to be GC-safe on go1.25#78
danieljoos merged 1 commit intodanieljoos:masterfrom
thaJeztah:fix_go_1.25

Conversation

@thaJeztah
Copy link
Copy Markdown
Contributor

@thaJeztah thaJeztah commented Oct 2, 2025

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.

Assisted-by: OpenAI ChatGPT

@thaJeztah thaJeztah mentioned this pull request Oct 2, 2025
Closed
@thaJeztah thaJeztah force-pushed the fix_go_1.25 branch 3 times, most recently from 28f87e8 to 130a4a7 Compare October 2, 2025 01:45
@thaJeztah thaJeztah marked this pull request as ready for review October 2, 2025 01:49
@thaJeztah
Copy link
Copy Markdown
Contributor Author

thaJeztah commented Oct 2, 2025

@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;

@thaJeztah thaJeztah mentioned this pull request Oct 2, 2025
Merged
2 tasks
@thaJeztah
Copy link
Copy Markdown
Contributor Author

thaJeztah commented Oct 2, 2025

A colleague (@akerouanton) suggested to try if just the runtime.KeepAlive(ncred) would already be enough; will give that a spin as well.

@thaJeztah
Copy link
Copy Markdown
Contributor Author

thaJeztah commented Oct 2, 2025

A colleague (@akerouanton) suggested to try if just the runtime.KeepAlive(ncred) would already be enough; will give that a spin as well.

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

A uintptr is an integer, not a reference. Converting a Pointer to a uintptr creates an integer value with no pointer semantics. Even if a uintptr holds the address of some object, the garbage collector will not update that uintptr's value if the object moves, nor will that uintptr keep the object from being reclaimed.

akerouanton
akerouanton approved these changes Oct 2, 2025
View reviewed changes
Copy link
Copy Markdown

@akerouanton akerouanton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@thaJeztah thaJeztah mentioned this pull request Oct 2, 2025
Merged
danieljoos
danieljoos approved these changes Oct 2, 2025
View reviewed changes
Copy link
Copy Markdown
Owner

@danieljoos danieljoos left a comment •
edited
Loading

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?

@thaJeztah
fix unsafe uintptr usage to be GC-safe on go1.25
dbb17d5 
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 <github@gone.nl>
@thaJeztah
Copy link
Copy Markdown
Contributor Author

thaJeztah commented Oct 2, 2025

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!

@danieljoos
Copy link
Copy Markdown
Owner

danieljoos commented Oct 2, 2025

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!

@danieljoos danieljoos merged commit 1565377 into danieljoos:master Oct 2, 2025
8 checks passed
@thaJeztah
Copy link
Copy Markdown
Contributor Author

thaJeztah commented Oct 2, 2025

Whoop, thank you!

@thaJeztah thaJeztah deleted the fix_go_1.25 branch October 2, 2025 19:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@danieljoos danieljoos danieljoos approved these changes

+1 more reviewer

@akerouanton akerouanton akerouanton approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@thaJeztah @danieljoos @akerouanton