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

[Pavel Labath via Phabricator via lldb-commits]

labath added a reviewer: labath.
labath added inline comments.


================
Comment at: include/lldb/Host/windows/PosixApi.h:79
+#define WNOHANG 1
+#define WUNTRACED 2
+
----------------
I don't see `WUNTRACED` being used anywhere within lldb. Why did you need this?


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


================
Comment at: source/Host/common/SocketAddress.cpp:268
+#endif
+    printf("SocketAddress::GetAddressInfo - %s\n", error.AsCString());
   }
----------------
Writing to stdout like this is very rude. If there isn't a suitable way to return this info, then I suggest writing this to the log instead.


================
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) {
----------------
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()`


================
Comment at: tools/lldb-server/lldb-server.cpp:40
 
-static void initialize() {
+namespace llsvr {
+void initialize() {
----------------
In other places we use `llgs` (for `LL`db `G`db `S`erver), so I suggest sticking to that.


Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D56233