[Lldb-commits] [PATCH] D61686: Enable lldb-server on Windows

[Pavel Labath via Phabricator via lldb-commits]

labath added inline comments.


================
Comment at: include/lldb/Host/windows/PosixApi.h:106-109
+inline pid_t waitpid(pid_t pid, int *status, int options) {
+  // To be implemented.
+  return pid_t(-1);
+}
----------------
As discussed in the review where this was forked from, we shouldn't be mocking posix apis. Instead we should provide platform-independent abstractions that each platform can implement in it's own way. Waitpid is right now used in only one place. Implementing it here (even just as a stub) just encourages others to add more uses of it.

Given that (I think) you don't care about the "server" mode of lldb-platform at the moment, and the fact that the piece of code calling this is already broken because the use of fork, I think that for now we should just #ifdef-out the block of code calling fork and waitpid (it's already structured in a way that's easy to #ifdef. This would enable you to even provide a friendly error message explaining that the requested feature is not available.


================
Comment at: source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerCommon.cpp:1188-1198
+#if defined(_WIN32)
+  response.Printf("pid:%" PRIx64 ";parent-pid:%" PRIx64 ";",
+                  proc_info.GetProcessID(), proc_info.GetParentProcessID());
+#else
   response.Printf("pid:%" PRIx64 ";parent-pid:%" PRIx64
                   ";real-uid:%x;real-gid:%x;effective-uid:%x;effective-gid:%x;",
                   proc_info.GetProcessID(), proc_info.GetParentProcessID(),
----------------
ProcessInstanceInfo already has the ability to say whether some fields are present/valid or not. So I think you should just replace this #ifdef with something like:
```
if (proc_info.UserIDIsValid())
  response.Printf(";real-uid:%x", proc_info.GetUserID()));
if(proc_info.GroupIDIsValid())
  ...
```


================
Comment at: tools/lldb-server/lldb-platform.cpp:72
 // Watch for signals
 static void signal_handler(int signo) {
+#if !defined(_WIN32)
----------------
It looks like this function will be unused after you disable the `signal` line below, so you might as well ifdef-out the entire function instead of just its body.


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

https://reviews.llvm.org/D61686