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

[Zachary Turner via Phabricator via lldb-commits]

zturner added inline comments.


================
Comment at: source/Host/windows/PipeWindows.cpp:34-38
   m_read = INVALID_HANDLE_VALUE;
   m_write = INVALID_HANDLE_VALUE;
 
-  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) {
  if (read != INVALID_HANDLE_VALUE)
    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};
 #else
+  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;`


Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D56234