tls: add gc-aware pthread slots#1347
Conversation
Summary of ChangesHello @cpunion, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the runtime's capabilities by introducing garbage collector-aware thread-local storage (TLS) using Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request introduces a GC-aware thread-local storage (TLS) implementation using pthread keys, which is a significant addition for managing thread-local data that may contain GC-managed pointers. The implementation is split across _gc and _nogc build-tagged files to correctly handle both scenarios. The accompanying tests are thorough, covering functionality, thread-safety, and resource cleanup.
Separately, this PR also includes a change in the SSA package to handle debug information generation for generic types by adding a hasTypeParam check. This seems to be an unrelated but useful fix.
My review found one high-severity issue in the TLS destructor logic where a panic in a user-provided destructor could lead to resource leaks and stale GC roots. I've provided a suggestion to make the cleanup more robust.
Code Review SummaryThis PR introduces a well-architected TLS package with GC-aware pthread slot management. The implementation demonstrates solid engineering with good separation of concerns. However, critical memory safety issues must be addressed before merging. Critical Issues 🚨
Documentation Gaps 📝
Positive Aspects ✅
Recommendation: Address critical memory safety issues before merge. See inline comments for specific fixes. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1347 +/- ##
=======================================
Coverage 90.18% 90.18%
=======================================
Files 43 43
Lines 12674 12752 +78
=======================================
+ Hits 11430 11501 +71
- Misses 1087 1092 +5
- Partials 157 159 +2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Thanks for the thorough pass! To the TOCTOU concern: pthread TLS slots are per-thread, and functions like Regarding the rest of your notes, we’ve:
Let us know if you spot anything else. Thanks again! |
35529bf to
1ed418e
Compare
Summary
runtime/internal/clite/tlsto provide pthread-based TLS handles that are GC-aware when building with BDWGCAddRoots/RemoveRoots) and hook TLS slots into them on GC builds, with TLS-specific unit tests and stress coveragego/typessizing does not panic when TLS generics appear—and add targeted tests forhasTypeParamMotivation
Loop defers keep their state in thread-local storage; because pthread TLS is invisible to BDWGC, the GC would reclaim defer nodes under heavy load. This PR adds a reusable TLS helper that registers each slot as a GC root, ensuring defer chains (and any other TLS-stored pointers) remain visible to the collector. When compiling without GC (
-tags nogc), the helper falls back to plain pthread TLS.The SSA change (
hasTypeParam) is required because TLS handles introduce generic types that reach the debug-info builder.go/types' GC sizing asserts when asked for alignments of types that still contain type parameters. By detecting those shapes and skipping DI emission, we prevent crashes when generating debug info while keeping coverage via dedicated tests.Testing