fallkrum updated this revision to Diff 269067. fallkrum added a comment. In D80112#2075097 <https://reviews.llvm.org/D80112#2075097>, @jingham wrote:
> If I understand the problem you are describing, it is that you suspended a > thread as a user-level suspend, so it's stop reason got stuck. That seems > okay, its probably useful to keep the stop reason of the thread at what it > was when you suspended it. But that means when gets asked to do its action > again, the action is stale and does the wrong thing. If that's what's going > wrong, Yes, you understood right, that's exactly what was going wrong. > Actually, you can more easily do that. The thread iteration where we call > PerformAction etc. works on a copy of the thread list (since one thread's > action could cause the thread list to change). So if you just change the copy > operation to only copy over threads which aren't user-suspended, then you > should be set. I don't see that's a copy, it seems that's a reference on the actual thread list: ThreadList &curr_thread_list = process_sp->GetThreadList(); So I decided to filter out all suspended thread as you recommended. > It would be good to write a end-to-end test that mirrors this behavior. It > would be pretty straightforward to write an API test that runs a process, > sets a breakpoint, hits it on one thread, suspends that thread, then hits it > on another thread and make sure the action didn't run twice on the second > stop. Don't see it is possible to write such a test due to StopInfoBreakpoint::PerformAction implementation. This method executes only once and saves results of execution so the next time we call PerformAction it will not execute breakpoint's callback and we have no chance to catch action. Wrote unit tests for ProcessEventData that mirror behaviour. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D80112/new/ https://reviews.llvm.org/D80112 Files: lldb/include/lldb/Target/Process.h lldb/source/Target/Process.cpp lldb/unittests/CMakeLists.txt lldb/unittests/Process/CMakeLists.txt lldb/unittests/Process/ProcessEventDataTest.cpp lldb/unittests/Thread/CMakeLists.txt lldb/unittests/Thread/ThreadTest.cpp
Index: lldb/unittests/Thread/ThreadTest.cpp =================================================================== --- /dev/null +++ lldb/unittests/Thread/ThreadTest.cpp @@ -0,0 +1,166 @@ +//===-- ThreadTest.cpp ------------------------------------------===// +// +// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions. +// See https://llvm.org/LICENSE.txt for license information. +// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception +// +//===----------------------------------------------------------------------===// + +#include "lldb/Target/Thread.h" +#include "Plugins/Platform/Linux/PlatformLinux.h" +#include "lldb/Core/Debugger.h" +#include "lldb/Host/FileSystem.h" +#include "lldb/Host/HostInfo.h" +#include "lldb/Target/Process.h" +#include "lldb/Target/StopInfo.h" +#include "lldb/Utility/ArchSpec.h" +#include "lldb/Utility/Reproducer.h" +#include "gtest/gtest.h" + +using namespace lldb_private; +using namespace lldb_private::repro; +using namespace lldb; + +namespace { +class ThreadTest : public ::testing::Test { +public: + void SetUp() override { + llvm::cantFail(Reproducer::Initialize(ReproducerMode::Off, llvm::None)); + FileSystem::Initialize(); + HostInfo::Initialize(); + platform_linux::PlatformLinux::Initialize(); + } + void TearDown() override { + platform_linux::PlatformLinux::Terminate(); + HostInfo::Terminate(); + FileSystem::Terminate(); + Reproducer::Terminate(); + } +}; + +class DummyProcess : public Process { +public: + using Process::Process; + + virtual bool CanDebug(lldb::TargetSP target, bool plugin_specified_by_name) { + return true; + } + virtual Status DoDestroy() { return {}; } + virtual void RefreshStateAfterStop() {} + virtual size_t DoReadMemory(lldb::addr_t vm_addr, void *buf, size_t size, + Status &error) { + return 0; + } + virtual bool UpdateThreadList(ThreadList &old_thread_list, + ThreadList &new_thread_list) { + return false; + } + virtual ConstString GetPluginName() { return ConstString("Dummy"); } + virtual uint32_t GetPluginVersion() { return 0; } + + ProcessModID &GetModIDNonConstRef() { return m_mod_id; } +}; + +class DummyThread : public Thread { +public: + using Thread::Thread; + + ~DummyThread() override { DestroyThread(); } + + void RefreshStateAfterStop() override {} + + lldb::RegisterContextSP GetRegisterContext() override { return nullptr; } + + lldb::RegisterContextSP + CreateRegisterContextForFrame(StackFrame *frame) override { + return nullptr; + } + + bool CalculateStopInfo() override { return false; } +}; +} // namespace + +TargetSP CreateTarget(DebuggerSP &debugger_sp, ArchSpec &arch) { + Status error; + PlatformSP platform_sp; + TargetSP target_sp; + error = debugger_sp->GetTargetList().CreateTarget( + *debugger_sp, "", arch, eLoadDependentsNo, platform_sp, target_sp); + + if (target_sp) { + debugger_sp->GetTargetList().SetSelectedTarget(target_sp.get()); + } + + return target_sp; +} + +TEST_F(ThreadTest, SetStopInfo) { + ArchSpec arch("powerpc64-pc-linux"); + + Platform::SetHostPlatform( + platform_linux::PlatformLinux::CreateInstance(true, &arch)); + + DebuggerSP debugger_sp = Debugger::CreateInstance(); + ASSERT_TRUE(debugger_sp); + + TargetSP target_sp = CreateTarget(debugger_sp, arch); + ASSERT_TRUE(target_sp); + + ListenerSP listener_sp(Listener::MakeListener("dummy")); + ProcessSP process_sp = std::make_shared<DummyProcess>(target_sp, listener_sp); + ASSERT_TRUE(process_sp); + + DummyProcess *process = static_cast<DummyProcess *>(process_sp.get()); + + ThreadSP thread_sp = std::make_shared<DummyThread>(*process_sp.get(), 0); + ASSERT_TRUE(thread_sp); + + StopInfoSP stopinfo_sp = + StopInfo::CreateStopReasonWithBreakpointSiteID(*thread_sp.get(), 0); + ASSERT_TRUE(stopinfo_sp->IsValid() == true); + + /* + Should make stopinfo valid. + */ + process->GetModIDNonConstRef().BumpStopID(); + ASSERT_TRUE(stopinfo_sp->IsValid() == false); + + thread_sp->SetStopInfo(stopinfo_sp); + ASSERT_TRUE(stopinfo_sp->IsValid() == true); +} + +TEST_F(ThreadTest, GetPrivateStopInfo) { + ArchSpec arch("powerpc64-pc-linux"); + + Platform::SetHostPlatform( + platform_linux::PlatformLinux::CreateInstance(true, &arch)); + + DebuggerSP debugger_sp = Debugger::CreateInstance(); + ASSERT_TRUE(debugger_sp); + + TargetSP target_sp = CreateTarget(debugger_sp, arch); + ASSERT_TRUE(target_sp); + + ListenerSP listener_sp(Listener::MakeListener("dummy")); + ProcessSP process_sp = std::make_shared<DummyProcess>(target_sp, listener_sp); + ASSERT_TRUE(process_sp); + + DummyProcess *process = static_cast<DummyProcess *>(process_sp.get()); + + ThreadSP thread_sp = std::make_shared<DummyThread>(*process_sp.get(), 0); + ASSERT_TRUE(thread_sp); + + StopInfoSP stopinfo_sp = + StopInfo::CreateStopReasonWithBreakpointSiteID(*thread_sp.get(), 0); + ASSERT_TRUE(stopinfo_sp); + + thread_sp->SetStopInfo(stopinfo_sp); + + /* + Should make stopinfo valid if thread is at last breakpoint hit. + */ + process->GetModIDNonConstRef().BumpStopID(); + ASSERT_TRUE(stopinfo_sp->IsValid() == false); + StopInfoSP new_stopinfo_sp = thread_sp->GetPrivateStopInfo(); + ASSERT_TRUE(new_stopinfo_sp && stopinfo_sp->IsValid() == true); +} Index: lldb/unittests/Thread/CMakeLists.txt =================================================================== --- /dev/null +++ lldb/unittests/Thread/CMakeLists.txt @@ -0,0 +1,15 @@ +add_lldb_unittest(ThreadTest + ThreadTest.cpp + + LINK_LIBS + lldbCore + lldbHost + lldbTarget + lldbSymbol + lldbUtility + lldbUtilityHelpers + lldbInterpreter + lldbBreakpoint + lldbPluginPlatformLinux + ) + Index: lldb/unittests/Process/ProcessEventDataTest.cpp =================================================================== --- /dev/null +++ lldb/unittests/Process/ProcessEventDataTest.cpp @@ -0,0 +1,258 @@ +//===-- ProcessEventDataTest.cpp ------------------------------------------===// +// +// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions. +// See https://llvm.org/LICENSE.txt for license information. +// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception +// +//===----------------------------------------------------------------------===// + +#include "Plugins/Platform/MacOSX/PlatformMacOSX.h" +#include "lldb/Core/Debugger.h" +#include "lldb/Host/FileSystem.h" +#include "lldb/Host/HostInfo.h" +#include "lldb/Target/Process.h" +#include "lldb/Target/StopInfo.h" +#include "lldb/Target/Thread.h" +#include "lldb/Utility/ArchSpec.h" +#include "lldb/Utility/Event.h" +#include "lldb/Utility/Reproducer.h" +#include "gtest/gtest.h" + +using namespace lldb_private; +using namespace lldb_private::repro; +using namespace lldb; + +namespace { +class ProcessEventDataTest : public ::testing::Test { +public: + void SetUp() override { + llvm::cantFail(Reproducer::Initialize(ReproducerMode::Off, llvm::None)); + FileSystem::Initialize(); + HostInfo::Initialize(); + PlatformMacOSX::Initialize(); + } + void TearDown() override { + PlatformMacOSX::Terminate(); + HostInfo::Terminate(); + FileSystem::Terminate(); + Reproducer::Terminate(); + } +}; + +class DummyProcess : public Process { +public: + using Process::Process; + + virtual bool CanDebug(lldb::TargetSP target, bool plugin_specified_by_name) { + return true; + } + virtual Status DoDestroy() { return {}; } + virtual void RefreshStateAfterStop() {} + virtual size_t DoReadMemory(lldb::addr_t vm_addr, void *buf, size_t size, + Status &error) { + return 0; + } + virtual bool UpdateThreadList(ThreadList &old_thread_list, + ThreadList &new_thread_list) { + return false; + } + virtual ConstString GetPluginName() { return ConstString("Dummy"); } + virtual uint32_t GetPluginVersion() { return 0; } + + ProcessModID &GetModIDNonConstRef() { return m_mod_id; } +}; + +class DummyThread : public Thread { +public: + using Thread::Thread; + + ~DummyThread() override { DestroyThread(); } + + void RefreshStateAfterStop() override {} + + lldb::RegisterContextSP GetRegisterContext() override { return nullptr; } + + lldb::RegisterContextSP + CreateRegisterContextForFrame(StackFrame *frame) override { + return nullptr; + } + + bool CalculateStopInfo() override { return false; } +}; + +class DummyStopInfo : public StopInfo { +public: + DummyStopInfo(Thread &thread, uint64_t value) + : StopInfo(thread, value), m_should_stop(true), + m_stop_reason(eStopReasonBreakpoint) {} + + bool ShouldStop(Event *event_ptr) override { return m_should_stop; } + + StopReason GetStopReason() const override { return m_stop_reason; } + + bool m_should_stop; + StopReason m_stop_reason; +}; + +class DummyProcessEventData : public Process::ProcessEventData { +public: + DummyProcessEventData(ProcessSP &process_sp, StateType state) + : ProcessEventData(process_sp, state), m_should_stop_hit_count(0) {} + bool ShouldStop(Event *event_ptr, bool *has_valid_stopinfo_ptr) override { + m_should_stop_hit_count++; + return false; + } + + int m_should_stop_hit_count; +}; +} // namespace + +typedef std::shared_ptr<Process::ProcessEventData> ProcessEventDataSP; +typedef std::shared_ptr<Event> EventSP; + +TargetSP CreateTarget(DebuggerSP &debugger_sp, ArchSpec &arch) { + Status error; + PlatformSP platform_sp; + TargetSP target_sp; + error = debugger_sp->GetTargetList().CreateTarget( + *debugger_sp, "", arch, eLoadDependentsNo, platform_sp, target_sp); + + if (target_sp) { + debugger_sp->GetTargetList().SetSelectedTarget(target_sp.get()); + } + + return target_sp; +} + +ThreadSP CreateThread(ProcessSP &process_sp, bool should_stop, + bool has_valid_stopinfo) { + ThreadSP thread_sp = std::make_shared<DummyThread>(*process_sp.get(), 0); + if (thread_sp == nullptr) { + return nullptr; + } + + if (has_valid_stopinfo) { + StopInfoSP stopinfo_sp = + std::make_shared<DummyStopInfo>(*thread_sp.get(), 0); + static_cast<DummyStopInfo *>(stopinfo_sp.get())->m_should_stop = + should_stop; + if (stopinfo_sp == nullptr) { + return nullptr; + } + + thread_sp->SetStopInfo(stopinfo_sp); + } + + process_sp->GetThreadList().AddThread(thread_sp); + + return thread_sp; +} + +TEST_F(ProcessEventDataTest, DoOnRemoval) { + ArchSpec arch("x86_64-apple-macosx-"); + + Platform::SetHostPlatform(PlatformMacOSX::CreateInstance(true, &arch)); + + DebuggerSP debugger_sp = Debugger::CreateInstance(); + ASSERT_TRUE(debugger_sp); + + TargetSP target_sp = CreateTarget(debugger_sp, arch); + ASSERT_TRUE(target_sp); + + ListenerSP listener_sp(Listener::MakeListener("dummy")); + ProcessSP process_sp = std::make_shared<DummyProcess>(target_sp, listener_sp); + ASSERT_TRUE(process_sp); + + /* + Should hit ShouldStop if state is eStateStopped + */ + ProcessEventDataSP event_data_sp = + std::make_shared<DummyProcessEventData>(process_sp, eStateStopped); + EventSP event_sp = std::make_shared<Event>(0, event_data_sp); + event_data_sp->SetUpdateStateOnRemoval(event_sp.get()); + event_data_sp->DoOnRemoval(event_sp.get()); + bool result = static_cast<DummyProcessEventData *>(event_data_sp.get()) + ->m_should_stop_hit_count == 1; + ASSERT_TRUE(result); + + /* + Should not hit ShouldStop if state is not eStateStopped + */ + event_data_sp = + std::make_shared<DummyProcessEventData>(process_sp, eStateStepping); + event_sp = std::make_shared<Event>(0, event_data_sp); + event_data_sp->SetUpdateStateOnRemoval(event_sp.get()); + event_data_sp->DoOnRemoval(event_sp.get()); + result = static_cast<DummyProcessEventData *>(event_data_sp.get()) + ->m_should_stop_hit_count == 0; + ASSERT_TRUE(result); +} + +TEST_F(ProcessEventDataTest, ShouldStop) { + ArchSpec arch("x86_64-apple-macosx-"); + + Platform::SetHostPlatform(PlatformMacOSX::CreateInstance(true, &arch)); + + DebuggerSP debugger_sp = Debugger::CreateInstance(); + ASSERT_TRUE(debugger_sp); + + TargetSP target_sp = CreateTarget(debugger_sp, arch); + ASSERT_TRUE(target_sp); + + ListenerSP listener_sp(Listener::MakeListener("dummy")); + ProcessSP process_sp = std::make_shared<DummyProcess>(target_sp, listener_sp); + ASSERT_TRUE(process_sp); + + // wants to stop and has valid StopInfo + ThreadSP thread_sp = CreateThread(process_sp, true, true); + + ProcessEventDataSP event_data_sp = + std::make_shared<Process::ProcessEventData>(process_sp, eStateStopped); + EventSP event_sp = std::make_shared<Event>(0, event_data_sp); + /* + Should stop if thread has valid StopInfo and not suspended + */ + bool has_valid_stopinfo_ptr = false; + bool should_stop = + event_data_sp->ShouldStop(event_sp.get(), &has_valid_stopinfo_ptr); + ASSERT_TRUE(should_stop == true && has_valid_stopinfo_ptr == true); + + /* + Should not stop if thread has valid StopInfo but was suspended + */ + has_valid_stopinfo_ptr = false; + thread_sp->SetResumeState(eStateSuspended); + should_stop = + event_data_sp->ShouldStop(event_sp.get(), &has_valid_stopinfo_ptr); + ASSERT_TRUE(should_stop == false && has_valid_stopinfo_ptr == true); + + /* + Should not stop, thread-reason of stop does not want to stop in its + callback and suspended thread who wants to (from previous stop) + must be ignored. + */ + event_data_sp = + std::make_shared<Process::ProcessEventData>(process_sp, eStateStopped); + event_sp = std::make_shared<Event>(0, event_data_sp); + process_sp->GetThreadList().Clear(); + + for (int i = 0; i < 6; i++) { + if (i == 2) { + // Does not want to stop but has valid StopInfo + thread_sp = CreateThread(process_sp, false, true); + } else if (i == 5) { + // Wants to stop and has valid StopInfo + thread_sp = CreateThread(process_sp, true, true); + thread_sp->SetResumeState(eStateSuspended); + } else { + // Thread has no StopInfo i.e is not the reason of stop + thread_sp = CreateThread(process_sp, false, false); + } + } + ASSERT_TRUE(process_sp->GetThreadList().GetSize() == 6); + + has_valid_stopinfo_ptr = false; + should_stop = + event_data_sp->ShouldStop(event_sp.get(), &has_valid_stopinfo_ptr); + ASSERT_TRUE(should_stop == false && has_valid_stopinfo_ptr == true); +} Index: lldb/unittests/Process/CMakeLists.txt =================================================================== --- lldb/unittests/Process/CMakeLists.txt +++ lldb/unittests/Process/CMakeLists.txt @@ -4,3 +4,17 @@ add_subdirectory(POSIX) endif() add_subdirectory(minidump) + +add_lldb_unittest(ProcessEventDataTest + ProcessEventDataTest.cpp + + LINK_LIBS + lldbCore + lldbHost + lldbTarget + lldbSymbol + lldbUtility + lldbUtilityHelpers + lldbInterpreter + lldbPluginPlatformMacOSX + ) Index: lldb/unittests/CMakeLists.txt =================================================================== --- lldb/unittests/CMakeLists.txt +++ lldb/unittests/CMakeLists.txt @@ -80,6 +80,7 @@ add_subdirectory(tools) add_subdirectory(UnwindAssembly) add_subdirectory(Utility) +add_subdirectory(Thread) if(LLDB_CAN_USE_DEBUGSERVER AND LLDB_TOOL_DEBUGSERVER_BUILD AND NOT LLDB_USE_SYSTEM_DEBUGSERVER) add_subdirectory(debugserver) Index: lldb/source/Target/Process.cpp =================================================================== --- lldb/source/Target/Process.cpp +++ lldb/source/Target/Process.cpp @@ -359,51 +359,141 @@ } static constexpr OptionDefinition g_process_launch_options[] = { - {LLDB_OPT_SET_ALL, false, "stop-at-entry", 's', OptionParser::eNoArgument, - nullptr, {}, 0, eArgTypeNone, + {LLDB_OPT_SET_ALL, + false, + "stop-at-entry", + 's', + OptionParser::eNoArgument, + nullptr, + {}, + 0, + eArgTypeNone, "Stop at the entry point of the program when launching a process."}, - {LLDB_OPT_SET_ALL, false, "disable-aslr", 'A', - OptionParser::eRequiredArgument, nullptr, {}, 0, eArgTypeBoolean, + {LLDB_OPT_SET_ALL, + false, + "disable-aslr", + 'A', + OptionParser::eRequiredArgument, + nullptr, + {}, + 0, + eArgTypeBoolean, "Set whether to disable address space layout randomization when launching " "a process."}, - {LLDB_OPT_SET_ALL, false, "plugin", 'p', OptionParser::eRequiredArgument, - nullptr, {}, 0, eArgTypePlugin, + {LLDB_OPT_SET_ALL, + false, + "plugin", + 'p', + OptionParser::eRequiredArgument, + nullptr, + {}, + 0, + eArgTypePlugin, "Name of the process plugin you want to use."}, - {LLDB_OPT_SET_ALL, false, "working-dir", 'w', - OptionParser::eRequiredArgument, nullptr, {}, 0, + {LLDB_OPT_SET_ALL, + false, + "working-dir", + 'w', + OptionParser::eRequiredArgument, + nullptr, + {}, + 0, eArgTypeDirectoryName, "Set the current working directory to <path> when running the inferior."}, - {LLDB_OPT_SET_ALL, false, "arch", 'a', OptionParser::eRequiredArgument, - nullptr, {}, 0, eArgTypeArchitecture, + {LLDB_OPT_SET_ALL, + false, + "arch", + 'a', + OptionParser::eRequiredArgument, + nullptr, + {}, + 0, + eArgTypeArchitecture, "Set the architecture for the process to launch when ambiguous."}, - {LLDB_OPT_SET_ALL, false, "environment", 'v', - OptionParser::eRequiredArgument, nullptr, {}, 0, eArgTypeNone, + {LLDB_OPT_SET_ALL, + false, + "environment", + 'v', + OptionParser::eRequiredArgument, + nullptr, + {}, + 0, + eArgTypeNone, "Specify an environment variable name/value string (--environment " "NAME=VALUE). Can be specified multiple times for subsequent environment " "entries."}, - {LLDB_OPT_SET_1 | LLDB_OPT_SET_2 | LLDB_OPT_SET_3, false, "shell", 'c', - OptionParser::eOptionalArgument, nullptr, {}, 0, eArgTypeFilename, + {LLDB_OPT_SET_1 | LLDB_OPT_SET_2 | LLDB_OPT_SET_3, + false, + "shell", + 'c', + OptionParser::eOptionalArgument, + nullptr, + {}, + 0, + eArgTypeFilename, "Run the process in a shell (not supported on all platforms)."}, - {LLDB_OPT_SET_1, false, "stdin", 'i', OptionParser::eRequiredArgument, - nullptr, {}, 0, eArgTypeFilename, + {LLDB_OPT_SET_1, + false, + "stdin", + 'i', + OptionParser::eRequiredArgument, + nullptr, + {}, + 0, + eArgTypeFilename, "Redirect stdin for the process to <filename>."}, - {LLDB_OPT_SET_1, false, "stdout", 'o', OptionParser::eRequiredArgument, - nullptr, {}, 0, eArgTypeFilename, + {LLDB_OPT_SET_1, + false, + "stdout", + 'o', + OptionParser::eRequiredArgument, + nullptr, + {}, + 0, + eArgTypeFilename, "Redirect stdout for the process to <filename>."}, - {LLDB_OPT_SET_1, false, "stderr", 'e', OptionParser::eRequiredArgument, - nullptr, {}, 0, eArgTypeFilename, + {LLDB_OPT_SET_1, + false, + "stderr", + 'e', + OptionParser::eRequiredArgument, + nullptr, + {}, + 0, + eArgTypeFilename, "Redirect stderr for the process to <filename>."}, - {LLDB_OPT_SET_2, false, "tty", 't', OptionParser::eNoArgument, nullptr, - {}, 0, eArgTypeNone, + {LLDB_OPT_SET_2, + false, + "tty", + 't', + OptionParser::eNoArgument, + nullptr, + {}, + 0, + eArgTypeNone, "Start the process in a terminal (not supported on all platforms)."}, - {LLDB_OPT_SET_3, false, "no-stdio", 'n', OptionParser::eNoArgument, nullptr, - {}, 0, eArgTypeNone, + {LLDB_OPT_SET_3, + false, + "no-stdio", + 'n', + OptionParser::eNoArgument, + nullptr, + {}, + 0, + eArgTypeNone, "Do not set up for terminal I/O to go to running process."}, - {LLDB_OPT_SET_4, false, "shell-expand-args", 'X', - OptionParser::eRequiredArgument, nullptr, {}, 0, eArgTypeBoolean, + {LLDB_OPT_SET_4, + false, + "shell-expand-args", + 'X', + OptionParser::eRequiredArgument, + nullptr, + {}, + 0, + eArgTypeBoolean, "Set whether to shell expand arguments to the process when launching."}, }; @@ -953,8 +1043,7 @@ process_sp->GetStatus(*stream); process_sp->GetThreadStatus(*stream, only_threads_with_stop_reason, start_frame, num_frames, - num_frames_with_source, - stop_format); + num_frames_with_source, stop_format); if (curr_thread_stop_info_sp) { lldb::addr_t crashing_address; ValueObjectSP valobj_sp = StopInfo::GetCrashingDereference( @@ -1156,7 +1245,7 @@ lldb::pid_t pid, bool exited, int signo, // Zero for no signal int exit_status // Exit value of process if signal is zero - ) { +) { Log *log(lldb_private::GetLogIfAnyCategoriesSet(LIBLLDB_LOG_PROCESS)); LLDB_LOGF(log, "Process::SetProcessExitStatus (pid=%" PRIu64 @@ -1211,7 +1300,7 @@ for (size_t i = 0; i < num_old_threads; ++i) old_thread_list.GetThreadAtIndex(i, false)->ClearBackingThread(); // See if the OS plugin reports all threads. If it does, then - // it is safe to clear unseen thread's plans here. Otherwise we + // it is safe to clear unseen thread's plans here. Otherwise we // should preserve them in case they show up again: clear_unused_threads = GetTarget().GetOSPluginReportsAllThreads(); @@ -1336,9 +1425,7 @@ return result; } -StateType Process::GetState() { - return m_public_state.GetValue(); -} +StateType Process::GetState() { return m_public_state.GetValue(); } void Process::SetPublicState(StateType new_state, bool restarted) { Log *log(lldb_private::GetLogIfAnyCategoriesSet(LIBLLDB_LOG_STATE | @@ -1400,8 +1487,7 @@ return error; } - ListenerSP listener_sp( - Listener::MakeListener(g_resume_sync_name)); + ListenerSP listener_sp(Listener::MakeListener(g_resume_sync_name)); HijackProcessEvents(listener_sp); Status error = PrivateResume(); @@ -1428,8 +1514,7 @@ bool Process::StateChangedIsExternallyHijacked() { if (IsHijackedForEvent(eBroadcastBitStateChanged)) { const char *hijacking_name = GetHijackingListenerName(); - if (hijacking_name && - strcmp(hijacking_name, g_resume_sync_name)) + if (hijacking_name && strcmp(hijacking_name, g_resume_sync_name)) return true; } return false; @@ -1438,8 +1523,7 @@ bool Process::StateChangedIsHijackedForSynchronousResume() { if (IsHijackedForEvent(eBroadcastBitStateChanged)) { const char *hijacking_name = GetHijackingListenerName(); - if (hijacking_name && - strcmp(hijacking_name, g_resume_sync_name) == 0) + if (hijacking_name && strcmp(hijacking_name, g_resume_sync_name) == 0) return true; } return false; @@ -1522,7 +1606,8 @@ const lldb::ABISP &Process::GetABI() { if (!m_abi_sp) - m_abi_sp = ABI::FindPlugin(shared_from_this(), GetTarget().GetArchitecture()); + m_abi_sp = + ABI::FindPlugin(shared_from_this(), GetTarget().GetArchitecture()); return m_abi_sp; } @@ -1569,7 +1654,8 @@ // for example, CPPLanguageRuntime will support eLanguageTypeC_plus_plus, // eLanguageTypeC_plus_plus_03, etc. Because of this, we should get the // primary language type and make sure that our runtime supports it. - assert(runtime->GetLanguageType() == Language::GetPrimaryLanguage(language)); + assert(runtime->GetLanguageType() == + Language::GetPrimaryLanguage(language)); return runtime; } @@ -1988,7 +2074,7 @@ return cache_bytes_read; } return 0; -#else // !defined(VERIFY_MEMORY_READS) +#else // !defined(VERIFY_MEMORY_READS) // Memory caching is enabled, without debug verification return m_memory_cache.Read(addr, buf, size, error); @@ -3626,8 +3712,8 @@ while (!receipt_received) { // Check for a receipt for n seconds and then check if the private // state thread is still around. - receipt_received = - event_receipt_sp->WaitForEventReceived(GetUtilityExpressionTimeout()); + receipt_received = event_receipt_sp->WaitForEventReceived( + GetUtilityExpressionTimeout()); if (!receipt_received) { // Check if the private state thread is still around. If it isn't // then we are done waiting @@ -3940,6 +4026,122 @@ return ProcessEventData::GetFlavorString(); } +bool Process::ProcessEventData::ShouldStop(Event *event_ptr, + bool *have_valid_stopinfo_ptr) { + ProcessSP process_sp(m_process_wp.lock()); + + *have_valid_stopinfo_ptr = false; + ThreadList &curr_thread_list = process_sp->GetThreadList(); + uint32_t num_threads = curr_thread_list.GetSize(); + uint32_t idx; + + // The actions might change one of the thread's stop_info's opinions about + // whether we should stop the process, so we need to query that as we go. + + // One other complication here, is that we try to catch any case where the + // target has run (except for expressions) and immediately exit, but if we + // get that wrong (which is possible) then the thread list might have + // changed, and that would cause our iteration here to crash. We could + // make a copy of the thread list, but we'd really like to also know if it + // has changed at all, so we make up a vector of the thread ID's and check + // what we get back against this list & bag out if anything differs. + ThreadList not_suspended_thread_list(process_sp.get()); + std::vector<uint32_t> thread_index_array(num_threads); + uint32_t not_suspended_idx = 0; + for (idx = 0; idx < num_threads; ++idx) { + lldb::ThreadSP thread_sp = curr_thread_list.GetThreadAtIndex(idx); + + /* + Filter out all suspended threads, they could not be the reason + of stop and no need to perform any actions on them. + */ + if (thread_sp->GetResumeState() != eStateSuspended) { + not_suspended_thread_list.AddThread(thread_sp); + thread_index_array[not_suspended_idx] = thread_sp->GetIndexID(); + not_suspended_idx++; + } else { + /* + For the sake of logical consistancy. For example we have only + one thread that was suspended but has valid StopInfo we get result + not_suspended_thread_list.GetSize() = 0 and must report that we should + not stop but do detected a valid StopInfo. + */ + StopInfoSP stop_info_sp = thread_sp->GetStopInfo(); + if (stop_info_sp && stop_info_sp->IsValid()) { + *have_valid_stopinfo_ptr = true; + } + } + } + + // Use this to track whether we should continue from here. We will only + // continue the target running if no thread says we should stop. Of course + // if some thread's PerformAction actually sets the target running, then it + // doesn't matter what the other threads say... + + bool still_should_stop = false; + + // Sometimes - for instance if we have a bug in the stub we are talking to, + // we stop but no thread has a valid stop reason. In that case we should + // just stop, because we have no way of telling what the right thing to do + // is, and it's better to let the user decide than continue behind their + // backs. + + for (idx = 0; idx < not_suspended_thread_list.GetSize(); ++idx) { + curr_thread_list = process_sp->GetThreadList(); + if (curr_thread_list.GetSize() != num_threads) { + Log *log(lldb_private::GetLogIfAnyCategoriesSet(LIBLLDB_LOG_STEP | + LIBLLDB_LOG_PROCESS)); + LLDB_LOGF( + log, + "Number of threads changed from %u to %u while processing event.", + num_threads, curr_thread_list.GetSize()); + break; + } + + lldb::ThreadSP thread_sp = not_suspended_thread_list.GetThreadAtIndex(idx); + + if (thread_sp->GetIndexID() != thread_index_array[idx]) { + Log *log(lldb_private::GetLogIfAnyCategoriesSet(LIBLLDB_LOG_STEP | + LIBLLDB_LOG_PROCESS)); + LLDB_LOGF(log, + "The thread at position %u changed from %u to %u while " + "processing event.", + idx, thread_index_array[idx], thread_sp->GetIndexID()); + break; + } + + StopInfoSP stop_info_sp = thread_sp->GetStopInfo(); + if (stop_info_sp && stop_info_sp->IsValid()) { + *have_valid_stopinfo_ptr = true; + bool this_thread_wants_to_stop; + if (stop_info_sp->GetOverrideShouldStop()) { + this_thread_wants_to_stop = + stop_info_sp->GetOverriddenShouldStopValue(); + } else { + stop_info_sp->PerformAction(event_ptr); + // The stop action might restart the target. If it does, then we + // want to mark that in the event so that whoever is receiving it + // will know to wait for the running event and reflect that state + // appropriately. We also need to stop processing actions, since they + // aren't expecting the target to be running. + + // FIXME: we might have run. + if (stop_info_sp->HasTargetRunSinceMe()) { + SetRestarted(true); + break; + } + + this_thread_wants_to_stop = stop_info_sp->ShouldStop(event_ptr); + } + + if (!still_should_stop) + still_should_stop = this_thread_wants_to_stop; + } + } + + return still_should_stop; +} + void Process::ProcessEventData::DoOnRemoval(Event *event_ptr) { ProcessSP process_sp(m_process_wp.lock()); @@ -3976,92 +4178,9 @@ // If we're stopped and haven't restarted, then do the StopInfo actions here: if (m_state == eStateStopped && !m_restarted) { - ThreadList &curr_thread_list = process_sp->GetThreadList(); - uint32_t num_threads = curr_thread_list.GetSize(); - uint32_t idx; - - // The actions might change one of the thread's stop_info's opinions about - // whether we should stop the process, so we need to query that as we go. - - // One other complication here, is that we try to catch any case where the - // target has run (except for expressions) and immediately exit, but if we - // get that wrong (which is possible) then the thread list might have - // changed, and that would cause our iteration here to crash. We could - // make a copy of the thread list, but we'd really like to also know if it - // has changed at all, so we make up a vector of the thread ID's and check - // what we get back against this list & bag out if anything differs. - std::vector<uint32_t> thread_index_array(num_threads); - for (idx = 0; idx < num_threads; ++idx) - thread_index_array[idx] = - curr_thread_list.GetThreadAtIndex(idx)->GetIndexID(); - - // Use this to track whether we should continue from here. We will only - // continue the target running if no thread says we should stop. Of course - // if some thread's PerformAction actually sets the target running, then it - // doesn't matter what the other threads say... - - bool still_should_stop = false; - - // Sometimes - for instance if we have a bug in the stub we are talking to, - // we stop but no thread has a valid stop reason. In that case we should - // just stop, because we have no way of telling what the right thing to do - // is, and it's better to let the user decide than continue behind their - // backs. - bool does_anybody_have_an_opinion = false; - - for (idx = 0; idx < num_threads; ++idx) { - curr_thread_list = process_sp->GetThreadList(); - if (curr_thread_list.GetSize() != num_threads) { - Log *log(lldb_private::GetLogIfAnyCategoriesSet(LIBLLDB_LOG_STEP | - LIBLLDB_LOG_PROCESS)); - LLDB_LOGF( - log, - "Number of threads changed from %u to %u while processing event.", - num_threads, curr_thread_list.GetSize()); - break; - } - - lldb::ThreadSP thread_sp = curr_thread_list.GetThreadAtIndex(idx); - - if (thread_sp->GetIndexID() != thread_index_array[idx]) { - Log *log(lldb_private::GetLogIfAnyCategoriesSet(LIBLLDB_LOG_STEP | - LIBLLDB_LOG_PROCESS)); - LLDB_LOGF(log, - "The thread at position %u changed from %u to %u while " - "processing event.", - idx, thread_index_array[idx], thread_sp->GetIndexID()); - break; - } - - StopInfoSP stop_info_sp = thread_sp->GetStopInfo(); - if (stop_info_sp && stop_info_sp->IsValid()) { - does_anybody_have_an_opinion = true; - bool this_thread_wants_to_stop; - if (stop_info_sp->GetOverrideShouldStop()) { - this_thread_wants_to_stop = - stop_info_sp->GetOverriddenShouldStopValue(); - } else { - stop_info_sp->PerformAction(event_ptr); - // The stop action might restart the target. If it does, then we - // want to mark that in the event so that whoever is receiving it - // will know to wait for the running event and reflect that state - // appropriately. We also need to stop processing actions, since they - // aren't expecting the target to be running. - - // FIXME: we might have run. - if (stop_info_sp->HasTargetRunSinceMe()) { - SetRestarted(true); - break; - } - - this_thread_wants_to_stop = stop_info_sp->ShouldStop(event_ptr); - } - - if (!still_should_stop) - still_should_stop = this_thread_wants_to_stop; - } - } + bool still_should_stop = + ShouldStop(event_ptr, &does_anybody_have_an_opinion); if (!GetRestarted()) { if (!still_should_stop && does_anybody_have_an_opinion) { @@ -4084,11 +4203,11 @@ process_sp->GetTarget().RunStopHooks(); if (process_sp->GetPrivateState() == eStateRunning) SetRestarted(true); + } } } } } -} void Process::ProcessEventData::Dump(Stream *s) const { ProcessSP process_sp(m_process_wp.lock()); @@ -4603,7 +4722,8 @@ HandleStoppedEvent(Thread &thread, const ThreadPlanSP &thread_plan_sp, RestorePlanState &restorer, const EventSP &event_sp, EventSP &event_to_broadcast_sp, - const EvaluateExpressionOptions &options, bool handle_interrupts) { + const EvaluateExpressionOptions &options, + bool handle_interrupts) { Log *log = GetLogIfAnyCategoriesSet(LIBLLDB_LOG_STEP | LIBLLDB_LOG_PROCESS); ThreadPlanSP plan = thread.GetCompletedPlan(); @@ -4619,7 +4739,8 @@ StopInfoSP stop_info_sp = thread.GetStopInfo(); if (stop_info_sp && stop_info_sp->GetStopReason() == eStopReasonBreakpoint && stop_info_sp->ShouldNotify(event_sp.get())) { - LLDB_LOG(log, "stopped for breakpoint: {0}.", stop_info_sp->GetDescription()); + LLDB_LOG(log, "stopped for breakpoint: {0}.", + stop_info_sp->GetDescription()); if (!options.DoesIgnoreBreakpoints()) { // Restore the plan state and then force Private to false. We are going // to stop because of this plan so we need it to become a public plan or @@ -4866,8 +4987,8 @@ // coalescing to cause us to lose OUR running event... ForceNextEventDelivery(); -// This while loop must exit out the bottom, there's cleanup that we need to do -// when we are done. So don't call return anywhere within it. + // This while loop must exit out the bottom, there's cleanup that we need to + // do when we are done. So don't call return anywhere within it. #ifdef LLDB_RUN_THREAD_HALT_WITH_EVENT // It's pretty much impossible to write test cases for things like: One @@ -5101,12 +5222,14 @@ LLDB_LOG(log, "Running function with one thread timeout timed out."); } else - LLDB_LOG(log, "Restarting function with all threads enabled and " - "timeout: {0} timed out, abandoning execution.", + LLDB_LOG(log, + "Restarting function with all threads enabled and " + "timeout: {0} timed out, abandoning execution.", timeout); } else - LLDB_LOG(log, "Running function with timeout: {0} timed out, " - "abandoning execution.", + LLDB_LOG(log, + "Running function with timeout: {0} timed out, " + "abandoning execution.", timeout); } @@ -5519,8 +5642,7 @@ continue; } thread_sp->GetStatus(strm, start_frame, num_frames, - num_frames_with_source, - stop_format); + num_frames_with_source, stop_format); ++num_thread_infos_dumped; } else { Log *log(lldb_private::GetLogIfAnyCategoriesSet(LIBLLDB_LOG_PROCESS)); @@ -5559,14 +5681,14 @@ void Process::ClearPreResumeActions() { m_pre_resume_actions.clear(); } -void Process::ClearPreResumeAction(PreResumeActionCallback callback, void *baton) -{ - PreResumeCallbackAndBaton element(callback, baton); - auto found_iter = std::find(m_pre_resume_actions.begin(), m_pre_resume_actions.end(), element); - if (found_iter != m_pre_resume_actions.end()) - { - m_pre_resume_actions.erase(found_iter); - } +void Process::ClearPreResumeAction(PreResumeActionCallback callback, + void *baton) { + PreResumeCallbackAndBaton element(callback, baton); + auto found_iter = std::find(m_pre_resume_actions.begin(), + m_pre_resume_actions.end(), element); + if (found_iter != m_pre_resume_actions.end()) { + m_pre_resume_actions.erase(found_iter); + } } ProcessRunLock &Process::GetRunLock() { @@ -5576,12 +5698,10 @@ return m_public_run_lock; } -bool Process::CurrentThreadIsPrivateStateThread() -{ +bool Process::CurrentThreadIsPrivateStateThread() { return m_private_state_thread.EqualsThread(Host::GetCurrentThread()); } - void Process::Flush() { m_thread_list.Flush(); m_extended_thread_list.Flush(); @@ -5837,10 +5957,8 @@ return retval; } - uint32_t branch_index = - insn_list->GetIndexOfNextBranchInstruction(insn_offset, target, - false /* ignore_calls*/, - nullptr); + uint32_t branch_index = insn_list->GetIndexOfNextBranchInstruction( + insn_offset, target, false /* ignore_calls*/, nullptr); if (branch_index == UINT32_MAX) { return retval; } @@ -5857,8 +5975,7 @@ return retval; } -Status -Process::GetMemoryRegions(lldb_private::MemoryRegionInfos ®ion_list) { +Status Process::GetMemoryRegions(lldb_private::MemoryRegionInfos ®ion_list) { Status error; @@ -5936,8 +6053,8 @@ // supports? for (uint32_t plugin_index = 0; !const_type_names.empty(); plugin_index++) { auto create_instance = - PluginManager::GetStructuredDataPluginCreateCallbackAtIndex( - plugin_index); + PluginManager::GetStructuredDataPluginCreateCallbackAtIndex( + plugin_index); if (!create_instance) break; Index: lldb/include/lldb/Target/Process.h =================================================================== --- lldb/include/lldb/Target/Process.h +++ lldb/include/lldb/Target/Process.h @@ -402,11 +402,11 @@ return GetStaticBroadcasterClass(); } -/// A notification structure that can be used by clients to listen -/// for changes in a process's lifetime. -/// -/// \see RegisterNotificationCallbacks (const Notifications&) @see -/// UnregisterNotificationCallbacks (const Notifications&) + /// A notification structure that can be used by clients to listen + /// for changes in a process's lifetime. + /// + /// \see RegisterNotificationCallbacks (const Notifications&) @see + /// UnregisterNotificationCallbacks (const Notifications&) typedef struct { void *baton; void (*initialize)(void *baton, Process *process); @@ -444,6 +444,8 @@ void Dump(Stream *s) const override; + virtual bool ShouldStop(Event *event_ptr, bool *have_valid_stopinfo_ptr); + void DoOnRemoval(Event *event_ptr) override; static const Process::ProcessEventData * @@ -760,32 +762,32 @@ /// public clients. virtual void WillPublicStop() {} -/// Register for process and thread notifications. -/// -/// Clients can register notification callbacks by filling out a -/// Process::Notifications structure and calling this function. -/// -/// \param[in] callbacks -/// A structure that contains the notification baton and -/// callback functions. -/// -/// \see Process::Notifications + /// Register for process and thread notifications. + /// + /// Clients can register notification callbacks by filling out a + /// Process::Notifications structure and calling this function. + /// + /// \param[in] callbacks + /// A structure that contains the notification baton and + /// callback functions. + /// + /// \see Process::Notifications void RegisterNotificationCallbacks(const Process::Notifications &callbacks); -/// Unregister for process and thread notifications. -/// -/// Clients can unregister notification callbacks by passing a copy of the -/// original baton and callbacks in \a callbacks. -/// -/// \param[in] callbacks -/// A structure that contains the notification baton and -/// callback functions. -/// -/// \return -/// Returns \b true if the notification callbacks were -/// successfully removed from the process, \b false otherwise. -/// -/// \see Process::Notifications + /// Unregister for process and thread notifications. + /// + /// Clients can unregister notification callbacks by passing a copy of the + /// original baton and callbacks in \a callbacks. + /// + /// \param[in] callbacks + /// A structure that contains the notification baton and + /// callback functions. + /// + /// \return + /// Returns \b true if the notification callbacks were + /// successfully removed from the process, \b false otherwise. + /// + /// \see Process::Notifications bool UnregisterNotificationCallbacks(const Process::Notifications &callbacks); //================================================================== @@ -1234,8 +1236,7 @@ size_t GetThreadStatus(Stream &ostrm, bool only_threads_with_stop_reason, uint32_t start_frame, uint32_t num_frames, - uint32_t num_frames_with_source, - bool stop_format); + uint32_t num_frames_with_source, bool stop_format); void SendAsyncInterrupt(); @@ -1778,8 +1779,7 @@ /// /// \return /// An error value. - virtual Status - GetMemoryRegions(lldb_private::MemoryRegionInfos ®ion_list); + virtual Status GetMemoryRegions(lldb_private::MemoryRegionInfos ®ion_list); virtual Status GetWatchpointSupportInfo(uint32_t &num) { Status error; @@ -2199,17 +2199,17 @@ void SetDynamicCheckers(DynamicCheckerFunctions *dynamic_checkers); -/// Prune ThreadPlanStacks for unreported threads. -/// -/// \param[in] tid -/// The tid whose Plan Stack we are seeking to prune. -/// -/// \return -/// \b true if the TID is found or \b false if not. -bool PruneThreadPlansForTID(lldb::tid_t tid); + /// Prune ThreadPlanStacks for unreported threads. + /// + /// \param[in] tid + /// The tid whose Plan Stack we are seeking to prune. + /// + /// \return + /// \b true if the TID is found or \b false if not. + bool PruneThreadPlansForTID(lldb::tid_t tid); -/// Prune ThreadPlanStacks for all unreported threads. -void PruneThreadPlans(); + /// Prune ThreadPlanStacks for all unreported threads. + void PruneThreadPlans(); /// Find the thread plan stack associated with thread with \a tid. /// @@ -2694,7 +2694,7 @@ PreResumeCallbackAndBaton(PreResumeActionCallback in_callback, void *in_baton) : callback(in_callback), baton(in_baton) {} - bool operator== (const PreResumeCallbackAndBaton &rhs) { + bool operator==(const PreResumeCallbackAndBaton &rhs) { return callback == rhs.callback && baton == rhs.baton; } }; @@ -2717,46 +2717,46 @@ lldb::ListenerSP m_private_state_listener_sp; // This is the listener for the // private state thread. HostThread m_private_state_thread; ///< Thread ID for the thread that watches - ///internal state events + /// internal state events ProcessModID m_mod_id; ///< Tracks the state of the process over stops and - ///other alterations. + /// other alterations. uint32_t m_process_unique_id; ///< Each lldb_private::Process class that is - ///created gets a unique integer ID that - ///increments with each new instance - uint32_t m_thread_index_id; ///< Each thread is created with a 1 based index - ///that won't get re-used. + /// created gets a unique integer ID that + /// increments with each new instance + uint32_t m_thread_index_id; ///< Each thread is created with a 1 based index + /// that won't get re-used. std::map<uint64_t, uint32_t> m_thread_id_to_index_id_map; int m_exit_status; ///< The exit status of the process, or -1 if not set. std::string m_exit_string; ///< A textual description of why a process exited. std::mutex m_exit_status_mutex; ///< Mutex so m_exit_status m_exit_string can - ///be safely accessed from multiple threads + /// be safely accessed from multiple threads std::recursive_mutex m_thread_mutex; ThreadList m_thread_list_real; ///< The threads for this process as are known - ///to the protocol we are debugging with + /// to the protocol we are debugging with ThreadList m_thread_list; ///< The threads for this process as the user will - ///see them. This is usually the same as + /// see them. This is usually the same as ///< m_thread_list_real, but might be different if there is an OS plug-in - ///creating memory threads + /// creating memory threads ThreadPlanStackMap m_thread_plans; ///< This is the list of thread plans for /// threads in m_thread_list, as well as /// threads we knew existed, but haven't /// determined that they have died yet. ThreadList m_extended_thread_list; ///< Owner for extended threads that may be - ///generated, cleared on natural stops + /// generated, cleared on natural stops uint32_t m_extended_thread_stop_id; ///< The natural stop id when - ///extended_thread_list was last updated + /// extended_thread_list was last updated QueueList m_queue_list; ///< The list of libdispatch queues at a given stop point uint32_t m_queue_list_stop_id; ///< The natural stop id when queue list was - ///last fetched + /// last fetched std::vector<Notifications> m_notifications; ///< The list of notifications - ///that this process can deliver. + /// that this process can deliver. std::vector<lldb::addr_t> m_image_tokens; lldb::ListenerSP m_listener_sp; ///< Shared pointer to the listener used for - ///public events. Can not be empty. + /// public events. Can not be empty. BreakpointSiteList m_breakpoint_site_list; ///< This is the list of breakpoint - ///locations we intend to insert in - ///the target. + /// locations we intend to insert + /// in the target. lldb::DynamicLoaderUP m_dyld_up; lldb::JITLoaderListUP m_jit_loaders_up; lldb::DynamicCheckerFunctionsUP m_dynamic_checkers_up; ///< The functions used
_______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
