Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

gh-108082: Use PyErr_FormatUnraisable() #111580

Merged

Conversation

serhiy-storchaka
Copy link
Member

@serhiy-storchaka serhiy-storchaka commented Oct 31, 2023

Replace most of calls of _PyErr_WriteUnraisableMsg() and some calls of PyErr_WriteUnraisable(NULL) with PyErr_FormatUnraisable().

Replace most of calls of _PyErr_WriteUnraisableMsg() and some
calls of PyErr_WriteUnraisable(NULL) with PyErr_FormatUnraisable().
@@ -1032,8 +1032,8 @@ delete_garbage(PyThreadState *tstate, GCState *gcstate,
Py_INCREF(op);
(void) clear(op);
if (_PyErr_Occurred(tstate)) {
_PyErr_WriteUnraisableMsg("in tp_clear of",
(PyObject*)Py_TYPE(op));
PyErr_FormatUnraisable("Exception ignored in tp_clear of %.200s",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO %.200s is a bug here. We should no longer truncate type names if they are too long. Many years ago, Python had an internal buffer of a fixed size and so the overall string must not be too long. But this has been fixed.

Please don't truncate type names.

Modules/getpath.c Outdated Show resolved Hide resolved
Modules/getpath.c Outdated Show resolved Hide resolved
Comment on lines 905 to 907
PyErr_FormatUnraisable("Exception ignored in m_clear of module%s%V\n",
m->md_name ? " " : "",
m->md_name, "");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
PyErr_FormatUnraisable("Exception ignored in m_clear of module%s%V\n",
m->md_name ? " " : "",
m->md_name, "");
PyErr_FormatUnraisable("Exception ignored in m_clear of module%s%V",
m->md_name ? " " : "",
m->md_name);

@@ -828,7 +828,7 @@ PyType_Modified(PyTypeObject *type)
if (bits & 1) {
PyType_WatchCallback cb = interp->type_watchers[i];
if (cb && (cb(type) < 0)) {
PyErr_WriteUnraisable((PyObject *)type);
PyErr_FormatUnraisable("Exception ignored in watcher callback for %R", i, type);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you want to log i variable, you should add %i somewhere.

Python/import.c Outdated Show resolved Hide resolved
Python/perf_trampoline.c Outdated Show resolved Hide resolved
Copy link
Member

@vstinner vstinner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. I just left two minor suggestions.

PyErr_WriteUnraisable(NULL);
PyErr_FormatUnraisable("Exception ignored in m_clear of module%s%V",
m->md_name ? " " : "",
m->md_name, "");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The last "" is not used, it can be removed.

Objects/typeobject.c Outdated Show resolved Hide resolved
@serhiy-storchaka serhiy-storchaka enabled auto-merge (squash) November 2, 2023 08:50
@serhiy-storchaka serhiy-storchaka merged commit 970e719 into python:main Nov 2, 2023
28 checks passed
@bedevere-bot
Copy link

⚠️⚠️⚠️ Buildbot failure ⚠️⚠️⚠️

Hi! The buildbot s390x Fedora Clang 3.x has failed when building commit 970e719.

What do you need to do:

  1. Don't panic.
  2. Check the buildbot page in the devguide if you don't know what the buildbots are or how they work.
  3. Go to the page of the buildbot that failed (https://buildbot.python.org/all/#builders/3/builds/4870) and take a look at the build logs.
  4. Check if the failure is related to this commit (970e719) or if it is a false positive.
  5. If the failure is related to this commit, please, reflect that on the issue and make a new Pull Request with a fix.

You can take a look at the buildbot page here:

https://buildbot.python.org/all/#builders/3/builds/4870

Summary of the results of the build (if available):

==

Click to see traceback logs
remote: Enumerating objects: 53, done.        
remote: Counting objects:   1% (1/53)        
remote: Counting objects:   3% (2/53)        
remote: Counting objects:   5% (3/53)        
remote: Counting objects:   7% (4/53)        
remote: Counting objects:   9% (5/53)        
remote: Counting objects:  11% (6/53)        
remote: Counting objects:  13% (7/53)        
remote: Counting objects:  15% (8/53)        
remote: Counting objects:  16% (9/53)        
remote: Counting objects:  18% (10/53)        
remote: Counting objects:  20% (11/53)        
remote: Counting objects:  22% (12/53)        
remote: Counting objects:  24% (13/53)        
remote: Counting objects:  26% (14/53)        
remote: Counting objects:  28% (15/53)        
remote: Counting objects:  30% (16/53)        
remote: Counting objects:  32% (17/53)        
remote: Counting objects:  33% (18/53)        
remote: Counting objects:  35% (19/53)        
remote: Counting objects:  37% (20/53)        
remote: Counting objects:  39% (21/53)        
remote: Counting objects:  41% (22/53)        
remote: Counting objects:  43% (23/53)        
remote: Counting objects:  45% (24/53)        
remote: Counting objects:  47% (25/53)        
remote: Counting objects:  49% (26/53)        
remote: Counting objects:  50% (27/53)        
remote: Counting objects:  52% (28/53)        
remote: Counting objects:  54% (29/53)        
remote: Counting objects:  56% (30/53)        
remote: Counting objects:  58% (31/53)        
remote: Counting objects:  60% (32/53)        
remote: Counting objects:  62% (33/53)        
remote: Counting objects:  64% (34/53)        
remote: Counting objects:  66% (35/53)        
remote: Counting objects:  67% (36/53)        
remote: Counting objects:  69% (37/53)        
remote: Counting objects:  71% (38/53)        
remote: Counting objects:  73% (39/53)        
remote: Counting objects:  75% (40/53)        
remote: Counting objects:  77% (41/53)        
remote: Counting objects:  79% (42/53)        
remote: Counting objects:  81% (43/53)        
remote: Counting objects:  83% (44/53)        
remote: Counting objects:  84% (45/53)        
remote: Counting objects:  86% (46/53)        
remote: Counting objects:  88% (47/53)        
remote: Counting objects:  90% (48/53)        
remote: Counting objects:  92% (49/53)        
remote: Counting objects:  94% (50/53)        
remote: Counting objects:  96% (51/53)        
remote: Counting objects:  98% (52/53)        
remote: Counting objects: 100% (53/53)        
remote: Counting objects: 100% (53/53), done.        
remote: Compressing objects:   4% (1/25)        
remote: Compressing objects:   8% (2/25)        
remote: Compressing objects:  12% (3/25)        
remote: Compressing objects:  16% (4/25)        
remote: Compressing objects:  20% (5/25)        
remote: Compressing objects:  24% (6/25)        
remote: Compressing objects:  28% (7/25)        
remote: Compressing objects:  32% (8/25)        
remote: Compressing objects:  36% (9/25)        
remote: Compressing objects:  40% (10/25)        
remote: Compressing objects:  44% (11/25)        
remote: Compressing objects:  48% (12/25)        
remote: Compressing objects:  52% (13/25)        
remote: Compressing objects:  56% (14/25)        
remote: Compressing objects:  60% (15/25)        
remote: Compressing objects:  64% (16/25)        
remote: Compressing objects:  68% (17/25)        
remote: Compressing objects:  72% (18/25)        
remote: Compressing objects:  76% (19/25)        
remote: Compressing objects:  80% (20/25)        
remote: Compressing objects:  84% (21/25)        
remote: Compressing objects:  88% (22/25)        
remote: Compressing objects:  92% (23/25)        
remote: Compressing objects:  96% (24/25)        
remote: Compressing objects: 100% (25/25)        
remote: Compressing objects: 100% (25/25), done.        
remote: Total 27 (delta 26), reused 2 (delta 2), pack-reused 0        
From https://github.com/python/cpython
 * branch                  main       -> FETCH_HEAD
Note: switching to '970e719a7a829bddc647bbaa668dd8603abdddef'.

You are in 'detached HEAD' state. You can look around, make experimental
changes and commit them, and you can discard any commits you make in this
state without impacting any branches by switching back to a branch.

If you want to create a new branch to retain commits you create, you may
do so (now or later) by using -c with the switch command. Example:

  git switch -c <new-branch-name>

Or undo this operation with:

  git switch -

Turn off this advice by setting config variable advice.detachedHead to false

HEAD is now at 970e719a7a gh-108082: Use PyErr_FormatUnraisable() (GH-111580)
Switched to and reset branch 'main'

configure: WARNING: s390x-ibm-linux-gnu/clang is not supported
configure: WARNING:

Platform "s390x-ibm-linux-gnu" with compiler "clang" is not supported by the
CPython core team, see https://peps.python.org/pep-0011/ for more information.


make: *** [Makefile:2065: buildbottest] Error 3

@vstinner
Copy link
Member

vstinner commented Nov 2, 2023

I wanted to do this change for many years. It's awful to get a warning without any context :-( Thanks for this nice enhancement! I like PyErr_FormatUnraisable() API!

@serhiy-storchaka serhiy-storchaka deleted the use-PyErr_FormatUnraisable branch November 2, 2023 11:12
@serhiy-storchaka
Copy link
Member Author

We both wanted it. But other tasks had higher priorities.

FullteaR pushed a commit to FullteaR/cpython that referenced this pull request Nov 3, 2023
Replace most of calls of _PyErr_WriteUnraisableMsg() and some
calls of PyErr_WriteUnraisable(NULL) with PyErr_FormatUnraisable().

Co-authored-by: Victor Stinner <[email protected]>
aisk pushed a commit to aisk/cpython that referenced this pull request Feb 11, 2024
Replace most of calls of _PyErr_WriteUnraisableMsg() and some
calls of PyErr_WriteUnraisable(NULL) with PyErr_FormatUnraisable().

Co-authored-by: Victor Stinner <[email protected]>
Glyphack pushed a commit to Glyphack/cpython that referenced this pull request Sep 2, 2024
Replace most of calls of _PyErr_WriteUnraisableMsg() and some
calls of PyErr_WriteUnraisable(NULL) with PyErr_FormatUnraisable().

Co-authored-by: Victor Stinner <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants