classification
Title: Try to print repr() when an C-level assert fails (in the garbage collector, beyond?)
Type: enhancement Stage: resolved
Components: Interpreter Core Versions: Python 3.3
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: bkabrda, dmalcolm, loewis, ncoghlan, pitrou, serhiy.storchaka, vstinner
Priority: normal Keywords: patch

Created on 2010-07-14 23:50 by dmalcolm, last changed 2018-11-22 16:40 by vstinner. This issue is now closed.

Files
File name Uploaded Description Edit
py3k-repr-on-gcmodule-assertions.patch dmalcolm, 2010-07-14 23:50 review
py3k-objdump-on-gcmodule-assertions-2010-11-22-001.patch dmalcolm, 2010-11-22 22:13 review
py3k-objdump-on-gcmodule-assertions-2011-01-10-001.patch dmalcolm, 2011-01-10 23:57 review
py3k-objdump-on-gcmodule-assertions-2011-01-10-002.patch dmalcolm, 2011-01-10 23:59 review
00170-gc-assertions.patch bkabrda, 2013-06-10 19:53
Pull Requests
URL Status Linked Edit
PR 10061 merged vstinner, 2018-10-23 15:14
PR 10062 merged vstinner, 2018-10-23 15:52
PR 10108 merged vstinner, 2018-10-25 22:33
PR 10109 merged vstinner, 2018-10-25 22:55
PR 10110 merged vstinner, 2018-10-25 23:14
PR 10111 merged vstinner, 2018-10-25 23:33
PR 10112 merged vstinner, 2018-10-25 23:40
PR 10661 merged vstinner, 2018-11-22 15:09
PR 10662 merged vstinner, 2018-11-22 15:37
PR 10663 merged vstinner, 2018-11-22 16:20
Messages (20)
msg110341 - (view) Author: Dave Malcolm (dmalcolm) (Python committer) Date: 2010-07-14 23:50
Modules/gcmodule.c contains various assertions which can fail due to reference counting errors elsewhere in either python, or an extension module.  These can be difficult to track down.

In the hope of maximizing the information from crash reports, the attached patch (against py3k) introduces a new assertion macro to Objects/object.h and Objects.c, which provides a richer debug message.  In particular, it identifies which object has the issue, and can more clearly spell out the problem.

The patch replaces all uses of assert() in Modules/gcmodule.c for which a specific object has an issue (e.g. bogus reference count).

The implementation may play somewhat fast-and-loose with rules about object invariants: you might have an only partially valid object, but the process is about to abort, so it seems acceptable to try to glean extra information on stderr.  (This may turn an abort into a segfault, of course)

Caveats:
- exact name of the API probably could be better
- I don't yet have a specific use for the "callback" idea; I was thinking of trying to display all objects that reference that object.  Might need a void* closure to be useful.  Might be a useless complication.
- Only tested on gcc-4.4.3 so far; the __STRING(expr) and __PRETTY_FUNCTION__ look non-portable.
- no test case; I thought about using ctypes to extract PyObject_IncRef from the implementation, but this is likely to lead to brittle test cases.  Alternatively, is xxmodule to be used for this kind of thing?

Thoughts?
msg122170 - (view) Author: Dave Malcolm (dmalcolm) (Python committer) Date: 2010-11-22 22:13
Attaching a simplified version of the patch; I got rid of the callbacks.

Still doesn't have test cases.

I suspect that the use of __STRING and __PRETTY_FUNCTION__ may be compatibility issues.  I believe that __FILE__ and __LINE__ and standard C though.
msg125959 - (view) Author: Dave Malcolm (dmalcolm) (Python committer) Date: 2011-01-10 23:57
Attaching updated version of the patch.

I've added a selftest which (in a sacrificial subprocess) abuses ctypes to break an ob_refcnt, and then triggers a garbage collection.

I also changed the printing to stderr to directly use fprintf and fflush to ensure that data leaves the process before abort kills it (not sure if this is a cross-platform or unicode no-no, though).
msg125960 - (view) Author: Dave Malcolm (dmalcolm) (Python committer) Date: 2011-01-10 23:59
As above, but I added an extra call to fflush in case the call to _PyObject_Dump leads to a segfault.
msg131016 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2011-03-15 18:39
How about turning these asserts into Py_FatalError()s and then enabling Victor's faulthandler extension?
msg131036 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2011-03-15 20:36
I'd suggest calling Py_FatalError rather than calling abort() directly in _PyObject_AssertFailed, but otherwise this looks like a nice improvement over standard C asserts for state invariants that may be broken by buggy C extensions.

