| msg94764 - (view) |
Author: Ilya Sandler (isandler) |
Date: 2009-10-31 19:37 |
| Currently, pressing Ctrl-C in pdb will terminate the program and throw the user into post-mortem debugging. Other debuggers (e.g gdb and pydb) treat Ctrl-C differently: Ctrl-C only stops the program and the user can resume it if needed. I believe current pdb behavior is user-unfriendly (as wanting to stop and then resume the execution is a very common use case which is not supported by pdb at all (I think)). The attached patch changes pdb's Ctrl-C behavior to match gdb's: Ctrl-C will stop the program and the user can resume the execution later. |
|
|
| msg94782 - (view) |
Author: Raghuram Devarakonda (draghuram)  |
Date: 2009-11-01 04:45 |
| It is better for this functionality to be added in "Cmd" module as that will benefit all users of that module. Please see bug #1294 which has a patch for this purpose. It would be nice if you can test with that patch and see if pdb works as you expected. |
|
|
| msg94811 - (view) |
Author: Ilya Sandler (isandler) |
Date: 2009-11-01 22:44 |
| No,I don't think patch in the issue #1294 addresses the problem which I'm trying to solve. I tried applying patch#1294, and Ctrl-C will still throws your debugger into postmortem mode and I don't think you can change that by overriding do_KeyboardInterrupt(): when do_KbdInterrupt() is called you cannot resume execution (or at least I don't know any way of doing it). My patch handles the SIGINT directly which allows it to set tracing and resume the program immediately (and keyboardinterrupt is never raised) I'll also add comments to patch#1294 |
|
|
| msg99661 - (view) |
Author: Gregory P. Smith (gregory.p.smith) *  |
Date: 2010-02-21 14:07 |
| http://codereview.appspot.com/216067/show |
|
|
| msg99702 - (view) |
Author: Ilya Sandler (isandler) |
Date: 2010-02-22 02:19 |
| I fixed some of the style issues mentioned on appspot. (I was not sure about some of them and responded to them in appspot comments). Also sigHandler became sighandler for consistency with the rest of pdb.py. The new version of the patch is attached. However, it appears that I've been a bit over-optimistic about the lack of side-effects. In particular, the patch results in an very ugly error message when Ctrl-C is pressed while at pdb prompt.. *** AttributeError: AttributeError("'NoneType' object has no attribute 'f_globals'",) Everything still seems to be working, but it's definitely ugly (and behaves differently on Windows and Linux). I will try to summarize possible Ctrl-C scenarios in a separate post |
|
|
| msg99706 - (view) |
Author: Ilya Sandler (isandler) |
Date: 2010-02-22 03:00 |
| Here is a list of Ctrl-C scenarios: ("current" below means the prepatch version of pdb). 1. program is running (last command was "c", "n", etc). Currently, Ctrl-C throws debugger into postmortem. Desired behavior: interrupt the program. This is the only scenario supported by the patch. 2. Program is stopped (debugger is interacting with the user). Currently, Ctrl-C will throw debugger into postmortem. Desired behaviour: either ignore Ctrl-C or abandon the current command (that's what gdb does). 3. Program is stopped and pdb runs one of the commands which catch exceptions (e.g "p"). Currently, Ctrl-C will abort the command and return pdb to the prompt. I think this behavior should be kept. 4. Program finished (debugger is in postmortem). Currently, Ctrl-C will quit the debugger. Desired behavior: either ignore Ctrl-C or abandon the current command. Essentially, I think the best behavior is to have Ctrl-C to interrupt whatever pdb is doing and return to the fresh prompt. I am assuming that behavior on Windows and Linux should be identical/nearly identical. Does the above list make sense? I would greatly appreciate any feedback/comments/guidance/etc. |
|
|
| msg99708 - (view) |
Author: R. David Murray (r.david.murray) * |
Date: 2010-02-22 03:28 |
| Your summary makes sense to me, at least. |
|
|
| msg100177 - (view) |
Author: Ilya Sandler (isandler) |
Date: 2010-02-27 04:11 |
| Another iteration of the patch. Now sigint_handler will generate KeyboardInterrupts when pdb is in the commandloop I think this guarantees consistent "Ctrl-C interrupts the current pdb action" behavior and the program is still resumable. The "commands" command required a bit of special handling to unroll the changes if KbdInt happens. The patch was tested on Linux and Vista. |
|
|
| msg100220 - (view) |
Author: Gregory P. Smith (gregory.p.smith) * |
Date: 2010-02-28 20:14 |
| I put your sig.patch.v2 in http://codereview.appspot.com/216067/show and added some comments there. (and attempted to have it CC the tracker so they show up here but its been a while since i've done that, not sure how to make that work) |
|
|
| msg100221 - (view) |
Author: Gregory P. Smith (gregory.p.smith) * |
Date: 2010-02-28 20:23 |
| Reviewers: gregory.p.smith, Benjamin, ilya.sandler, Message: Also, can you take a look at how the pdb unittests work and see if you can come up with a way to unittest the KeyboardInterrupt behavior? Particular for the 4 scenarios you outlined in your prior comments in the bug (those all make sense to me). http://codereview.appspot.com/216067/diff/2001/2002 File Lib/pdb.py (right): http://codereview.appspot.com/216067/diff/2001/2002#newcode63 Lib/pdb.py:63: def sigint_handler(self, signum, frame): Please move this below the __init__ definition. It makes classes odd to read when __init__ isn't the first method defined when people are looking for the constructor to see how to use it. http://codereview.appspot.com/216067/diff/2001/2002#newcode64 Lib/pdb.py:64: if self.allow_kbdint: Initialize self.allow_kdbint in __init__ so that a SIGINT coming in before _cmdloop has run doesn't cause an AttributeError. http://codereview.appspot.com/216067/diff/2001/2002#newcode215 Lib/pdb.py:215: # keyboard interrupts allow for an easy way to interrupt "to cancel the current command" http://codereview.appspot.com/216067/diff/2001/2002#newcode356 Lib/pdb.py:356: #it appears that that when pdb is reading input from a pipe Space after the # please. Also, could you add a comment in here describing what the effect of this code is? It looks like you catch KeyboardInterrupt here and remove the particular interrupted command from the list of commands to run at the current breakpoint but I may be misreading things as I haven't spent much time in pdb.py. Please review this at http://codereview.appspot.com/216067/show Affected files: M Lib/pdb.py Index: Lib/pdb.py =================================================================== --- Lib/pdb.py (revision 78520) +++ Lib/pdb.py (working copy) @@ -13,8 +13,10 @@ import re import pprint import traceback +import signal + class Restart(Exception): """Causes a debugger to be restarted for the debugged python program.""" pass @@ -58,6 +60,13 @@ class Pdb(bdb.Bdb, cmd.Cmd): + def sigint_handler(self, signum, frame): + if self.allow_kbdint: + raise KeyboardInterrupt() + print >>self.stdout, "\nProgram interrupted. (Use 'cont' to resume)." + self.set_step() + self.set_trace(frame) + def __init__(self, completekey='tab', stdin=None, stdout=None, skip=None): bdb.Bdb.__init__(self, skip=skip) cmd.Cmd.__init__(self, completekey, stdin, stdout) @@ -72,6 +81,7 @@ import readline except ImportError: pass + signal.signal(signal.SIGINT, self.sigint_handler) # Read $HOME/.pdbrc and ./.pdbrc self.rcLines = [] @@ -176,7 +186,7 @@ if not self.commands_silent[currentbp]: self.print_stack_entry(self.stack[self.curindex]) if self.commands_doprompt[currentbp]: - self.cmdloop() + self._cmdloop() self.forget() return return 1 @@ -199,11 +209,22 @@ self.interaction(frame, exc_traceback) # General interaction function + def _cmdloop(self): + while 1: + try: + # keyboard interrupts allow for an easy way to interrupt + # cancel current command + self.allow_kbdint = True + self.cmdloop() + self.allow_kbdint = False + break + except KeyboardInterrupt: + print >>self.stdout, '--KeyboardInterrupt--' def interaction(self, frame, traceback): self.setup(frame, traceback) self.print_stack_entry(self.stack[self.curindex]) - self.cmdloop() + self._cmdloop() self.forget() def displayhook(self, obj): @@ -329,9 +350,18 @@ prompt_back = self.prompt self.prompt = '(com) ' self.commands_defining = True - self.cmdloop() - self.commands_defining = False - self.prompt = prompt_back + try: + self.cmdloop() + except (KeyboardInterrupt, IOError): + #it appears that that when pdb is reading input from a pipe + # we may get IOErrors, rather than KeyboardInterrupt + self.commands.pop(bnum) # remove this cmd list + self.commands_doprompt.pop(bnum) + self.commands_silent.pop(bnum) + raise KeyboardInterrupt() + finally: + self.commands_defining = False + self.prompt = prompt_back def do_break(self, arg, temporary = 0): # break [ ([filename:]lineno | function) [, "condition"] ] |
|
|
| msg100542 - (view) |
Author: Ilya Sandler (isandler) |
Date: 2010-03-06 19:20 |
| Another version of the patch is attached ( I think, I fixed all the remaining style issues). I'll answer the testing question in a separate post |
|
|
| msg100543 - (view) |
Author: Ilya Sandler (isandler) |
Date: 2010-03-06 19:27 |
| new version of the patch is uploaded to bugs.python.org http://codereview.appspot.com/216067/diff/2001/2002 File Lib/pdb.py (right): http://codereview.appspot.com/216067/diff/2001/2002#newcode63 Lib/pdb.py:63: def sigint_handler(self, signum, frame): On 2010/02/28 20:12:00, gregory.p.smith wrote: > Please move this below the __init__ definition. It makes classes odd to read > when __init__ isn't the first method defined when people are looking for the > constructor to see how to use it. Done. http://codereview.appspot.com/216067/diff/2001/2002#newcode64 Lib/pdb.py:64: if self.allow_kbdint: On 2010/02/28 20:12:00, gregory.p.smith wrote: > Initialize self.allow_kdbint in __init__ so that a SIGINT coming in before > _cmdloop has run doesn't cause an AttributeError. Done. http://codereview.appspot.com/216067/diff/2001/2002#newcode215 Lib/pdb.py:215: # keyboard interrupts allow for an easy way to interrupt On 2010/02/28 20:12:00, gregory.p.smith wrote: > "to cancel the current command" I changed the wording a bit, should be ok now. http://codereview.appspot.com/216067/diff/2001/2002#newcode356 Lib/pdb.py:356: #it appears that that when pdb is reading input from a pipe On 2010/02/28 20:12:00, gregory.p.smith wrote: > Space after the # please. this code > is? particular > interrupted command from the list of commands to run at the current breakpoint > but I may be misreading things as I haven't spent much time in pdb.py. Done. I've also added a comment to explain what's going on. http://codereview.appspot.com/216067/show |
|
|
| msg100555 - (view) |
Author: Ilya Sandler (isandler) |
Date: 2010-03-07 00:10 |
| > Also, can you take a look at how the pdb unittests work and see if you can come up with a way to unittest the KeyboardInterrupt behavior? Yes, this is doable. In fact, I already have such tests written (unix only though). The tests are assert based but it should not be difficult to convert them to unittest. Moreover, I have many more tests for pdb written in the same style. However, these tests are not easily (and possibly not at all) integratable with existing test_pdb.py module. (See below for the problems). So would it be acceptable to have these tests as a separate module (test_pdb2.py or some such)? (I might also need some help with integrating the module) Background ---------- Here is the basic problem: testing/debugging a debugger is hard by itself (you need to deal with 2 programs at once: the one being debugged and the debugger which run intermittently), now throw in doctest and it becomes much harder (as everything you do now gets covered by another layer). And now we need to test signals and some of the tests will likely be platform specific which makes it even worse. In contrast, in my tests the snippets of code are written into a tmp file and then are fed into debugger, the debugger itself is run as a subprocess. So if a test fails, it can be easily rerun under debugger manually and you can test for things like pdb exits and stalls. Here is a snippet from my pdb tests (just to give an idea how they would look like: essentially, send a command to pdb, check the response). pdb=PdbTester("pdb_t_hello","""\ import time for i in xrange(100000000): time.sleep(0.05) """) pdb.pdbHandle.stdin.write("c\n") time.sleep(0.01) #pdb_t_hello running, try to interrupt it pdb.pdbHandle.send_signal(signal.SIGINT) pdb.waitForPrompt() pdb.queryAndMatch("p i", "0") pdb.query("n") pdb.query("n") pdb.queryAndMatch("p i", "1") pdb.query("s") pdb.query("s") pdb.queryAndMatch("p i","2") pdb.pdbHandle.stdin.write("c\n") time.sleep(0.03) pdb.pdbHandle.send_signal(signal.SIGINT) pdb.waitForPrompt() pdb.queryAndMatch("p i","3") |
|
|
| msg101482 - (view) |
Author: Ilya Sandler (isandler) |
Date: 2010-03-22 05:56 |
| I'm attaching a test for Ctrl-C behavior on Linux (the patch itself works on Windows too, but I am not sure how to send Ctrl-C on windows programatically and subprocess does not have this functionality either). The test_pdb2.py module is generic and can be extended to test other functionality. But, as discussed earlier, it cannot be easily (if at all) integrated into existing test_pdb.py. Note that the test module will NOT work with existing pdb (as it doesnot have expected Ctrl-C) behavior, but on can specify alternative pdb location from command line: env DEBUG_PDB=./pdb.py ./test_pdb2.py |
|
|
| msg101971 - (view) |
Author: Ilya Sandler (isandler) |
Date: 2010-03-31 02:27 |
| Is there anything else I can do for this patch? |
|
|
| msg104739 - (view) |
Author: Ilya Sandler (isandler) |
Date: 2010-05-01 19:10 |
| a note on testing: it should be possible to integrate the tests into existing test_pdb.py by simply placing subprocess based tests next to doctest-based tests. This way pdb tests will at least be in a single module. (this is an approach taken by a patch in http://bugs.python.org/issue7964) Would it be better? |
|
|
| msg105348 - (view) |
Author: Gregory P. Smith (gregory.p.smith) * |
Date: 2010-05-08 23:39 |
| Committed to trunk in r81012. though as this missed 2.7beta2 its possible that will be rejected and this becomes a 3.x only feature. I'm porting to py3k now. |
|
|
| msg105360 - (view) |
Author: Gregory P. Smith (gregory.p.smith) * |
Date: 2010-05-09 01:21 |
| And reverted in trunk r81013. Multiple buildbot problems from the initial commit due to the unittest. This is likely to be py3k only at this point. I do believe sig.patch.v3 is fine, but its the test_pdb2 unittest that is difficult to make work well. |
|
|
| msg106031 - (view) |
Author: Ilya Sandler (isandler) |
Date: 2010-05-19 03:48 |
| I tried to understand the cause of failures and I don't see how test_pdb2 could have caused them ;-). Here is the relevant part of buildbot timeline: http://www.python.org/dev/buildbot/trunk/?last_time=1273368351&show_time=7400 The only test failures which have log files were sparc solaris 10 and ia64 and both failures were in test_unittest. All other buildbot failures don't have test logs associated with them.. Is there a way to access test logs for other platforms? |
|
|
| msg112107 - (view) |
Author: Georg Brandl (georg.brandl) * |
Date: 2010-07-30 22:21 |
| I committed the "commands" part of the patch in r83308. For the SIGINT part, I'm concerned that the SIGINT handler is set in the Pdb constructor, since it's a processwide setting I think it should a) be set only before sys.settrace() and b) be restored appropriately when debugging is finished. |
|
|
| msg112108 - (view) |
Author: Georg Brandl (georg.brandl) * |
Date: 2010-07-30 22:30 |
| Adding the rest of the patch adapted for py3k trunk as of today. |
|
|
| msg122757 - (view) |
Author: Ilya Sandler (isandler) |
Date: 2010-11-29 01:48 |
| The patch tries to write to stdout in signal handler. This currently does not work in the python 3.x (see http://bugs.python.org/issue10478). |
|
|
| msg123368 - (view) |
Author: Georg Brandl (georg.brandl) * |
Date: 2010-12-04 16:15 |
| Committed rest of patch in r87045. Will try to get test_pdb2 in shape and working on all platforms. |
|
|