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

[Kamil Rytarowski via Phabricator via lldb-commits]

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) {
    static_pointer_cast<NativeThreadNetBSD>(thread_sp)->SetStoppedBySignal(
        SIGSTOP);
}
```

I think we need the equivalent of this code there.

Similar for:


```
/* Set process stopped */
SetState(StateType::eStateStopped);
```


================
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?


https://reviews.llvm.org/D33778