GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — gh-106670: Fix Pdb handling of chained Exceptions with no stacks. by Carreau · Pull Request #108865 · python/cpython

Conversation

@Carreau

@Carreau Carreau commented Sep 4, 2023 •
edited by bedevere-bot
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

The introduction of chained exception in gh-106676 would sometime lead to

File .../Lib/pdb.py", line 298, in setup
  self.curframe = self.stack[self.curindex][0]
                 ~~~~~~~~~~^^^^^^^^^^^^^^^
IndexError: list index out of range

This fixes that by filtering exceptions that that do not have a stack/traceback. Update tests to not use stack-less exceptions when testing another feature, and add an explicit test on how we handle stackless exceptions.

  • Issue: Support to jump between chained exception in Pdb #106670

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@Carreau
pythongh-106670: Fix Pdb handling of chained Exceptions with no stacks.
b0c7dbd
The introduction of chained exception in pythongh-106676 would sometime lead to

    File .../Lib/pdb.py", line 298, in setup
      self.curframe = self.stack[self.curindex][0]
                    ~~~~~~~~~~^^^^^^^^^^^^^^^
    IndexError: list index out of range

This fixes that by filtering exceptions that that do not have a
stack/traceback. Update tests to not use stack-less exceptions when
testing another feature, and add an explicit test on how we handle
stackless exceptions.
@bedevere-bot bedevere-bot added the awaiting review label Sep 4, 2023
@bedevere-bot bedevere-bot mentioned this pull request Sep 4, 2023
Support to jump between chained exception in Pdb #106670
Closed
@Carreau

Carreau commented Sep 4, 2023

Copy link
Copy Markdown
Contributor Author

I can't add reviewers, can someone add @gaogaotiantian and @iritkatriel ?

I'm not sure this needs a news as it's a fix for something that was added a week ago.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@Carreau Carreau mentioned this pull request Sep 4, 2023
gh-106670: Allow Pdb to move between chained exceptions #106676
Merged
@AlexWaygood
AlexWaygood requested review from gaogaotiantian, iritkatriel and sunmy2019 and removed request for sunmy2019 September 4, 2023 09:44
@iritkatriel

iritkatriel commented Sep 4, 2023

Copy link
Copy Markdown
Member

How do I reproduce the bug on pdb?

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

iritkatriel
iritkatriel reviewed Sep 4, 2023
View reviewed changes
Comment thread Lib/test/test_pdb.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

@Carreau

Carreau commented Sep 4, 2023

Copy link
Copy Markdown
Contributor Author

How do I reproduce the bug on pdb?

Oh, yes, sure:

Python 3.13.0a0 (heads/main-dirty:08727d0af61, Sep  4 2023, 11:11:14) [Clang 14.0.6 ] on darwin
Type "help", "copyright", "credits" or "license" for more information.
>>> def f():
...     raise Exception() from Exception()
...
>>> f()
Exception

The above exception was the direct cause of the following exception:

Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "<stdin>", line 2, in f
Exception
>>> import pdb
>>> pdb.pm()
> <stdin>(2)f()
(Pdb) exceptions
    0 Exception()
>   1 Exception()
(Pdb) exceptions 0
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/Users/bussonniermatthias/dev/cpython/Lib/pdb.py", line 2026, in pm
    post_mortem(sys.last_exc)
  File "/Users/bussonniermatthias/dev/cpython/Lib/pdb.py", line 2022, in post_mortem
    p.interaction(None, t)
  File "/Users/bussonniermatthias/dev/cpython/Lib/pdb.py", line 501, in interaction
    self._cmdloop()
  File "/Users/bussonniermatthias/dev/cpython/Lib/pdb.py", line 405, in _cmdloop
    self.cmdloop()
  File "/Users/bussonniermatthias/dev/cpython/Lib/cmd.py", line 138, in cmdloop
    stop = self.onecmd(line)
           ^^^^^^^^^^^^^^^^^
  File "/Users/bussonniermatthias/dev/cpython/Lib/pdb.py", line 592, in onecmd
    return cmd.Cmd.onecmd(self, line)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/bussonniermatthias/dev/cpython/Lib/cmd.py", line 217, in onecmd
    return func(arg)
           ^^^^^^^^^
  File "/Users/bussonniermatthias/dev/cpython/Lib/pdb.py", line 1178, in do_exceptions
    self.setup(None, self._chained_exceptions[number].__traceback__)
  File "/Users/bussonniermatthias/dev/cpython/Lib/pdb.py", line 298, in setup
    self.curframe = self.stack[self.curindex][0]
                    ~~~~~~~~~~^^^^^^^^^^^^^^^
