[Lldb-commits] [PATCH] D98822: [lldb] [Process] Watch for fork/vfork notifications

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

In D98822#2658780 <https://reviews.llvm.org/D98822#2658780>, @mgorny wrote:

> What about debug registers on FreeBSD? This system is pretty unique because child processes inherit dbregs from parent, and therefore we need to clear them. GDB doesn't do that and watchpoints crash forked children. Should we keep the clearing part as a hack specific to FreeBSD, or have lldb generic clear all hardware breakpoints and watchpoints on fork?

I think we can keep the freebsd-specific clearing code (in the server). Clearing all hardware breakpoints in the client would be pretty logical, but if we wanted to make that work consistently, then we might need to add code to /other/ operating system implementations to /set/ the breakpoints (so that the client can clear them properly)...



================
Comment at: lldb/include/lldb/Host/linux/Host.h:13
+#include "llvm/ADT/Optional.h"
+
+#include "lldb/lldb-types.h"
----------------
These spaces are pretty common in lldb, but that's not actually consistent with how the rest of llvm formats them...


================
Comment at: lldb/source/Plugins/Process/FreeBSD/NativeProcessFreeBSD.cpp:738
+    ::pid_t wait_pid =
+        llvm::sys::RetryAfterSignal(-1, waitpid, -1, &status, WNOHANG);
 
----------------
And the netbsd comment might apply to freebsd as well...


================
Comment at: lldb/source/Plugins/Process/FreeBSD/NativeRegisterContextFreeBSD_arm64.cpp:290-291
+#ifdef LLDB_HAS_FREEBSD_WATCHPOINT
+  llvm::Error error = ReadHardwareDebugInfo();
+  if (error)
+    return error;
----------------



================
Comment at: lldb/source/Plugins/Process/Linux/NativeProcessLinux.cpp:647-665
+  case (SIGTRAP | (PTRACE_EVENT_FORK << 8)): {
+    unsigned long data = 0;
+    if (GetEventMessage(thread.GetID(), &data).Fail())
+      data = 0;
+
+    if (!MonitorClone(data, {{PTRACE_EVENT_FORK, thread.GetID()}}))
+      WaitForCloneNotification(data);
----------------
It looks like these two (and probably also PTRACE_EVENT_CLONE) could be bundled together.


================
Comment at: lldb/source/Plugins/Process/Linux/NativeProcessLinux.cpp:659
+    unsigned long data = 0;
+    if (GetEventMessage(thread.GetID(), &data).Fail())
+      data = 0;
----------------
If this fails (I guess that can only happen if the entire process was SIGKILLED), how does continuing with pid=0 help?
I'm not sure that resuming the parent (as the clone case does) is significantly better, but it seems like we should at least be consistent...


================
Comment at: lldb/source/Plugins/Process/Linux/NativeProcessLinux.cpp:906
+
+  auto parent_thread = GetThreadByID(clone_info->parent_tid);
+  assert(parent_thread);
----------------
auto *


================
Comment at: lldb/source/Plugins/Process/Linux/NativeProcessLinux.cpp:931
+  }
+  // fallthrough
+  case PTRACE_EVENT_FORK: {
----------------
I think we have some macro or attribute for that these days...


================
Comment at: lldb/source/Plugins/Process/Linux/NativeProcessLinux.cpp:936
+                                     m_arch,    unused_loop,   {child_pid}};
+    child_process.m_software_breakpoints = m_software_breakpoints;
+    child_process.Detach();
----------------
Copying the breakpoints is not needed yet, right? We could just group these two (fork/vfork) paths for now...


================
Comment at: lldb/source/Plugins/Process/Linux/NativeProcessLinux.cpp:952
+  default:
+    assert(false && "unknown clone_info.event");
+  }
----------------
llvm_unreachable("unknown clone_info.event");


================
Comment at: lldb/source/Plugins/Process/Linux/NativeProcessLinux.cpp:955
+
+  m_pending_pid_map.erase(child_pid);
+  return true;
----------------
erase(find_it) would be slightly more efficient. (And it might be better to put this before the switch, to make sure it executes in case of early exits.)


================
Comment at: lldb/source/Plugins/Process/NetBSD/NativeProcessNetBSD.cpp:784
+    int status;
+    ::pid_t wait_pid = llvm::sys::RetryAfterSignal(-1, waitpid, -1, &status,
+                                                   WALLSIG | WNOHANG);
----------------
For NetBSD, it might be cleaner to keep waiting for the parent only, and then do a separate waitpid for the child once you get its process id. That would make the sequence of events (and the relevant code) predictable.

I think this is how this interface was meant to be used, even on linux, but linux makes it tricky because it's not possible to wait for all threads belonging to a given process in a single call -- one would have to iterate through the threads and wait for each one separately (it might be interesting to try to implement and benchmark that).


================
Comment at: lldb/test/Shell/Subprocess/Inputs/clone.c:23
+int main() {
+  char stack[4096];
+
----------------
Better include an alignas directive, just in case.


================
Comment at: lldb/test/Shell/Subprocess/Inputs/vfork.c:1
+#include <sys/types.h>
+#include <sys/wait.h>
----------------
Maybe merge these three files into one, parameterized by a define or something?


================
Comment at: lldb/test/Shell/Subprocess/clone-follow-parent-wp.test:1
+# REQUIRES: native && (system-linux || system-netbsd) && (target-x86 || target-x86_64 || target-aarch64) && dbregs-set
+# RUN: %clang_host -g %p/Inputs/clone.c -o %t
----------------
Why only these three arches?


================
Comment at: lldb/test/Shell/Subprocess/fork-follow-parent.test:1
+# REQUIRES: native
+# RUN: %clang_host %p/Inputs/fork.c -o %t
----------------
This (and maybe others) should have `UNSUPPORTED: system-windows`, I guess.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D98822/new/

https://reviews.llvm.org/D98822