Fix process attach and detach for Windows

[amccarth]

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.

[amccarth]

Does anyone have time to take a look at this?

[chaoren]

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]

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.

[amccarth]

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]

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.

[chaoren]

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?

[amccarth]

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]

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.

[amccarth]

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]

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.

[labath]

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]

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]

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.

[labath]

Is this still active?

[amccarth]

http://reviews.llvm.org/rL240157

source/Plugins/Process/Windows/DebuggerThread.h

 class DebuggerThread : public std::enable_shared_from_this<DebuggerThread>
 {
   public:
     DebuggerThread(DebugDelegateSP debug_delegate);
     virtual ~DebuggerThread();
 
     Error DebugLaunch(const ProcessLaunchInfo &launch_info);
     Error DebugAttach(lldb::pid_t pid, const ProcessAttachInfo &attach_info);
+    void DebugDetach(lldb::pid_t pid);
 
     HostProcess
     GetProcess() const
     {
         return m_process;
     }
     HostThread
     GetMainThread() const
private:
 
     Predicate<ExceptionResult> m_exception_pred; // A predicate which gets signalled when an exception
                                                  // is finished processing and the debug loop can be
                                                  // continued.
 
     HANDLE m_debugging_ended_event; // An event which gets signalled by the debugger thread when it
                                     // exits the debugger loop and is detached from the inferior.
 
+    DWORD m_pid_to_detach;  // Signals the loop to detach from the process (specified by pid).
+    bool m_detached;  // Indicates we've detached from the inferior process and the debug loop can exit.
+
     static lldb::thread_result_t DebuggerThreadLaunchRoutine(void *data);
     lldb::thread_result_t DebuggerThreadLaunchRoutine(const ProcessLaunchInfo &launch_info);
     static lldb::thread_result_t DebuggerThreadAttachRoutine(void *data);
     lldb::thread_result_t DebuggerThreadAttachRoutine(lldb::pid_t pid, const ProcessAttachInfo &launch_info);
 };
 }
 
 #endif

source/Plugins/Process/Windows/DebuggerThread.cpp

struct DebugAttachContext
     ProcessAttachInfo m_attach_info;
 };
 }
 
 DebuggerThread::DebuggerThread(DebugDelegateSP debug_delegate)
     : m_debug_delegate(debug_delegate)
     , m_image_file(nullptr)
     , m_debugging_ended_event(nullptr)
+    , m_pid_to_detach(0)
+    , m_detached(false)
 {
     m_debugging_ended_event = ::CreateEvent(nullptr, TRUE, FALSE, nullptr);
 }
 
 DebuggerThread::~DebuggerThread()
 {
     ::CloseHandle(m_debugging_ended_event);
 }
DebuggerThread::DebugAttach(lldb::pid_t pid, const ProcessAttachInfo &attach_info)
     {
         WINERR_IFALL(WINDOWS_LOG_PROCESS, "DebugAttach couldn't attach to process '%u'.  %s", (DWORD)pid,
                      error.AsCString());
     }
 
     return error;
 }
 
+void
+DebuggerThread::DebugDetach(lldb::pid_t pid)
+{
+    // Detaching must be done on the same thread as the debugger loop, so this
+    // just sets a flag to detach on the next breakpoint exception event.
+    m_pid_to_detach = pid;
+}
+
 lldb::thread_result_t
 DebuggerThread::DebuggerThreadLaunchRoutine(void *data)
 {
     DebugLaunchContext *context = static_cast<DebugLaunchContext *>(data);
     lldb::thread_result_t result = context->m_thread->DebuggerThreadLaunchRoutine(context->m_launch_info);
     delete context;
     return result;
 }
if (terminate)
         DWORD wait_result = WaitForSingleObject(m_debugging_ended_event, 5000);
         if (wait_result != WAIT_OBJECT_0)
         {
             error.SetError(GetLastError(), eErrorTypeWin32);
             WINERR_IFALL(WINDOWS_LOG_PROCESS, "StopDebugging WaitForSingleObject(0x%p, 5000) returned %u",
                          m_debugging_ended_event, wait_result);
         }
         else
         {
             WINLOG_IFALL(WINDOWS_LOG_PROCESS, "StopDebugging detach from process %u completed successfully.", pid);
         }
     }
     else
     {
         error.SetErrorString("Detach not yet supported on Windows.");
         // TODO: Implement detach.
     }
