Enable debugging of multithreaded programs on Windows.

[amccarth]

Summary

Rewrote test app to use std::thread instead of pthreads because Windows doesn't have pthreads. This involved changing it from a C application to a C++ application.

Added missing code in HandleCreateThreadEvent and HandleExitThreadEvent to actually track the threads.

Modified ExceptionRecord to keep track of the ID of the thread that caused the exception. This resolves the discrepancy between the PC and the breakpoint address in the exception record, which allows the code to find the breakpoint that was hit and thus recognizes that the thread needs stop info.

Changed ProcessWindowsData::m_exited_threads to be just a set of thread IDs, since there are no HostThread objects to map to for threads that are gone.

Fixed some of the logging in ProcessWindows (like the order of some parameters and more consistent use of hexadecimal for thread IDs).

Fixed crash in ProcessWindows::OnExitThread when terminating a multi-threaded inferior.

Plumbed through the exit code for a thread, but not actually using it yet.

[zturner]

Awesome! going to take a look in a little bit. Does this cause any tests to pass that previously failed? If so, how many?

[zturner]

Looks good, just some minor changes and this should be good to go.

source/Plugins/Process/Windows/ExceptionRecord.h

33

You can probably make this a DWORD. We need to use the lldb types whenever it's somethign that's exposed from the plugin through an API, or when it's in some code that otherwise needs to have a stable interface among different platforms. But for internal types we should be using the native OS types. lldb::tid_t and pid_t are both 64 bits I think, and process / thread ids are 32 bits on windows, so there's ugly casting going on at every call site if we use the lldb public types internally.

