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

[Adrian McCarthy]

On Thu, Jun 18, 2015 at 5:53 PM, Pavel Labath <labath at google.com> wrote:

> ================
> 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.


That's the hitch.  Although it's not documented, DebugActiveProcessStop
appears to work only if it's issued from the debugger thread that initially
attached with DebugActiveProcess.  That's why we set the flag, and then
call DebugBreakProcess:  It's the only way we came up with to signal the
debugger thread to detach.  I don't see a way to make
ContinueAsyncException  happen on that 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.
>

I agree that I should work on making it better.  The fact is that attach
was only sort-of working and detach was missing altogether, both of which
were blocking progress on the multithreaded tests.

So I'm going to commit this as a working concept, and I'll open a bug
against myself the find a way to make it bullet-proof.


>
> ================
> 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/
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/lldb-commits/attachments/20150619/c9e18b0d/attachment.html>