gh-102381: don't call watcher callback with dead object

[carljm]

Fixes #102381, #102351 and #102385.

Ensure that func_dealloc, code_dealloc, and dict_dealloc temporarily incref the function/code object/dict before calling the watcher callback, so we don't pass around dead objects.

A watcher callback could take a reference to the object; check for this and allow it to be resurrected in that case, rather than going ahead and freeing it (making the taken reference a pointer to freed memory.)

Also, don't pass the object itself to PyErr_WriteUnraisable if the callback raises, allowing an unraisablehook to take a reference to the object and resurrect it and, in the worst case, causing an infinite loop. Safer to construct an informative debug string and pass that as context to PyErr_WriteUnraisable. This actually provides more debug information than before, since the new message clarifies that the error occurred in a watcher callback for the mentioned object.

• Issue: raising an exception in a dict/code/function watcher callback will segfault on a DEALLOCATED event #102381

[carljm]

Skipping news since this is just fixing a bug in an unreleased new feature.

[bedevere-bot]

🤖 New build scheduled with the buildbot fleet by @carljm for commit 6d0b789 🤖

If you want to schedule another build, you need to add the 🔨 test-with-refleak-buildbots label again.

[JelleZijlstra]

Looks good, thanks for fixing this! I'd like other reviewers to also have a look though.

[bedevere-bot]

🤖 New build scheduled with the buildbot fleet by @carljm for commit 9b938c7 🤖

If you want to schedule another build, you need to add the 🔨 test-with-refleak-buildbots label again.

[carljm]

Thanks @Yhg1s for the review! I've updated per all comments.

[carljm]

@Yhg1s I pushed the explicit exhaustiveness check, and one possible way of addressing your comments about the x-macros. I really don't feel strongly about how to handle the event type logging, so if you have a clearly preferred approach, just let me know and I'll do that.

[JelleZijlstra]

Sorry, noticed one more thing. I think this can affect users who include the header in C code that independently defines a DEF_EVENT macro.

[JelleZijlstra]

And as discussed offline, let's run the buildbots again to be safe; after that feel free to merge.

[bedevere-bot]

🤖 New build scheduled with the buildbot fleet by @carljm for commit 05e2ce0 🤖

If you want to schedule another build, you need to add the 🔨 test-with-refleak-buildbots label again.

[kumaraditya303]

Is this Py_SET_REFCNT necessary? IIUC this is redundant here otherwise it isn't obvious to me and a comment would be nice.

[carljm]

In the normal case, assuming the watcher has not taken any new reference to the object, the refcount will be 1 at this point (because we set it to 1 above.) We should set it to 0 before continuing with deallocation, because objects being deallocated should have a refcount of zero.

[markshannon]

The watcher must take a reference to any object it is watching, otherwise the refcount is incorrect and we crash and burn.

The GC, or an optimizer, can legitimately assume that any object with a reference count of zero and no weakrefs is to be freed and re-use the memory. That's not going to end well if the watcher still thinks that memory is still a dictionary.

[carljm]

@markshannon I think your comment is more relevant to the discussion below (about why to have dealloc watchers at all) than it is to this thread, so I'll reply below.

The answer to @kumaraditya303's question here is that this Py_SET_REFCNT(op, 0) is the decref that has to be paired with the incref Py_SET_REFCNT(op, 1); that occurs on line 799. We use explicit Py_SET_REFCNT instead of Py_INCREF and Py_DECREF because Py_DECREF would cause recursive deallocation, and if we use Py_SET_REFCNT for the decref we have to use it for the incref also, otherwise the total refcount tracking will be off.

[markshannon]

I can't help feeling that we would be better off without these watchers.
They appear to be bug magnets. Handling the "zombie" objects is tricky and error prone.
How is the watcher interacting with the freed object if there are no references to it?

This PR adds quite a lot of code to dict_dealloc which is called for every dictionary created, which has performance implications.

I think we should just remove these watchers. What are they for?
I've been going through possible optimizations for CPython and these watchers aren't needed for any of them.
I don't see why Cinder would need them either.

(As an aside: we won't need the creation events either, but I can see why Cinder wants those).

[carljm]

This PR adds quite a lot of code to dict_dealloc which is called for every dictionary created, which has performance implications.

I was also initially concerned about adding an incref and decref in this path, but on a pyperformance run (with --enable-optimizations build, on a bare-metal AWS host) this PR was 1.00x faster than the base commit in main, so it doesn't seem to have a detectable impact?

I don't see why Cinder would need them either.

In general, in Cinder they are used for de-registering objects from JIT registries consisting of borrowed references, to avoid having dangling pointers in these registries. E.g. for each watched dict, we remember what key(s) we care about in that dict, and for each key we may also watch a different builtins (and maybe also globals) dict that that key might actually have been found in (or might still need to be found in if the key goes away in the main dict). If the main dict is deallocated we unwatch the additional globals/builtins dicts, and erase the whole entry from the registry. Similarly, we have a registry of created functions/code-objects that are pending compilation (in some modes we don't compile eagerly but do a multi-threaded compilation of everything pending compilation, right before fork), so if a function or code object is deallocated we remove it from that registry.

In every case the deallocated handler doesn't do anything with the zombie PyObject itself, it just needs the pointer value to erase any registry entry for that pointer value.

[carljm]

The watcher must take a reference to any object it is watching, otherwise the refcount is incorrect and we crash and burn.

Cinder JIT currently doesn't take references to objects it is watching, so as not to keep them alive if they would otherwise die. There is no "crash and burn" precisely because of the dealloc events; if the object is deallocated we know about it and can erase any pointer to it we were keeping. Effectively it's like we are keeping a weakref, but without the allocation and indirection overhead of an actual weakref object. Perhaps we should be keeping an actual weakref instead; it would take some experimentation to check the performance impact of that.