(Also feel free to fix up any instances of this where I didn't follow my own rule)

source/Plugins/Process/Windows/ProcessWindows.cpp

437

consider using lldbassert() here. In Debug mode it is equivalent to assert(), but in release mode it will print a stack trace and continue on instead of doing nothing. See Utility/LLDBAssert.h.

775

Generally prefer to invert conditionals and early-out to keep indentation low. So change this to

if (!m_session_data)
    return;

[amccarth]

It looks like this might have exposed a couple of latent test failures, so I'm looking at those now:

Two other tests are occasionally crashing because they're getting debug events for new threads after the session data has been cleaned up.

source/Plugins/Process/Windows/ExceptionRecord.h

33

I saw the the exception address was saved as an lldb::addr_t and figured that I should do the same in case we ever need to pass these values to platform-independent APIs. There's no casting caused by my case, since lldb::tid_t is large enough to handle the thread IDs from any platform, which I assumed was by design.

Using DWORD consistently internally requires including <windows.h> in more headers (like IDebugDelegate.h and LoadDebugDelegate.h), which I'm hesitant to do since the Windows headers aren't well-behaved.

source/Plugins/Process/Windows/ProcessWindows.cpp

437

I've actually removed the assert altogether (as was originally my intent). I put it there to assure myself that we were getting the correct stop thread (which we are now) and to convince myself that your TODO was unnecessary.

775

Done under protest.

Generally speaking[1], I'm not a fan of conditional returns in the middle of a function. And this can't be moved to the top of the function like a true guard clause, because it must be done under the lock.

It reverses the natural logic of checking a prerequisite before doing a condition. Checking prerequisites is a way to avoid excessive indentation without introducing exit points in the middle of a function.

[1]: There are appropriate times to return from the middle of a function, but I don't feel this is one of them.

[amccarth]

I'm not convinced that the intermittent test crash on Windows (in TestWatchpointSetErrorCases) is related to this change. I'd like to get this submitted and continue to debug the crash as a separate effort.

[zturner]

sounds good.

[amccarth]

This is just the Windows-specific portion of this patch.

My attempt to commit the entire patch was rolled back because converting the inferior for TestNumThreads.py from pthreads to std::thread somehow caused it to fail on Linux (but not Mac or Windows). I'm still investigating the root cause of that. In the mean time, I'd like to land the Windows-specific part of the changes.

[Eugene.Zelenko]

Committed in r237392.

source/Plugins/Process/Windows/DebuggerThread.cpp

DebuggerThread::DebugLoop()
     FreeProcessHandles();
 }
 
 ExceptionResult
 DebuggerThread::HandleExceptionEvent(const EXCEPTION_DEBUG_INFO &info, DWORD thread_id)
 {
     bool first_chance = (info.dwFirstChance != 0);
 
-    m_active_exception.reset(new ExceptionRecord(info.ExceptionRecord));
+    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 %u",
+                 "HandleExceptionEvent encountered %s chance exception 0x%x on thread 0x%x",
                  first_chance ? "first" : "second", info.ExceptionRecord.ExceptionCode, thread_id);
 
     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);
 
     WINLOG_IFANY(WINDOWS_LOG_EVENT|WINDOWS_LOG_EXCEPTION,
         "DebuggerThread::HandleExceptionEvent got ExceptionPred = %u",
          m_exception_pred.GetValue());
 
     return result;
 }
 
 DWORD
 DebuggerThread::HandleCreateThreadEvent(const CREATE_THREAD_DEBUG_INFO &info, DWORD thread_id)
 {
     WINLOG_IFANY(WINDOWS_LOG_EVENT|WINDOWS_LOG_THREAD,
-        "HandleCreateThreadEvent Thread %u spawned in process %I64u",
-        m_process.GetProcessId(), thread_id);
-
+        "HandleCreateThreadEvent Thread 0x%x spawned in process %I64u",
+        thread_id, m_process.GetProcessId());
+    HostThread thread(info.hThread);
+    thread.GetNativeThread().SetOwnsHandle(false);
+    m_debug_delegate->OnCreateThread(thread);
     return DBG_CONTINUE;
 }
 
 DWORD
 DebuggerThread::HandleCreateProcessEvent(const CREATE_PROCESS_DEBUG_INFO &info, DWORD thread_id)
 {
     uint32_t process_id = ::GetProcessId(info.hProcess);
 

 }
 
 DWORD
 DebuggerThread::HandleExitThreadEvent(const EXIT_THREAD_DEBUG_INFO &info, DWORD thread_id)
 {
     WINLOG_IFANY(WINDOWS_LOG_EVENT|WINDOWS_LOG_THREAD,
         "HandleExitThreadEvent Thread %u exited with code %u in process %I64u",
         thread_id, info.dwExitCode, m_process.GetProcessId());
-
+    m_debug_delegate->OnExitThread(thread_id, info.dwExitCode);
     return DBG_CONTINUE;
 }
 
 DWORD
 DebuggerThread::HandleExitProcessEvent(const EXIT_PROCESS_DEBUG_INFO &info, DWORD thread_id)
 {
     WINLOG_IFANY(WINDOWS_LOG_EVENT|WINDOWS_LOG_THREAD,
         "HandleExitProcessEvent process %I64u exited with code %u",
         m_process.GetProcessId(), info.dwExitCode);
 
-    FreeProcessHandles();
-
     m_debug_delegate->OnExitProcess(info.dwExitCode);
+
+    FreeProcessHandles();
     return DBG_CONTINUE;
 }
 
 DWORD
 DebuggerThread::HandleLoadDllEvent(const LOAD_DLL_DEBUG_INFO &info, DWORD thread_id)
 {
     if (info.hFile == nullptr)
     {

source/Plugins/Process/Windows/ExceptionRecord.h

 // ExceptionRecord
 //
 // ExceptionRecord defines an interface which allows implementors to receive
 // notification of events that happen in a debugged process.
 //----------------------------------------------------------------------
 class ExceptionRecord
 {
   public:
-    explicit ExceptionRecord(const EXCEPTION_RECORD &record)
+    ExceptionRecord(const EXCEPTION_RECORD &record, lldb::tid_t thread_id)
     {
         m_code = record.ExceptionCode;
         m_continuable = (record.ExceptionFlags == 0);
         if (record.ExceptionRecord)
-            m_next_exception.reset(new ExceptionRecord(*record.ExceptionRecord));
+            m_next_exception.reset(new ExceptionRecord(*record.ExceptionRecord, thread_id));
         m_exception_addr = reinterpret_cast<lldb::addr_t>(record.ExceptionAddress);
+        m_thread_id = thread_id;
         m_arguments.assign(record.ExceptionInformation, record.ExceptionInformation + record.NumberParameters);
     }
     virtual ~ExceptionRecord() {}
 
     DWORD
     GetExceptionCode() const
     {
         return m_code;
GetNextException() const
         return m_next_exception.get();
     }
     lldb::addr_t
     GetExceptionAddress() const
     {
         return m_exception_addr;
     }
 
+    lldb::tid_t
+    GetThreadID() const
+    {
+        return m_thread_id;
+    }
+
   private:
     DWORD m_code;
     bool m_continuable;
     std::shared_ptr<ExceptionRecord> m_next_exception;
     lldb::addr_t m_exception_addr;
+    lldb::tid_t m_thread_id;
     std::vector<ULONG_PTR> m_arguments;
 };
 }
 
 #endif

source/Plugins/Process/Windows/IDebugDelegate.h

 {
   public:
     virtual ~IDebugDelegate() {}
 
     virtual void OnExitProcess(uint32_t exit_code) = 0;
     virtual void OnDebuggerConnected(lldb::addr_t image_base) = 0;
     virtual ExceptionResult OnDebugException(bool first_chance, const ExceptionRecord &record) = 0;
     virtual void OnCreateThread(const HostThread &thread) = 0;
-    virtual void OnExitThread(const HostThread &thread) = 0;
+    virtual void OnExitThread(lldb::tid_t thread_id, uint32_t exit_code) = 0;
     virtual void OnLoadDll(const ModuleSpec &module_spec, lldb::addr_t module_addr) = 0;
     virtual void OnUnloadDll(lldb::addr_t module_addr) = 0;
     virtual void OnDebugString(const std::string &string) = 0;
     virtual void OnDebuggerError(const Error &error, uint32_t type) = 0;
 };
 }
 
 #endif

