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



_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

Reply via email to