[Lldb-commits] [PATCH] Implement a framework for debugging on Windows.

[Scott Graham]

All minor other than the WaitForDebugEvent setup.

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

================
Comment at: source/Plugins/Process/Windows/DebugMonitorMessageResults.h:44
@@ +43,3 @@
+  protected:
+    DebugMonitorMessageResult(const DebugMonitorMessage *message);
+
----------------
explicit?

================
Comment at: source/Plugins/Process/Windows/DebugMonitorMessages.h:56
@@ +55,3 @@
+  protected:
+    DebugMonitorMessage(MonitorMessageType message_type);
+
----------------
explicit?

================
Comment at: source/Plugins/Process/Windows/DebugProcessLauncher.h:32
@@ +31,3 @@
+  public:
+    DebugProcessLauncher(lldb::ProcessSP process);
+    virtual HostProcess LaunchProcess(const ProcessLaunchInfo &launch_info, Error &error);
----------------
explicit, if not removed?

================
Comment at: source/Plugins/Process/Windows/DebugProcessLauncher.h:36
@@ +35,3 @@
+  private:
+    lldb::ProcessSP m_process;
+};
----------------
This looks to be unused. Might call it m_parent_process if it's going to be used eventually.

================
Comment at: source/Plugins/Process/Windows/DebugStatusMonitorThread.cpp:30
@@ +29,3 @@
+    m_monitor_event = ::CreateEvent(NULL, FALSE, FALSE, NULL);
+    ::CreatePipe(&m_monitor_pipe_read, &m_monitor_pipe_write, NULL, 1024);
+}
----------------
Maybe check returns here.

================
Comment at: source/Plugins/Process/Windows/DebugStatusMonitorThread.cpp:30
@@ +29,3 @@
+    m_monitor_event = ::CreateEvent(NULL, FALSE, FALSE, NULL);
+    ::CreatePipe(&m_monitor_pipe_read, &m_monitor_pipe_write, NULL, 1024);
+}
----------------
scottmg wrote:
> Maybe check returns here.
Need to ::CloseHandle all these somewhere.

================
Comment at: source/Plugins/Process/Windows/DebugStatusMonitorThread.cpp:148
@@ +147,3 @@
+        // See if any new processes are ready for debug monitoring.
+        DWORD result = WaitForSingleObject(monitor_thread->m_monitor_event, 1000);
+        if (result != WAIT_OBJECT_0 && result != WAIT_TIMEOUT)
----------------
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.

================
Comment at: source/Plugins/Process/Windows/DebugStatusMonitorThread.cpp:162
@@ +161,3 @@
+        // Empty out the debug event queue before checking whether new processes are trying to be
+        // debugged.  Don't spend alot of time blocking on this, because we want LLDB to be
+        // responsive to launching new processes under a debugger.
----------------
"a lot" (http://hyperboleandahalf.blogspot.com/2010/04/alot-is-better-than-you-at-everything.html :p)

================
Comment at: source/Plugins/Process/Windows/DebugStatusMonitorThread.cpp:171
@@ +170,3 @@
+                MonitorData *monitor_data = iter->second;
+                if (debug_event.dwDebugEventCode == EXIT_PROCESS_DEBUG_EVENT)
+                {
----------------
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.

http://reviews.llvm.org/D6037