source/Plugins/Process/Windows/LocalDebugDelegate.h

 {
   public:
     explicit LocalDebugDelegate(lldb::ProcessSP process);
 
     void OnExitProcess(uint32_t exit_code) override;
     void OnDebuggerConnected(lldb::addr_t image_base) override;
     ExceptionResult OnDebugException(bool first_chance, const ExceptionRecord &record) override;
     void OnCreateThread(const HostThread &thread) override;
-    void OnExitThread(const HostThread &thread) override;
+    void OnExitThread(lldb::tid_t thread_id, uint32_t exit_code) override;
     void OnLoadDll(const lldb_private::ModuleSpec &module_spec, lldb::addr_t module_addr) override;
     void OnUnloadDll(lldb::addr_t module_addr) override;
     void OnDebugString(const std::string &message) override;
     void OnDebuggerError(const Error &error, uint32_t type) override;
 
   private:
     lldb::ProcessSP m_process;
 };
 }
 
 #endif

source/Plugins/Process/Windows/LocalDebugDelegate.cpp

 
 void
 LocalDebugDelegate::OnCreateThread(const HostThread &thread)
 {
     ((ProcessWindows &)*m_process).OnCreateThread(thread);
 }
 
 void
-LocalDebugDelegate::OnExitThread(const HostThread &thread)
+LocalDebugDelegate::OnExitThread(lldb::tid_t thread_id, uint32_t exit_code)
 {
-    ((ProcessWindows &)*m_process).OnExitThread(thread);
+    ((ProcessWindows &)*m_process).OnExitThread(thread_id, exit_code);
 }
 
 void
 LocalDebugDelegate::OnLoadDll(const lldb_private::ModuleSpec &module_spec, lldb::addr_t module_addr)
 {
     ((ProcessWindows &)*m_process).OnLoadDll(module_spec, module_addr);
 }
 

source/Plugins/Process/Windows/ProcessWindows.h

public:
     size_t DoWriteMemory(lldb::addr_t vm_addr, const void *buf, size_t size, lldb_private::Error &error) override;
     lldb_private::Error GetMemoryRegionInfo(lldb::addr_t vm_addr, lldb_private::MemoryRegionInfo &info) override;
 
     // IDebugDelegate overrides.
     void OnExitProcess(uint32_t exit_code) override;
     void OnDebuggerConnected(lldb::addr_t image_base) override;
     ExceptionResult OnDebugException(bool first_chance, const lldb_private::ExceptionRecord &record) override;
     void OnCreateThread(const lldb_private::HostThread &thread) override;
-    void OnExitThread(const lldb_private::HostThread &thread) override;
+    void OnExitThread(lldb::tid_t thread_id, uint32_t exit_code) override;
     void OnLoadDll(const lldb_private::ModuleSpec &module_spec, lldb::addr_t module_addr) override;
     void OnUnloadDll(lldb::addr_t module_addr) override;
     void OnDebugString(const std::string &string) override;
     void OnDebuggerError(const lldb_private::Error &error, uint32_t type) override;
 
   private:
     llvm::sys::Mutex m_mutex;
 
     // Data for the active debugging session.
     std::unique_ptr<lldb_private::ProcessWindowsData> m_session_data;
 };
 
 #endif  // liblldb_Plugins_Process_Windows_ProcessWindows_H_

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"
 
 // C++ Includes
 #include <list>
 #include <mutex>
