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
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
