Fix lldb crash while handling concurrent vfork() #81564

jeffreytan81 · 2024-02-13T02:09:17Z

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.

github-actions · 2024-02-13T02:11:34Z

✅ With the latest revision this PR passed the C/C++ code formatter.

llvmbot · 2024-02-13T19:59:40Z

@llvm/pr-subscribers-lldb

Author: None (jeffreytan81)

Changes

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 --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);
+}

clayborg · 2024-02-13T20:26:56Z

lldb/source/Plugins/Process/Linux/NativeThreadLinux.cpp

+      description = std::to_string(stop_info.details.fork.child_pid);
+      description += " ";
+      description += std::to_string(stop_info.details.fork.child_tid);


Why are we adding it here and not changing the original "m_stop_description" to contain it? We want everyone to get all of the information.

If this is a human readable string, we should probably print out something like:

child-pid = 12345, child-tid = 345645

instead of two magic numbers separate by a space.

Or is this output used in the GDB remote packets somehow? Don't we need to add the child pid and tid to the JSON stop info for the other threads in a key/value pair encoding?

clayborg · 2024-02-13T20:31:46Z

lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp

+  --m_vfork_in_progress;
+  assert(m_vfork_in_progress >= 0);


assert(m_vfork_in_progress > 0); --m_vfork_in_progress;

clayborg · 2024-02-13T20:32:58Z

lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp

