-
Notifications
You must be signed in to change notification settings - Fork 371
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
Add Ctrl-C handling to the REPL #537
Conversation
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.
Thanks for your contribution! This seems like a great feature for the repl to have.
However, as implemented there is a performance penalty and some surprising behavior to people who are using luau as a library.
VM/src/lvmexecute.cpp
Outdated
#define VM_NEXT() goto*(SingleStep ? &&dispatch : kDispatchTable[LUAU_INSN_OP(*pc)]) | ||
|
||
#define VM_NEXT() \ | ||
if (sigint_received)\ |
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 check is too expensive to run after every opcode. The VM_INTERRUPT macro already checks the status in places where loops and calls can occur. Just wait for the PC to reach one of these existing checks.
VM/src/lvmexecute.cpp
Outdated
#define VM_NEXT() goto dispatch | ||
#define VM_NEXT() \ | ||
if (sigint_received)\ | ||
{ sigint_received = 0; } \ |
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.
See previous comment, but also wouldn't the behavior need to be the same as above for this to work as submitted?
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.
Definitely! Just forgot to duplicate my changes from the macro above.
VM/src/lvmexecute.cpp
Outdated
@@ -320,6 +332,8 @@ static void luau_execute(lua_State* L) | |||
base = L->base; | |||
k = cl->l.p->k; | |||
|
|||
signal(SIGINT, handle_sig); |
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.
Not every implementor is going to want this behavior. This should be setup inside the Repl.cpp. There the CLI could have a signal handler that reaches into its global lua_State and sets the status variable.
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.
Correction: it should be sufficient for this entire feature to be implemented in Repl; instead of setting a status variable (and checking it in VM_INTERRUPT), it should be sufficient to dynamically set the interrupt callback from Repl. Some care needs to be taken in the profiler support code (in Repl) so that the profile callback doesn't overwrite the interrupt callback, but otherwise the interrupt callback is a fully general mechanism that should be sufficient to perform termination here.
I believe this would also need some Windows-specific code as SIGINT isn't supported on Windows. |
I went ahead and reimplemented the functionality using the interrupt handler scheme as suggested by @zeux. Let me know how this looks 😃 I had to add a couple things to lapi due to a circular import problem I didn't quite understand...not sure if that was the correct approach. Haven't yet had a chance to look into reproducing this for Windows; maybe tomorrow evening! |
This still has core changes that are not necessary:
Additionally, the preferred mode of operation for interrupt handlers is to set them lazily (this can be done on any thread / signal), in this case instead of the flag what should probably happen is that the signal handler should set the interrupt callback that errors unconditionally, and the top-level should clear the interrupt handler after handling the error. |
CLI/Repl.cpp
Outdated
void ihandler(lua_State* L, int k) { | ||
if (sigint_received) { | ||
sigint_received = 0; | ||
std::runtime_error error("Execution interrupted"); |
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 should be luaL_error(L, "Execution interrupted");
(compatible with building Luau without use of exceptions)
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.
So, I tried replacing the exception with a call to luaL_error in my most recent commit. This works fine as a drop-in replacement in the release build, but results in the following error on Ctrl-C in the debug build:
VM/src/lapi.cpp(600): ASSERTION FAILED: L->top < L->ci->top
Illegal instruction
Is this behavior expected? or should I file an issue?
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 required lua_rawcheckstack(L, 1);
as inside the interrupt callback the LUA_MINSTACK guarantee is (intentionally) not upheld.
The most recent commit raises an error with luaL_error. I didn't understand the latter concern about resetting the sigint flag to break out of
I've addressed both of these concerns.
I removed my custom interrupt handler setter and use
This is also addressed in the most recent commit, although I had to keep track of a global lua_State for the repl because I'm not able to pass any context beyond the signal value to the signal handler. I'm not certain this is the best way to do this. |
Sorry this got stuck a little bit, I think this looks good now modulo some style issues, I'll clean this up a little and merge it next week. |
Sounds good, @zeux. Thanks! :) |
I noticed that you can't stop execution with a SIGINT when working interactively at the REPL (it does seem to work for loaded files) so I went ahead and tried adding it. Would be glad to know if there is a less invasive way of doing this!