[windows] LLDB shows the wrong values when register read is executed at a frame other than zero

Summary:
This is a clean version of the change suggested here: https://bugs.llvm.org/show_bug.cgi?id=37495

The main change is to follow the same pattern as non-windows targets and use an unwinder object to retrieve the register context. I also changed a couple of the comments to actually log, so that issues with unsupported scenarios can be tracked down more easily. Lastly, ClearStackFrames is implemented in the base class, so individual thread implementations don't have to override it.

Reviewers: asmith, zturner, aleksandr.urakov

Reviewed By: aleksandr.urakov

Subscribers: emaste, stella.stamenova, tatyana-krasnukha, llvm-commits

Differential Revision: https://reviews.llvm.org/D49111

llvm-svn: 336732

9 files changed, 76 insertions, 70 deletions

diff --git a/lldb/source/Plugins/Process/FreeBSD/FreeBSDThread.cpp b/lldb/source/Plugins/Process/FreeBSD/FreeBSDThread.cpp
index 4b0d640c6c2..3576a7f26f8 100644
--- a/lldb/source/Plugins/Process/FreeBSD/FreeBSDThread.cpp
+++ b/lldb/source/Plugins/Process/FreeBSD/FreeBSDThread.cpp
@@ -57,7 +57,7 @@ using namespace lldb_private;
 
 FreeBSDThread::FreeBSDThread(Process &process, lldb::tid_t tid)
     : Thread(process, tid), m_frame_ap(), m_breakpoint(),
