Skip to content

Commit

Permalink
[lldb] Fixed PipeWindows bugs; added Pipe::WriteWithTimeout() (llvm#1…
Browse files Browse the repository at this point in the history 
…01383)

Added also the test for async read/write.
  • Loading branch information
@slydiman
slydiman authored Aug 5, 2024
1 parent badf34a commit ddb9869
Show file tree
Hide file tree
Showing 7 changed files with 202 additions and 51 deletions.
5 changes: 4 additions & 1 deletion lldb/include/lldb/Host/PipeBase.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,10 @@ class PipeBase {
// Delete named pipe.
virtual Status Delete(llvm::StringRef name) = 0;

virtual Status Write(const void *buf, size_t size, size_t &bytes_written) = 0;
virtual Status WriteWithTimeout(const void *buf, size_t size,
const std::chrono::microseconds &timeout,
size_t &bytes_written) = 0;
Status Write(const void *buf, size_t size, size_t &bytes_written);
virtual Status ReadWithTimeout(void *buf, size_t size,
const std::chrono::microseconds &timeout,
size_t &bytes_read) = 0;
Expand Down
4 changes: 3 additions & 1 deletion lldb/include/lldb/Host/posix/PipePosix.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,9 @@ class PipePosix : public PipeBase {

Status Delete(llvm::StringRef name) override;

Status Write(const void *buf, size_t size, size_t &bytes_written) override;
Status WriteWithTimeout(const void *buf, size_t size,
const std::chrono::microseconds &timeout,
size_t &bytes_written) override;
Status ReadWithTimeout(void *buf, size_t size,
const std::chrono::microseconds &timeout,
size_t &bytes_read) override;
Expand Down
5 changes: 3 additions & 2 deletions lldb/include/lldb/Host/windows/PipeWindows.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,6 @@ class PipeWindows : public PipeBase {
Status CreateNew(bool child_process_inherit) override;

// Create a named pipe.
Status CreateNewNamed(bool child_process_inherit);
Status CreateNew(llvm::StringRef name, bool child_process_inherit) override;
Status CreateWithUniqueName(llvm::StringRef prefix,
bool child_process_inherit,
Expand Down Expand Up @@ -60,7 +59,9 @@ class PipeWindows : public PipeBase {

Status Delete(llvm::StringRef name) override;

Status Write(const void *buf, size_t size, size_t &bytes_written) override;
Status WriteWithTimeout(const void *buf, size_t size,
const std::chrono::microseconds &timeout,
size_t &bytes_written) override;
Status ReadWithTimeout(void *buf, size_t size,
const std::chrono::microseconds &timeout,
size_t &bytes_read) override;
Expand Down
5 changes: 5 additions & 0 deletions lldb/source/Host/common/PipeBase.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,11 @@ Status PipeBase::OpenAsWriter(llvm::StringRef name,
std::chrono::microseconds::zero());
}

Status PipeBase::Write(const void *buf, size_t size, size_t &bytes_written) {
return WriteWithTimeout(buf, size, std::chrono::microseconds::zero(),
bytes_written);
}

Status PipeBase::Read(void *buf, size_t size, size_t &bytes_read) {
return ReadWithTimeout(buf, size, std::chrono::microseconds::zero(),
bytes_read);
Expand Down
6 changes: 4 additions & 2 deletions lldb/source/Host/posix/PipePosix.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -335,15 +335,17 @@ Status PipePosix::ReadWithTimeout(void *buf, size_t size,
return error;
}

Status PipePosix::Write(const void *buf, size_t size, size_t &bytes_written) {
Status PipePosix::WriteWithTimeout(const void *buf, size_t size,
const std::chrono::microseconds &timeout,
size_t &bytes_written) {
std::lock_guard<std::mutex> guard(m_write_mutex);
bytes_written = 0;
if (!CanWriteUnlocked())
return Status(EINVAL, eErrorTypePOSIX);

const int fd = GetWriteFileDescriptorUnlocked();
SelectHelper select_helper;
select_helper.SetTimeout(std::chrono::seconds(0));
select_helper.SetTimeout(timeout);
select_helper.FDSetWrite(fd);

Status error;
Expand Down
127 changes: 82 additions & 45 deletions lldb/source/Host/windows/PipeWindows.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -58,30 +58,15 @@ PipeWindows::PipeWindows(pipe_t read, pipe_t write)
}

ZeroMemory(&m_read_overlapped, sizeof(m_read_overlapped));
m_read_overlapped.hEvent = ::CreateEventA(nullptr, TRUE, FALSE, nullptr);

ZeroMemory(&m_write_overlapped, sizeof(m_write_overlapped));
m_write_overlapped.hEvent = ::CreateEventA(nullptr, TRUE, FALSE, nullptr);
}

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

Status PipeWindows::CreateNew(bool child_process_inherit) {
// Create an anonymous pipe with the specified inheritance.
SECURITY_ATTRIBUTES sa{sizeof(SECURITY_ATTRIBUTES), 0,
child_process_inherit ? TRUE : FALSE};
BOOL result = ::CreatePipe(&m_read, &m_write, &sa, 1024);
if (result == FALSE)
return Status(::GetLastError(), eErrorTypeWin32);

m_read_fd = _open_osfhandle((intptr_t)m_read, _O_RDONLY);
ZeroMemory(&m_read_overlapped, sizeof(m_read_overlapped));
m_read_overlapped.hEvent = ::CreateEventA(nullptr, TRUE, FALSE, nullptr);

m_write_fd = _open_osfhandle((intptr_t)m_write, _O_WRONLY);
ZeroMemory(&m_write_overlapped, sizeof(m_write_overlapped));

return Status();
}

Status PipeWindows::CreateNewNamed(bool child_process_inherit) {
// 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.
Expand All @@ -105,12 +90,18 @@ Status PipeWindows::CreateNew(llvm::StringRef name,
std::string pipe_path = g_pipe_name_prefix.str();
pipe_path.append(name.str());

SECURITY_ATTRIBUTES sa{sizeof(SECURITY_ATTRIBUTES), 0,
child_process_inherit ? TRUE : FALSE};

// Always open for overlapped i/o. We implement blocking manually in Read
// and Write.
DWORD read_mode = FILE_FLAG_OVERLAPPED;
m_read = ::CreateNamedPipeA(
pipe_path.c_str(), PIPE_ACCESS_INBOUND | read_mode,
PIPE_TYPE_BYTE | PIPE_WAIT, 1, 1024, 1024, 120 * 1000, NULL);
m_read =
::CreateNamedPipeA(pipe_path.c_str(), PIPE_ACCESS_INBOUND | read_mode,
PIPE_TYPE_BYTE | PIPE_WAIT, /*nMaxInstances=*/1,
/*nOutBufferSize=*/1024,
/*nInBufferSize=*/1024,
/*nDefaultTimeOut=*/0, &sa);
if (INVALID_HANDLE_VALUE == m_read)
return Status(::GetLastError(), eErrorTypeWin32);
m_read_fd = _open_osfhandle((intptr_t)m_read, _O_RDONLY);
Expand Down Expand Up @@ -155,7 +146,7 @@ Status PipeWindows::CreateWithUniqueName(llvm::StringRef prefix,
Status PipeWindows::OpenAsReader(llvm::StringRef name,
bool child_process_inherit) {
if (CanRead())
return Status(ERROR_ALREADY_EXISTS, eErrorTypeWin32);
return Status(); // Note the name is ignored.

return OpenNamedPipe(name, child_process_inherit, true);
}
Expand All @@ -165,7 +156,7 @@ PipeWindows::OpenAsWriterWithTimeout(llvm::StringRef name,
bool child_process_inherit,
const std::chrono::microseconds &timeout) {
if (CanWrite())
return Status(ERROR_ALREADY_EXISTS, eErrorTypeWin32);
return Status(); // Note the name is ignored.

return OpenNamedPipe(name, child_process_inherit, false);
}
Expand All @@ -177,8 +168,8 @@ Status PipeWindows::OpenNamedPipe(llvm::StringRef name,

assert(is_read ? !CanRead() : !CanWrite());

SECURITY_ATTRIBUTES attributes = {};
attributes.bInheritHandle = child_process_inherit;
SECURITY_ATTRIBUTES attributes{sizeof(SECURITY_ATTRIBUTES), 0,
child_process_inherit ? TRUE : FALSE};

std::string pipe_path = g_pipe_name_prefix.str();
pipe_path.append(name.str());
Expand All @@ -202,6 +193,7 @@ Status PipeWindows::OpenNamedPipe(llvm::StringRef name,
m_write_fd = _open_osfhandle((intptr_t)m_write, _O_WRONLY);

ZeroMemory(&m_write_overlapped, sizeof(m_write_overlapped));
m_write_overlapped.hEvent = ::CreateEventA(nullptr, TRUE, FALSE, nullptr);
}

return Status();
Expand All @@ -228,6 +220,8 @@ int PipeWindows::ReleaseWriteFileDescriptor() {
return PipeWindows::kInvalidDescriptor;
int result = m_write_fd;
m_write_fd = PipeWindows::kInvalidDescriptor;
if (m_write_overlapped.hEvent)
::CloseHandle(m_write_overlapped.hEvent);
m_write = INVALID_HANDLE_VALUE;
ZeroMemory(&m_write_overlapped, sizeof(m_write_overlapped));
return result;
Expand All @@ -250,6 +244,9 @@ void PipeWindows::CloseWriteFileDescriptor() {
if (!CanWrite())
return;

if (m_write_overlapped.hEvent)
::CloseHandle(m_write_overlapped.hEvent);

_close(m_write_fd);
m_write = INVALID_HANDLE_VALUE;
m_write_fd = PipeWindows::kInvalidDescriptor;
Expand Down Expand Up @@ -280,15 +277,21 @@ Status PipeWindows::ReadWithTimeout(void *buf, size_t size,
return Status(ERROR_INVALID_HANDLE, eErrorTypeWin32);

bytes_read = 0;
DWORD sys_bytes_read = size;
BOOL result = ::ReadFile(m_read, buf, sys_bytes_read, &sys_bytes_read,
&m_read_overlapped);
if (!result && GetLastError() != ERROR_IO_PENDING)
return Status(::GetLastError(), eErrorTypeWin32);
DWORD sys_bytes_read = 0;
BOOL result =
::ReadFile(m_read, buf, size, &sys_bytes_read, &m_read_overlapped);
if (result) {
bytes_read = sys_bytes_read;
return Status();
}

DWORD failure_error = ::GetLastError();
if (failure_error != ERROR_IO_PENDING)
return Status(failure_error, eErrorTypeWin32);

DWORD timeout = (duration == std::chrono::microseconds::zero())
? INFINITE
: duration.count() * 1000;
: duration.count() / 1000;
DWORD wait_result = ::WaitForSingleObject(m_read_overlapped.hEvent, timeout);
if (wait_result != WAIT_OBJECT_0) {
// The operation probably failed. However, if it timed out, we need to
Expand All @@ -298,10 +301,10 @@ Status PipeWindows::ReadWithTimeout(void *buf, size_t size,
// happens, the original operation should be considered to have been
// successful.
bool failed = true;
DWORD failure_error = ::GetLastError();
failure_error = ::GetLastError();
if (wait_result == WAIT_TIMEOUT) {
BOOL cancel_result = CancelIoEx(m_read, &m_read_overlapped);
if (!cancel_result && GetLastError() == ERROR_NOT_FOUND)
BOOL cancel_result = ::CancelIoEx(m_read, &m_read_overlapped);
if (!cancel_result && ::GetLastError() == ERROR_NOT_FOUND)
failed = false;
}
if (failed)
Expand All @@ -310,27 +313,61 @@ Status PipeWindows::ReadWithTimeout(void *buf, size_t size,

// Now we call GetOverlappedResult setting bWait to false, since we've
// already waited as long as we're willing to.
if (!GetOverlappedResult(m_read, &m_read_overlapped, &sys_bytes_read, FALSE))
if (!::GetOverlappedResult(m_read, &m_read_overlapped, &sys_bytes_read,
FALSE))
return Status(::GetLastError(), eErrorTypeWin32);

bytes_read = sys_bytes_read;
return Status();
}

Status PipeWindows::Write(const void *buf, size_t num_bytes,
size_t &bytes_written) {
Status PipeWindows::WriteWithTimeout(const void *buf, size_t size,
const std::chrono::microseconds &duration,
size_t &bytes_written) {
if (!CanWrite())
return Status(ERROR_INVALID_HANDLE, eErrorTypeWin32);

DWORD sys_bytes_written = 0;
BOOL write_result = ::WriteFile(m_write, buf, num_bytes, &sys_bytes_written,
&m_write_overlapped);
if (!write_result && GetLastError() != ERROR_IO_PENDING)
return Status(::GetLastError(), eErrorTypeWin32);
bytes_written = 0;
DWORD sys_bytes_write = 0;
BOOL result =
::WriteFile(m_write, buf, size, &sys_bytes_write, &m_write_overlapped);
if (result) {
bytes_written = sys_bytes_write;
return Status();
}

DWORD failure_error = ::GetLastError();
if (failure_error != ERROR_IO_PENDING)
return Status(failure_error, eErrorTypeWin32);

BOOL result = GetOverlappedResult(m_write, &m_write_overlapped,
&sys_bytes_written, TRUE);
if (!result)
DWORD timeout = (duration == std::chrono::microseconds::zero())
? INFINITE
: duration.count() / 1000;
DWORD wait_result = ::WaitForSingleObject(m_write_overlapped.hEvent, timeout);
if (wait_result != WAIT_OBJECT_0) {
// The operation probably failed. However, if it timed out, we need to
// cancel the I/O. Between the time we returned from WaitForSingleObject
// and the time we call CancelIoEx, the operation may complete. If that
// hapens, CancelIoEx will fail and return ERROR_NOT_FOUND. If that
// happens, the original operation should be considered to have been
// successful.
bool failed = true;
failure_error = ::GetLastError();
if (wait_result == WAIT_TIMEOUT) {
BOOL cancel_result = ::CancelIoEx(m_write, &m_write_overlapped);
if (!cancel_result && ::GetLastError() == ERROR_NOT_FOUND)
failed = false;
}
if (failed)
return Status(failure_error, eErrorTypeWin32);
}

// Now we call GetOverlappedResult setting bWait to false, since we've
// already waited as long as we're willing to.
if (!::GetOverlappedResult(m_write, &m_write_overlapped, &sys_bytes_write,
FALSE))
return Status(::GetLastError(), eErrorTypeWin32);

bytes_written = sys_bytes_write;
return Status();
}
Loading

0 comments on commit ddb9869

Please sign in to comment.