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

[Pavel Labath]

================
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,
----------------
amccarth wrote:
> 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.
Yes, I believe an atomic variable would suffice in this case.

================
Comment at: source/Plugins/Process/Windows/ProcessWindows.cpp:474
@@ +473,3 @@
+
+    SetPrivateState(eStateDetached);
+
----------------
amccarth wrote:
> 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!
These things are tricky. Wait until you get around to TestConcurrentEvents. :)
We also need a separate debug thread on linux, and getting these synchronization things right took a lot of time, and I'm still not convinced we have everything covered.

http://reviews.llvm.org/D10404

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