-
Notifications
You must be signed in to change notification settings - Fork 125
Fix segmentation fault on Python 3.13 #960
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
base: main
Are you sure you want to change the base?
Conversation
provinzkraut
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.
Seems like a reasonable fix to me.
I'm curious though how you tracked it down to this?
Also, can you re-enable the regular tests for 3.13 again here?
msgspec/.github/workflows/ci.yml
Lines 63 to 64 in 1bc64dd
| # TODO: Enable this once fixed https://github.com/jcrist/msgspec/issues/910 | |
| # - "3.13" |
It was difficult :) The short version: From In next step, I found that (gdb) frame 4
#4 0x00000000023d7c59 in make_dict_from_instance_attributes (interp=0x2fcc2e8 <_PyRuntime+104400>, keys=0x7ffff4e24f20, values=0x7ffff54dc070) at /home/shadchin/arc/arcadia/contrib/tools/python3/Objects/dictobject.c:6736
6736 track += _PyObject_GC_MAY_BE_TRACKED(val);
(gdb) info locals
val = <unknown at remote 0xcdcdcdcdcdcdcdcd>
i = 0
used = 1
track = 0
size = 30
res = 0x7ffff1b535b0
(gdb) print values
$1 = (PyDictValues *) 0x7ffff54dc070
(gdb) print keys
$2 = (PyDictKeysObject *) 0x7ffff4e24f20
(gdb) x/10gx values
0x7ffff54dc070: 0xcdcdcdcdcdcdcdcd 0xcdcdcdcdcdcdcdcd
0x7ffff54dc080: 0xcdcdcdcdcdcdcdcd 0xcdcdcdcdcdcdcdcd
0x7ffff54dc090: 0xcdcdcdcdcdcdcdcd 0xcdcdcdcdcdcdcdcd
0x7ffff54dc0a0: 0xcdcdcdcdcdcdcdcd 0xcdcdcdcdcdcdcdcd
0x7ffff54dc0b0: 0xcdcdcdcdcdcdcdcd 0xcdcdcdcdcdcdcdcdThen I had to study what |
|
Do we know why 3.14 is fine? |
|
@shadchin seems like |
Python 3.14+ does this initialization itself - https://github.com/python/cpython/blob/main/Python/gc.c#L2377-L2379 |
src/msgspec/_core.c
Outdated
| memset((char *)obj + sizeof(PyObject), '\0', type->tp_basicsize - sizeof(PyObject)); | ||
| #if PY313_PLUS && !PY314_PLUS | ||
| if (type->tp_flags & Py_TPFLAGS_INLINE_VALUES) { | ||
| _PyObject_InitInlineValues(obj, type); |
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.
Maybe it's worth asking upstream about how to deal with this without relying on private APIs?
Not like we don't rely on private APIs elsewhere, so I wouldn't consider it a blocker, but if there's a way, that might be nice?
We have our own build system, fix is working for us, but I forgot to check on a clean environment, sorry. I tried to make a fix but it didn't help. There's a conflict with I'll go upstream with it. |
|
I took a different approach and it helped) Maybe something else will prompt in upstream. |
ZeroIntensity
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.
Coming from upstream
src/msgspec/_core.c
Outdated
| #endif | ||
|
|
||
| if (is_gc) { | ||
| obj = PyObject_GC_New(PyObject, type); |
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.
Manually calling PyObject_GC_New/PyObject_New doesn't really work for subtypes. It might work in practice sometimes, but it's not something that's supposed to be supported.
I believe the root of this bug comes from the fact that on 3.13, inline values are initialized in the tp_alloc slot of type objects, which is skipped here. In 3.14, we moved that logic to PyObject_GC_New because we decided to support more types.
So, to fix this, just call type->tp_alloc instead; that will invoke the supertype (PyType_GenericAlloc) and initialize the inline values.
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.
Thanks, I switched to type->tp_alloc
src/msgspec/_core.c
Outdated
| bool is_gc = MS_TYPE_IS_GC(type); | ||
|
|
||
| #if PY313_PLUS && !PY314_PLUS | ||
| type->tp_flags &= ~Py_TPFLAGS_INLINE_VALUES; |
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.
I'm almost certain you'll trigger assertion failures (or worse, get spurious crashes) with this. Supertypes don't expect their flags to be different in subtypes.
|
Thanks! How would that change affect performance? |
https://github.com/jcrist/msgspec/blob/main/benchmarks/bench_structs.py main: this pr: https://github.com/jcrist/msgspec/blob/main/benchmarks/bench_gc.py main: this pr: |
Fix for #910 and #868
The problem is only with Python 3.13, in Python 3.14
PyObject_GC_Newdoes this initialization itself, see python/cpython#123192