[Lldb-commits] [PATCH] Fix process attach and detach for Windows

[Adrian McCarthy]

Thanks for the feedback.  I'll work on those issues.


================
Comment at: source/Plugins/Process/Windows/DebuggerThread.cpp:374
@@ -358,1 +373,3 @@
 
+    if (m_pid_to_detach != 0 && m_active_exception->GetExceptionCode() == EXCEPTION_BREAKPOINT) {
+        WINLOG_IFANY(WINDOWS_LOG_EVENT | WINDOWS_LOG_EXCEPTION | WINDOWS_LOG_PROCESS,
----------------
labath wrote:
> This is almost a nit, since it will generally work, but technically, this looks like a data race ( => UDB) on the `m_pid_to_detach` variable: You are accessing the variable here and in DebugDetach() and one of those accesses is write. Is there some synchronization here that I am not aware of which is preventing these accesses to occur simultaneously?
I see your point.  DebugDetach, which sets the pid, is called only from ProcessWindows under its lock, and then it triggers a breakpoint exception, so in the normal case it does work.  But if a breakpoint is hit as we're setting the pid, there is indeed a data race.  Nice catch.

I suppose I could make m_pid_to_detach a std::atomic.

================
Comment at: source/Plugins/Process/Windows/ProcessWindows.cpp:474
@@ +473,3 @@
+
+    SetPrivateState(eStateDetached);
+
----------------
labath wrote:
> I don't know enough about ProcessWindows, but a thought comes to mind here:
> You set the process state to be eStateDetached, but that is not exactly true (yet). You have merely requested detachment, which is a process that can take a while (especially if the inferior is in the middle of processing some other breakpoint). It seems likely that this activity (processing a breakpoint hit) will race with whatever happens after DoDetach() returns. I would expect some kind of synchronization here that waits for a signal from the debugger thread that the detachment was successful.
> 
> As I said, I'm not an expert, but I was wondering if you have considered this scenario.
I'm pretty new to this as well.  No, I hadn't considered the scenario where we're already stopped at a breakpoint.  I was trying to get TestAttachResume to pass, which detaches while the inferior is still running.

I'll try to work through the implications of that and update the patch as needed.  Thanks!

http://reviews.llvm.org/D10404

EMAIL PREFERENCES
  http://reviews.llvm.org/settings/panel/emailpreferences/