This is an archive of the discontinued LLVM Phabricator instance.

[lldb-server] Add unnamed pipe support to PipeWindows
ClosedPublic

Authored by asmith on Jan 2 2019, 4:49 PM.

Details

Summary

This adds unnamed pipe support in PipeWindows to support communication between a debug server and child process.
Modify PipeWindows::CreateNew to support the creation of an unnamed pipe.
Rename the previous method that created a named pipe to PipeWindows::CreateNewNamed.

Diff Detail

Event Timeline

asmith created this revision.Jan 2 2019, 4:49 PM
labath added a subscriber: labath.Jan 3 2019, 6:49 AM
labath added inline comments.
source/Host/windows/PipeWindows.cpp
37

This would be better as llvm::StringLiteral (or just a char array) to avoid global constructors. Also, static in an anonymous namespace is overkill (anon namespace implies static linkage).

72

inherited

123–124

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?

source/Plugins/Process/gdb-remote/GDBRemoteCommunication.cpp
1075

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.

zturner added inline comments.Jan 7 2019, 9:46 AM
source/Host/windows/PipeWindows.cpp
38–63

Perhaps we should use inline member initialization for these fields in the class definition.

63–64

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.

74

Is the ternary necessary here? can't we just assign it?

76–81

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);
}
123–124

I actually think the previous comment was fine?

source/Plugins/Process/gdb-remote/GDBRemoteCommunication.cpp
1075

That's one possibility, but at the very least we should at least move the CreateNew and logging out of the ifdef.

tools/lldb-server/lldb-gdbserver.cpp
220–224

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.

331–332

This needs to change to unnamed_pipe_fd_or_handle != Pipe::kInvalidPipe

384

lldb::pipe_t unnamed_pipe_fd_or_handle;

Hui added a subscriber: Hui.Jan 9 2019, 2:43 PM
Hui added inline comments.
source/Plugins/Process/gdb-remote/GDBRemoteCommunication.cpp
1052

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.

1075

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); }

asmith updated this revision to Diff 180939.Jan 9 2019, 3:04 PM

Add lldb::pipe_t type

asmith marked 13 inline comments as done.Jan 9 2019, 3:06 PM
zturner accepted this revision.Jan 9 2019, 3:37 PM

This looks much nicer with the pipe_t type.

source/Host/posix/PipePosix.cpp
70

I don't think you need the cast, since the type is already int. (Some compilers will warn if you cast something to its own type).

This revision is now accepted and ready to land.Jan 9 2019, 3:37 PM
asmith updated this revision to Diff 180965.Jan 9 2019, 4:48 PM
asmith closed this revision.Jan 9 2019, 4:49 PM

Looks very nice. Thanks.

include/lldb/Host/windows/PipeWindows.h
52

missing override (that will likely be flagged as a warning by clang)?

labath added inline comments.Jan 10 2019, 12:54 AM
include/lldb/Host/windows/PipeWindows.h
71–74

Are these still necessary?

Hui added a comment.Jan 10 2019, 12:16 PM

Looks very nice. Thanks.

There is a similar type file_t widely used in llvm.