This is an archive of the discontinued LLVM Phabricator instance.

Fix process attach and detach for Windows
ClosedPublic

Authored by amccarth on Jun 11 2015, 4:38 PM.

Details

Reviewers
vharron
labath
Summary

When attaching to a process, an extra step is needed to get the executable module loaded. This makes it possible to set breakpoints.

Detaching has to happen from the debugger thread, so there are some flags and a forced breakpoint exception to trigger that.

More work is needed on the tests (TestAttachResume.py, lldbtest.py, etc.) to get them to pass completely, but I'm separating that work from this patch.

Diff Detail

Event Timeline

amccarth updated this revision to Diff 27555.Jun 11 2015, 4:38 PM
amccarth retitled this revision from to Fix process attach and detach for Windows.
amccarth updated this object.
amccarth edited the test plan for this revision. (Show Details)
amccarth added reviewers: zturner, chaoren.
amccarth added a subscriber: Unknown Object (MLST).

Does anyone have time to take a look at this?

chaoren edited edge metadata.Jun 16 2015, 11:38 AM

Sorry, I'm OOO right now.
Does anyone have time to take a look at this?

http://reviews.llvm.org/D10404

EMAIL PREFERENCES

http://reviews.llvm.org/settings/panel/emailpreferences/
labath added a subscriber: labath.Jun 16 2015, 3:17 PM

Hi, I have a couple of questions about the thread synchronization here. Take them with a grain of salt though, as I am looking at this code for the first time.

source/Plugins/Process/Windows/DebuggerThread.cpp
374

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?

source/Plugins/Process/Windows/ProcessWindows.cpp
474

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.

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

source/Plugins/Process/Windows/DebuggerThread.cpp
374

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.

source/Plugins/Process/Windows/ProcessWindows.cpp
474

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!

labath added inline comments.Jun 17 2015, 11:34 AM
source/Plugins/Process/Windows/DebuggerThread.cpp
374

Yes, I believe an atomic variable would suffice in this case.

source/Plugins/Process/Windows/ProcessWindows.cpp
474

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.

I know very little about ProcessWindows as well, so here's some bikeshedding.

source/Plugins/Process/Windows/ProcessWindows.cpp
71
&file_name[0]

I think this is undefined behavior (might be different in recent C++ standards). Please use a std::vector<char> and std::vector::data().

775

Is there a way to check if we're actually preparing to attach? There are probably other things we can check too (e.g., if any module exists).

813

Shouldn't resolve = true?

Thanks for the feedback. I'm still sorting through the potential data races Pavel caught, so I'll have a new diff up soon.

source/Plugins/Process/Windows/ProcessWindows.cpp
71

It's defined behavior in C++11, but I'll switch to a std::vector if you insist.

The data() member functions of std::vector and std::string return const pointers, so they're not useful for this purpose.

775

I'm not exactly sure what you're asking. It's not entirely clear to me what CanDebug is supposed to do. This ProcessWindows implementation of it looks like it was cloned from ProcessLinux and ProcessPOSIX, which do no further checking. Process::CanDebug doesn't have a comment explaining what it should do.

GetExecutableModule returns the first module if it can't find an actual executable module. Thus, if it fails, we already know that there are no modules.

813

file_name is already a fully-resolved, absolute path (because that's what GetProcessExecutableName returns). Is there something more that FileSpec's resolve_path will do?

chaoren added inline comments.Jun 18 2015, 10:44 AM
source/Plugins/Process/Windows/ProcessWindows.cpp
71

[[ http://en.cppreference.com/w/cpp/container/vector/data | std::vector::data ]] can also be non-const.

775

Yeah, it does look like it's copied from ProcessPOSIX. Please ignore me then.

813

It marks FileSpec::m_is_resolved to true, though I'm not sure if it's actually necessary for anything.

Thanks all. New patch coming momentarily.

source/Plugins/Process/Windows/DebuggerThread.cpp
374

Done.

source/Plugins/Process/Windows/ProcessWindows.cpp
71

Done.

Note, that std::vector::data is new with C++11, which is the same version of the spec that guarantees that std::string would work as I was using it.

474

Ah, I see now that StopDebugging already had a stub for Detach. I've reworked the solution to use that, which handles resuming from an existing breakpoint and waits for the debugger thread to finish (which avoids prematurely setting the process state to detached).

775

Acknowledged.

813

Done. Setting it seems to have no negative effects.

amccarth updated this revision to Diff 27969.Jun 18 2015, 3:48 PM
amccarth edited edge metadata.

Handles the data races and synchronization problem with setting the process state to detached.

Moved setting of the debugging-ended event to the end of the debug loop, since there are now multiple callers of the debug loop.

Other small improvements to address comments.

Can now attach, set a breakpoint, continue, and detach on Windows. You can also detach without continuing.

Looks better, but I have one more hypothetical question.

source/Plugins/Process/Windows/DebuggerThread.cpp
241

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?

source/Plugins/Process/Windows/ProcessWindows.cpp
992

This looks unnecessary.

amccarth added inline comments.Jun 18 2015, 5:02 PM
source/Plugins/Process/Windows/DebuggerThread.cpp
241

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

source/Plugins/Process/Windows/ProcessWindows.cpp
992

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

labath added inline comments.Jun 18 2015, 5:53 PM
source/Plugins/Process/Windows/DebuggerThread.cpp
241

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.

source/Plugins/Process/Windows/ProcessWindows.cpp
992

Ok, nevermind then.

chaoren edited reviewers, added: labath; removed: chaoren.Jul 1 2015, 4:01 PM
chaoren added a subscriber: chaoren.
labath accepted this revision.Jul 2 2015, 6:00 AM
labath edited edge metadata.
This revision is now accepted and ready to land.Jul 2 2015, 6:00 AM
labath added a comment.Jul 8 2015, 1:54 AM

Is this still active?