feat: matrix build python-version: ['3.11', '3.12'] - #399
Conversation
The unconditional _gopy_clear_go_tls() call (issue #370) performs a hardcoded TLS store (movq $0, %fs:-8 on linux/amd64). On glibc + CPython 3.12+ that offset overlaps CPython's current-thread-state TLS slot, so the store nulls it and the interpreter segfaults on the first CGo entry in the common single-extension case (issue #395). Add a -clear-go-tls build flag (default false) and gate the single call site on it. The C helper and its pybindgen registration are left in place so opt-in restores the previous behavior exactly. RTLD_GLOBAL-local loading, which is what actually isolates each runtime's goroutine-pointer TLS, is untouched and remains unconditional. Fixes #395
Ensure GIL state before allocating; fixes stale thread-state segfault.
This reverts commit 1273f46.
|
Thanks for merging #398! Wiring up the 3.12 matrix is exactly the coverage this needed, I'm happy to have a look. I've reproduced #400 locally end-to-end before writing this, so below is what I've found, a confirmation that your fix is correct, and one structural observation you may want to weigh for later. As I now understand, #400 is a real bug, fully independent of #398. Your EnvironmentGentoo, glibc, system CPython 3.14.6, go 1.26.4, gcc 16, x86_64. Note 3.12 is only the floor: this class of crash is the glibc + CPython-3.12+ layout, not a specific minor, so 3.14 reproduces #400 identically. Your CI hitting it on 3.12 and me hitting it on 3.14 are the same bug. 1. #400 is real and orthogonal to #398I built
So with the TLS clear already off, complex still crashes while everything else is fine; this is not #395 wearing a complex hat, it is a distinct bug. Confirmed crash signature matches your report: 2. Root cause: allocation inside gopy's GIL-released windowThe generated wrapper releases the GIL for the entire body: //export probe_Comp64Add
func probe_Comp64Add(i *C.PyObject, j *C.PyObject) *C.PyObject {
_saved_thread := C.PyEval_SaveThread() // GIL released, current tstate nulled on this thread
defer C.PyEval_RestoreThread(_saved_thread) // re-acquired on return
return complex64GoToPy(probe.Comp64Add(complex64PyToGo(i), complex64PyToGo(j)))
// ^^^^^^^^^^^^^^^^ PyComplex_FromDoubles allocates here, GIL-released -> null tstate
}On CPython 3.12+, object allocation dereferences the current thread state (allocator / GC bookkeeping), so allocating with the GIL released and
3. As I see your fix is correct and verified
It also matches gopy's own precedent: the Python-callback path in 4. Two a little stinky places, they're optional and may be for later
The minimal, robust generalization would probably be to not |
|
@satarsa Thank you so much for the detailed feedback 🙇 I stacked a PR on top of this one, and I wonder if you'd be generous enough to look at it as well: #401 One thing to note: I tested locally (with the changes from PR 401) and reverting d41db33 on top of changes in 401. I expected that would be enough to resolve the original segfault, since 401 moves the complex arg marshalling ahead of Tracing it: 401 only fixes the argument side (your point 1 above — So it looks like your fix and the two commits in 401 are covering two different, adjacent windows (return-side allocation vs. argument-side read, plus the tuple builder from your point 2) rather than one overlapping one. As far as I can tell, we need all of the above 🙂 |
This PR is serving two goals:
While we shouldn't combine the two, I'm doing so to be pragmatic and move quickly (for now). Depending on feedback, I may split this PR into 2 separate parts.
Fixes: #400