-      m_thread_name_valid(false), m_thread_name(), m_posix_thread(NULL) {
+      m_thread_name_valid(false), m_thread_name(), m_posix_thread(nullptr) {
   Log *log(ProcessPOSIXLog::GetLogIfAllCategoriesSet(POSIX_LOG_THREAD));
   LLDB_LOGV(log, "tid = {0}", tid);
 
@@ -106,7 +106,7 @@ void FreeBSDThread::RefreshStateAfterStop() {
   }
 }
 
-const char *FreeBSDThread::GetInfo() { return NULL; }
+const char *FreeBSDThread::GetInfo() { return nullptr; }
 
 void FreeBSDThread::SetName(const char *name) {
   m_thread_name_valid = (name && name[0]);
@@ -157,15 +157,15 @@ const char *FreeBSDThread::GetName() {
   }
 
   if (m_thread_name.empty())
-    return NULL;
+    return nullptr;
   return m_thread_name.c_str();
 }
 
 lldb::RegisterContextSP FreeBSDThread::GetRegisterContext() {
   if (!m_reg_context_sp) {
-    m_posix_thread = NULL;
+    m_posix_thread = nullptr;
 
-    RegisterInfoInterface *reg_interface = NULL;
+    RegisterInfoInterface *reg_interface = nullptr;
     const ArchSpec &target_arch = GetProcess()->GetTarget().GetArchitecture();
 
     assert(target_arch.GetTriple().getOS() == llvm::Triple::FreeBSD);
@@ -281,7 +281,7 @@ bool FreeBSDThread::CalculateStopInfo() {
 }
 
 Unwind *FreeBSDThread::GetUnwinder() {
-  if (m_unwinder_ap.get() == NULL)
+  if (!m_unwinder_ap)
     m_unwinder_ap.reset(new UnwindLLDB(*this));
 
   return m_unwinder_ap.get();
@@ -480,7 +480,7 @@ void FreeBSDThread::BreakNotify(const ProcessMessage &message) {
     // make any assumptions about the thread ID so we must always report the
     // breakpoint regardless of the thread.
     if (bp_site->ValidForThisThread(this) ||
-        GetProcess()->GetOperatingSystem() != NULL)
+        GetProcess()->GetOperatingSystem() != nullptr)
       SetStopInfo(StopInfo::CreateStopReasonWithBreakpointSiteID(*this, bp_id));
     else {
       const bool should_stop = false;
@@ -544,7 +544,7 @@ void FreeBSDThread::TraceNotify(const ProcessMessage &message) {
   // assumptions about the thread ID so we must always report the breakpoint
   // regardless of the thread.
   if (bp_site && (bp_site->ValidForThisThread(this) ||
-                  GetProcess()->GetOperatingSystem() != NULL))
+                  GetProcess()->GetOperatingSystem() != nullptr))
     SetStopInfo(StopInfo::CreateStopReasonWithBreakpointSiteID(
         *this, bp_site->GetID()));
   else {
diff --git a/lldb/source/Plugins/Process/MacOSX-Kernel/ThreadKDP.cpp b/lldb/source/Plugins/Process/MacOSX-Kernel/ThreadKDP.cpp
index 8aaacccf4c1..7fca0fc24fd 100644
--- a/lldb/source/Plugins/Process/MacOSX-Kernel/ThreadKDP.cpp
+++ b/lldb/source/Plugins/Process/MacOSX-Kernel/ThreadKDP.cpp
@@ -52,11 +52,11 @@ ThreadKDP::~ThreadKDP() {
 
 const char *ThreadKDP::GetName() {
   if (m_thread_name.empty())
-    return NULL;
+    return nullptr;
   return m_thread_name.c_str();
 }
 
-const char *ThreadKDP::GetQueueName() { return NULL; }
+const char *ThreadKDP::GetQueueName() { return nullptr; }
 
 void ThreadKDP::RefreshStateAfterStop() {
   // Invalidate all registers in our register context. We don't set "force" to
@@ -79,8 +79,8 @@ void ThreadKDP::Dump(Log *log, uint32_t index) {}
 
 bool ThreadKDP::ShouldStop(bool &step_more) { return true; }
 lldb::RegisterContextSP ThreadKDP::GetRegisterContext() {
-  if (m_reg_context_sp.get() == NULL)
-    m_reg_context_sp = CreateRegisterContextForFrame(NULL);
+  if (!m_reg_context_sp)
+    m_reg_context_sp = CreateRegisterContextForFrame(nullptr);
   return m_reg_context_sp;
 }
 
@@ -119,7 +119,7 @@ ThreadKDP::CreateRegisterContextForFrame(StackFrame *frame) {
     }
   } else {
     Unwind *unwinder = GetUnwinder();
-    if (unwinder)
+    if (unwinder != nullptr)
       reg_ctx_sp = unwinder->CreateRegisterContextForFrame(frame);
   }
   return reg_ctx_sp;
diff --git a/lldb/source/Plugins/Process/Utility/ThreadMemory.cpp b/lldb/source/Plugins/Process/Utility/ThreadMemory.cpp
index 5ff928c69e5..0c7c195815a 100644
--- a/lldb/source/Plugins/Process/Utility/ThreadMemory.cpp
+++ b/lldb/source/Plugins/Process/Utility/ThreadMemory.cpp
@@ -62,7 +62,7 @@ ThreadMemory::CreateRegisterContextForFrame(StackFrame *frame) {
     reg_ctx_sp = GetRegisterContext();
   } else {
     Unwind *unwinder = GetUnwinder();
-    if (unwinder)
+    if (unwinder != nullptr)
       reg_ctx_sp = unwinder->CreateRegisterContextForFrame(frame);
   }
   return reg_ctx_sp;
diff --git a/lldb/source/Plugins/Process/Windows/Common/TargetThreadWindows.cpp b/lldb/source/Plugins/Process/Windows/Common/TargetThreadWindows.cpp
index 154c8a4cb2d..3903280918c 100644
--- a/lldb/source/Plugins/Process/Windows/Common/TargetThreadWindows.cpp
+++ b/lldb/source/Plugins/Process/Windows/Common/TargetThreadWindows.cpp
@@ -16,10 +16,10 @@
 #include "lldb/Utility/Log.h"
 #include "lldb/Utility/Logging.h"
 
+#include "Plugins/Process/Utility/UnwindLLDB.h"
 #include "ProcessWindows.h"
 #include "ProcessWindowsLog.h"
 #include "TargetThreadWindows.h"
-#include "Plugins/Process/Utility/UnwindLLDB.h"
 
 #if defined(_WIN64)
 #include "x64/RegisterContextWindows_x64.h"
@@ -33,7 +33,7 @@ using namespace lldb_private;
 TargetThreadWindows::TargetThreadWindows(ProcessWindows &process,
                                          const HostThread &thread)
     : Thread(process, thread.GetNativeThread().GetThreadId()),
-      m_host_thread(thread) {}
+      m_thread_reg_ctx_sp(), m_host_thread(thread) {}
 
 TargetThreadWindows::~TargetThreadWindows() { DestroyThread(); }
 
@@ -49,40 +49,53 @@ void TargetThreadWindows::DidStop() {}
 
 RegisterContextSP TargetThreadWindows::GetRegisterContext() {
   if (!m_reg_context_sp)
-    m_reg_context_sp = CreateRegisterContextForFrameIndex(0);
+    m_reg_context_sp = CreateRegisterContextForFrame(nullptr);
 
   return m_reg_context_sp;
 }
 
 RegisterContextSP
 TargetThreadWindows::CreateRegisterContextForFrame(StackFrame *frame) {
-  return CreateRegisterContextForFrameIndex(frame->GetConcreteFrameIndex());
-}
-
-RegisterContextSP
-TargetThreadWindows::CreateRegisterContextForFrameIndex(uint32_t idx) {
-  if (!m_reg_context_sp) {
-    ArchSpec arch = HostInfo::GetArchitecture();
-    switch (arch.GetMachine()) {
-    case llvm::Triple::x86:
+  RegisterContextSP reg_ctx_sp;
+  uint32_t concrete_frame_idx = 0;
+  Log *log(GetLogIfAllCategoriesSet(LIBLLDB_LOG_THREAD));
+
+  if (frame)
+    concrete_frame_idx = frame->GetConcreteFrameIndex();
+
+  if (concrete_frame_idx == 0) {
+    if (!m_thread_reg_ctx_sp) {
+      ArchSpec arch = HostInfo::GetArchitecture();
+      switch (arch.GetMachine()) {
+      case llvm::Triple::x86:
 #if defined(_WIN64)
-// FIXME: This is a Wow64 process, create a RegisterContextWindows_Wow64
+        // FIXME: This is a Wow64 process, create a RegisterContextWindows_Wow64
+        LLDB_LOG(log, "This is a Wow64 process, we should create a "
+                      "RegisterContextWindows_Wow64, but we don't.");
 #else
-      m_reg_context_sp.reset(new RegisterContextWindows_x86(*this, idx));
+        m_thread_reg_ctx_sp.reset(
+            new RegisterContextWindows_x86(*this, concrete_frame_idx));
 #endif
-      break;
-    case llvm::Triple::x86_64:
+        break;
+      case llvm::Triple::x86_64:
 #if defined(_WIN64)
-      m_reg_context_sp.reset(new RegisterContextWindows_x64(*this, idx));
+        m_thread_reg_ctx_sp.reset(
+            new RegisterContextWindows_x64(*this, concrete_frame_idx));
 #else
-// LLDB is 32-bit, but the target process is 64-bit.  We probably can't debug
-// this.
+        LLDB_LOG(log, "LLDB is 32-bit, but the target process is 64-bit.");
 #endif
-    default:
-      break;
+      default:
+        break;
+      }
     }
+    reg_ctx_sp = m_thread_reg_ctx_sp;
+  } else {
+    Unwind *unwinder = GetUnwinder();
+    if (unwinder != nullptr)
+      reg_ctx_sp = unwinder->CreateRegisterContextForFrame(frame);
   }
-  return m_reg_context_sp;
+
+  return reg_ctx_sp;
 }
 
 bool TargetThreadWindows::CalculateStopInfo() {
@@ -93,7 +106,7 @@ bool TargetThreadWindows::CalculateStopInfo() {
 Unwind *TargetThreadWindows::GetUnwinder() {
   // FIXME: Implement an unwinder based on the Windows unwinder exposed through
   // DIA SDK.
-  if (m_unwinder_ap.get() == NULL)
+  if (!m_unwinder_ap)
     m_unwinder_ap.reset(new UnwindLLDB(*this));
   return m_unwinder_ap.get();
 }
@@ -118,9 +131,10 @@ Status TargetThreadWindows::DoResume() {
     DWORD previous_suspend_count = 0;
     HANDLE thread_handle = m_host_thread.GetNativeThread().GetSystemHandle();
     do {
-      // ResumeThread returns -1 on error, or the thread's *previous* suspend count on success.
-      // This means that the return value is 1 when the thread was restarted.
-      // Note that DWORD is an unsigned int, so we need to explicitly compare with -1.
+      // ResumeThread returns -1 on error, or the thread's *previous* suspend
+      // count on success. This means that the return value is 1 when the thread
+      // was restarted. Note that DWORD is an unsigned int, so we need to
+      // explicitly compare with -1.
       previous_suspend_count = ::ResumeThread(thread_handle);
 
       if (previous_suspend_count == (DWORD)-1)
diff --git a/lldb/source/Plugins/Process/Windows/Common/TargetThreadWindows.h b/lldb/source/Plugins/Process/Windows/Common/TargetThreadWindows.h
index 7b93952c01a..952c0f55b57 100644
--- a/lldb/source/Plugins/Process/Windows/Common/TargetThreadWindows.h
+++ b/lldb/source/Plugins/Process/Windows/Common/TargetThreadWindows.h
@@ -42,10 +42,9 @@ public:
   HostThread GetHostThread() const { return m_host_thread; }
 
 private:
-  lldb::RegisterContextSP CreateRegisterContextForFrameIndex(uint32_t idx);
-
+  lldb::RegisterContextSP m_thread_reg_ctx_sp;
   HostThread m_host_thread;
 };
-}
+} // namespace lldb_private
 
 #endif
diff --git a/lldb/source/Plugins/Process/elf-core/ThreadElfCore.cpp b/lldb/source/Plugins/Process/elf-core/ThreadElfCore.cpp
index c808a15cfc2..d9b90c8e902 100644
--- a/lldb/source/Plugins/Process/elf-core/ThreadElfCore.cpp
+++ b/lldb/source/Plugins/Process/elf-core/ThreadElfCore.cpp
@@ -55,16 +55,9 @@ void ThreadElfCore::RefreshStateAfterStop() {
   GetRegisterContext()->InvalidateIfNeeded(false);
 }
 
-void ThreadElfCore::ClearStackFrames() {
-  Unwind *unwinder = GetUnwinder();
-  if (unwinder)
-    unwinder->Clear();
-  Thread::ClearStackFrames();
-}
-
 RegisterContextSP ThreadElfCore::GetRegisterContext() {
-  if (m_reg_context_sp.get() == NULL) {
-    m_reg_context_sp = CreateRegisterContextForFrame(NULL);
+  if (!m_reg_context_sp) {
+    m_reg_context_sp = CreateRegisterContextForFrame(nullptr);
   }
   return m_reg_context_sp;
 }
@@ -84,7 +77,7 @@ ThreadElfCore::CreateRegisterContextForFrame(StackFrame *frame) {
 
     ProcessElfCore *process = static_cast<ProcessElfCore *>(GetProcess().get());
     ArchSpec arch = process->GetArchitecture();
-    RegisterInfoInterface *reg_interface = NULL;
+    RegisterInfoInterface *reg_interface = nullptr;
 
     switch (arch.GetTriple().getOS()) {
     case llvm::Triple::FreeBSD: {
@@ -234,8 +227,10 @@ ThreadElfCore::CreateRegisterContextForFrame(StackFrame *frame) {
     }
 
     reg_ctx_sp = m_thread_reg_ctx_sp;
-  } else if (m_unwinder_ap.get()) {
-    reg_ctx_sp = m_unwinder_ap->CreateRegisterContextForFrame(frame);
+  } else {
+    Unwind *unwinder = GetUnwinder();
+    if (unwinder != nullptr)
+      reg_ctx_sp = unwinder->CreateRegisterContextForFrame(frame);
   }
   return reg_ctx_sp;
 }
@@ -340,7 +335,7 @@ size_t ELFLinuxPrPsInfo::GetSize(const lldb_private::ArchSpec &arch) {
       return sizeof(ELFLinuxPrPsInfo);
     return mips_linux_pr_psinfo_size_o32_n32;
   }
-  
+
   switch (arch.GetCore()) {
   case lldb_private::ArchSpec::eCore_s390x_generic:
   case lldb_private::ArchSpec::eCore_x86_64_x86_64:
@@ -382,9 +377,9 @@ Status ELFLinuxPrPsInfo::Parse(const DataExtractor &data,
     pr_uid = data.GetU32(&offset);
     pr_gid = data.GetU32(&offset);
   } else {
-  // 16 bit on 32 bit platforms, 32 bit on 64 bit platforms
-  pr_uid = data.GetMaxU64(&offset, data.GetAddressByteSize() >> 1);
-  pr_gid = data.GetMaxU64(&offset, data.GetAddressByteSize() >> 1);
+    // 16 bit on 32 bit platforms, 32 bit on 64 bit platforms
+    pr_uid = data.GetMaxU64(&offset, data.GetAddressByteSize() >> 1);
+    pr_gid = data.GetMaxU64(&offset, data.GetAddressByteSize() >> 1);
   }
 
   pr_pid = data.GetU32(&offset);
diff --git a/lldb/source/Plugins/Process/elf-core/ThreadElfCore.h b/lldb/source/Plugins/Process/elf-core/ThreadElfCore.h
index 335f698dbb2..167fd6edc4c 100644
--- a/lldb/source/Plugins/Process/elf-core/ThreadElfCore.h
+++ b/lldb/source/Plugins/Process/elf-core/ThreadElfCore.h
@@ -146,8 +146,6 @@ public:
   lldb::RegisterContextSP
   CreateRegisterContextForFrame(lldb_private::StackFrame *frame) override;
 
-  void ClearStackFrames() override;
-
   static bool ThreadIDIsValid(lldb::tid_t thread) { return thread != 0; }
 
   const char *GetName() override {
diff --git a/lldb/source/Plugins/Process/gdb-remote/ThreadGDBRemote.cpp b/lldb/source/Plugins/Process/gdb-remote/ThreadGDBRemote.cpp
index 87b78ae1f64..a525c16b9f1 100644
--- a/lldb/source/Plugins/Process/gdb-remote/ThreadGDBRemote.cpp
+++ b/lldb/source/Plugins/Process/gdb-remote/ThreadGDBRemote.cpp
@@ -55,7 +55,7 @@ ThreadGDBRemote::~ThreadGDBRemote() {
 
 const char *ThreadGDBRemote::GetName() {
   if (m_thread_name.empty())
-    return NULL;
+    return nullptr;
   return m_thread_name.c_str();
 }
 
@@ -109,7 +109,7 @@ const char *ThreadGDBRemote::GetQueueName() {
         return m_dispatch_queue_name.c_str();
     }
   }
-  return NULL;
+  return nullptr;
 }
 
 QueueKind ThreadGDBRemote::GetQueueKind() {
@@ -289,8 +289,8 @@ void ThreadGDBRemote::Dump(Log *log, uint32_t index) {}
 
 bool ThreadGDBRemote::ShouldStop(bool &step_more) { return true; }
 lldb::RegisterContextSP ThreadGDBRemote::GetRegisterContext() {
-  if (m_reg_context_sp.get() == NULL)
-    m_reg_context_sp = CreateRegisterContextForFrame(NULL);
+  if (!m_reg_context_sp)
+    m_reg_context_sp = CreateRegisterContextForFrame(nullptr);
   return m_reg_context_sp;
 }
 
@@ -317,7 +317,7 @@ ThreadGDBRemote::CreateRegisterContextForFrame(StackFrame *frame) {
     }
   } else {
     Unwind *unwinder = GetUnwinder();
-    if (unwinder)
+    if (unwinder != nullptr)
       reg_ctx_sp = unwinder->CreateRegisterContextForFrame(frame);
   }
   return reg_ctx_sp;
diff --git a/lldb/source/Plugins/Process/mach-core/ThreadMachCore.cpp b/lldb/source/Plugins/Process/mach-core/ThreadMachCore.cpp
index 4538ce5bcb1..c262dd47f97 100644
--- a/lldb/source/Plugins/Process/mach-core/ThreadMachCore.cpp
+++ b/lldb/source/Plugins/Process/mach-core/ThreadMachCore.cpp
@@ -43,7 +43,7 @@ ThreadMachCore::~ThreadMachCore() { DestroyThread(); }
 
 const char *ThreadMachCore::GetName() {
   if (m_thread_name.empty())
-    return NULL;
+    return nullptr;
   return m_thread_name.c_str();
 }
 
@@ -63,8 +63,8 @@ void ThreadMachCore::RefreshStateAfterStop() {
 bool ThreadMachCore::ThreadIDIsValid(lldb::tid_t thread) { return thread != 0; }
 
 lldb::RegisterContextSP ThreadMachCore::GetRegisterContext() {
-  if (m_reg_context_sp.get() == NULL)
-    m_reg_context_sp = CreateRegisterContextForFrame(NULL);
+  if (!m_reg_context_sp)
+    m_reg_context_sp = CreateRegisterContextForFrame(nullptr);
   return m_reg_context_sp;
 }
 
@@ -89,7 +89,7 @@ ThreadMachCore::CreateRegisterContextForFrame(StackFrame *frame) {
     reg_ctx_sp = m_thread_reg_ctx_sp;
   } else {
     Unwind *unwinder = GetUnwinder();
-    if (unwinder)
+    if (unwinder != nullptr)
       reg_ctx_sp = unwinder->CreateRegisterContextForFrame(frame);
   }
   return reg_ctx_sp;