labath created this revision.
labath added reviewers: clayborg, zturner.
labath added a subscriber: lldb-commits.
Herald added subscribers: danalbert, tberghammer.
AdbClient was attempting to handle the case where the socket input arrived in
pieces, but it was
failing to handle the case where the connection was closed before that
happened. In this case, it
would just spin in an infinite loop calling Connection::Read. (This was also
the cause of the
spurious timeouts on the darwin->android buildbot. The exact cause of the
premature EOF remains
to be investigated, but is likely a server bug.)
Since this wait-for-a-certain-number-of-bytes seems like a useful functionality
to have, I am
moving it (with the infinite loop fixed) to the Connection class, and adding an
appropriate test for it.
http://reviews.llvm.org/D19533
Files:
include/lldb/Core/Connection.h
source/Core/Connection.cpp
source/Plugins/Platform/Android/AdbClient.cpp
unittests/Core/CMakeLists.txt
unittests/Core/ConnectionTest.cpp
Index: unittests/Core/ConnectionTest.cpp
===================================================================
--- /dev/null
+++ unittests/Core/ConnectionTest.cpp
@@ -0,0 +1,123 @@
+//===-- ConnectionTest.cpp --------------------------------------*- C++ -*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#if defined(_MSC_VER) && (_HAS_EXCEPTIONS == 0)
+// Workaround for MSVC standard library bug, which fails to include <thread> when
+// exceptions are disabled.
+#include <eh.h>
+#endif
+
+#include <future>
+
+#include "gtest/gtest.h"
+
+#include "lldb/Core/Error.h"
+#include "lldb/Host/ConnectionFileDescriptor.h"
+#include "lldb/Host/common/TCPSocket.h"
+
+using namespace lldb_private;
+using namespace lldb;
+
+class ConnectionTest : public testing::Test
+{
+public:
+ void
+ SetUp() override
+ {
+#if defined(_MSC_VER)
+ WSADATA data;
+ ::WSAStartup(MAKEWORD(2, 2), &data);
+#endif
+ }
+
+ void
+ TearDown() override
+ {
+#if defined(_MSC_VER)
+ ::WSACleanup();
+#endif
+ }
+};
+
+TEST_F(ConnectionTest, ReadAll)
+{
+ const bool child_process_inherit = false;
+ Error error;
+ TCPSocket listen_socket(child_process_inherit, error);
+ ASSERT_TRUE(error.Success()) << error.AsCString();
+
+ error = listen_socket.Listen("localhost:0", 1);
+ ASSERT_TRUE(error.Success()) << error.AsCString();
+
+ uint16_t listen_port = listen_socket.GetLocalPortNumber();
+
+ std::unique_ptr<Socket> accepted_socket;
+ ConnectionFileDescriptor conn;
+ {
+ // Scope for accepted_socket_ptr.
+ Socket *accepted_socket_ptr;
+ std::future<Error> future_error = std::async(std::launch::async, [&listen_socket, &accepted_socket_ptr]() {
+ return listen_socket.Accept("localhost:0", child_process_inherit, accepted_socket_ptr);
+ });
+
+ std::ostringstream connect_url;
+ connect_url << "connect://localhost:" << listen_port;
+
+ conn.Connect(connect_url.str().c_str(), &error);
+ ASSERT_TRUE(error.Success()) << error.AsCString();
+ ASSERT_TRUE(future_error.get().Success()) << future_error.get().AsCString();
+ accepted_socket.reset(accepted_socket_ptr);
+ }
+
+ // First, make sure ReadAll returns nothing.
+ const uint32_t k_reasonable_timeout_us = 10 * 1000;
+ char buffer[100];
+ ConnectionStatus status;
+ size_t bytes_read = conn.ReadAll(buffer, sizeof buffer, k_reasonable_timeout_us, status, &error);
+ ASSERT_TRUE(error.Success()) << error.AsCString();
+ ASSERT_EQ(eConnectionStatusTimedOut, status);
+ ASSERT_EQ(0u, bytes_read);
+
+ // Write some data, and make sure it arrives.
+ const char data[] = {1, 2, 3, 4};
+ size_t bytes_written = sizeof data;
+ error = accepted_socket->Write(data, bytes_written);
+ ASSERT_TRUE(error.Success()) << error.AsCString();
+ ASSERT_EQ(sizeof data, bytes_written);
+ bytes_read = conn.ReadAll(buffer, sizeof data, k_reasonable_timeout_us, status, &error);
+ ASSERT_TRUE(error.Success()) << error.AsCString();
+ ASSERT_EQ(eConnectionStatusSuccess, status);
+ ASSERT_EQ(sizeof data, bytes_read);
+ ASSERT_EQ(0, memcmp(buffer, data, sizeof data));
+ memset(buffer, 0, sizeof buffer);
+
+ // Write the data in two chunks. Make we read all of it.
+ std::future<Error> future_error = std::async(std::launch::async, [&accepted_socket, data]() {
+ size_t bytes_written = sizeof(data) / 2;
+ Error error = accepted_socket->Write(data, bytes_written);
+ if (error.Fail())
+ return error;
+ std::this_thread::sleep_for(std::chrono::microseconds(k_reasonable_timeout_us / 10));
+ bytes_written = sizeof(data) / 2;
+ return accepted_socket->Write(data + bytes_written, bytes_written);
+ });
+ bytes_read = conn.ReadAll(buffer, sizeof data, k_reasonable_timeout_us, status, &error);
+ ASSERT_TRUE(error.Success()) << error.AsCString();
+ ASSERT_EQ(eConnectionStatusSuccess, status);
+ ASSERT_EQ(sizeof data, bytes_read);
+ ASSERT_TRUE(future_error.get().Success()) << future_error.get().AsCString();
+ ASSERT_EQ(0, memcmp(buffer, data, sizeof data));
+
+ // Close the remote end, make sure ReadAll result is reasonable.
+ accepted_socket.reset();
+ bytes_read = conn.ReadAll(buffer, sizeof buffer, k_reasonable_timeout_us, status, &error);
+ ASSERT_TRUE(error.Success()) << error.AsCString();
+ ASSERT_EQ(eConnectionStatusEndOfFile, status);
+ ASSERT_EQ(0u, bytes_read);
+}
Index: unittests/Core/CMakeLists.txt
===================================================================
--- unittests/Core/CMakeLists.txt
+++ unittests/Core/CMakeLists.txt
@@ -1,4 +1,5 @@
add_lldb_unittest(LLDBCoreTests
+ ConnectionTest.cpp
DataExtractorTest.cpp
ScalarTest.cpp
)
Index: source/Plugins/Platform/Android/AdbClient.cpp
===================================================================
--- source/Plugins/Platform/Android/AdbClient.cpp
+++ source/Plugins/Platform/Android/AdbClient.cpp
@@ -492,17 +492,12 @@
{
Error error;
ConnectionStatus status;
- char *read_buffer = static_cast<char*>(buffer);
-
- size_t tota_read_bytes = 0;
- while (tota_read_bytes < size)
- {
- auto read_bytes = m_conn.Read (read_buffer + tota_read_bytes, size - tota_read_bytes, kReadTimeout, status, &error);
- if (error.Fail ())
- return error;
- tota_read_bytes += read_bytes;
- }
- return error;
+ size_t read_bytes = m_conn.ReadAll(buffer, size, kReadTimeout, status, &error);
+ if (error.Fail())
+ return error;
+ if (read_bytes < size)
+ return Error("Unable to read full buffer.");
+ return Error();
}
Error
Index: source/Core/Connection.cpp
===================================================================
--- source/Core/Connection.cpp
+++ source/Core/Connection.cpp
@@ -19,6 +19,7 @@
#include "lldb/Host/ConnectionFileDescriptor.h"
+using namespace lldb;
using namespace lldb_private;
Connection::Connection ()
@@ -38,3 +39,31 @@
#endif
return new ConnectionFileDescriptor();
}
+
+size_t
+Connection::ReadAll(void *dst, size_t dst_len, uint32_t timeout_usec, ConnectionStatus &status, Error *error_ptr)
+{
+ char *read_buffer = static_cast<char *>(dst);
+
+ size_t total_read_bytes = 0;
+ while (total_read_bytes < dst_len)
+ {
+ size_t read_bytes =
+ Read(read_buffer + total_read_bytes, dst_len - total_read_bytes, timeout_usec, status, error_ptr);
+ total_read_bytes += read_bytes;
+ switch (status)
+ {
+ case eConnectionStatusSuccess:
+ break; // Try again.
+
+ case eConnectionStatusEndOfFile:
+ case eConnectionStatusError:
+ case eConnectionStatusTimedOut:
+ case eConnectionStatusNoConnection:
+ case eConnectionStatusLostConnection:
+ case eConnectionStatusInterrupted:
+ return total_read_bytes;
+ }
+ }
+ return total_read_bytes;
+}
Index: include/lldb/Core/Connection.h
===================================================================
--- include/lldb/Core/Connection.h
+++ include/lldb/Core/Connection.h
@@ -101,7 +101,9 @@
IsConnected () const = 0;
//------------------------------------------------------------------
- /// The read function that attempts to read from the connection.
+ /// The read function that attempts to read from the connection. The
+ /// function returns as soon as at least part of the data is
+ /// available.
///
/// @param[in] dst
/// A destination buffer that must be at least \a dst_len bytes
@@ -136,6 +138,41 @@
Error *error_ptr) = 0;
//------------------------------------------------------------------
+ /// The read function that attempts to read from the connection.
+ /// This function will attempt to read the full buffer. If the
+ /// buffer cannot be filled, the function will set the connection
+ /// status to a value other than eConnectionStatusSuccess (the exact
+ /// value depends on the error cause).
+ ///
+ /// @param[in] dst
+ /// A destination buffer that must be at least \a dst_len bytes
+ /// long.
+ ///
+ /// @param[in] dst_len
+ /// The number of bytes to attempt to read, and also the max
+ /// number of bytes that can be placed into \a dst.
+ ///
+ /// @param[in] timeout_usec
+ /// The number of microseconds to wait for the data.
+ ///
+ /// @param[out] status
+ /// On return, indicates whether the call was successful or terminated
+ /// due to some error condition.
+ ///
+ /// @param[out] error_ptr
+ /// A pointer to an error object that should be given an
+ /// appropriate error value if this method returns zero. This
+ /// value can be NULL if the error value should be ignored.
+ ///
+ /// @return
+ /// The number of bytes actually read.
+ ///
+ /// @see size_t Communication::Read (void *, size_t, uint32_t);
+ //------------------------------------------------------------------
+ size_t
+ ReadAll(void *dst, size_t dst_len, uint32_t timeout_usec, lldb::ConnectionStatus &status, Error *error_ptr);
+
+ //------------------------------------------------------------------
/// The actual write function that attempts to write to the
/// communications protocol.
///
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
