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

[Hui Huang via Phabricator via llvm-commits]

Hui added inline comments.


================
Comment at: source/Plugins/Process/gdb-remote/GDBRemoteCommunication.cpp:1051
 // and fill the data into "command_output_ptr"
 #if defined(__APPLE__)
         // Binding to port zero, we need to figure out what port it ends up
----------------
My first attempt was to enable windows specific along with apple's since named pipe is already supported. Seemed there was some problem for client pipe to open a server pipe on the same machine. This might not be the scope of this discussion. 


================
Comment at: source/Plugins/Process/gdb-remote/GDBRemoteCommunication.cpp:1074
+        }
+        auto write_handle = socket_pipe.GetWriteNativeHandle();
+        debugserver_args.AppendArgument(llvm::StringRef("--pipe"));
----------------
zturner wrote:
> labath wrote:
> > 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.
> That's one possibility, but at the very least we should at least move the `CreateNew` and logging out of the ifdef.
Based on the lldb::pipe_t, using one of the introduced overloads could just put the windows specific all the way into the existing non-apple implementation.   AppendCloseFileAction seemed yet impact anything.

lldb::pipe_t GetReadPipe() const { return lldb::pipe_t(m_read); }
lldb::pipe_t GetWritePipe() const { return lldb::pipe_t(m_write); }



Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D56234