For the tests, take a look at test.script_helper - it provides some convenience wrappers for spawning subprocesses for tests that would cause problems if run in the current process.
msg190929 - (view) Author: Bohuslav "Slavek" Kabrda (bkabrda) * Date: 2013-06-10 19:53
I'm currently patching Python 3.3.2 with this, so I thought it might be nice to attach an up-to-date patch. The only notable difference is that I added self.preclean() at the beginning of test_refcount_errors - without it, running test suite produced a huge number of these lines:

Exception AttributeError: "'GCCallbackTests' object has no attribute 'visit'" in <bound method GCCallbackTests.cb1 of <__main__.GCCallbackTests testMethod=test_refcount_errors>> ignored
msg328323 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2018-10-23 15:39
New changeset 82af0b63b07aa8d92b50098e382b458143cfc677 by Victor Stinner in branch 'master':
bpo-9263: _PyObject_Dump() detects freed memory (GH-10061)
https://github.com/python/cpython/commit/82af0b63b07aa8d92b50098e382b458143cfc677
msg328327 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2018-10-23 17:07
00170-gc-assertions.patch is used in the Fedora package of Python.

I converted the patch to a pull request: PR 10062.

My PR only uses the new assertion macro once. I plan to write more changes to use the new assertion macros in more places, once the first PR is merged.
msg328452 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2018-10-25 15:31
New changeset 626bff856840f471e98ec627043f780c111a063d by Victor Stinner in branch 'master':
bpo-9263: Dump Python object on GC assertion failure (GH-10062)
https://github.com/python/cpython/commit/626bff856840f471e98ec627043f780c111a063d
msg328507 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2018-10-26 00:12
New changeset 3ec9af75f6825a32f369ee182a388c365db241b6 by Victor Stinner in branch 'master':
bpo-9263: _Py_NegativeRefcount() use _PyObject_AssertFailed() (GH-10109)
https://github.com/python/cpython/commit/3ec9af75f6825a32f369ee182a388c365db241b6
msg328560 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2018-10-26 15:16
New changeset 24702044afb1d4ad7568bf6aa7450b14dc44a38f by Victor Stinner in branch 'master':
bpo-9263: Use _PyObject_ASSERT() in object.c (GH-10110)
https://github.com/python/cpython/commit/24702044afb1d4ad7568bf6aa7450b14dc44a38f
msg328566 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2018-10-26 16:00
New changeset a4b2bc70f69d93d8252861b455052c051b7167ae by Victor Stinner in branch 'master':
bpo-9263: Use _PyObject_ASSERT() in gcmodule.c (GH-10112)
https://github.com/python/cpython/commit/a4b2bc70f69d93d8252861b455052c051b7167ae
msg328568 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2018-10-26 16:39
New changeset 0862505a0377c12e8004b2eb8de0555f26ce9530 by Victor Stinner in branch 'master':
bpo-9263: Use _PyObject_ASSERT() in typeobject.c (GH-10111)
https://github.com/python/cpython/commit/0862505a0377c12e8004b2eb8de0555f26ce9530
msg328569 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2018-10-26 16:47
New changeset 50fe3f8913c503e63f4cfb8ddcf8641ef7ad0722 by Victor Stinner in branch 'master':
bpo-9263: _PyXXX_CheckConsistency() use _PyObject_ASSERT() (GH-10108)
https://github.com/python/cpython/commit/50fe3f8913c503e63f4cfb8ddcf8641ef7ad0722
msg328575 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2018-10-26 17:13
I pushed the change and even more, so I consider that the issue can now be closed... 8 years later! Thank you very much Dave Malcolm for this nice idea, and for its implementation. Thanks Bohuslav "Slavek" Kabrda for the rebase in 2013, and thanks to my colleagues who rebased the patch frequently since 2013 in the Fedora package!

Maybe some people (like me?) want to use _PyObject_ASSERT() in more places, but I consider that we don't need to leave this issue open just for that.


I took the 00170-gc-assertions.patch rebased on Python 3.7.1 by my colleagues for the Fedora package, and I rebased it on master. I modified more functions in object.c and typeobject.c to use _PyObject_ASSERT(). I tried to not replace all assert(), but only when it's revelant.

I added code to detect if the object memory has been freed to avoid derefering 0xdbdbdbdbdbdbdbdb pointers which is very likely to cause a segmantation fault. It should reduce the risk of crash when dumping the faulty object.


Dave Malcolm:
> - Only tested on gcc-4.4.3 so far; the __STRING(expr) and __PRETTY_FUNCTION__ look non-portable.

I used Py_STRINGIFY() and __func__ in the final patch. __func__ is part of the C99 standard which is now required since Python 3.6: see PEP 7.


Dave Malcolm:
> - no test case; I thought about using ctypes to extract PyObject_IncRef from the implementation, but this is likely to lead to brittle test cases.  Alternatively, is xxmodule to be used for this kind of thing?

