[3.7] bpo-38070: visit_decref() calls _PyObject_IsFreed() in debug mode

[vstinner]

• _PyMem_IsPtrFreed(ptr) now also returns 1 if ptr is NULL
  (equals to 0).

• Reorder _PyObject_Dump() to write safe fields first, and only
  attempt to render repr() at the end.

• Add _PyObject_AssertFailed() function and _PyObject_ASSERT() macro.

• bpo-38070: Enhance visit_decref() debug trace (bpo-38070: Enhance visit_decref() debug trace #16631)

subtract_refs() now pass the parent object to visit_decref() which
pass it to _PyObject_ASSERT(). So if the "is freed" assertion fails,
the parent is used in debug trace, rather than the freed object. The
parent object is more likely to contain useful information. Freed
objects cannot be inspected and are displayed as "" with no other detail.

https://bugs.python.org/issue36389

[vstinner]

I backported the code manually from master (without git cherry-pick -x).

I made a similar backport for 3.8: commit f82ce5b (this one using git cherry-pick -x).

[vstinner]

Patch to test this change:

diff --git a/Modules/signalmodule.c b/Modules/signalmodule.c
index a0722b731c..79506c98a2 100644
--- a/Modules/signalmodule.c
+++ b/Modules/signalmodule.c
@@ -1237,6 +1237,7 @@ PyInit__signal(void)
     d = PyModule_GetDict(m);
 
     x = DefaultHandler = PyLong_FromVoidPtr((void *)SIG_DFL);
+    Py_DECREF(x);
     if (!x || PyDict_SetItemString(d, "SIG_DFL", x) < 0)
         goto finally;
 
diff --git a/Objects/longobject.c b/Objects/longobject.c
index 202f652fc6..5fbef931b4 100644
--- a/Objects/longobject.c
+++ b/Objects/longobject.c
@@ -16,10 +16,10 @@ class int "PyObject *" "&PyLong_Type"
 /*[clinic end generated code: output=da39a3ee5e6b4b0d input=ec0275e3422a36e3]*/
 
 #ifndef NSMALLPOSINTS
-#define NSMALLPOSINTS           257
+#define NSMALLPOSINTS           0
 #endif
 #ifndef NSMALLNEGINTS
-#define NSMALLNEGINTS           5
+#define NSMALLNEGINTS           0
 #endif
 
 _Py_IDENTIFIER(little);

Example with this change + the patch:

$ ./python -X faulthandler -c pass
Modules/gcmodule.c:270: visit_decref: Assertion "!_PyObject_IsFreed(op)" failed
Enable tracemalloc to get the memory block allocation traceback

object address  : 0x7fe79f61b4e0
object refcount : 1
object type     : 0x78f5e0
object type name: dict
object repr     : Fatal Python error: Segmentation fault

Current thread 0x00007fe7ac727740 (most recent call first):
Segmentation fault (core dumped)

[ned-deily]

What bug is this backport trying to solve in 3.7? This seems like a new feature and one that would not be appropriate in 3.7 especially at this stage in its life cycle.

[vstinner]

@ned-deily: "What bug is this backport trying to solve in 3.7? This seems like a new feature and one that would not be appropriate in 3.7 especially at this stage in its life cycle."

I adjusted the PR title.

With this change, when Python is built in debug mode, Python can now detects some bugs in C extensions during a garbage collection. Bugs in visit_decref() are the most complex to debug, the function can crash for various reasons. This change helps to detect some kinds of visit_decref() bugs.

My notes on visit_decref():
https://pythondev.readthedocs.io/debug_tools.html#debug-crash-in-garbage-collection-visit-decref

This change is a minimalist and simplified backport of the work I did in the master branch.

The change has no effect when Python is built in release mode. In debug mode, it only changes visit_decref() when it's called on an invalid object. Without this change, Python will crash anyway in this case. For example, deferencing a NULL pointer will crash. Same for op=0xFDFDFDFDFDFDFDFD.

I'm not sure if it makes sense to backport the change to Python 3.7, since it's not trivial to replace a release Python with a debug Python: the ABI is not compatible and C extensions must be recompiled. I'm targeting bugs in third party code, not really bugs in CPython (even if the change will benefit to both).

@pablogsal: What do you think of this change for Python 3.7?

[pablogsal]

@pablogsal: What do you think of this change for Python 3.7?

I think the change is great and super beneficial and will help a lot with debugging, but is technically a new feature and not a bugfix, so I personally don't think it should be backported, even if only affects debug mode.

I also don't know our policy regarding backports that only affects debug mode features....

[pablogsal]

Sorry, I accidentally closed the PR when commenting. I reopened It again. Apologies

[ned-deily]

I understand that this could be a useful debugging enhancement. But we don't have any good way of knowing how our users are using debug mode and this would be a change in behavior that does not fall in our criteria for acceptable bugfix release changes. Plus it is making changes to some of the most fundamental parts of the interpreter. 3.7 is more than halfway through its bugfix support lifecycle; our commitment to our users is to keep it as stable as possible now. A change like this might be appropriate very early in a release's lifecycle (you should ask @ambv what he thinks for 3.8.1) but it seems definitely out of place for a 3.7.6.

[vstinner]

But we don't have any good way of knowing how our users are using debug mode and this would be a change in behavior that does not fall in our criteria for acceptable bugfix release changes.

Yeah, I agree with that. I wasn't sure if "switch Python from release to debug mode" was a realistic ues case for Python 3.7. As I wrote, it's painful before Python 3.8.

Plus it is making changes to some of the most fundamental parts of the interpreter. 3.7 is more than halfway through its bugfix support lifecycle; our commitment to our users is to keep it as stable as possible now. A change like this might be appropriate very early in a release's lifecycle (you should ask @ambv what he thinks for 3.8.1) but it seems definitely out of place for a 3.7.6.

Sure, that makes sense. Thanks for looking at this Ned ;-) I close my PR.