+  if (GetFollowForkMode() == eFollowChild) {
+    if (m_vfork_in_progress > 0)
+      --m_vfork_in_progress;
+    assert(m_vfork_in_progress >= 0);


This assert does nothing as you won't decrement it if it is not > 0. Remove

clayborg · 2024-02-13T20:33:52Z

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;


Do we really want a number that can go below zero? Doesn't make sense. I would store this as a uint32_t and never let it get decremented if it is zero.

clayborg · 2024-02-13T20:36:11Z

lldb/test/API/functionalities/fork/concurrent_vfork/TestConcurrentVFork.py

+    NO_DEBUG_INFO_TESTCASE = True
+
+    @skipIfWindows
+    def test_vfork_follow_parent(self):


Can you document what you are testing for here a bit more? I know what you are doing, but others might not.

clayborg · 2024-02-13T20:37:01Z

lldb/test/API/functionalities/fork/concurrent_vfork/TestConcurrentVFork.py

+            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"])


Can we check for a output from our process that matches "Parent process" to verify it worked as expected?

clayborg · 2024-02-13T20:37:23Z

lldb/test/API/functionalities/fork/concurrent_vfork/TestConcurrentVFork.py

+            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"])


Can we check for a output from our process that matches "Child process" to verify it worked as expected?

clayborg · 2024-02-13T20:41:52Z

lldb/test/API/functionalities/fork/concurrent_vfork/main.cpp

+#include <thread>
+#include <unistd.h>
+#include <vector>
+


Might be a good idea to create a global std::vector<pid_t> and a mutex to protect it. And any child processes that get forked get added to this vector. So maybe add:

std::mutex g_child_pids_mutex; std::vector<pid_t> g_child_pids;

I will comment below where to use it.

clayborg · 2024-02-13T20:43:18Z

lldb/test/API/functionalities/fork/concurrent_vfork/main.cpp

+    _exit(0); // Exit the child process
+  } else {
+    // This code is executed by the parent process
+    printf("Parent process\n");


Add the pid to the global vector:

printf("Parent process\n"); std::lock_guard<std::mutex> Lock(g_child_pids_mutex); g_child_pids.append(child_pid);

clayborg · 2024-02-13T20:53:32Z

lldb/test/API/functionalities/fork/concurrent_vfork/main.cpp

+int main() {
+  int num_threads = 5; // break here
+  create_threads(num_threads);
+}


Now reap the children you spawned:

std::lock_guard<std::mutex> Lock(g_child_pids_mutex); for (pid_t child_pid: g_child_pids) { int child_status = 0; pid_t pid = waitpid(child_pid, &child_status, 0); if (child_status != 0) _exit(1); // This will let our program know that some child processes didn't exist with a zero exit status. if (pid != child_pid) _exit(2); // This will let our program know it didn't succeed }

This will ensure that if we stayed with the parent, then we successfully resumed all of the child processes and that they didn't crash and that they exited. If we don't resume the child processes correctly, they can get caught in limbo if they weren't resumed and this test will deadlock waiting for the child process to exit. There are some options you can do that allows you to not hang where instead of zero passed as the last parameter of waitpid(), you can specify WNOHANG, but that will get racy. We would poll for a few seconds. We will want to make sure that the test doesn't just deadlock and cause the test suite to never complete.

github-actions · 2024-02-14T19:44:49Z

✅ With the latest revision this PR passed the Python code formatter.

clayborg · 2024-02-20T20:11:37Z

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.


// Number of fork() or vfork() operations being handled.

This is only used by vfork() not fork() though.

clayborg · 2024-02-20T20:26:45Z

lldb/source/Plugins/Process/Linux/NativeThreadLinux.cpp

+    // Include child process PID/TID for forks.
+    // Client expects "<fork_pid> <fork_tid>" format for parsing.
+    if (stop_info.reason == eStopReasonFork ||
+        stop_info.reason == eStopReasonVFork) {
+      m_stop_description = std::to_string(stop_info.details.fork.child_pid);
+      m_stop_description += " ";
+      m_stop_description += std::to_string(stop_info.details.fork.child_tid);
+    }


We should be doing this work in:

void NativeThreadLinux::SetStoppedByFork(bool is_vfork, lldb::pid_t child_pid);

clayborg · 2024-02-23T17:11:16Z

lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp

+      log,
+      "ProcessGDBRemote::DidFork() called for child_pid: {0}, child_tid {1}",
+      child_pid, child_tid);
+  assert(m_vfork_in_progress >= 0);


remove this assert now that m_vfork_in_progress in unsigned, it doesn't do anything.

clayborg · 2024-02-23T17:13:54Z

lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp

+  assert(m_vfork_in_progress_count > 0);
+  if (m_vfork_in_progress_count > 0)
+    --m_vfork_in_progress_count;


We decrement m_vfork_in_progress_count in two places, here and in ProcessGDBRemote::DidExec(). Before this wouldn't affect anything because it was a boolean, but I fear we will decrement this twice now. We probably need to add an exec() into our test case for this to ensure this assertion doesn't fire as I believe the assertion inside of ProcessGDBRemote::DidExec() will assert and crash.

clayborg · 2024-02-23T17:17:32Z

lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp

+    assert(m_vfork_in_progress_count > 0);
+    if (m_vfork_in_progress_count > 0)
+      --m_vfork_in_progress_count;


We need to verify this is needed here. Usually someone does a fork() or vfork() and then they do an exec*() call (there are many different flavors of exec:

int execl(const char *path, const char *arg0, ..., /*, (char *)0, */); int execle(const char *path, const char *arg0, ..., /* (char *)0 char *const envp[] */); int execlp(const char *file, const char *arg0, ..., /*, (char *)0, */); int execv(const char *path, char *const argv[]); int execvp(const char *file, char *const argv[]); int execvP(const char *file, const char *search_path, char *const argv[]);

So we need to verify in our test case that if we do a fork() + exec*() or vfork() + exec*() call that we don't run both ProcessGDBRemote::DidVForkDone() and ProcessGDBRemote::DidExec() because if we do this assertion will fire.

clayborg · 2024-02-23T17:18:58Z

lldb/test/API/functionalities/fork/concurrent_vfork/main.cpp

+std::mutex g_child_pids_mutex;
+std::vector<pid_t> g_child_pids;
+
+int call_vfork(int index) {


We probably want to test with both fork() and vfork() here and we want to know if we want to call exec*() or not. If we add a const char *exec_path to the call and this is not NULL, then we call exec. So maybe change this to be:

int call_vfork(int index, bool use_vfork, const char *exec_path)

clayborg · 2024-02-23T17:19:53Z

lldb/test/API/functionalities/fork/concurrent_vfork/main.cpp

+std::vector<pid_t> g_child_pids;
+
+int call_vfork(int index) {
+  pid_t child_pid = vfork();


Use either fork() of vfork() depending on the value of the use_vfork argument:

pid_t child_pid = use_vfork ? vfork() : fork();

clayborg · 2024-02-23T17:33:10Z

lldb/test/API/functionalities/fork/concurrent_vfork/main.cpp

+int main() {
+  g_pid = getpid();
+  printf("Entering main() pid: %d\n", g_pid);
+


parse args manually to check if we should use fork or vfork and if we should exec:

bool use_vfork = true; bool call_exec = false; for (int i=1; i<argc; ++i) { if (strcmp(argv[i], "--fork") == 0) use_vfork = false; else if (strcmp(argv[i], "--exec") == 0) call_exec = true; }

clayborg · 2024-02-23T17:34:39Z

lldb/test/API/functionalities/fork/concurrent_vfork/main.cpp

+  printf("Entering main() pid: %d\n", g_pid);
+
+  int num_threads = 5; // break here
+  create_threads(num_threads);


pass use_vfork and exec_path into this function so we can pass it to int call_vfork(int index, bool use_vfork, const char *exec_path):

create_threads(num_threads, use_vfork, call_exec ? argv[0] : nullptr);

clayborg · 2024-02-23T17:46:28Z

lldb/test/API/functionalities/fork/concurrent_vfork/TestConcurrentVFork.py

+        return target.FindFirstGlobalVariable("g_pid").GetValueAsUnsigned()
+
+    @skipIfWindows
+    def test_vfork_follow_parent(self):


We need to test this with exec and without exec to make sure things work without asserts firing from decrementing the fork count. So this function should be:

def vfork_follow_parent(self, call_exec):

Then we wall this twice:

def test_vfork_follow_parent_no_exec(self): return self.vfork_follow_parent(False); def test_vfork_follow_parent_with_exec(self): return self.vfork_follow_parent(True);

clayborg · 2024-02-23T17:55:00Z

lldb/test/API/functionalities/fork/concurrent_vfork/TestConcurrentVFork.py

+        And follow-parent successfully detach all child processes and exit debugger.
+        """
+
+        self.build()


We need to add arguments to this to specify if we are calling vfork or fork and if we are calling exec.

We should make a helper function that calls lldbutil.run_to_source_breakpoint so it can be used correctly with the right arguemnts getting to the program we launch:

def run_to_breakpoint(self, use_fork, call_exec): args = [self.getBuildArtifact("a.out")] if use_fork: args.append("--fork"); if call_exec: args.append("--exec"); launch_info = lldb.SBLaunchInfo(args) launch_info.SetWorkingDirectory(self.getBuildDir()) lldbutil.run_to_source_breakpoint( self, "// break here", lldb.SBFileSpec("main.cpp"), launch_info=launch_info )

Then we can call this function from each of the variations we need:

def test_vfork_follow_parent_no_exec(self): def test_vfork_follow_parent_with_exec(self): def test_vfork_follow_child_no_exec(self): def test_vfork_follow_child_with_exec(self):

clayborg · 2024-02-23T17:57:10Z

lldb/test/API/functionalities/fork/concurrent_vfork/TestConcurrentVFork.py

+        lldbutil.run_to_source_breakpoint(
+            self, "// break here", lldb.SBFileSpec("main.cpp")
+        )


This will become:

use_fork = False # Call `vfork()` call_exec = False # Don't call `exec()` self.run_to_breakpoint(use_fork, call_exec);

And all of the other variations will need to set use_fork and call_exec correctly.

clayborg · 2024-03-05T23:58:22Z

lldb/test/API/functionalities/fork/concurrent_vfork/TestConcurrentVFork.py

+        launch_info = lldb.SBLaunchInfo(args)
+        launch_info.SetWorkingDirectory(self.getBuildDir())
+
+        lldbutil.run_to_source_breakpoint(


Get the target, process, thread and breakpoint from the return values:

(target, process, thread, bkpt) = lldbutil.run_to_source_breakpoint(

clayborg · 2024-03-05T23:59:17Z

lldb/test/API/functionalities/fork/concurrent_vfork/TestConcurrentVFork.py

+    NO_DEBUG_INFO_TESTCASE = True
+
+    def get_pid_from_variable(self):
+        target = self.dbg.GetTargetAtIndex(0)


Don't use this, the target should be returned from the call to lldbutil.run_to_source_breakpoint. I put example code in an inline comment below. So pass the target into here. I don't think we can rely on the debugger having only one target.

clayborg · 2024-03-06T00:01:37Z

lldb/test/API/functionalities/fork/concurrent_vfork/TestConcurrentVFork.py

+
+        lldbutil.run_to_source_breakpoint(
+            self, "// break here", lldb.SBFileSpec("main.cpp")
+        )


return all of the info from lldbutil.run_to_source_breakpoint from this function:

return (target, process, thread, bkpt)

clayborg · 2024-03-06T00:01:59Z

lldb/test/API/functionalities/fork/concurrent_vfork/TestConcurrentVFork.py

+        )
+
+    def follow_parent_helper(self, use_fork, call_exec):
+        self.build_run_to_breakpoint(use_fork, call_exec)


(target, process, thread, bkpt) = self.build_run_to_breakpoint(use_fork, call_exec)

clayborg · 2024-03-06T00:02:23Z

lldb/test/API/functionalities/fork/concurrent_vfork/TestConcurrentVFork.py

+    def follow_parent_helper(self, use_fork, call_exec):
+        self.build_run_to_breakpoint(use_fork, call_exec)
+
+        parent_pid = self.get_pid_from_variable()


pass target into this function:

parent_pid = self.get_pid_from_variable(target)

clayborg · 2024-03-06T00:02:32Z

lldb/test/API/functionalities/fork/concurrent_vfork/TestConcurrentVFork.py

+        )
+
+    def follow_child_helper(self, use_fork, call_exec):
+        self.build_run_to_breakpoint(use_fork, call_exec)


(target, process, thread, bkpt) = self.build_run_to_breakpoint(use_fork, call_exec)

clayborg

One little nit you can fix as suggested in the inline comments.

clayborg · 2024-03-06T18:07:25Z

lldb/test/API/functionalities/fork/concurrent_vfork/TestConcurrentVFork.py

+            use_fork, call_exec
+        )
+
+        parent_pid = self.get_pid_from_variable(target)


Only one caller to this function, inline the code:

parent_pid = target.FindFirstGlobalVariable("g_pid").GetValueAsUnsigned()

clayborg · 2024-03-06T18:07:46Z

lldb/test/API/functionalities/fork/concurrent_vfork/TestConcurrentVFork.py

+    def get_pid_from_variable(self, target):
+        return target.FindFirstGlobalVariable("g_pid").GetValueAsUnsigned()


only one caller to this, just inline the code as suggested below.

jeffreytan81 · 2024-03-06T20:13:29Z

The buildbot seems to complain about strcmp not available. Unfortunately I can't reproduce on my linux machine.
Guess draft patch #84224 in case anyone has access to build machine can reproduce and verify this.

The buildbot seems to complain about `strcmp` function not available in the vfork patch (#81564): https://lab.llvm.org/buildbot/#/builders/68/builds/70093/steps/6/logs/stdio Unfortunately, I can't reproduce the failure on my linux machine so this is a guessing fix. If anyone has a way to reproduce and very this fix, please feel free to merge this change. Co-authored-by: jeffreytan81 <[email protected]>

Fix lldb crash while handling concurrent vfork()

d65900f

Fix format

b4c60c3

jeffreytan81 requested review from clayborg, kusmour and zhyty February 13, 2024 19:58

jeffreytan81 marked this pull request as ready for review February 13, 2024 19:59

jeffreytan81 requested a review from JDevlieghere as a code owner February 13, 2024 19:59

llvmbot added the lldb label Feb 13, 2024

clayborg requested changes Feb 13, 2024

View reviewed changes

Address review feedback

2e7bf23

Fix python format

7340372

clayborg requested changes Feb 20, 2024

View reviewed changes

Address review comments

6a528dd

clayborg requested changes Feb 23, 2024

View reviewed changes

Adding exec() and fork() testcases

bc36011

clayborg requested changes Mar 6, 2024

View reviewed changes

Use target from run_to_source_breakpoint() API

f06d81a

clayborg approved these changes Mar 6, 2024

View reviewed changes

Fix nit comment

476511f

jeffreytan81 merged commit 8bdddcf into llvm:main Mar 6, 2024

jeffreytan81 mentioned this pull request Mar 6, 2024

Fix vfork test strcmp buildbot failure #84224

Merged

		def get_pid_from_variable(self, target):
		return target.FindFirstGlobalVariable("g_pid").GetValueAsUnsigned()

Fix lldb crash while handling concurrent vfork() #81564

Fix lldb crash while handling concurrent vfork() #81564

Uh oh!

Conversation

jeffreytan81 commented Feb 13, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Feb 13, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

llvmbot commented Feb 13, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Feb 14, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jeffreytan81 Feb 20, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

clayborg left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jeffreytan81 commented Feb 13, 2024 •

edited

Loading

github-actions bot commented Feb 13, 2024 •

edited

Loading

github-actions bot commented Feb 14, 2024 •

edited

Loading

jeffreytan81 Feb 20, 2024 •

edited

Loading