I reworked the unit test to not use ctypes, but write the crashing code in C instead.


Antoine Pitrou:
> How about turning these asserts into Py_FatalError()s and then enabling Victor's faulthandler extension?

Done.
msg328576 - (view) Author: Dave Malcolm (dmalcolm) (Python committer) Date: 2018-10-26 17:26
Thanks!
msg330266 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2018-11-22 15:33
New changeset 2cf5d32fd9e61488e8b0be55a2e92a752ba8b06b by Victor Stinner in branch 'master':
bpo-9263: Fix _PyObject_Dump() for freed object (#10661)
https://github.com/python/cpython/commit/2cf5d32fd9e61488e8b0be55a2e92a752ba8b06b
msg330267 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2018-11-22 16:15
New changeset 95036ea25d47f0081bda2ba96ea327f3375cb6a4 by Victor Stinner in branch '3.7':
[3.7] bpo-9263: _PyObject_Dump() detects freed memory (GH-10061) (GH-10662)
https://github.com/python/cpython/commit/95036ea25d47f0081bda2ba96ea327f3375cb6a4
msg330270 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2018-11-22 16:40
New changeset c9b3fc6b59b625c36c31ad437253e7140938af1a by Victor Stinner in branch '3.6':
bpo-9263: _PyObject_Dump() detects freed memory (GH-10061) (GH-10662) (GH-10663)
https://github.com/python/cpython/commit/c9b3fc6b59b625c36c31ad437253e7140938af1a
History
Date User Action Args
2018-11-22 16:40:57vstinnersetmessages: + msg330270
2018-11-22 16:20:14vstinnersetpull_requests: + pull_request9916
2018-11-22 16:15:41vstinnersetmessages: + msg330267
2018-11-22 15:37:23vstinnersetpull_requests: + pull_request9915
2018-11-22 15:33:01vstinnersetmessages: + msg330266
2018-11-22 15:09:11vstinnersetpull_requests: + pull_request9914
2018-10-26 17:26:04dmalcolmsetmessages: + msg328576
2018-10-26 17:13:51vstinnersetstatus: open -> closed
resolution: fixed
messages: + msg328575

stage: patch review -> resolved
2018-10-26 16:47:23vstinnersetmessages: + msg328569
2018-10-26 16:39:22vstinnersetmessages: + msg328568
2018-10-26 16:00:20vstinnersetmessages: + msg328566
2018-10-26 15:16:41vstinnersetmessages: + msg328560
2018-10-26 00:12:40vstinnersetmessages: + msg328507
2018-10-25 23:40:27vstinnersetpull_requests: + pull_request9444
2018-10-25 23:33:18vstinnersetpull_requests: + pull_request9443
2018-10-25 23:14:49vstinnersetpull_requests: + pull_request9442
2018-10-25 22:55:30vstinnersetpull_requests: + pull_request9441
2018-10-25 22:33:15vstinnersetpull_requests: + pull_request9440
2018-10-25 15:31:17vstinnersetmessages: + msg328452
2018-10-23 17:47:48serhiy.storchakasetnosy: + serhiy.storchaka
2018-10-23 17:07:48vstinnersetmessages: + msg328327
2018-10-23 15:52:09vstinnersetpull_requests: + pull_request9398
2018-10-23 15:39:47vstinnersetnosy: + vstinner
messages: + msg328323
2018-10-23 15:14:23vstinnersetpull_requests: + pull_request9397
2013-06-10 19:53:31bkabrdasetfiles: + 00170-gc-assertions.patch
nosy: + bkabrda
messages: + msg190929

2011-03-15 20:36:39ncoghlansetnosy: + ncoghlan
messages: + msg131036
2011-03-15 18:39:23pitrousetnosy: loewis, pitrou, dmalcolm
messages: + msg131016
2011-03-15 18:28:49dmalcolmsetnosy: + loewis
2011-01-10 23:59:57dmalcolmsetfiles: + py3k-objdump-on-gcmodule-assertions-2011-01-10-002.patch
nosy: pitrou, dmalcolm
messages: + msg125960
2011-01-10 23:57:33dmalcolmsetfiles: + py3k-objdump-on-gcmodule-assertions-2011-01-10-001.patch
nosy: pitrou, dmalcolm
messages: + msg125959
2011-01-03 20:07:22pitrousetnosy: pitrou, dmalcolm
versions: + Python 3.3, - Python 3.2
2010-11-22 22:13:23dmalcolmsetfiles: + py3k-objdump-on-gcmodule-assertions-2010-11-22-001.patch

messages: + msg122170
2010-11-22 21:59:37dmalcolmsetnosy: + pitrou
2010-07-14 23:50:49dmalcolmcreate