bpo-33622: Fix issues with handling errors in the GC.

[serhiy-storchaka]

• Fixed a leak when the GC fails to add an object with __del__ into the gc.garbage list.
• PyGC_Collect() can now be called when an exception is set and preserves it.
• Fixed an UB with comparing a dead pointer with NULL.

https://bugs.python.org/issue33622

[tim-one]

Looks good to me! Note: I don't know what "UB" means in the "Fixed an UB with ..." comment means.

[serhiy-storchaka]

UB = undefined behavior. A common abbreviation in the C/C++ word.

[tim-one]

In that case, what about the behavior was undefined? Memory-releasing functions don't magically change the value of a pointer passed to them; r == NULL after Py_XDECREF(r) if and only if r == NULL before it. If r isn't NULL and wasn't obtained by a legit memory allocation function to begin with, or the memory referenced by r was already freed, that's UB.

So I found the rewrite slightly clearer, but didn't see a need for it.

[serhiy-storchaka]

In C the value of a pointer becomes invalid after releasing a memory. It can "work" on some platforms with specific versions of compilers, but the Standard doesn't promise anything. The next version of the compiler can generate the code that will crash or remove the code of the whole function. This is what called an undefined behavior.

[pitrou]

Looks good to me.

[miss-islington]

Thanks @serhiy-storchaka for the PR 🌮🎉.. I'm working now to backport this PR to: 2.7, 3.6, 3.7.
🐍🍒⛏🤖

[bedevere-bot]

GH-7094 is a backport of this pull request to the 3.7 branch.

[miss-islington]

Sorry, @serhiy-storchaka, I could not cleanly backport this to 3.6 due to a conflict.
Please backport using cherry_picker on command line.
cherry_picker 301e3cc8a5bc68c5347ab6ac6f83428000d31ab2 3.6

[miss-islington]

Sorry, @serhiy-storchaka, I could not cleanly backport this to 2.7 due to a conflict.
Please backport using cherry_picker on command line.
cherry_picker 301e3cc8a5bc68c5347ab6ac6f83428000d31ab2 2.7

[bedevere-bot]

GH-7095 is a backport of this pull request to the 3.6 branch.

[bedevere-bot]

GH-7096 is a backport of this pull request to the 2.7 branch.

[tim-one]

In C the value of a pointer becomes invalid after releasing a memory. It can "work" on some platforms with specific versions of compilers, but the Standard doesn't promise anything.

Not true. Dereferencing a freed pointer gives UB. The code here never dereferenced r after Py_XDECREF(r), it merely tested whether r == NULL. Here's one copy of the C standard. The cases it gives for UB after free() are the ones I already typed. Use your head here: no function can change the value of a pointer passed to it - to pull that off it would have to be passed the address of the pointer (or otherwise have access to that address). C passes pointers by value, not by reference.

As I said, I found your rewrite slightly clearer in this case anyway, so I don't care. I just object to calling a thing UB when it isn't, lest we see a slew of needless changes all over the place based on hallucinations 😉.

[serhiy-storchaka]

J.2 Undefined behavior
The behavior is undefined in the following circumstances:
...
— The value of a pointer that refers to space deallocated by a call to the free or
realloc function is used (7.20.3).

From the implementation point of view, the pointer can be not an arbitrary integer, but a more complex object. For example any use of the pointer (including comparing with NULL) may require loading into a segment register, and this can crash if the segment is no longer valid.

Maybe comparing the pointer to a deallocated memory with NULL will not cause any problems on any platform supported by Python. I don't know. But is better to write a code that conforms the Standard (unless practicality beats purity).

[tim-one]

Excellent! Thank you for the reference - I stand gratefully corrected 😃