Skip to content

[lldb] Remove child_process_inherit argument from Pipe #145516

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 3 additions & 7 deletions lldb/include/lldb/Host/PipeBase.h
Original file line number Diff line number Diff line change
Expand Up @@ -21,18 +21,14 @@ class PipeBase {
public:
virtual ~PipeBase();

virtual Status CreateNew(bool child_process_inherit) = 0;
virtual Status CreateNew(llvm::StringRef name,
bool child_process_inherit) = 0;
virtual Status CreateNew() = 0;
virtual Status CreateNew(llvm::StringRef name) = 0;
virtual Status CreateWithUniqueName(llvm::StringRef prefix,
bool child_process_inherit,
llvm::SmallVectorImpl<char> &name) = 0;

virtual Status OpenAsReader(llvm::StringRef name,
bool child_process_inherit) = 0;
virtual Status OpenAsReader(llvm::StringRef name) = 0;

virtual llvm::Error OpenAsWriter(llvm::StringRef name,
bool child_process_inherit,
const Timeout<std::micro> &timeout) = 0;

virtual bool CanRead() const = 0;
Expand Down
10 changes: 4 additions & 6 deletions lldb/include/lldb/Host/posix/PipePosix.h
Original file line number Diff line number Diff line change
Expand Up @@ -32,14 +32,12 @@ class PipePosix : public PipeBase {

~PipePosix() override;

Status CreateNew(bool child_process_inherit) override;
Status CreateNew(llvm::StringRef name, bool child_process_inherit) override;
Status CreateNew() override;
Status CreateNew(llvm::StringRef name) override;
Status CreateWithUniqueName(llvm::StringRef prefix,
bool child_process_inherit,
llvm::SmallVectorImpl<char> &name) override;
Status OpenAsReader(llvm::StringRef name,
bool child_process_inherit) override;
llvm::Error OpenAsWriter(llvm::StringRef name, bool child_process_inherit,
Status OpenAsReader(llvm::StringRef name) override;
llvm::Error OpenAsWriter(llvm::StringRef name,
const Timeout<std::micro> &timeout) override;

bool CanRead() const override;
Expand Down
13 changes: 5 additions & 8 deletions lldb/include/lldb/Host/windows/PipeWindows.h
Original file line number Diff line number Diff line change
Expand Up @@ -29,16 +29,14 @@ class PipeWindows : public PipeBase {
~PipeWindows() override;

// Create an unnamed pipe.
Status CreateNew(bool child_process_inherit) override;
Status CreateNew() override;

// Create a named pipe.
Status CreateNew(llvm::StringRef name, bool child_process_inherit) override;
Status CreateNew(llvm::StringRef name) override;
Status CreateWithUniqueName(llvm::StringRef prefix,
bool child_process_inherit,
llvm::SmallVectorImpl<char> &name) override;
Status OpenAsReader(llvm::StringRef name,
bool child_process_inherit) override;
llvm::Error OpenAsWriter(llvm::StringRef name, bool child_process_inherit,
Status OpenAsReader(llvm::StringRef name) override;
llvm::Error OpenAsWriter(llvm::StringRef name,
const Timeout<std::micro> &timeout) override;

bool CanRead() const override;
Expand Down Expand Up @@ -72,8 +70,7 @@ class PipeWindows : public PipeBase {
HANDLE GetWriteNativeHandle();

private:
Status OpenNamedPipe(llvm::StringRef name, bool child_process_inherit,
bool is_read);
Status OpenNamedPipe(llvm::StringRef name, bool is_read);

HANDLE m_read;
HANDLE m_write;
Expand Down
2 changes: 1 addition & 1 deletion lldb/source/Host/common/Socket.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,7 @@ SharedSocket::SharedSocket(const Socket *socket, Status &error) {
m_fd = kInvalidFD;

// Create a pipe to transfer WSAPROTOCOL_INFO to the child process.
error = m_socket_pipe.CreateNew(true);
error = m_socket_pipe.CreateNew();
if (error.Fail())
return;

Expand Down
2 changes: 1 addition & 1 deletion lldb/source/Host/posix/ConnectionFileDescriptorPosix.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -92,7 +92,7 @@ void ConnectionFileDescriptor::OpenCommandPipe() {

Log *log = GetLog(LLDBLog::Connection);
// Make the command file descriptor here:
Status result = m_pipe.CreateNew(/*child_processes_inherit=*/false);
Status result = m_pipe.CreateNew();
if (!result.Success()) {
LLDB_LOGF(log,
"%p ConnectionFileDescriptor::OpenCommandPipe () - could not "
Expand Down
2 changes: 1 addition & 1 deletion lldb/source/Host/posix/MainLoopPosix.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -208,7 +208,7 @@ void MainLoopPosix::RunImpl::ProcessReadEvents() {
#endif

MainLoopPosix::MainLoopPosix() {
Status error = m_interrupt_pipe.CreateNew(/*child_process_inherit=*/false);
Status error = m_interrupt_pipe.CreateNew();
assert(error.Success());

// Make the write end of the pipe non-blocking.
Expand Down
31 changes: 11 additions & 20 deletions lldb/source/Host/posix/PipePosix.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -79,24 +79,22 @@ PipePosix &PipePosix::operator=(PipePosix &&pipe_posix) {

PipePosix::~PipePosix() { Close(); }

Status PipePosix::CreateNew(bool child_processes_inherit) {
Status PipePosix::CreateNew() {
std::scoped_lock<std::mutex, std::mutex> guard(m_read_mutex, m_write_mutex);
if (CanReadUnlocked() || CanWriteUnlocked())
return Status(EINVAL, eErrorTypePOSIX);

Status error;
#if PIPE2_SUPPORTED
if (::pipe2(m_fds, (child_processes_inherit) ? 0 : O_CLOEXEC) == 0)
if (::pipe2(m_fds, O_CLOEXEC) == 0)
return error;
#else
if (::pipe(m_fds) == 0) {
#ifdef FD_CLOEXEC
if (!child_processes_inherit) {
if (!SetCloexecFlag(m_fds[0]) || !SetCloexecFlag(m_fds[1])) {
error = Status::FromErrno();
CloseUnlocked();
return error;
}
if (!SetCloexecFlag(m_fds[0]) || !SetCloexecFlag(m_fds[1])) {
error = Status::FromErrno();
CloseUnlocked();
return error;
}
#endif
return error;
Expand All @@ -109,7 +107,7 @@ Status PipePosix::CreateNew(bool child_processes_inherit) {
return error;
}

Status PipePosix::CreateNew(llvm::StringRef name, bool child_process_inherit) {
Status PipePosix::CreateNew(llvm::StringRef name) {
std::scoped_lock<std::mutex, std::mutex> guard(m_read_mutex, m_write_mutex);
if (CanReadUnlocked() || CanWriteUnlocked())
return Status::FromErrorString("Pipe is already opened");
Expand All @@ -121,7 +119,6 @@ Status PipePosix::CreateNew(llvm::StringRef name, bool child_process_inherit) {
}

Status PipePosix::CreateWithUniqueName(llvm::StringRef prefix,
bool child_process_inherit,
llvm::SmallVectorImpl<char> &name) {
llvm::SmallString<128> named_pipe_path;
llvm::SmallString<128> pipe_spec((prefix + ".%%%%%%").str());
Expand All @@ -137,24 +134,21 @@ Status PipePosix::CreateWithUniqueName(llvm::StringRef prefix,
do {
llvm::sys::fs::createUniquePath(tmpdir_file_spec.GetPath(), named_pipe_path,
/*MakeAbsolute=*/false);
error = CreateNew(named_pipe_path, child_process_inherit);
error = CreateNew(named_pipe_path);
} while (error.GetError() == EEXIST);

if (error.Success())
name = named_pipe_path;
return error;
}

Status PipePosix::OpenAsReader(llvm::StringRef name,
bool child_process_inherit) {
Status PipePosix::OpenAsReader(llvm::StringRef name) {
std::scoped_lock<std::mutex, std::mutex> guard(m_read_mutex, m_write_mutex);

if (CanReadUnlocked() || CanWriteUnlocked())
return Status::FromErrorString("Pipe is already opened");

int flags = O_RDONLY | O_NONBLOCK;
if (!child_process_inherit)
flags |= O_CLOEXEC;
int flags = O_RDONLY | O_NONBLOCK | O_CLOEXEC;

Status error;
int fd = FileSystem::Instance().Open(name.str().c_str(), flags);
Expand All @@ -167,15 +161,12 @@ Status PipePosix::OpenAsReader(llvm::StringRef name,
}

llvm::Error PipePosix::OpenAsWriter(llvm::StringRef name,
bool child_process_inherit,
const Timeout<std::micro> &timeout) {
std::lock_guard<std::mutex> guard(m_write_mutex);
if (CanReadUnlocked() || CanWriteUnlocked())
return llvm::createStringError("Pipe is already opened");

int flags = O_WRONLY | O_NONBLOCK;
if (!child_process_inherit)
flags |= O_CLOEXEC;
int flags = O_WRONLY | O_NONBLOCK | O_CLOEXEC;

using namespace std::chrono;
std::optional<time_point<steady_clock>> finish_time;
Expand Down
3 changes: 1 addition & 2 deletions lldb/source/Host/posix/ProcessLauncherPosixFork.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -255,8 +255,7 @@ ProcessLauncherPosixFork::LaunchProcess(const ProcessLaunchInfo &launch_info,
Status &error) {
// A pipe used by the child process to report errors.
PipePosix pipe;
const bool child_processes_inherit = false;
error = pipe.CreateNew(child_processes_inherit);
error = pipe.CreateNew();
if (error.Fail())
return HostProcess();

Expand Down
23 changes: 9 additions & 14 deletions lldb/source/Host/windows/PipeWindows.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -66,19 +66,18 @@ PipeWindows::PipeWindows(pipe_t read, pipe_t write)

PipeWindows::~PipeWindows() { Close(); }

Status PipeWindows::CreateNew(bool child_process_inherit) {
Status PipeWindows::CreateNew() {
// Even for anonymous pipes, we open a named pipe. This is because you
// cannot get overlapped i/o on Windows without using a named pipe. So we
// synthesize a unique name.
uint32_t serial = g_pipe_serial.fetch_add(1);
std::string pipe_name = llvm::formatv(
"lldb.pipe.{0}.{1}.{2}", GetCurrentProcessId(), &g_pipe_serial, serial);

return CreateNew(pipe_name.c_str(), child_process_inherit);
return CreateNew(pipe_name.c_str());
}

Status PipeWindows::CreateNew(llvm::StringRef name,
bool child_process_inherit) {
Status PipeWindows::CreateNew(llvm::StringRef name) {
if (name.empty())
return Status(ERROR_INVALID_PARAMETER, eErrorTypeWin32);

Expand Down Expand Up @@ -109,7 +108,7 @@ Status PipeWindows::CreateNew(llvm::StringRef name,

// Open the write end of the pipe. Note that closing either the read or
// write end of the pipe could directly close the pipe itself.
Status result = OpenNamedPipe(name, child_process_inherit, false);
Status result = OpenNamedPipe(name, false);
if (!result.Success()) {
CloseReadFileDescriptor();
return result;
Expand All @@ -119,7 +118,6 @@ Status PipeWindows::CreateNew(llvm::StringRef name,
}

Status PipeWindows::CreateWithUniqueName(llvm::StringRef prefix,
bool child_process_inherit,
llvm::SmallVectorImpl<char> &name) {
llvm::SmallString<128> pipe_name;
Status error;
Expand All @@ -133,7 +131,7 @@ Status PipeWindows::CreateWithUniqueName(llvm::StringRef prefix,
pipe_name += "-";
pipe_name += reinterpret_cast<char *>(unique_string);
::RpcStringFreeA(&unique_string);
error = CreateNew(pipe_name, child_process_inherit);
error = CreateNew(pipe_name);
} else {
error = Status(status, eErrorTypeWin32);
}
Expand All @@ -142,25 +140,22 @@ Status PipeWindows::CreateWithUniqueName(llvm::StringRef prefix,
return error;
}

Status PipeWindows::OpenAsReader(llvm::StringRef name,
bool child_process_inherit) {
Status PipeWindows::OpenAsReader(llvm::StringRef name) {
if (CanRead())
return Status(); // Note the name is ignored.

return OpenNamedPipe(name, child_process_inherit, true);
return OpenNamedPipe(name, true);
}

llvm::Error PipeWindows::OpenAsWriter(llvm::StringRef name,
bool child_process_inherit,
const Timeout<std::micro> &timeout) {
if (CanWrite())
return llvm::Error::success(); // Note the name is ignored.

return OpenNamedPipe(name, child_process_inherit, false).takeError();
return OpenNamedPipe(name, false).takeError();
}

Status PipeWindows::OpenNamedPipe(llvm::StringRef name,
bool child_process_inherit, bool is_read) {
Status PipeWindows::OpenNamedPipe(llvm::StringRef name, bool is_read) {
if (name.empty())
return Status(ERROR_INVALID_PARAMETER, eErrorTypeWin32);

Expand Down
2 changes: 1 addition & 1 deletion lldb/source/Interpreter/ScriptInterpreter.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -220,7 +220,7 @@ ScriptInterpreterIORedirect::ScriptInterpreterIORedirect(
m_input_file_sp = debugger.GetInputFileSP();

Pipe pipe;
Status pipe_result = pipe.CreateNew(false);
Status pipe_result = pipe.CreateNew();
#if defined(_WIN32)
lldb::file_t read_file = pipe.GetReadNativeHandle();
pipe.ReleaseReadFileDescriptor();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -955,7 +955,7 @@ Status GDBRemoteCommunication::StartDebugserverProcess(
// Binding to port zero, we need to figure out what port it ends up
// using using a named pipe...
Status error = socket_pipe.CreateWithUniqueName("debugserver-named-pipe",
false, named_pipe_path);
named_pipe_path);
if (error.Fail()) {
LLDB_LOG(log, "named pipe creation failed: {0}", error);
return error;
Expand All @@ -965,15 +965,17 @@ Status GDBRemoteCommunication::StartDebugserverProcess(
#else
// Binding to port zero, we need to figure out what port it ends up
// using using an unnamed pipe...
Status error = socket_pipe.CreateNew(true);
Status error = socket_pipe.CreateNew();
if (error.Fail()) {
LLDB_LOG(log, "unnamed pipe creation failed: {0}", error);
return error;
}
pipe_t write = socket_pipe.GetWritePipe();
debugserver_args.AppendArgument(llvm::StringRef("--pipe"));
debugserver_args.AppendArgument(llvm::to_string(write));
launch_info.AppendCloseFileAction(socket_pipe.GetReadFileDescriptor());
launch_info.AppendDuplicateFileAction(
(int64_t)socket_pipe.GetWritePipe(),
(int64_t)socket_pipe.GetWritePipe());
#endif
} else {
// No host and port given, so lets listen on our end and make the
Expand Down Expand Up @@ -1075,7 +1077,7 @@ Status GDBRemoteCommunication::StartDebugserverProcess(

Status error;
if (named_pipe_path.size() > 0) {
error = socket_pipe.OpenAsReader(named_pipe_path, false);
error = socket_pipe.OpenAsReader(named_pipe_path);
if (error.Fail()) {
LLDB_LOG(log, "failed to open named pipe {0} for reading: {1}",
named_pipe_path, error);
Expand Down
2 changes: 1 addition & 1 deletion lldb/source/Target/Process.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -4614,7 +4614,7 @@ class IOHandlerProcessSTDIO : public IOHandler {
m_process(process),
m_read_file(GetInputFD(), File::eOpenOptionReadOnly, false),
m_write_file(write_fd, File::eOpenOptionWriteOnly, false) {
m_pipe.CreateNew(false);
m_pipe.CreateNew();
}

~IOHandlerProcessSTDIO() override = default;
Expand Down
2 changes: 1 addition & 1 deletion lldb/tools/lldb-server/lldb-gdbserver.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -185,7 +185,7 @@ Status writeSocketIdToPipe(const char *const named_pipe_path,
llvm::StringRef socket_id) {
Pipe port_name_pipe;
// Wait for 10 seconds for pipe to be opened.
if (llvm::Error err = port_name_pipe.OpenAsWriter(named_pipe_path, false,
if (llvm::Error err = port_name_pipe.OpenAsWriter(named_pipe_path,
std::chrono::seconds{10}))
return Status::FromError(std::move(err));

Expand Down
3 changes: 1 addition & 2 deletions lldb/unittests/Core/CommunicationTest.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -141,8 +141,7 @@ TEST_F(CommunicationTest, SynchronizeWhileClosing) {
#if LLDB_ENABLE_POSIX
TEST_F(CommunicationTest, WriteAll) {
Pipe pipe;
ASSERT_THAT_ERROR(pipe.CreateNew(/*child_process_inherit=*/false).ToError(),
llvm::Succeeded());
ASSERT_THAT_ERROR(pipe.CreateNew().ToError(), llvm::Succeeded());

// Make the write end non-blocking in order to easily reproduce a partial
// write.
Expand Down
3 changes: 1 addition & 2 deletions lldb/unittests/Host/HostTest.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -145,8 +145,7 @@ TEST(Host, LaunchProcessDuplicatesHandle) {
exit(1);
}
Pipe pipe;
ASSERT_THAT_ERROR(pipe.CreateNew(/*child_process_inherit=*/false).takeError(),
llvm::Succeeded());
ASSERT_THAT_ERROR(pipe.CreateNew().takeError(), llvm::Succeeded());
SCOPED_TRACE(llvm::formatv("Pipe handles are: {0}/{1}",
(uint64_t)pipe.GetReadPipe(),
(uint64_t)pipe.GetWritePipe())
Expand Down
Loading
Loading