[lldb] Fixed PipeWindows bugs; added Pipe::WriteWithTimeout() #101383
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@llvm/pr-subscribers-lldb Author: Dmitry Vasilyev (slydiman) ChangesFixed few bugs in PipeWindows. Added the test for async read/write. Full diff: https://github.com/llvm/llvm-project/pull/101383.diff 7 Files Affected:
diff --git a/lldb/include/lldb/Host/PipeBase.h b/lldb/include/lldb/Host/PipeBase.h
index 48c19b899cef6..d51d0cd54e036 100644
--- a/lldb/include/lldb/Host/PipeBase.h
+++ b/lldb/include/lldb/Host/PipeBase.h
@@ -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;
diff --git a/lldb/include/lldb/Host/posix/PipePosix.h b/lldb/include/lldb/Host/posix/PipePosix.h
index ec4c752a24e94..2e291160817c4 100644
--- a/lldb/include/lldb/Host/posix/PipePosix.h
+++ b/lldb/include/lldb/Host/posix/PipePosix.h
@@ -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;
diff --git a/lldb/include/lldb/Host/windows/PipeWindows.h b/lldb/include/lldb/Host/windows/PipeWindows.h
index 4b5be28d7ae6c..e28d104cc60ec 100644
--- a/lldb/include/lldb/Host/windows/PipeWindows.h
+++ b/lldb/include/lldb/Host/windows/PipeWindows.h
@@ -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,
@@ -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;
diff --git a/lldb/source/Host/common/PipeBase.cpp b/lldb/source/Host/common/PipeBase.cpp
index b3e0ab34a58df..904a2df12392d 100644
--- a/lldb/source/Host/common/PipeBase.cpp
+++ b/lldb/source/Host/common/PipeBase.cpp
@@ -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);
diff --git a/lldb/source/Host/posix/PipePosix.cpp b/lldb/source/Host/posix/PipePosix.cpp
index f35c348990df6..00c6242f3f2e8 100644
--- a/lldb/source/Host/posix/PipePosix.cpp
+++ b/lldb/source/Host/posix/PipePosix.cpp
@@ -335,7 +335,9 @@ 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())
@@ -343,7 +345,7 @@ Status PipePosix::Write(const void *buf, size_t size, size_t &bytes_written) {
const int fd = GetWriteFileDescriptorUnlocked();
SelectHelper select_helper;
- select_helper.SetTimeout(std::chrono::seconds(0));
+ select_helper.SetTimeout(timeout);
select_helper.FDSetWrite(fd);
Status error;
diff --git a/lldb/source/Host/windows/PipeWindows.cpp b/lldb/source/Host/windows/PipeWindows.cpp
index c82c919607b5b..41087d87b1e90 100644
--- a/lldb/source/Host/windows/PipeWindows.cpp
+++ b/lldb/source/Host/windows/PipeWindows.cpp
@@ -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.
@@ -105,12 +90,19 @@ 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, 1,
+ 1024, // Out buffer size
+ 1024, // In buffer size
+ 0, // Default timeout in ms, 0 means 50ms
+ &sa);
if (INVALID_HANDLE_VALUE == m_read)
return Status(::GetLastError(), eErrorTypeWin32);
m_read_fd = _open_osfhandle((intptr_t)m_read, _O_RDONLY);
@@ -177,8 +169,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());
@@ -202,6 +194,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();
@@ -228,6 +221,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;
@@ -250,6 +245,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;
@@ -280,15 +278,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
@@ -298,10 +302,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)
@@ -310,27 +314,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);
+
+ 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);
+ }
- BOOL result = GetOverlappedResult(m_write, &m_write_overlapped,
- &sys_bytes_written, TRUE);
- if (!result)
+ // 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();
}
diff --git a/lldb/unittests/Host/PipeTest.cpp b/lldb/unittests/Host/PipeTest.cpp
index 35a44ccf03733..d2ad5568c7f72 100644
--- a/lldb/unittests/Host/PipeTest.cpp
+++ b/lldb/unittests/Host/PipeTest.cpp
@@ -19,6 +19,67 @@ class PipeTest : public testing::Test {
SubsystemRAII<FileSystem, HostInfo> subsystems;
};
+TEST_F(PipeTest, WriteWithTimeout) {
+ Pipe pipe;
+ ASSERT_THAT_ERROR(pipe.CreateNew(false).ToError(), llvm::Succeeded());
+ // Note write_chunk_size must be less than pipe buffer.
+ // The pipe buffer is 1024 for PipeWindows and 4096 for PipePosix.
+ const size_t buf_size = 8192;
+ const size_t write_chunk_size = 256;
+ const size_t read_chunk_size = 300;
+ std::unique_ptr<int32_t[]> write_buf_ptr(
+ new int32_t[buf_size / sizeof(int32_t)]);
+ int32_t *write_buf = write_buf_ptr.get();
+ std::unique_ptr<int32_t[]> read_buf_ptr(
+ new int32_t[(buf_size + 100) / sizeof(int32_t)]);
+ int32_t *read_buf = read_buf_ptr.get();
+ for (int i = 0; i < buf_size / sizeof(int32_t); ++i) {
+ write_buf[i] = i;
+ read_buf[i] = -i;
+ }
+
+ char *write_ptr = (char *)write_buf;
+ size_t write_bytes = 0;
+ char *read_ptr = (char *)read_buf;
+ size_t read_bytes = 0;
+ size_t num_bytes = 0;
+ Status error;
+ while (write_bytes < buf_size) {
+ error = pipe.WriteWithTimeout(write_ptr + write_bytes, write_chunk_size,
+ std::chrono::milliseconds(10), num_bytes);
+ if (error.Fail()) {
+ ASSERT_TRUE(read_bytes < buf_size);
+ error = pipe.ReadWithTimeout(read_ptr + read_bytes, read_chunk_size,
+ std::chrono::milliseconds(10), num_bytes);
+ if (error.Fail())
+ FAIL();
+ else
+ read_bytes += num_bytes;
+ } else
+ write_bytes += num_bytes;
+ }
+ // Read the rest data.
+ while (read_bytes < buf_size) {
+ error = pipe.ReadWithTimeout(read_ptr + read_bytes, buf_size - read_bytes,
+ std::chrono::milliseconds(10), num_bytes);
+ if (error.Fail())
+ FAIL();
+ else
+ read_bytes += num_bytes;
+ }
+
+ // Be sure the pipe is empty.
+ error = pipe.ReadWithTimeout(read_ptr + read_bytes, 100,
+ std::chrono::milliseconds(10), num_bytes);
+ ASSERT_TRUE(error.Fail());
+
+ // Compare data
+ ASSERT_EQ(write_bytes, read_bytes);
+
+ for (int i = 0; i < buf_size / sizeof(int32_t); ++i)
+ ASSERT_EQ(write_buf[i], read_buf[i]);
+}
+
TEST_F(PipeTest, CreateWithUniqueName) {
Pipe pipe;
llvm::SmallString<0> name;
|
slydiman
added a commit
to slydiman/llvm-project
that referenced
this pull request
Jul 31, 2024
…t on Windows `lldb-server platform --server` works on Windows now w/o multithreading. The rest functionality remains unchanged. Depends on llvm#101383. Fixes llvm#90923, fixes llvm#56346. This is the part 1 of the replacement of llvm#100670. In the part 2 I plan to switch `lldb-server gdbserver` to use `--fd` and listen a common gdb port for all gdbserver connections. Then we can remove gdb port mapping to fix llvm#97537.
slydiman
force-pushed
the
lldb-pipe-WriteWithTimeout
branch
from
1d6edb8 to
4593037
Compare
|
@labath |
slydiman
force-pushed
the
lldb-pipe-WriteWithTimeout
branch
2 times, most recently
from
f88510e to
9c5005d
Compare
Fixed few bugs in PipeWindows. Added the test for async read/write.
slydiman
force-pushed
the
lldb-pipe-WriteWithTimeout
branch
from
9c5005d to
14a653c
Compare
slydiman
changed the title
|
I have reverted the original
We cannot use ::CreatePipe() because this pipe does not support async I/O. Note the inheritance did not work for name pipes on Windows because of missing or incorrectly initialized SECURITY_ATTRIBUTES. Note ReadWithTimeout() never worked as expected because of I have also fixed the logic of error checking. |
labath
reviewed
Aug 5, 2024
Comment on lines
101
to
104
| PIPE_TYPE_BYTE | PIPE_WAIT, 1, | ||
| 1024, // Out buffer size | ||
| 1024, // In buffer size | ||
| 0, // Default timeout in ms, 0 means 50ms |
There was a problem hiding this comment.
Suggested change
| PIPE_TYPE_BYTE | PIPE_WAIT, 1, | |
| 1024, // Out buffer size | |
| 1024, // In buffer size | |
| 0, // Default timeout in ms, 0 means 50ms | |
| PIPE_TYPE_BYTE | PIPE_WAIT, /*nMaxInstances=*/1, | |
| /*nOutBufferSize=*/1024, | |
| /*nInBufferSize=*/1024, | |
| /*nDefaultTimeOut=*/0, |
This is the llvm style.
lldb/unittests/Host/PipeTest.cpp
Outdated
| @@ -44,8 +42,70 @@ TEST_F(PipeTest, OpenAsReader) { | |||
| size_t name_len = name.size(); | |||
| name += "foobar"; | |||
| llvm::StringRef name_ref(name.data(), name_len); | |||
| // Note OpenAsReader() do nothing on Windows, the pipe is already opened for | |||
There was a problem hiding this comment.
Let's keep this change out of the patch. I think it'd be better to redesign this API to provide a better abstraction over the differences in system behavior.
lldb/unittests/Host/PipeTest.cpp
Outdated
| Pipe pipe; | ||
| ASSERT_THAT_ERROR(pipe.CreateNew(false).ToError(), llvm::Succeeded()); | ||
| // Note write_chunk_size must be less than the pipe buffer. | ||
| // The pipe buffer is 1024 for PipeWindows and 4096 for PipePosix. |
There was a problem hiding this comment.
The 4096 is actually system-dependent. It appears to be 512 on darwin, but I could imagine it can be bigger on some other system. Ideally the test would actually verify that the assumption is true. This is why I like the test plan I proposed in the other thread better. An assertion like this can be placed naturally within the code when it is structured as a sequence of steps, rather than a loop. E.g. something like this
- write to the pipe until it is full -- this automatically ensures the above condition is met
- attempt a write with a long(ish) timeout, check that it fails, and that it waits (a sufficient amount of time has passed)
- attempt a write with a short timeout, check that it does not wait too long
- drain the pipe
- check that we got what we wrote
- write to the pipe again and check that it succeeds
I hate to be a sore, but I think this provides better test coverage, and makes it clearer about what is being tested.
lldb/unittests/Host/PipeTest.cpp
Outdated
Comment on lines
62
to
71
| std::unique_ptr<int32_t[]> write_buf_ptr( | ||
| new int32_t[buf_size / sizeof(int32_t)]); | ||
| int32_t *write_buf = write_buf_ptr.get(); | ||
| std::unique_ptr<int32_t[]> read_buf_ptr( | ||
| new int32_t[(buf_size + 100) / sizeof(int32_t)]); | ||
| int32_t *read_buf = read_buf_ptr.get(); | ||
| for (int i = 0; i < buf_size / sizeof(int32_t); ++i) { | ||
| write_buf[i] = i; | ||
| read_buf[i] = -i; | ||
| } |
There was a problem hiding this comment.
std::vector<int32_t> write_buf(buf_size / sizeof(int32_t));
std:iota(write_buf.begin(), write_buf.end(), 0);
std::vector<int32_t> read_buf(write_buf.size()+100, -1);
or something similar, depending on how the rest of the test ends up looking like.
The result is shorter, and there's only /sizeof in the entire snippet.
lldb/unittests/Host/PipeTest.cpp
Outdated
Comment on lines
109
to
110
| ASSERT_EQ(write_bytes, read_bytes); | ||
| ASSERT_EQ(memcmp(write_buf, read_buf, buf_size), 0); |
There was a problem hiding this comment.
read_buf.resize(read_bytes/sizeof)
ASSERT_EQ(read_bytes, write_bytes);
point being it avoids low-level operations and has a better chance of producing a reasonable error msg.
There was a problem hiding this comment.
I do not understand what is the purpose of read_buf.resize(read_bytes/sizeof) you suggested?
We need to compare the content of write_buf and read_buf.
There was a problem hiding this comment.
Sorry, I messed that comment up. The second line was supposed to be ASSERT_EQ(read_buf, write_buf), and the purpose of the resize is to ensure the vector contains only the data that we've actually read (so that it can then be compared using operator==).
(if you can ensure the vector is of the same size already, then you can obviously skip the resize step.)
lldb/unittests/Host/PipeTest.cpp
Outdated
Comment on lines
97
to
99
| if (error.Fail()) | ||
| FAIL(); | ||
| else |
There was a problem hiding this comment.
ASSERT(error.Success()) << error.AsCString();
|
@labath Thanks for the review. I have updated everything. |
labath
approved these changes
Aug 5, 2024
lldb/unittests/Host/PipeTest.cpp
Outdated
Comment on lines
70
to
71
| char *write_ptr = (char *)&write_buf.front(); | ||
| char *read_ptr = (char *)&read_buf.front(); |
There was a problem hiding this comment.
Suggested change
| char *write_ptr = (char *)&write_buf.front(); | |
| char *read_ptr = (char *)&read_buf.front(); | |
| char *write_ptr = reinterpret_cast<char *>(write_buf.data()); | |
| char *read_ptr = reinterpret_cast<char *>read_buf.data()); |
lldb/unittests/Host/PipeTest.cpp
Outdated
| break; // The write buffer is full | ||
| write_bytes += num_bytes; | ||
| } | ||
| ASSERT_TRUE(write_bytes + write_chunk_size <= buf_size); |
There was a problem hiding this comment.
Suggested change
| ASSERT_TRUE(write_bytes + write_chunk_size <= buf_size); | |
| ASSERT_LE(write_bytes + write_chunk_size, buf_size) << "Pipe buffer larger than expected"; |
lldb/unittests/Host/PipeTest.cpp
Outdated
Comment on lines
94
to
97
| auto dur = std::chrono::duration_cast<std::chrono::milliseconds>( | ||
| std::chrono::steady_clock::now() - start_time) | ||
| .count(); | ||
| ASSERT_GE(dur, 2000); |
There was a problem hiding this comment.
Suggested change
| auto dur = std::chrono::duration_cast<std::chrono::milliseconds>( | |
| std::chrono::steady_clock::now() - start_time) | |
| .count(); | |
| ASSERT_GE(dur, 2000); | |
| auto dur = std::chrono::steady_clock::now() - start_time; | |
| ASSERT_GE(dur, std::chrono::seconds(2)); |
If it compiles (which I think it should).
lldb/unittests/Host/PipeTest.cpp
Outdated
| auto start_time = std::chrono::steady_clock::now(); | ||
| ASSERT_THAT_ERROR( | ||
| pipe.WriteWithTimeout(write_ptr + write_bytes, write_chunk_size, | ||
| std::chrono::milliseconds(2000), num_bytes) |
There was a problem hiding this comment.
Suggested change
| std::chrono::milliseconds(2000), num_bytes) | |
| std::chrono::seconds(2), num_bytes) |
lldb/unittests/Host/PipeTest.cpp
Outdated
Comment on lines
109
to
110
| ASSERT_GE(dur, 200); | ||
| ASSERT_LT(dur, 300); |
There was a problem hiding this comment.
This is going to be flaky when the test is run on a loaded machine. To be safe, this needs to be at least an order of magnitude larger than the expected timeout (so e.g. to check that this took less than 2 seconds). But that's fine, the main thing I wanted to check by this is that it does not wait ~forever (like with the bug you found where we multiplied by 1000 instead of dividing).
Comment on lines
+131
to
+133
| ASSERT_TRUE(std::equal(write_buf.begin(), | ||
| write_buf.begin() + write_bytes / sizeof(uint32_t), | ||
| read_buf.begin())); |
slydiman
force-pushed
the
lldb-pipe-WriteWithTimeout
branch
from
d4bed52 to
b06518a
Compare
banach-space
pushed a commit
to banach-space/llvm-project
that referenced
this pull request
Aug 7, 2024
…01383) Added also the test for async read/write.
kstoimenov
pushed a commit
to kstoimenov/llvm-project
that referenced
this pull request
Aug 15, 2024
…01383) Added also the test for async read/write.
adrian-prantl
pushed a commit
to adrian-prantl/llvm-project
that referenced
this pull request
Oct 16, 2024
adrian-prantl
added a commit
to swiftlang/llvm-project
that referenced
this pull request
Oct 17, 2024
[lldb] Fixed PipeWindows bugs; added Pipe::WriteWithTimeout() (llvm#101383)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Added also the test for async read/write.