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

[Zachary Turner via Phabricator via lldb-commits]

zturner added inline comments.


================
Comment at: include/lldb/Host/windows/PosixApi.h:78-79
 
+#define WNOHANG 1
+#define WUNTRACED 2
+
----------------
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.


================
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);
+}
----------------
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.


================
Comment at: source/Host/common/SocketAddress.cpp:263-267
+#ifdef _WIN32
+    error.SetError(err, lldb::eErrorTypeWin32);
+#else
+    error.SetError(err, lldb::eErrorTypePOSIX);
+#endif
----------------
Perhaps we could define something like `lldb::eErrorTypeHostNative` to avoid this ifdef.


================
Comment at: tools/lldb-server/SystemInitializerLLGS.cpp:37-52
+#if defined(_WIN32)
+  if (g_ref_count++ == 0) {
+    // Require Windows Sockets version 2.2.
+    auto wVersion = MAKEWORD(2, 2);
+    WSADATA wsaData;
+    auto err = WSAStartup(wVersion, &wsaData);
+    if (err == 0) {
----------------
labath wrote:
> I think a better option here would be to move this code to something like `Socket::Initialize`. Then we could call this from `SystemInitializerCommon::Initialize`, and it would be available to everyone. This would allow us to remove the copy of this code in PlatformWindows, and replace the `WSAStartup` calls in various unittests with `Socket::Initialize()`
Also, why do we need the `g_ref_count`?  Can't we just print an error message and call exit if the version is not what we expect?


================
Comment at: tools/lldb-server/SystemInitializerLLGS.cpp:58-61
+#if defined(_WIN32)
+  if (g_ref_count && --g_ref_count == 0)
+    WSACleanup();
+#endif
----------------
I don't think Initialize and Terminate need to be re-entrant.  So we can probably delete the ref count code here


================
Comment at: tools/lldb-server/lldb-server.cpp:40
 
-static void initialize() {
+namespace llsvr {
+void initialize() {
----------------
labath wrote:
> In other places we use `llgs` (for `LL`db `G`db `S`erver), so I suggest sticking to that.
The `static` from before indicates that the function should not have external linkage, so I think we should keep that.  Whether we put it in a namespace or not is orthogonal, but for global functions in cpp functions, we generally prefer to mark them all static.


Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D56233