[Lldb-commits] [PATCH] D56234: [lldb-server] Add unnamed pipe support to PipeWindows

[Pavel Labath via Phabricator via lldb-commits]

labath added inline comments.


================
Comment at: source/Host/windows/PipeWindows.cpp:28
 std::atomic<uint32_t> g_pipe_serial(0);
-}
+static std::string g_pipe_name_prefix = "\\\\.\\Pipe\\";
+} // namespace
----------------
This would be better as llvm::StringLiteral (or just a char array) to avoid global constructors. Also, `static` in an anonymous namespace is overkill (anon namespace implies static linkage).


================
Comment at: source/Host/windows/PipeWindows.cpp:47
+  // in the current process and usually crashes the program.  Pass handles
+  // instead since the handle can be inheritated.
+  m_read = read_handle;
----------------
inherited


================
Comment at: source/Host/windows/PipeWindows.cpp:120
 
-  // Open the write end of the pipe.
+  // FIXME: For server end pipe, it's not applicable to use CreateFile to get
+  // read or write end of the pipe for the reason that closing of the file will
----------------
I find it strange reading about "servers" in a low-level utility class that seemingly has nothing to do with serving. I think this will confuse future readers as they will not look at this in the context of this patch. Could you rephrase this?


================
Comment at: source/Plugins/Process/gdb-remote/GDBRemoteCommunication.cpp:1074
+        }
+        auto write_handle = socket_pipe.GetWriteNativeHandle();
+        debugserver_args.AppendArgument(llvm::StringRef("--pipe"));
----------------
What do you think about adding `GetWriteNativeHandle` to the generic Pipe interface? In posix the file descriptor _is_ the native handle, so PipePosix could just return the fd, but it would allow you to write this bit of code generically without ifdefs (well, maybe except the `AppendCloseFileAction` below). Similarly, we could have a generic Pipe factory function(*) which takes the handles as input, which would allow you to implement the lldb-server side of things without ifdefs.

(*) I think a factory function (`Pipe::FromNativeHandles`?) would be better than a constructor to avoid ambiguities between descriptors and handles.


Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D56234