From 45d8134c3b57b04654b771fc6ba440d9e8e0ff64 Mon Sep 17 00:00:00 2001 From: Stella Stamenova Date: Tue, 10 Jul 2018 22:05:33 +0000 Subject: [PATCH] [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 --- .../Plugins/Process/FreeBSD/FreeBSDThread.cpp | 16 +++--- .../Plugins/Process/MacOSX-Kernel/ThreadKDP.cpp | 10 ++-- .../Plugins/Process/Utility/ThreadMemory.cpp | 2 +- .../Process/Windows/Common/TargetThreadWindows.cpp | 66 +++++++++++++--------- .../Process/Windows/Common/TargetThreadWindows.h | 5 +- .../Plugins/Process/elf-core/ThreadElfCore.cpp | 27 ++++----- .../Plugins/Process/elf-core/ThreadElfCore.h | 2 - .../Plugins/Process/gdb-remote/ThreadGDBRemote.cpp | 10 ++-- .../Plugins/Process/mach-core/ThreadMachCore.cpp | 8 +-- 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 4b0d640..3576a7f 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 8aaaccc..7fca0fc 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 5ff928c..0c7c195 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 154c8a4..3903280 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 7b93952..952c0f5 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 c808a15..d9b90c8 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(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 335f698..167fd6e 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 87b78ae..a525c16 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 4538ce5..c262dd4 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; -- 2.7.4