while (should_debug)
                         should_debug = false;
                     break;
             }
 
             WINLOGD_IFALL(WINDOWS_LOG_EVENT, "DebugLoop calling ContinueDebugEvent(%u, %u, %u) on thread %u.",
                           dbe.dwProcessId, dbe.dwThreadId, continue_status, ::GetCurrentThreadId());
 
             ::ContinueDebugEvent(dbe.dwProcessId, dbe.dwThreadId, continue_status);
+
+            if (m_detached)
+            {
+                should_debug = false;
+            }
         }
         else
         {
             WINERR_IFALL(WINDOWS_LOG_EVENT,
                 "DebugLoop returned FALSE from WaitForDebugEvent.  Error = %u",
                 ::GetCurrentThreadId(), ::GetLastError());
 
             should_debug = false;

 {
     bool first_chance = (info.dwFirstChance != 0);
 
     m_active_exception.reset(new ExceptionRecord(info.ExceptionRecord, thread_id));
     WINLOG_IFANY(WINDOWS_LOG_EVENT | WINDOWS_LOG_EXCEPTION,
                  "HandleExceptionEvent encountered %s chance exception 0x%x on thread 0x%x",
                  first_chance ? "first" : "second", info.ExceptionRecord.ExceptionCode, thread_id);
 
+    if (m_pid_to_detach != 0 && m_active_exception->GetExceptionCode() == EXCEPTION_BREAKPOINT) {
+        WINLOG_IFANY(WINDOWS_LOG_EVENT | WINDOWS_LOG_EXCEPTION | WINDOWS_LOG_PROCESS,
+                     "Breakpoint exception is cue to detach from process 0x%x",
+                     m_pid_to_detach);
+        if (::DebugActiveProcessStop(m_pid_to_detach)) {
+            m_detached = true;
+            return ExceptionResult::MaskException;
+        } else {
+            WINLOG_IFANY(WINDOWS_LOG_PROCESS, "Failed to detach, treating as a regular breakpoint");
+        }
+    }
+
     ExceptionResult result = m_debug_delegate->OnDebugException(first_chance,
                                                                 *m_active_exception);
     m_exception_pred.SetValue(result, eBroadcastNever);
 
     WINLOG_IFANY(WINDOWS_LOG_EVENT|WINDOWS_LOG_EXCEPTION,
         "DebuggerThread::HandleExceptionEvent waiting for ExceptionPred != BreakInDebugger");
 
     m_exception_pred.WaitForValueNotEqualTo(ExceptionResult::BreakInDebugger, result);

source/Plugins/Process/Windows/ProcessWindows.h

public:
     void DidLaunch() override;
     void DidAttach(lldb_private::ArchSpec &arch_spec) override;
 
     void RefreshStateAfterStop() override;
     lldb::addr_t GetImageInfoAddress() override;
 
     bool CanDebug(lldb_private::Target &target, bool plugin_specified_by_name) override;
     bool
-    DetachRequiresHalt() override
-    {
-        return true;
-    }
-    bool
     DestroyRequiresHalt() override
     {
         return false;
     }
     bool UpdateThreadList(lldb_private::ThreadList &old_thread_list, lldb_private::ThreadList &new_thread_list) override;
     bool IsAlive() override;
 
     size_t DoReadMemory(lldb::addr_t vm_addr, void *buf, size_t size, lldb_private::Error &error) override;

source/Plugins/Process/Windows/ProcessWindows.cpp

 //===-- ProcessWindows.cpp --------------------------------------*- C++ -*-===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 
 // Windows includes
 #include "lldb/Host/windows/windows.h"
+#include <psapi.h>
 
 // C++ Includes
 #include <list>
 #include <mutex>
 #include <set>
 #include <vector>
 
 // Other libraries and framework includes

 #include "llvm/Support/Format.h"
 #include "llvm/Support/raw_ostream.h"
 
 using namespace lldb;
 using namespace lldb_private;
 
 #define BOOL_STR(b) ((b) ? "true" : "false")
 
+namespace
+{
+
+std::string
+GetProcessExecutableName(HANDLE process_handle)
+{
+    std::string file_name;
+    DWORD file_name_size = MAX_PATH;  // first guess, not an absolute limit
+    DWORD copied = 0;
+    do
+    {
+        file_name_size *= 2;
+        file_name.resize(file_name_size);
+        copied = ::GetModuleFileNameEx(process_handle, NULL, &file_name[0], file_name_size);
+    } while (copied >= file_name_size);
+    file_name.resize(copied);
+    return file_name;
+}
+
+std::string
+GetProcessExecutableName(DWORD pid)
+{
+    std::string file_name;
+    HANDLE process_handle = ::OpenProcess(PROCESS_QUERY_INFORMATION | PROCESS_VM_READ, FALSE, pid);
+    if (process_handle != NULL)
+    {
+        file_name = GetProcessExecutableName(process_handle);
+        ::CloseHandle(process_handle);
+    }
+    return file_name;
+}
+
+}  // anonymous namespace
+
 namespace lldb_private
 {
 
 // We store a pointer to this class in the ProcessWindows, so that we don't expose Windows
 // OS specific types and implementation details from a public header file.
 class ProcessWindowsData
 {
   public:

 ProcessWindows::GetPluginVersion()
 {
     return 1;
 }
 
 Error
 ProcessWindows::DoDetach(bool keep_stopped)
 {
-    Error error;
-    return error;
+    llvm::sys::ScopedLock lock(m_mutex);
+
+    if (!m_session_data)
+    {
+        WINWARN_IFALL(WINDOWS_LOG_PROCESS,
+                      "WillDetach called while state = %u, but there is no active session.",
+                      GetPrivateState());
+        return Error();
+    }
+
+    // Tell the debugger thread that we want to detach.
+    m_session_data->m_debugger->DebugDetach(GetID());
+
+    // Force a break so that the detach can happen from the debugger thread.
+    if (!::DebugBreakProcess(m_session_data->m_debugger->GetProcess().GetNativeProcess().GetSystemHandle()))
+    {
+        return Error(::GetLastError(), eErrorTypeWin32);
+    }
+
+    SetPrivateState(eStateDetached);
+
+    return Error();
 }
 
 Error
 ProcessWindows::DoDestroy()
 {
     DebuggerThreadSP debugger_thread;
     StateType private_state;
     {

 {
     if (plugin_specified_by_name)
         return true;
 
     // For now we are just making sure the file exists for a given module
     ModuleSP exe_module_sp(target.GetExecutableModule());
     if (exe_module_sp.get())
         return exe_module_sp->GetFileSpec().Exists();
-    return false;
+    // However, if there is no executable module, we return true since we might be preparing to attach.
+    return true;
 }
 
 void
 ProcessWindows::OnExitProcess(uint32_t exit_code)
 {
     // No need to acquire the lock since m_session_data isn't accessed.
     WINLOG_IFALL(WINDOWS_LOG_PROCESS, "Process %u exited with code %u", GetID(), exit_code);
 
     ModuleSP executable_module = GetTarget().GetExecutableModule();
     ModuleList unloaded_modules;
     unloaded_modules.Append(executable_module);
     GetTarget().ModulesDidUnload(unloaded_modules, true);
 
     SetProcessExitStatus(nullptr, GetID(), true, 0, exit_code);
     SetPrivateState(eStateExited);
 }
 
 void
 ProcessWindows::OnDebuggerConnected(lldb::addr_t image_base)
 {
     DebuggerThreadSP debugger = m_session_data->m_debugger;
 
-    WINLOG_IFALL(WINDOWS_LOG_PROCESS, "Debugger established connected to process %I64u.  Image base = 0x%I64x",
+    WINLOG_IFALL(WINDOWS_LOG_PROCESS, "Debugger connected to process %I64u.  Image base = 0x%I64x",
                  debugger->GetProcess().GetProcessId(), image_base);
 
     ModuleSP module = GetTarget().GetExecutableModule();
+    if (!module)
+    {
+        // During attach, we won't have the executable module, so find it now.
+        const DWORD pid = debugger->GetProcess().GetProcessId();
+        const std::string file_name = GetProcessExecutableName(pid);
+        if (file_name.empty())
+        {
+            return;
+        }
+
+        FileSpec executable_file(file_name, false);
+        ModuleSpec module_spec(executable_file);
+        Error error;
+        module = GetTarget().GetSharedModule(module_spec, &error);
+        if (!module)
+        {
+            return;
+        }
+
+        GetTarget().SetExecutableModule(module, false);
+    }
+
     bool load_addr_changed;
     module->SetLoadAddress(GetTarget(), image_base, false, load_addr_changed);
 
     ModuleList loaded_modules;
     loaded_modules.Append(module);
     GetTarget().ModulesDidLoad(loaded_modules);
 
     // Add the main executable module to the list of pending module loads.  We can't call
else
         // method wakes up and returns a failure.
         m_session_data->m_launch_error = error;
         ::SetEvent(m_session_data->m_initial_stop_event);
         WINERR_IFALL(WINDOWS_LOG_PROCESS, "Error %u occurred launching the process before the initial stop.  %s",
                      error.GetError(), error.AsCString());
         return;
     }
 }
+