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

Zachary Turner via Phabricator via lldb-commits lldb-commits at lists.llvm.org
Mon Jan 7 09:46:06 PST 2019

zturner added inline comments.

Comment at: source/Host/windows/PipeWindows.cpp:34-38
-  m_read_fd = -1;
-  m_write_fd = -1;
+  m_read_fd = PipeWindows::kInvalidDescriptor;
+  m_write_fd = PipeWindows::kInvalidDescriptor;
Perhaps we should use inline member initialization for these fields in the class definition.

Comment at: source/Host/windows/PipeWindows.cpp:51-56
+  m_read_fd = (read_handle == INVALID_HANDLE_VALUE)
+                  ? PipeWindows::kInvalidDescriptor
+                  : _open_osfhandle((intptr_t)read_handle, _O_RDONLY);
+  m_write_fd = (write_handle == INVALID_HANDLE_VALUE)
+                   ? PipeWindows::kInvalidDescriptor
+                   : _open_osfhandle((intptr_t)write_handle, _O_WRONLY);
With inline member initialization, this becomes:

PipeWindows::PipeWindows(HANDLE read, HANDLE write)
  : m_read(read), m_write(write) {
    m_read_fd = _open_osfhandle((intptr_t)read, _O_RDONLY);
  if (write != INVALID_HANDLE_VALUE)
    m_write_fd = _open_osfhandle((intptr_t)write, _O_RDONLY);

Comment at: source/Host/windows/PipeWindows.cpp:58-59
   ZeroMemory(&m_read_overlapped, sizeof(m_read_overlapped));
   ZeroMemory(&m_write_overlapped, sizeof(m_write_overlapped));
Is it valid for a creator of a `PipeWindows` to pass in invalid handles when using this constructor?  If not, we should assert.  Certainly it doesn't make sense if *both* of the handles are invalid, so we can definitely at least assert that case.

Comment at: source/Host/windows/PipeWindows.cpp:69
+  sa.lpSecurityDescriptor = 0;
+  sa.bInheritHandle = child_process_inherit ? TRUE : FALSE;
+  BOOL result = ::CreatePipe(&m_read, &m_write, &sa, 1024);
Is the ternary necessary here?  can't we just assign it?

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
labath wrote:
> 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?
I actually think the previous comment was fine?

Comment at: source/Plugins/Process/gdb-remote/GDBRemoteCommunication.cpp:1074
+        }
+        auto write_handle = socket_pipe.GetWriteNativeHandle();
+        debugserver_args.AppendArgument(llvm::StringRef("--pipe"));
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.

Comment at: tools/lldb-server/lldb-gdbserver.cpp:223-226
 #if defined(_WIN32)
-  return Status("Unnamed pipes are not supported on Windows.");
+  Pipe port_pipe{Pipe::kInvalidHandle, (HANDLE)unnamed_pipe_fd_or_handle};
+  Pipe port_pipe{Pipe::kInvalidDescriptor, (int)unnamed_pipe_fd_or_handle};
One way to get rid of all these ifdefs is to to define a type called `lldb::pipe_t` and change `Pipe::kInvalidDescriptor` to be `lldb::pipe_t kInvalidPipe` (for example, see definition of `lldb::thread_t` in `lldb-types.h`).  Then you could change the constructor to be `Pipe::Pipe(lldb::pipe_t read, lldb::pipe_t write)` and this function could be written as:

Status writeSocketIdToPipe(lldb::pipe_t pipe) {
  Pipe port_pipe(Pipe::kInvalidPipe, pipe);
  return writeSocketIdToPipe(port_pipe, socket_id);

At that point though, maybe we don't even need a separate function for this anymore.

Comment at: tools/lldb-server/lldb-gdbserver.cpp:335
         // now.
-        else if (unnamed_pipe_fd >= 0) {
-          error = writeSocketIdToPipe(unnamed_pipe_fd, socket_id);
+        else if (unnamed_pipe_fd_or_handle >= 0) {
+          error = writeSocketIdToPipe(unnamed_pipe_fd_or_handle, socket_id);
This needs to change to `unnamed_pipe_fd_or_handle != Pipe::kInvalidPipe`

Comment at: tools/lldb-server/lldb-gdbserver.cpp:388
       log_channels; // e.g. "lldb process threads:gdb-remote default:linux all"
-  int unnamed_pipe_fd = -1;
+  int64_t unnamed_pipe_fd_or_handle = -1;
   bool reverse_connect = false;
`lldb::pipe_t unnamed_pipe_fd_or_handle;`




More information about the lldb-commits mailing list