-
Notifications
You must be signed in to change notification settings - Fork 18.3k
Description
CL 109699 added support for debugger function call injection, but has an annoying limitation: it requires that the debugger resume the entire Go process after injecting the function call (and, to inject into a runnable but not running goroutine, it requires resuming the entire process even before injecting the call).
@heschik argued that this is a pretty bad experience. E.g., all the user wants to do is call String() to format something, and the entire process moves under them in the meantime. It's also different from what other debuggers do, which could surprise users.
This is tricky to solve. Simply resuming only the thread where the call was injected doesn't work because 1) it could easily lead to runtime-level deadlocks if any other thread is in the runtime, 2) the runtime could just switch to a different goroutine on that thread, and 3) if the GC kicks in it will try to cooperatively stop the other threads and deadlock.
I think solving this requires at least a little help from the runtime to pause all other user goroutines during the injected call. I'm not sure what exact form this should take, but I'm imagining the debugger could use call injection to first inject a runtime call to stop user goroutines, and then inject the real call.
However, even this gets tricky with non-cooperative safe points (e.g., the runtime would need to use the register maps to immediately preempt the other goroutines rather than waiting for them to reach a cooperative safe point) and with goroutines that are at unsafe points (particularly goroutines that are in the runtime). One possibility would be to have the debugger inject this "stop" call on every running thread. Using the call injection mechanism takes care of stopping at non-cooperative points, and would give the debugger the opportunity to step other goroutines past unsafe points and out of the runtime before injecting the stop. This puts some complexity into the debugger, but it should already have most of the core mechanisms necessary to do this (such as single stepping ability). Specifically, I'm picturing a protocol like:
- For each thread, attempt to inject a
runtime.debugStop
call. Let all threads resume. - These calls will notify the debugger when the goroutine is successfully stopped, or the debug call injection will fail.
- For injection that failed because the thread is in the runtime, unwind the stack and insert a breakpoint at the first return to user code. At that breakpoint attempt another
debugStop
. For injection that failed because the thread is at an unsafe point, single step the thread, attempting to injectdebugStop
at each step. - Let the remaining threads continue running. Repeat steps 2 through 4 until all threads are stopped.
This is basically a debugger-assisted non-cooperative stop-the-world. For Go 1.12, I plan to implement non-cooperative preemption directly in the runtime, which may move much of this logic into the runtime itself.
/cc @aarzilli
Activity
aarzilli commentedon May 28, 2018
That being the case, personally, I'd rather wait for 1.12's cycle for this. I think it's unlikely that we are going to finish implementing the debugger side of call injection before we go is deep in 1.12's cycle, even as it is now.
aarzilli commentedon May 28, 2018
cc @derekparker
derekparker commentedon May 29, 2018
Fair enough, but if we do end up making more movement on the implementation of call injection on our end reliably, I'd rather get this feature into users hands sooner rather than later, this is a particularly exciting one.
aclements commentedon May 29, 2018
Note that debugger function call injection is supported by the runtime now. It's just that the debugger has to let the whole process resume execution during the injected call. You probably have a better sense of the impact that has on the user experience than I do.
As a data point, GDB by default continues the entire process during things like call injection and stepping (though
set scheduler-locking
can turn that off on Linux at least).gopherbot commentedon Sep 11, 2018
Change https://golang.org/cl/134779 mentions this issue:
runtime: support disabling goroutine scheduling by class
runtime: support disabling goroutine scheduling by class
derekparker commentedon Feb 27, 2019
I wanted to ping for an update on this issue as @aarzilli @heschik @hyangah and I spoke about it today during our meeting.
It seems like some of the foundation is there to prevent the runtime from scheduling goroutines that aren't already in the running state. However if I understand correctly were still a bit short on this feature. We would still need cooperation from the runtime to "unschedule" any running goroutine aside from the one Delve wants to be allowed to execute correct? As I understand it, even if we inject
schedEnableUser(false)
into every thread, that thread will still be_Grunning
meaning it will still execute, correct? I assume the above CL prevents any non-running goroutine from being scheduled, but does not deschedule any running goroutine.If that is the case, then what is the proposed path forward in order to finish this feature? From the original description @aclements suggested injecting a call on each thread which would stop the goroutine. From the standpoint of Delve that makes sense as it would allow us to selectively inject this call onto any thread running a goroutine besides the one we wish to continue execution.
We would really like to get this feature completed as I think that having other side effects during a user-injected function call is less than ideal. Is there any bandwidth to get this done during the 1.13 cycle?
prattmic commentedon Sep 6, 2022
We have asynchronous preemption now, which makes things a bit simpler, but there are still several edge cases to consider.
A scheme similar to what @aclements originally proposed is a good start, but there are still a lot of holes here:
_Grunnable
(or_Gdebugstop
?) and then returns to the debugger._PCDATA_UnsafePoint
and continue / single step until the next safe point._Grunning|Gdebugrun
._Grunning|Gdebugrun
threads.prattmic commentedon Sep 6, 2022
I think a lot of this would be simplified if we could allow more skew. i.e., set a global
debugPreempt
and then periodically sendsigPreempt
to all threads until all goroutines stop. But this is much more asynchronous and would allow more code to run on goroutines that you want to freeze. I don't know if that would be an acceptable user experience in a debugger?aarzilli commentedon Sep 12, 2022
At the moment delve doesn't read any of the pcdata/funcdata structures of the runtime. This is nice because we don't have to worry about keeping up with changes made to them. If we decide that delve needs to start reading those we'll also have to figure out a way to at least detect that the format has changed and we need to update delve.
Probably not. Much of the motivation behind this feature is to be used in conjunction with either call injection or next/step/stepout, if we allow other goroutines to execute a lot of code before actually stopping it wouldn't be much different from the status quo.
However, if we allow delve to read
_PCDATA_UnsafePoint
we could do something like thedebugPreempt
with minimal skew:debugPreempt = true
, also "somehow" communicate to the runtime which goroutine(s) we want to keep runningThis should work fine with other STW in progress (according to my limited understanding of GC).
But what about windows? The asyncpreempt mechanism used on windows is incompatible with debuggers, I'm guessing there's no solution here.
prattmic commentedon Sep 12, 2022
FWIW, I recently added a test to detect these failures in x/debug, which does some pcdata/funcdata parsing. If we revamped x/debug to provide more public API, then perhaps delve could use that.
prattmic commentedon Sep 12, 2022
I am not familiar with debugging on Windows. Am I correct that this is because we use
SuspendThread
, which is what I assume debuggers use as well?If so, then in theory I think delve itself could implement
preempM
. Though that of course makes it very sensitive to runtime internals.aarzilli commentedon Sep 13, 2022
Yes, see: #36494
Also, I forgot to mention one thing. Because one possible use of this is in combination with call injection I think we should plan for it to play nice with it, but call injection invoves spawning a new goroutine to execute the call, which means that that goroutine should run, should this apply to all new goroutines, or no? weird.
make gardener-debug
as a skaffold-based debugging experience gardener/gardener#7755