IndexError: list index out of range
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

Carreau
Carreau commented Sep 4, 2023
View reviewed changes
Comment thread Lib/pdb.py Outdated
Comment on lines +494 to +497
if not _chained_exceptions and isinstance(tb_or_exc, BaseException):
raise ValueError(
"A valid traceback must be passed if no exception is being handled"
)

@Carreau Carreau Sep 4, 2023 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

This is supposed to catch this reproducer

>>> import sys
>>> import pdb
>>> sys.last_exc = Exception()
>>> pdb.pm()

It's contrived, but as it is already handled in pdb.post_mortem(), I assume there are case where this can happen in production.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@Carreau
change wording of exception
3607006
@Carreau
Carreau force-pushed the fix-pdb-no-tb branch from 2e6c7de to 3607006 Compare September 4, 2023 17:09
iritkatriel
iritkatriel reviewed Sep 4, 2023
View reviewed changes
Comment thread Lib/pdb.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

iritkatriel
iritkatriel reviewed Sep 4, 2023
View reviewed changes
Comment thread Lib/test/test_pdb.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

@Carreau
take comments into account
fbd394d
@gaogaotiantian

gaogaotiantian commented Sep 4, 2023

Copy link
Copy Markdown
Member

Is there any meaningful case where the latest exception (the one passed into pm() or sys.last_exc) does not have traceback, but the chained exceptions could be useful?

Here's what I thought:

  • It's probably common where a chained exception does not have traceback, but it's normally not the exception raised and caught.
  • When the exception pdb catches (through pm(), interaction is not documented API) does not have the traceback, there's normally nothing interesting.

However:

  • We SHOULD keep the exceptions without tracebacks when possible, because they are part of the chain and it would confuse the users if there's an exception missing just because there's no trackback attached.
  • We should try NOT to raise exceptions from a common function deep in pdb - it would be frustrating to users when pdb errors out and interaction is a function used else where. It may only raise the exception in this very specific situation now, but in the future it could be triggered under other circumstances when there are some code changes. If we want to raise an exception, we should raise early.

So, I think the better way to do it is:

  • Check whether we can handle it (whether the raised exception has traceback) in post_mortem(), the same place as the existing ValueError. If not, raise the exception there.
  • Once interaction() is called, we do not ever raise any exceptions. The code should make sure the latest exception has a trackback to display, and do_exceptions should check if the "target exception" has a traceback and display a message to user when it does not. No exception raised.

It would be much clearer to the future maintainers that pdb does not raise exception under a very complicated condition, and it would make the code in interaction() easier to understand - keep the exception related logic in do_exceptions.

Thoughts @iritkatriel ?

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@Carreau

Carreau commented Sep 4, 2023

Copy link
Copy Markdown
Contributor Author

@gaogaotiantian I'll see what I can do, but it might probably be quite invasive to support exceptions w/o tb. I can maybe keep them but refuse to exceptions #tb-without-traceback and/or display them differently in exceptions.

Note that interaction() is fairly early in Pdb's life, were you maybe thinkingof _cmdloop maybe ?

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@iritkatriel

iritkatriel commented Sep 4, 2023

Copy link
Copy Markdown
Member

Yes, I think it would be good if all exceptions were shown, and those without traceback would display that somehow (rather than erroring).

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@gaogaotiantian

gaogaotiantian commented Sep 4, 2023

Copy link
Copy Markdown
Member

@gaogaotiantian I'll see what I can do, but it might probably be quite invasive to support exceptions w/o tb. I can maybe keep them but refuse to exceptions #tb-without-traceback and/or display them differently in exceptions.

Note that interaction() is fairly early in Pdb's life, were you maybe thinkingof _cmdloop maybe ?

Yes, as we can guarantee the default exception(latest one) is with traceback, we can simply decline all the access to the exceptions without tracebacks when the users do exceptions #exception_without_traceback. Send a message saying there's no traceback, and keep them in the current exception - so we don't need to take care of all the commands in a setup without the traceback.

