[Lldb-commits] [PATCH] D33778: Add a NativeProcessProtocol Factory class

Kamil Rytarowski via Phabricator via lldb-commits lldb-commits at lists.llvm.org
Thu Jun 1 15:01:54 PDT 2017

krytarowski added inline comments.

Comment at: source/Plugins/Process/NetBSD/NativeProcessNetBSD.cpp:112
+  if (wpid != pid || !WIFSTOPPED(wstatus)) {
+    std::error_code EC(errno, std::generic_category());
+    LLDB_LOG(
I can imagine that in some cases there might be returned `errno=0` and eg. `WIFSIGNALED()=true`. `wstatus` wouldn't be readable for end-user - I think it's better to check for `(wpid != -1)` following the same code between `{}` and add dedicated code for `(wpid != pid || !WIFSTOPPED(wstatus))` that the state is unexpected and terminating.

Comment at: source/Plugins/Process/NetBSD/NativeProcessNetBSD.cpp:133
-  return error;
+  status = process_sp->ReinitializeThreads();
+  if (status.Fail())
ReinitializeThreads() was designed to not set stopped reason in threads.

  for (const auto &thread_sp : m_threads) {

I think we need the equivalent of this code there.

Similar for:

/* Set process stopped */

Comment at: source/Plugins/Process/NetBSD/NativeProcessNetBSD.cpp:834
+Status NativeProcessNetBSD::Attach() {
+  if (m_pid <= 1)
+    return Status("Attaching to process 1 is not allowed.");
Technically it is possible on NetBSD, with uid=root and in kernel in INSECURE mode. This way root can debug `init`. In all other cases `ptrace`(2) returns error for PID 1.

I think it should be allowed, or rather not explicitly prohibited behavior.

Comment at: tools/lldb-server/lldb-gdbserver.cpp:65
+#elif defined(__NetBSD__)
+#include "Plugins/Process/NetBSD/NativeProcessNetBSD.h"
+typedef process_netbsd::NativeProcessNetBSD::Factory NativeProcessFactory;
Included twice?


More information about the lldb-commits mailing list