llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT--> @llvm/pr-subscribers-lldb Author: None (jeffreytan81) <details> <summary>Changes</summary> We got user reporting lldb crash while the debuggee is calling vfork() concurrently from multiple threads. The crash happens because the current implementation can only handle single vfork, vforkdone protocol transaction. This diff fixes the crash by lldb-server storing forked debuggee's <pid, tid> pair in jstopinfo which will be decoded by lldb client to create StopInfoVFork for follow parent/child policy. Each StopInfoVFork will later have a corresponding vforkdone packet. So the patch also changes the `m_vfork_in_progress` to be reference counting based. Two new test cases are added which crash/assert without the changes in this patch. --- Full diff: https://github.com/llvm/llvm-project/pull/81564.diff 6 Files Affected: - (modified) lldb/source/Plugins/Process/Linux/NativeThreadLinux.cpp (+11-1) - (modified) lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp (+15-9) - (modified) lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.h (+2-1) - (added) lldb/test/API/functionalities/fork/concurrent_vfork/Makefile (+4) - (added) lldb/test/API/functionalities/fork/concurrent_vfork/TestConcurrentVFork.py (+31) - (added) lldb/test/API/functionalities/fork/concurrent_vfork/main.cpp (+45) ``````````diff diff --git a/lldb/source/Plugins/Process/Linux/NativeThreadLinux.cpp b/lldb/source/Plugins/Process/Linux/NativeThreadLinux.cpp index b62e9f643fa792..cf8a1e7d34392a 100644 --- a/lldb/source/Plugins/Process/Linux/NativeThreadLinux.cpp +++ b/lldb/source/Plugins/Process/Linux/NativeThreadLinux.cpp @@ -120,7 +120,7 @@ bool NativeThreadLinux::GetStopReason(ThreadStopInfo &stop_info, case eStateCrashed: case eStateExited: case eStateSuspended: - case eStateUnloaded: + case eStateUnloaded: { if (log) LogThreadStopInfo(*log, m_stop_info, "m_stop_info in thread:"); stop_info = m_stop_info; @@ -128,7 +128,17 @@ bool NativeThreadLinux::GetStopReason(ThreadStopInfo &stop_info, if (log) LogThreadStopInfo(*log, stop_info, "returned stop_info:"); + // Include child process PID/TID for forks. + // Client expects "<fork_pid> <fork_tid>" format. + if (stop_info.reason == eStopReasonFork || + stop_info.reason == eStopReasonVFork) { + description = std::to_string(stop_info.details.fork.child_pid); + description += " "; + description += std::to_string(stop_info.details.fork.child_tid); + } + return true; + } case eStateInvalid: case eStateConnected: diff --git a/lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp b/lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp index 629b191f3117aa..6fdb062e712c78 100644 --- a/lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp +++ b/lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp @@ -263,10 +263,9 @@ ProcessGDBRemote::ProcessGDBRemote(lldb::TargetSP target_sp, m_continue_C_tids(), m_continue_s_tids(), m_continue_S_tids(), m_max_memory_size(0), m_remote_stub_max_memory_size(0), m_addr_to_mmap_size(), m_thread_create_bp_sp(), - m_waiting_for_attach(false), - m_command_sp(), m_breakpoint_pc_offset(0), + m_waiting_for_attach(false), m_command_sp(), m_breakpoint_pc_offset(0), m_initial_tid(LLDB_INVALID_THREAD_ID), m_allow_flash_writes(false), - m_erased_flash_ranges(), m_vfork_in_progress(false) { + m_erased_flash_ranges(), m_vfork_in_progress(0) { m_async_broadcaster.SetEventName(eBroadcastBitAsyncThreadShouldExit, "async thread should exit"); m_async_broadcaster.SetEventName(eBroadcastBitAsyncContinue, @@ -5615,8 +5614,12 @@ void ProcessGDBRemote::DidFork(lldb::pid_t child_pid, lldb::tid_t child_tid) { void ProcessGDBRemote::DidVFork(lldb::pid_t child_pid, lldb::tid_t child_tid) { Log *log = GetLog(GDBRLog::Process); - assert(!m_vfork_in_progress); - m_vfork_in_progress = true; + LLDB_LOG( + log, + "ProcessGDBRemote::DidFork() called for child_pid: {0}, child_tid {1}", + child_pid, child_tid); + assert(m_vfork_in_progress >= 0); + ++m_vfork_in_progress; // Disable all software breakpoints for the duration of vfork. if (m_gdb_comm.SupportsGDBStoppointPacket(eBreakpointSoftware)) @@ -5670,8 +5673,8 @@ void ProcessGDBRemote::DidVFork(lldb::pid_t child_pid, lldb::tid_t child_tid) { } void ProcessGDBRemote::DidVForkDone() { - assert(m_vfork_in_progress); - m_vfork_in_progress = false; + --m_vfork_in_progress; + assert(m_vfork_in_progress >= 0); // Reenable all software breakpoints that were enabled before vfork. if (m_gdb_comm.SupportsGDBStoppointPacket(eBreakpointSoftware)) @@ -5681,7 +5684,10 @@ void ProcessGDBRemote::DidVForkDone() { void ProcessGDBRemote::DidExec() { // If we are following children, vfork is finished by exec (rather than // vforkdone that is submitted for parent). - if (GetFollowForkMode() == eFollowChild) - m_vfork_in_progress = false; + if (GetFollowForkMode() == eFollowChild) { + if (m_vfork_in_progress > 0) + --m_vfork_in_progress; + assert(m_vfork_in_progress >= 0); + } Process::DidExec(); } diff --git a/lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.h b/lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.h index c1ea1cc7905587..29ed770c1275ea 100644 --- a/lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.h +++ b/lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.h @@ -301,7 +301,8 @@ class ProcessGDBRemote : public Process, using FlashRange = FlashRangeVector::Entry; FlashRangeVector m_erased_flash_ranges; - bool m_vfork_in_progress; + // Number of vfork in process. + int m_vfork_in_progress; // Accessors bool IsRunning(lldb::StateType state) { diff --git a/lldb/test/API/functionalities/fork/concurrent_vfork/Makefile b/lldb/test/API/functionalities/fork/concurrent_vfork/Makefile new file mode 100644 index 00000000000000..c46619c6623481 --- /dev/null +++ b/lldb/test/API/functionalities/fork/concurrent_vfork/Makefile @@ -0,0 +1,4 @@ +CXX_SOURCES := main.cpp +ENABLE_THREADS := YES + +include Makefile.rules diff --git a/lldb/test/API/functionalities/fork/concurrent_vfork/TestConcurrentVFork.py b/lldb/test/API/functionalities/fork/concurrent_vfork/TestConcurrentVFork.py new file mode 100644 index 00000000000000..fcd26d6f936850 --- /dev/null +++ b/lldb/test/API/functionalities/fork/concurrent_vfork/TestConcurrentVFork.py @@ -0,0 +1,31 @@ +""" +Make sure that the concurrent vfork() from multiple threads works correctly. +""" + + +import lldb +import lldbsuite.test.lldbutil as lldbutil +from lldbsuite.test.lldbtest import * +from lldbsuite.test.decorators import * + + +class TestConcurrentVFork(TestBase): + NO_DEBUG_INFO_TESTCASE = True + + @skipIfWindows + def test_vfork_follow_parent(self): + self.build() + lldbutil.run_to_source_breakpoint( + self, "// break here", lldb.SBFileSpec("main.cpp") + ) + self.runCmd("settings set target.process.follow-fork-mode parent") + self.expect("continue", substrs=["exited with status = 0"]) + + @skipIfWindows + def test_vfork_follow_child(self): + self.build() + lldbutil.run_to_source_breakpoint( + self, "// break here", lldb.SBFileSpec("main.cpp") + ) + self.runCmd("settings set target.process.follow-fork-mode child") + self.expect("continue", substrs=["exited with status = 0"]) diff --git a/lldb/test/API/functionalities/fork/concurrent_vfork/main.cpp b/lldb/test/API/functionalities/fork/concurrent_vfork/main.cpp new file mode 100644 index 00000000000000..1b75852c3164d0 --- /dev/null +++ b/lldb/test/API/functionalities/fork/concurrent_vfork/main.cpp @@ -0,0 +1,45 @@ +#include <iostream> +#include <thread> +#include <unistd.h> +#include <vector> + +int call_vfork() { + printf("Before vfork\n"); + + pid_t child_pid = vfork(); + + if (child_pid == -1) { + // Error handling + perror("vfork"); + return 1; + } else if (child_pid == 0) { + // This code is executed by the child process + printf("Child process\n"); + _exit(0); // Exit the child process + } else { + // This code is executed by the parent process + printf("Parent process\n"); + } + + printf("After vfork\n"); + return 0; +} + +void worker_thread() { call_vfork(); } + +void create_threads(int num_threads) { + std::vector<std::thread> threads; + for (int i = 0; i < num_threads; ++i) { + threads.emplace_back(std::thread(worker_thread)); + } + printf("Created %d threads, joining...\n", + num_threads); // end_of_create_threads + for (auto &thread : threads) { + thread.join(); + } +} + +int main() { + int num_threads = 5; // break here + create_threads(num_threads); +} `````````` </details> https://github.com/llvm/llvm-project/pull/81564 _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
