-
-
Notifications
You must be signed in to change notification settings - Fork 8.2k
Control-C isn't handled in all cases #1722
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
Comments
So what's being reported here? Segfault on Ctrl+C? High-priority, need more info, there should be links to existing reports (we have 2). One exception thrown instead of another? Lower priority. We discussed similar issue recently, should be linked. Generally, original reporters should be motivated to submit proper bugreport, and do it well - provide info, describe issue clearly. |
I didn't hit a segfault. What happened for me was this:
i.e. the second time I run sys.stdin.read(1) and hit Control-C micropython exited entirely. I think that this happens because the mp_pending_exception never got cleared due to the OSError exception being raised instead. This was confirmed by doing something else after the first read:
So when it entered the for loop, it finally n 8000 oticed that mp_pending_exception was set and then finally raised the KeyboardInterrupt. So in the second example, I only pressed Control-C once, during the sys.stdin.read(1). |
In addition to Daves cases I also get the following, by pressing Return right after the first Ctrl-C, seemingly random:
|
The reaction to Ctrl-C during read is different, depending on the environment and the version used. I got:
Running micropython with gdb on 32 bit Mint linux results in: Program received signal SIGINT, Interrupt. with the followin stack trace: (gdb) bt |
ctd.: Running under gdb seems more stable. If I enter the gdb command 'c' after getting the gdb prompt follwing Ctrl-C, micropython continues normally. So it might be that by default the signal handler is not set properly, or the default signal handler is at fault.. |
Checking for pending exceptions was supposed to be fixed by 5ae3ddc, but that patch only checks if there were no exceptions raised (whereas the issue at hand has one exception being raised, while also the pending exception being set). Well, it certainly needs a proper solution. |
@robert-hh : Thanks for details. Please rebuild on your 32-bit system using "make DEBUG=1 -B", and run that under gdb, and post stacktrace for that. |
Let's concentrate here on resolving long-standing issue of segfault on Ctrl+C on 32-bit x86. Previously, I could not reproduce it under gdb. Regarding Ctrl+C handling in general, previous discussion on that happened in #1667, so should continue from there. |
Ctd # 2: I'll make the try with debug enabled, but meanwhile I made the following attemt, by providing a signal handler, which seemed to work:
|
OK. Here's the stack trace: sys.stdin.read(1) |
That will be the backtrace of where the Control-C was pressed. Try using continue and seeing if it segfaults. Under gdb, you can use the handle command before running to cause SIGINT to not be captured by gdb, which means you won't be able to break into the program, but it will still stop if it gets a segfault.
|
Indeed, @dhylands rights, and backtraces above do not show segfault, which happens when running without gdb. Here're previous bugreports of this issue: micropython/micropython-lib#24 , #1035 . So, if someone can help with triaging that and provide exact instructions how to reproduce segfault while running under gdb, that would be very helpful and appreciated. |
I was able to reproduce the segfault running a 32-bit version of the code, however the backtrace was less than useful. I built micropython using:
and when I ran it and hit Control-C I got:
which suggests to me that it may be screwing up in the signal handler (not our code, but the kernel code). I threw some prints into the signal handler and immediately before and after the read (which seems to support this hypothesis), and I get the print leaving the signal handler, but I don't get the print after the read. |
My build system for this was Ubuntu 14.04 LTS with gcc (Ubuntu 4.8.4-2ubuntu1~14.04) 4.8.4
and build using:
then the segfault goes away. If you want to make gcc 4.9 be the default, then you can do:
The last command allows you to switch between using 4.8 or 4.9 as the default. |
I mentioned above that after the SIGINT catched by GDB, the c command would continue Micropython normally. So at the moment I see two ways to avoid the issue: |
I tried to compile with gcc-4.9. That did not change the picture. I still get the Segmentation fault. Systen & gcc-version: |
I discovered that when you pass in CC=gcc-4.8 (or CC=gcc-4.9) it ignores the MICROPY_FORCE_32BIT=1 and builds a 64-bit executable. So I wasn't testing with a 32-bit executable. I found I also had to install
and either switch to 4.9 as the default or use CC='gcc-4.9 -m32' to get a 32-bit executable, and indeed it still segfaults. And the backtrace under 4.9 looks more or less the same. And it leaves the signal handler and never gets to the line after the read. Which still suggests a problem outside of our code. |
Hi Dave, do you understand the difference between Ctrl-C, entered at the REPL prompt, which behaves stable, and Ctrl-C in the sys.stdin.read() call, which causes the segfault? Robert |
@robert-hh Yep - If you look at the traces I did above, you'll see that Control-C was being pressed during sys.stdin.read(1). My problem (with 4.9) was that I was building a 64-bit executable (which doesn't segfault). Now that I'm actually building a 32-bit executable, it still segfaults (you may have missed my last comment). |
@dhylands Yes, I understand the traces and I have seen your comment. I was just wondering why Ctrl-C is catched well in one situation (REPL Prompt, reading from ?) and causes a segfault in another, while reading from stdin. |
@dhylands : Thanks for testing. |
Guys, instead of "avoiding" (== finding workarounds), it's better to push and fix it. Because once the real fix is found, effort on workarounds turns out to be waste of time. |
(My previous comment assumes that you read linked pervious bugreports for this issue - seeing whole picture, it's clear that the issue is non-deterministic, so "solving" it by switching compiler is impossible, assuming otherwise means assuming that gcc 4.8 has bug in generating signal-related code for one of the most used platform, and that bug is not fixed in 4.8, but fixed in 4.9; that's about impossible.) |
Segfault is fixed in 3db2b23 . I was especially interested in fixing that, because for fixing 2nd part of the issue - raising KeyboardInterrupt. So, per my sly plan, cool way to do that is to raise it directly from signal handler. POSIX doesn't give indulgence to do it (actually, it doesn't approve of doing much less innocent things in signal handler). But it works well on Linux, per casual testing. But if it suddenly starts to segfault on somebody, we want to be sure, it's new segfault, not that boring old one. And of course, such solution needs to be tested on other supported platforms - macosx, freedos. (And then probably still #ifdefed). |
Refactored in 3c2b377 |
Added basic support for handling Ctrl+C under windows in 2195046 , needs testing. |
And I'll confirm that the segfault is gone with On Thu, Dec 17, 2015 at 3:17 PM, Paul Sokolovsky notifications@github.com
Dave Hylands |
Next step of solving this issue is posted as #1733 . It needs extensive testing on other systems than Linux. 2195046#commitcomment-15080573 gives quick overview of recommended testing process. |
Hi all, I did not know where the to-be-tested version is located, so I just took the Zip-File from https://github.com/micropython/micropython. Test environment:
2.3: Ctrl-C during a small python script, consisting of nested loops and (hopefully) not doing system calls in between.
Started from the REPL Prompt, interrupted with Ctrl-C. Prints a traceback as expected:
That could be repeated several times with identical result. Two unexpected (at least when the happened first) observations: |
Just as a side note: If I do that on Linux, by first interrupting sys.stdin.read(1) with Ctrl-C, and then entering a command at the REPL prompt, that's what I get.
That may be caused by the attempt to do something reasonable with Ctrl-C instead of just interrupting and go back to the baseline.
P.S.: I'm happy to repeat tests on Windows, but usually I use it only for Office tasks and stuff that runs on Windows only, and very often on Windows XP only. My preferred environment for Python is Linux. |
Thanks for being eager to test it, but nope, the above is not the right code to test - it doesn't contain #1733. What needs to be done is #1733 is applied on top of micropython master and that test. And it needs to be tested on MacOSX and FreeDOS, if you don't have access to those systems, there's nothing new to test (well, you can test it on Linux - it should work just like CPython3 modulo minor cosmetic diffs). If you test #1733, please post further comments directly to it. Thanks! |
Note that windows builds use windows_mphal.c so pfalcon's changes to unix_mphal.c like in #1733 do not affect this environment. Also support for mingw (the one from http://mingw.org/ which I assume is what you mean with mingw32?) for uPy was basically dropped in favour of mingw-w64, because the latter supports 64bit build and more importantly it is still actively developped (afaik mingw is pretty much dead since 2013). As such it's probably also recommended to use MSYS2 instead for identical reasons. |
Yes. Nor #1733 would do any good for Windows, per above. So, #1733 is ultimate solution for Linux. It's unknown how it affects MacOSX and FreeDOS, so that needs to be tested before #1733 is merged. After that, Windows and bare-metal ports are left. Issues with them will be resolved in less efficient manner (there's still option to have efficient solution for Cortex-M3+). |
Ok, so #1733 is merged. From high-level perspective, it represents "fast path" for handling KeyboardInterrupt for unix sub-ports which support longjmp'ing out of signal handler. As I mentioned before, there should be possibility to provide similar fast path for Cortex-M3/M4 - longjmp out of PENDSV interrupt handler. This should be cute hack for folks who love dealing with tricks of a particular architecture, so welcome. Otherwise, having fast paths (or ideas for fast path) for common cases, we need to provide general fallback solution which will work for everything else (at the expense of some loss of efficiency) and will solve all ctrl+c issues once and for all. In this regard, @stinos' #1738 isn't such solution, because it resolve it for Windows, but not baremetal ports. Instead, I think it makes sense to follow @dpgeorge's previous suggestion to check pending exception not just for VM jump instructions, bit also for "ret". And I think we should follow @dhylands idea too and provide such check as a standalone function, so long-running builtin functions could use it easily (VM still should use inlined version of course for efficiency). That's my plan, if anyone has criticism, please share it, otherwise I'll get to coding it when time permits. |
stmhal already does this: first ctrl-C will signal to VM, and if that doesn't get through then second ctrl-C will rejig the stack to jump to the inner-most nlr handler. Or did you mean something else?
Sounds good! |
As far as I can tell from WiPy and Pyboard, a running script is interrupted by Ctrl-C, as well as the REPL prompt. Only "System" calls like sys.stdin.read() do not receive the interrupt until the next char is entered. Hitting Ctr-C twice does not work in that case. |
It should on the pyboard. I just tested it and it does: MicroPython v1.5.1-168-g1c9210b on 2015-12-23; PYBLITEv1.0 with STM32F411RE
Type "help()" for more information.
>>> import sys
>>> sys.stdin.read(1)
[hit ctrl-c two times]
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
KeyboardInterrupt:
>>> |
Confirmed, my fault. The latest build does the magic. No change for WiPy. |
OK I need to implement that for the WiPy. |
PEP 475 is implemented in #5723, which essentially does the same thing suggested in the original comment here. |
Also, 5a91cd9 fixed the case of checking for pending exceptions right at the end of code execution. So I think this issue can be closed. |
This was raised over here: http://forum.micropython.org/viewtopic.php?p=7566#p7566
A very specific patch that addresses that particular issue (very narrowly) would be something like this (patching unix/file.c):
I think that every system call needs something like this in order to have Control-C throw a KeyboardInterrupt rather than some other error (OSError 4 in the above example).
The if check probably best belongs in a macro or function (to improve readability and/or to minimize code size increase), but I figured the issue probably warranted further discussion, so I just posted the patch above rather than creating a PR which I knew would need to be closed.
The text was updated successfully, but these errors were encountered: