[Lldb-commits] [PATCH] D56233: [lldb-server] Add initial support for building lldb-server on Windows

[Pavel Labath via Phabricator via lldb-commits]

labath added inline comments.


================
Comment at: include/lldb/Host/windows/PosixApi.h:78-79
 
+#define WNOHANG 1
+#define WUNTRACED 2
+
----------------
zturner wrote:
> krytarowski wrote:
> > I think that these symbols should not be leaked here in the first place.
> In general we should avoid mocking posix APIs for Windows.  If something is currently implemented in terms of a Posix API that doesn't exist on Windows, we should provide an implementation of that function with an entirely new name and platform-agnostic set of parameters whose interface need not resemble the posix interface in any way..  
> 
> For example, I see this is used in `lldb-platform.cpp` when we call `waitpid()`.  Instead, we could add something like `void WaitForAllChildProcessesToExit();` and then just implement that function on posix and Windows completely separately.  This is more flexible and portable than trying to fit Windows semantics into a posix api, which doesn't always work.
For the record, the intention of that code in lldb-platform is `reapAllChildProcessesThatHaveAlreadyExited()`


================
Comment at: include/lldb/Host/windows/PosixApi.h:108-111
+inline pid_t waitpid(pid_t pid, int *status, int options) {
+  // To be implemented.
+  return pid_t(-1);
+}
----------------
zturner wrote:
> labath wrote:
> > What are your plans for this?
> > 
> > I assume you needed this for the fork-server in lldb-platform.cpp. I think the best way to address that would be to change the code from spawning a separate process for each connection to having a new thread for each connection. That should be much more portable and will avoid the need to mock posix apis on windows. I can help you with that if you run into any problems there.
> Are you ok with the behavioral change?  What if one connection crashes?
> 
> Assuming we wanted to implement this behavior on Windows, the way to do it would be to create all child processes in a single "job" (aka process group), as that is the best way to get notifications for when all childs have terminated.
Yes, I would be fine with that (in fact I wanted to do that at some point anyway). The reason the fork server was added was to enable parallel running of remote tests (before that we only ever handled one concurrent connection). Also the platform mode of llgs doesn't do anything fancy. It's mostly just about copying files around and launching gdb-remote processes (so the real debugging still happens in a separate process), so the crashing surface is pretty limited.

BTW, the only reason we call waitpid there is that on unix, you *have* to do that or you will die of zombie infestation. So, if on windows you can create fire-and-forget child processes, then this whole code is unnecessary there.


Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D56233