gh-150868: Fix sum() deallocating a float subclass as an exact float - #151063
gh-150868: Fix sum() deallocating a float subclass as an exact float#151063eendebakpt wants to merge 2 commits into
Conversation
…float The complex-total fast path in builtin_sum_impl() guarded with PyFloat_Check() (which accepts subclasses) but freed the item with _Py_DECREF_SPECIALIZED(item, _PyFloat_ExactDealloc), which assumes an exact float. A float subclass instance reaching refcount 0 in that loop aborts on an assertion in debug builds, and in release builds skips the subclass tp_dealloc (type refcount leak, __del__ never runs, subclass object pushed onto the float freelist). Use PyFloat_CheckExact() instead, matching the float-total branch, so subclass instances fall through to the generic PyNumber_Add() path. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
StanFromIreland
changed the title
|
There was already a PR opened earlier, lets give that a chance to be merged. |
|
I like this pr, it has a test. @eendebakpt, could you extend this fix with this (and add tests too): |
…) loops Per review: use PyLong_CheckExact with the specialized exact-int deallocator in the float and complex fast-sum loops, so int subclasses take the generic path. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
@skirpichev I updated the branch with your suggestion. I think this will need backports. |
skirpichev
added
needs backport to 3.13
|
CC @picnixz |
| _Py_DECREF_SPECIALIZED(item, _PyFloat_ExactDealloc); | ||
| continue; | ||
| } | ||
| if (PyLong_Check(item)) { |
There was a problem hiding this comment.
Is this change really necessary? we lose a fast past for int subclasses here or am I missing something?
There was a problem hiding this comment.
It is needed in combination with the _Py_DECREF_SPECIALIZED that was added. I think we can keep the PyLong_Check and use the normal decref. Not sure either approach is much better for performance.
There was a problem hiding this comment.
Yes, but what I meant is that we do not need to have a _Py_DECREF_SPECIALIZED for the int case right? I'm not even sure why we need the float case. Was there a real need for exact floats or was it an historical behavior? I don't remember anymore :(
There was a problem hiding this comment.
The specialized DECREF was added by commit da6c785, when sum() already checked for PyFloat_CheckExact():
- Py_DECREF(item);
+ _Py_DECREF_SPECIALIZED(item, _PyFloat_ExactDealloc);Supporting int and float subclasses require to decide how to deal with overriden __index__() and __float__() methods.
class StrangeInt(int):
def __index__(self):
return 42.0
a = 1
b = StrangeInt(2)
print(sum([a, b]))
class StrangeFloat(float):
def __float__(self):
return 42.0
a = 1.0
b = StrangeFloat(2.0)
print(sum([a, b]))On Python 3.16, this code displays 3 and 3.0. So it seems like __index__() and __float__() methods are not called in these cases.
There was a problem hiding this comment.
Is this change really necessary? we lose a fast past for int subclasses here or am I missing something?
I think that specialization for subclasses was added not intentionally. In other cases we use strict checks.
WRT performance, I doubt we will have too much from specialized decrefs. But lets keep this to emphasize that we deal with base classes only.
Supporting int and float subclasses require to decide how to deal with overriden
__index__()and__float__()methods.
All examples, where it matters, looks broken for me (like above).
There was a problem hiding this comment.
Ok we can keep the strict checks only and wonder later about what to do with the checks. AFAIR the PyLong_AsDouble doesn'z care about conversion functions but it also doesn't check for exact type.
4 participants
Uh oh!
There was an error while loading. Please reload this page.