+#include <set>
 #include <vector>
 
 // Other libraries and framework includes
 #include "lldb/Core/Module.h"
 #include "lldb/Core/ModuleSpec.h"
 #include "lldb/Core/PluginManager.h"
 #include "lldb/Core/Section.h"
 #include "lldb/Core/State.h"
public:
 
     ProcessLaunchInfo m_launch_info;
     lldb_private::Error m_launch_error;
     lldb_private::DebuggerThreadSP m_debugger;
     StopInfoSP m_pending_stop_info;
     HANDLE m_initial_stop_event;
     bool m_initial_stop_received;
     std::map<lldb::tid_t, HostThread> m_new_threads;
-    std::map<lldb::tid_t, HostThread> m_exited_threads;
+    std::set<lldb::tid_t> m_exited_threads;
 };
 }
 //------------------------------------------------------------------------------
 // Static functions.
 
 ProcessSP
 ProcessWindows::CreateInstance(Target &target, Listener &listener, const FileSpec *)
 {
ProcessWindows::RefreshStateAfterStop()
     {
         WINERR_IFALL(WINDOWS_LOG_PROCESS, "RefreshStateAfterStop called for process %I64u but there is no "
                                           "active exception.  Why is the process stopped?",
                      m_session_data->m_debugger->GetProcess().GetProcessId());
         return;
     }
 
     StopInfoSP stop_info;
+    m_thread_list.SetSelectedThreadByID(active_exception->GetThreadID());
     ThreadSP stop_thread = m_thread_list.GetSelectedThread();
     RegisterContextSP register_context = stop_thread->GetRegisterContext();
 
     // The current EIP is AFTER the BP opcode, which is one byte.
-    // TODO(zturner): Can't we just use active_exception->GetExceptionAddress()?
     uint64_t pc = register_context->GetPC() - 1;
     if (active_exception->GetExceptionCode() == EXCEPTION_BREAKPOINT)
     {
         BreakpointSiteSP site(GetBreakpointSiteList().FindByAddress(pc));
 
         if (site)
         {
             WINLOG_IFANY(WINDOWS_LOG_BREAKPOINTS | WINDOWS_LOG_EXCEPTION,
                          "RefreshStateAfterStop detected breakpoint in process %I64u at "
                          "address 0x%I64x with breakpoint site %d",
                          m_session_data->m_debugger->GetProcess().GetProcessId(), pc, site->GetID());
 
             if (site->ValidForThisThread(stop_thread.get()))
             {
                 WINLOG_IFALL(WINDOWS_LOG_BREAKPOINTS | WINDOWS_LOG_EXCEPTION,
-                             "Breakpoint site %d is valid for this thread, creating stop info.", site->GetID());
+                             "Breakpoint site %d is valid for this thread (0x%I64x), creating stop info.",
+                             site->GetID(), stop_thread->GetID());
 
                 stop_info = StopInfo::CreateStopReasonWithBreakpointSiteID(
                     *stop_thread, site->GetID());
                 register_context->SetPC(pc);
             }
             else
             {
                 WINLOG_IFALL(WINDOWS_LOG_BREAKPOINTS | WINDOWS_LOG_EXCEPTION,
else if (active_exception->GetExceptionCode() == EXCEPTION_SINGLE_STEP)
         stop_thread->SetStopInfo(stop_info);
         WINLOG_IFANY(WINDOWS_LOG_EXCEPTION | WINDOWS_LOG_STEP, "RefreshStateAfterStop single stepping thread %u",
                      stop_thread->GetID());
     }
     else
     {
         std::string desc;
         llvm::raw_string_ostream desc_stream(desc);
-        desc_stream << "Exception 0x" << llvm::format_hex(active_exception->GetExceptionCode(), 8)
-                    << " encountered at address 0x" << llvm::format_hex(pc, 8);
+        desc_stream << "Exception " << llvm::format_hex(active_exception->GetExceptionCode(), 8)
+                    << " encountered at address " << llvm::format_hex(pc, 8);
         stop_info = StopInfo::CreateStopReasonWithException(*stop_thread, desc_stream.str().c_str());
         stop_thread->SetStopInfo(stop_info);
         WINLOG_IFALL(WINDOWS_LOG_EXCEPTION, desc_stream.str().c_str());
     }
 }
 
 bool
 ProcessWindows::IsAlive()
ProcessWindows::OnDebugException(bool first_chance, const ExceptionRecord &record)
     // FIXME: Without this check, occasionally when running the test suite there is
     // an issue where m_session_data can be null.  It's not clear how this could happen
     // but it only surfaces while running the test suite.  In order to properly diagnose
     // this, we probably need to first figure allow the test suite to print out full
     // lldb logs, and then add logging to the process plugin.
     if (!m_session_data)
     {
         WINERR_IFANY(WINDOWS_LOG_EXCEPTION,
-                     "Debugger thread reported exception 0x%u at address 0x%I64x, but there is no session.",
+                     "Debugger thread reported exception 0x%x at address 0x%I64x, but there is no session.",
                      record.GetExceptionCode(), record.GetExceptionAddress());
         return ExceptionResult::SendToApplication;
     }
 
     if (!first_chance)
     {
         // Any second chance exception is an application crash by definition.
         SetPrivateState(eStateCrashed);
switch (record.GetExceptionCode())
             SetPrivateState(eStateStopped);
             break;
         case EXCEPTION_SINGLE_STEP:
             result = ExceptionResult::BreakInDebugger;
             SetPrivateState(eStateStopped);
             break;
         default:
             WINLOG_IFANY(WINDOWS_LOG_EXCEPTION,
-                         "Debugger thread reported exception 0x%u at address 0x%I64x (first_chance=%s)",
+                         "Debugger thread reported exception 0x%x at address 0x%I64x (first_chance=%s)",
                          record.GetExceptionCode(), record.GetExceptionAddress(), BOOL_STR(first_chance));
             // For non-breakpoints, give the application a chance to handle the exception first.
             if (first_chance)
                 result = ExceptionResult::SendToApplication;
             else
                 result = ExceptionResult::BreakInDebugger;
     }
 
     return result;
 }
 
 void
 ProcessWindows::OnCreateThread(const HostThread &new_thread)
 {
     llvm::sys::ScopedLock lock(m_mutex);
     const HostThreadWindows &wnew_thread = new_thread.GetNativeThread();
     m_session_data->m_new_threads[wnew_thread.GetThreadId()] = new_thread;
 }
 
 void
-ProcessWindows::OnExitThread(const HostThread &exited_thread)
+ProcessWindows::OnExitThread(lldb::tid_t thread_id, uint32_t exit_code)
 {
     llvm::sys::ScopedLock lock(m_mutex);
 
+    // On a forced termination, we may get exit thread events after the session
+    // data has been cleaned up.
+    if (!m_session_data)
+        return;
+
     // A thread may have started and exited before the debugger stopped allowing a refresh.
     // Just remove it from the new threads list in that case.
-    const HostThreadWindows &wexited_thread = exited_thread.GetNativeThread();
-    auto iter = m_session_data->m_new_threads.find(wexited_thread.GetThreadId());
+    auto iter = m_session_data->m_new_threads.find(thread_id);
     if (iter != m_session_data->m_new_threads.end())
         m_session_data->m_new_threads.erase(iter);
     else
-        m_session_data->m_exited_threads[wexited_thread.GetThreadId()] = exited_thread;
+        m_session_data->m_exited_threads.insert(thread_id);
 }
 
 void
 ProcessWindows::OnLoadDll(const ModuleSpec &module_spec, lldb::addr_t module_addr)
 {
     // Confusingly, there is no Target::AddSharedModule.  Instead, calling GetSharedModule() with
     // a new module will add it to the module list and return a corresponding ModuleSP.
     Error error;
     ModuleSP module = GetTarget().GetSharedModule(module_spec, &error);
     bool load_addr_changed = false;

source/Target/ThreadList.cpp

ThreadList::ShouldStop (Event *event_ptr)
     // Running events should never stop, obviously...
 
     Log *log(lldb_private::GetLogIfAllCategoriesSet (LIBLLDB_LOG_STEP));
 
     // The ShouldStop method of the threads can do a whole lot of work,
     // figuring out whether the thread plan conditions are met.  So we don't want
     // to keep the ThreadList locked the whole time we are doing this.
     // FIXME: It is possible that running code could cause new threads
-    // to be created.  If that happens we will miss asking them whether
-    // then should stop.  This is not a big deal, since we haven't had
+    // to be created.  If that happens, we will miss asking them whether
+    // they should stop.  This is not a big deal since we haven't had
     // a chance to hang any interesting operations on those threads yet.
     
     collection threads_copy;
     {
         // Scope for locker
         Mutex::Locker locker(GetMutex());
 
         m_process->UpdateThreadListIfNeeded();