gh-130030: Fix crash on 32-bit Linux with free threading #130043
Conversation
The `gc_get_refs` assertion needs to be after we check the alive and unreachable bits. Otherwise, `ob_tid` may store the actual thread id instead of the computed `gc_refs`, which may trigger the assertion if the `ob_tid` looks like a negative value. Also fix a few type warnings on 32-bit systems.
There was a problem hiding this comment.
Other than my preference of leaving "in" and "out" unsigned, this LGTM.
The fix by moving the assert looks correct. With the "mark alive" pass, objects that are marked alive don't have gc_maybe_init_refs() called on them. So ob_tid doesn't contain a refcount.
Python/gc_free_threading.c
Outdated
| unsigned int in; | ||
| unsigned int out; | ||
| Py_ssize_t in; | ||
| Py_ssize_t out; |
There was a problem hiding this comment.
Warnings could also be fixed by changing this line:
Py_ssize_t space = BUFFER_HI - gc_mark_buffer_len(args);
to
unsigned int space = BUFFER_HI - gc_mark_buffer_len(args);
The assert should be changed in that case, since size can't be negative.
assert(space <= gc_mark_buffer_avail(args));
assert(space <= BUFFER_HI);
Performance wise I would doubt that using Py_ssize_t makes any difference. On 32-bit platforms maybe a little since it would use a 64-bit int rather than a 32-bit one?
I would still prefer to use the unsigned integer types since my understanding is that the C standard defines overflow behavior for unsigned integers but not signed ones. The buffer "in" and "out" calculations depend on the overflow behavior.
There was a problem hiding this comment.
That makes sense -- I switched back to unsigned int.
On 32-bit platforms maybe a little since it would use a 64-bit int rather than a 32-bit one?
Py_ssize_t is the same size as size_t, so typically 32-bits on a 32-bit platform.
…pythongh-130043) The `gc_get_refs` assertion needs to be after we check the alive and unreachable bits. Otherwise, `ob_tid` may store the actual thread id instead of the computed `gc_refs`, which may trigger the assertion if the `ob_tid` looks like a negative value. Also fix a few type warnings on 32-bit systems.
The
gc_get_refsassertion needs to be after we check the alive and unreachable bits. Otherwise,ob_tidmay store the actual thread id instead of the computedgc_refs, which may trigger the assertion if theob_tidlooks like a negative value.Also fix a few type warnings on 32-bit systems.