interaction() is fairly early, but it's still called for every trace trigger - including the exception trigger which might take advantage of the exception feature in the future. We want to put the exception as closer to the surface as possible.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@Carreau
Try to handle exceptions without traceback
b138dde
@Carreau

Carreau commented Sep 5, 2023

Copy link
Copy Markdown
Contributor Author

Ok, what about the following ? in do_exceptions I print - as number for exceptions that do not have a traceback (keeping the numbering though, so you would get something like

 (Pdb) exceptions
     - Exception()
     1 Exception()
     2 Exception()
     - Exception()
 >   4 Exception()
     5 Exception()
     - Exception()
     7 Exception()

And on attempt to jump to it, it says: This exception has not traceback, cannot jump to it

There is one caveat WRT testing, I can't properly test post_mortem() as the testing needs to pdb.Pdb(nosigint=True, readrc=False) and post_mortem() create its own instance I have no control over.

iritkatriel reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@Carreau
introduce private _post_mortem
7b697f1
@Carreau

Carreau commented Sep 5, 2023

Copy link
Copy Markdown
Contributor Author

There is one caveat WRT testing, I can't properly test post_mortem() as the testing needs to pdb.Pdb(nosigint=True, readrc=False) and post_mortem() create its own instance I have no control over.

I added a private version of post-mortem that takes an instance so that I can use it in the test.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

Carreau
Carreau commented Sep 5, 2023
View reviewed changes
Comment thread Lib/pdb.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

@Carreau
Update Lib/pdb.py
e47906c
Carreau
Carreau commented Sep 5, 2023
View reviewed changes
Comment thread Lib/pdb.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

iritkatriel
iritkatriel reviewed Sep 5, 2023
View reviewed changes
Comment thread Lib/pdb.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

Carreau and others added 4 commits September 5, 2023 02:27
@Carreau @iritkatriel
Update Lib/pdb.py
02f5b32
Co-authored-by: Irit Katriel <[email protected]>
@Carreau
protect interaction, if an exception is given and has not tb jumpt to…
087e39a
… the next previous one with tb
@Carreau
Merge remote-tracking branch 'github/fix-pdb-no-tb' into fix-pdb-no-tb
c69a3b7
@Carreau
fix wording
f90a453
iritkatriel
iritkatriel reviewed Sep 5, 2023
View reviewed changes
Comment thread Lib/pdb.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

Carreau
Carreau commented Sep 5, 2023
View reviewed changes
Comment thread Lib/pdb.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

Carreau added 2 commits September 5, 2023 11:02
@Carreau
Update Lib/pdb.py
e7533c9
@Carreau
remove autoumping to last exception w/o tb
368222c
iritkatriel
iritkatriel approved these changes Sep 6, 2023
View reviewed changes
@bedevere-bot bedevere-bot added awaiting merge and removed awaiting review labels Sep 6, 2023
@iritkatriel
iritkatriel enabled auto-merge (squash) September 6, 2023 09:41
@iritkatriel iritkatriel added skip news stdlib Standard Library Python modules in the Lib/ directory labels Sep 6, 2023
@iritkatriel
iritkatriel merged commit 5f3433f into python:main Sep 6, 2023
@bedevere-bot bedevere-bot removed the awaiting merge label Sep 6, 2023
@Carreau

Carreau commented Sep 6, 2023 via email

Copy link
Copy Markdown
Contributor Author
thanks !
…
On Wed, Sep 6, 2023 at 11:42 Irit Katriel ***@***.***> wrote: Merged #108865 <#108865> into main. — Reply to this email directly, view it on GitHub <#108865 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AACR5TZLKHNFVV2YC73XJL3XZBAPBANCNFSM6AAAAAA4KEPKUQ> . You are receiving this because you authored the thread.Message ID: ***@***.***>
iritkatriel reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@iritkatriel iritkatriel iritkatriel approved these changes
@gaogaotiantian gaogaotiantian Awaiting requested review from gaogaotiantian gaogaotiantian is a code owner

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

skip news stdlib Standard Library Python modules in the Lib/ directory

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

4 participants

@Carreau @iritkatriel @gaogaotiantian @bedevere-bot
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.