Page MenuHomePhabricator

Implement a framework for debugging on Windows.
ClosedPublic

Authored by zturner on Oct 30 2014, 1:19 PM.

Details

Summary

Note: All of this is Windows specific, and I'm kind of the only person working on this. So I'm not really sure who should review this. I'm adding Jim and Greg, but feel free to ignore if you don't care about this. Also adding a few Windows people from here, although if ultimately nobody cares enough to review this, since it is entirely Windows specific, I will just go in with it.

This implements ProcessWindows::DoLaunch() in such a way that a connection is created between LLDB and the Windows kernel that allows us to obtain notifications of interesting things that happen in an inferior process on Windows.

Put another way, this implements the "ptrace" part of debugging for Windows processes.

This is complicated by some technical restrictions surrounding how processes are monitored for debug events on Windows. Specifically:

  1. LLDB's generic process launching code expects that launching a process and monitoring a process can be treated as two distinct operations. It is possible to split this into two separate operations on Windows, but the code would be very cumbersome, and it is much simpler to allow the monitoring and launching to occur as one high level operation. This is largely because:
  2. In Windows, the core debug loop (i.e. the ptrace equivalent) must happen on the same thread that spawns the process, and this is a loop that ultimately blocks until the process dies. Therefore, we cannot spawn the inferior process on LLDB's main thread, which presents a complication for the design that LLDB currently uses which expects launching a process to be a synchronous operation, after which monitoring can begin.

The solution to these problems implemented here is for ProcessWindows to completely circumvent Host::LaunchProcess. When ProcessWindows is initialized at program startup, we create a single background thread. This background thread does two things:

a) Pump an application-specific message queue and wait for messages from the main thread that tell it to do things (e.g. launching processes, attaching to processes etc).
b) When we are actively debugging processes (as a result of messages received by step a described above), also pump Windows' message queue for events related to processes we're debugging.

We currently only understand one type of application specific message, which is that of launching a process, and we still do not correctly update LLDB's internal state so that it knows the process has been launched. But this should provide the framework for future operations to be implemented more easily.

Diff Detail

Repository
rL LLVM

Event Timeline

zturner updated this revision to Diff 15576.Oct 30 2014, 1:19 PM
zturner retitled this revision from to Implement a framework for debugging on Windows..
zturner updated this object.
zturner edited the test plan for this revision. (Show Details)
zturner added reviewers: clayborg, jingham, rnk, majnemer, scottmg.
zturner updated this object.
zturner added a subscriber: Unknown Object (MLST).
zturner updated this object.Oct 30 2014, 1:23 PM

It's also worth pointing out that all of the code here should be reusable
if / when Windows switches to llgs.

clayborg accepted this revision.Oct 30 2014, 1:56 PM
clayborg edited edge metadata.

Looks good.

This revision is now accepted and ready to land.Oct 30 2014, 1:56 PM
scottmg edited edge metadata.Oct 31 2014, 10:59 AM

All minor other than the WaitForDebugEvent setup.

Feel free to ignore me if I'm sounding nonsensical. :)

source/Plugins/Process/Windows/DebugMonitorMessageResults.h
44 ↗(On Diff #15576)

explicit?

source/Plugins/Process/Windows/DebugMonitorMessages.h
56 ↗(On Diff #15576)

explicit?

source/Plugins/Process/Windows/DebugProcessLauncher.h
32 ↗(On Diff #15576)

explicit, if not removed?

36 ↗(On Diff #15576)

This looks to be unused. Might call it m_parent_process if it's going to be used eventually.

source/Plugins/Process/Windows/DebugStatusMonitorThread.cpp
30 ↗(On Diff #15576)

Maybe check returns here.

30 ↗(On Diff #15576)

Need to ::CloseHandle all these somewhere.

148 ↗(On Diff #15576)

Does this mean it'll frequently block for 1s in the middle of debugging? Seems unfortunate.

There's only one DebugStatusMonitorThread instance, right? I feel like it shouldn't be the thread that does the spawning and WaitForDebugEvent. Instead, it should create subthreads that CreateProcess+WFDE, and send back to this thread, which can then to a WaitForMultipleObjects on events coming from the app, and from the child processes being debugged.

162 ↗(On Diff #15576)
171 ↗(On Diff #15576)

I know you're just getting started here, but might want to add RIP_EVENT to this so that the map is maintained in that case.

zturner added inline comments.Oct 31 2014, 11:12 AM
source/Plugins/Process/Windows/DebugStatusMonitorThread.cpp
148 ↗(On Diff #15576)

Yea I felt in the back of my mind that this would have to be revisited at some point, but I also wasn't super happy with the thought of creating 1 thread for each debugged process. I will think about it a little harder, but if I still can't figure out a way to get O(1) number of threads for all processes with true asynchronous multiplexing on both debug events and application message pump events, then the way you propose is probably better than what I have here.

zturner updated this revision to Diff 15730.Nov 3 2014, 1:56 PM
zturner edited edge metadata.

Refactored the code to spawn a single driver thread on startup, and then spawn slave threads for each debugee.

rnk edited edge metadata.Nov 3 2014, 3:12 PM

I went to take a look at this, and found that I don't have much useful feedback to give. So, go for it. :)

lgtm, I think this is much better than the first rev, though a bit more complicated.

source/Plugins/Process/Windows/DebugOneProcessThread.cpp
180 ↗(On Diff #15730)

Sneaky! I was wondering how to get out of the WFDE loop.

zturner closed this revision.Nov 3 2014, 4:11 PM
zturner updated this revision to Diff 15740.

Closed by commit rL221207 (authored by @zturner).