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

[Pavel Labath]

================
Comment at: source/Plugins/Process/Windows/DebuggerThread.cpp:229
@@ -231,1 +228,3 @@
+        // Force a fresh break so that the detach can happen from the debugger thread.
+        if (!::DebugBreakProcess(GetProcess().GetNativeProcess().GetSystemHandle()))
         {
----------------
amccarth wrote:
> labath wrote:
> > If I understand correctly, you have let the process run freely a couple of lines back.(?)
> > What will happen if the process stops again (another breakpoint, watchpoint, access violation, ..) before you get a chance to stop it here? Can such a thing happen?
> I have thought of that, but I'm not sure how to solve it.
> 
> What happens depends on whether m_pid_to_detach is set before or after the inferior hits another exception.
> 
> If m_pid_to_detach is already set, and the inferior hits a breakpoint, we will detach.
> 
> If it's not yet set, or if the inferior stops for another reason (like an access violation, since watchpoints aren't implemented on Windows yet), then the process will stop, then we'll finish the detach, leaving a non-running process on the system.  I managed to make this happen once, at which point I was able to re-attach to the process.
> 
> I could reduce the risk of the latter situation by setting the m_pid_to_detach before the continue (just as the terminate case does), but I even then, I don't think I can guarantee that the next stop will be from our DebugBreakProcess call.
> 
> What I'd like to do is create more tests for all these cases, which I assume may involve solving the same types of problems for at least some of the other platforms.  But my first goal is to get TestAttachResume and TestBreakAfterAttach working, and this gets us much closer (there are follow-up patches for making those tests cross platform and for eliminating more POSIX dependencies from lldbtest.py).
Yeah, this is tricky. On linux, we are detaching from a stopped state, which makes things easier, since we don't have to worry about the inferior doing anything funny while we are trying to detach (or do any other operation for that matter).

The safest way for this that I can see is to do the `ContinueAsyncException` call and the `DebugActiveProcessStop` on the same thread. So we could have the debug thread listen for two things (if such a thing is possible): 
- regular debug events as it is doing now
- other events via some other communication channel: using this channel we can signal it to execute the detach
Since these things will then happen on the same thread, we can be sure that the processing some debug event does not interfere with our detach efforts. Does that make any sense?

>What I'd like to do is create more tests for all these cases, which I assume may involve solving the same types of problems for at least some of the other platforms. But my first goal is to get TestAttachResume and TestBreakAfterAttach working, and this gets us much closer (there are follow-up patches for making those tests cross platform and for eliminating more POSIX dependencies from lldbtest.py).
Yes please, the more tests the better. I'm not saying that the other platforms are perfect. And just because your test exposes a bug in some platform, it doesn't mean you have to fix it.

If you want to commit this as a working concept now, I'm fine with that, but I think we should find a bullet-proof way to do this.

================
Comment at: source/Plugins/Process/Windows/ProcessWindows.cpp:1013
@@ -913,1 +1012,2 @@
 }
+
----------------
amccarth wrote:
> labath wrote:
> > This looks unnecessary.
> The last line '}' was unterminated, and my editor seems to insist on making sure it ends with a newline (which is a requirement of POSIX).
Ok, nevermind then.

http://reviews.llvm.org/D10404

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