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
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
