From 1ebc85f86fd423db1366ba30ec029f19182bd4ea Mon Sep 17 00:00:00 2001 From: Pavel Labath Date: Thu, 9 Nov 2017 15:45:09 +0000 Subject: [PATCH] llgs-tests: Replace the "log+return false" pattern with llvm::Error Summary: These tests used to log the error message and return plain bool mainly because at the time they we written, we did not have a nice way to assert on llvm::Error values. That is no longer true, so replace this pattern with a more idiomatic approach. As a part of this patch, I also move the formatting of GDBRemoteCommunication::PacketResult values out of the test code, as that can be useful elsewhere. Reviewers: zturner, eugene Subscribers: mgorny, lldb-commits Differential Revision: https://reviews.llvm.org/D39790 llvm-svn: 317795 --- .../Process/gdb-remote/GDBRemoteCommunication.cpp | 36 +++++ .../Process/gdb-remote/GDBRemoteCommunication.h | 10 ++ .../tools/lldb-server/tests/CMakeLists.txt | 2 + .../tools/lldb-server/tests/MessageObjects.cpp | 17 +-- .../tools/lldb-server/tests/MessageObjects.h | 2 +- .../tools/lldb-server/tests/TestClient.cpp | 160 +++++++-------------- .../unittests/tools/lldb-server/tests/TestClient.h | 28 ++-- .../lldb-server/tests/ThreadIdsInJstopinfoTest.cpp | 18 +-- 8 files changed, 133 insertions(+), 140 deletions(-) diff --git a/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunication.cpp b/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunication.cpp index ac475fa..c3f0d5d 100644 --- a/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunication.cpp +++ b/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunication.cpp @@ -1376,3 +1376,39 @@ void GDBRemoteCommunication::AppendBytesToCache(const uint8_t *bytes, } } } + +void llvm::format_provider::format( + const GDBRemoteCommunication::PacketResult &result, raw_ostream &Stream, + StringRef Style) { + using PacketResult = GDBRemoteCommunication::PacketResult; + + switch (result) { + case PacketResult::Success: + Stream << "Success"; + break; + case PacketResult::ErrorSendFailed: + Stream << "ErrorSendFailed"; + break; + case PacketResult::ErrorSendAck: + Stream << "ErrorSendAck"; + break; + case PacketResult::ErrorReplyFailed: + Stream << "ErrorReplyFailed"; + break; + case PacketResult::ErrorReplyTimeout: + Stream << "ErrorReplyTimeout"; + break; + case PacketResult::ErrorReplyInvalid: + Stream << "ErrorReplyInvalid"; + break; + case PacketResult::ErrorReplyAck: + Stream << "ErrorReplyAck"; + break; + case PacketResult::ErrorDisconnected: + Stream << "ErrorDisconnected"; + break; + case PacketResult::ErrorNoSequenceLock: + Stream << "ErrorNoSequenceLock"; + break; + } +} diff --git a/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunication.h b/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunication.h index ce90de3..ecc9386 100644 --- a/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunication.h +++ b/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunication.h @@ -290,4 +290,14 @@ private: } // namespace process_gdb_remote } // namespace lldb_private +namespace llvm { +template <> +struct format_provider< + lldb_private::process_gdb_remote::GDBRemoteCommunication::PacketResult> { + static void format(const lldb_private::process_gdb_remote:: + GDBRemoteCommunication::PacketResult &state, + raw_ostream &Stream, StringRef Style); +}; +} // namespace llvm + #endif // liblldb_GDBRemoteCommunication_h_ diff --git a/lldb/unittests/tools/lldb-server/tests/CMakeLists.txt b/lldb/unittests/tools/lldb-server/tests/CMakeLists.txt index 4ef4165..a0cac01 100644 --- a/lldb/unittests/tools/lldb-server/tests/CMakeLists.txt +++ b/lldb/unittests/tools/lldb-server/tests/CMakeLists.txt @@ -10,6 +10,8 @@ add_lldb_unittest(LLDBServerTests lldbTarget lldbPluginPlatformLinux lldbPluginProcessGDBRemote + + LLVMTestingSupport LINK_COMPONENTS Support ) diff --git a/lldb/unittests/tools/lldb-server/tests/MessageObjects.cpp b/lldb/unittests/tools/lldb-server/tests/MessageObjects.cpp index 158a2dd..f396963 100644 --- a/lldb/unittests/tools/lldb-server/tests/MessageObjects.cpp +++ b/lldb/unittests/tools/lldb-server/tests/MessageObjects.cpp @@ -65,19 +65,16 @@ StringRef ThreadInfo::ReadRegister(unsigned int register_id) const { return m_registers.lookup(register_id); } -bool ThreadInfo::ReadRegisterAsUint64(unsigned int register_id, - uint64_t &value) const { +Expected +ThreadInfo::ReadRegisterAsUint64(unsigned int register_id) const { + uint64_t value; std::string value_str(m_registers.lookup(register_id)); - if (!llvm::to_integer(value_str, value, 16)) { - GTEST_LOG_(ERROR) - << formatv("ThreadInfo: Unable to parse register value at {0}.", - register_id) - .str(); - return false; - } + if (!llvm::to_integer(value_str, value, 16)) + return make_parsing_error("ThreadInfo value for register {0}: {1}", + register_id, value_str); sys::swapByteOrder(value); - return true; + return value; } //====== JThreadsInfo ========================================================== diff --git a/lldb/unittests/tools/lldb-server/tests/MessageObjects.h b/lldb/unittests/tools/lldb-server/tests/MessageObjects.h index 64525a6..19982d6 100644 --- a/lldb/unittests/tools/lldb-server/tests/MessageObjects.h +++ b/lldb/unittests/tools/lldb-server/tests/MessageObjects.h @@ -48,7 +48,7 @@ public: const RegisterMap ®isters, unsigned int signal); llvm::StringRef ReadRegister(unsigned int register_id) const; - bool ReadRegisterAsUint64(unsigned int register_id, uint64_t &value) const; + llvm::Expected ReadRegisterAsUint64(unsigned int register_id) const; private: std::string m_name; diff --git a/lldb/unittests/tools/lldb-server/tests/TestClient.cpp b/lldb/unittests/tools/lldb-server/tests/TestClient.cpp index 12b9420..f5352ee 100644 --- a/lldb/unittests/tools/lldb-server/tests/TestClient.cpp +++ b/lldb/unittests/tools/lldb-server/tests/TestClient.cpp @@ -42,7 +42,7 @@ TestClient::TestClient(const std::string &test_name, TestClient::~TestClient() {} -bool TestClient::StartDebugger() { +llvm::Error TestClient::StartDebugger() { const ArchSpec &arch_spec = HostInfo::GetArchitecture(); Args args; args.AppendArgument(LLDB_SERVER); @@ -57,13 +57,11 @@ bool TestClient::StartDebugger() { if (log_file_name.size()) args.AppendArgument("--log-file=" + log_file_name); - Status error; + Status status; TCPSocket listen_socket(true, false); - error = listen_socket.Listen("127.0.0.1:0", 5); - if (error.Fail()) { - GTEST_LOG_(ERROR) << "Unable to open listen socket."; - return false; - } + status = listen_socket.Listen("127.0.0.1:0", 5); + if (status.Fail()) + return status.ToError(); char connect_remote_address[64]; snprintf(connect_remote_address, sizeof(connect_remote_address), @@ -73,12 +71,9 @@ bool TestClient::StartDebugger() { m_server_process_info.SetArchitecture(arch_spec); m_server_process_info.SetArguments(args, true); - Status status = Host::LaunchProcess(m_server_process_info); - if (status.Fail()) { - GTEST_LOG_(ERROR) - << formatv("Failure to launch lldb server: {0}.", status).str(); - return false; - } + status = Host::LaunchProcess(m_server_process_info); + if (status.Fail()) + return status.ToError(); char connect_remote_uri[64]; snprintf(connect_remote_uri, sizeof(connect_remote_uri), "connect://%s", @@ -88,10 +83,10 @@ bool TestClient::StartDebugger() { SetConnection(new ConnectionFileDescriptor(accept_socket)); SendAck(); // Send this as a handshake. - return true; + return llvm::Error::success(); } -bool TestClient::StopDebugger() { +llvm::Error TestClient::StopDebugger() { std::string response; // Debugserver (non-conformingly?) sends a reply to the k packet instead of // simply closing the connection. @@ -100,13 +95,14 @@ bool TestClient::StopDebugger() { return SendMessage("k", response, result); } -bool TestClient::SetInferior(llvm::ArrayRef inferior_args) { +Error TestClient::SetInferior(llvm::ArrayRef inferior_args) { StringList env; Host::GetEnvironment(env); for (size_t i = 0; i < env.GetSize(); ++i) { if (SendEnvironmentPacket(env[i].c_str()) != 0) { - GTEST_LOG_(ERROR) << "failed to set environment variable `" << env[i] << "`"; - return false; + return make_error( + formatv("Failed to set environment variable: {0}", env[i]).str(), + inconvertibleErrorCode()); } } std::stringstream command; @@ -118,36 +114,32 @@ bool TestClient::SetInferior(llvm::ArrayRef inferior_args) { command << hex_encoded.size() << ',' << i << ',' << hex_encoded; } - if (!SendMessage(command.str())) - return false; - if (!SendMessage("qLaunchSuccess")) - return false; + if (Error E = SendMessage(command.str())) + return E; + if (Error E = SendMessage("qLaunchSuccess")) + return E; std::string response; - if (!SendMessage("qProcessInfo", response)) - return false; + if (Error E = SendMessage("qProcessInfo", response)) + return E; auto create_or_error = ProcessInfo::Create(response); - if (auto create_error = create_or_error.takeError()) { - GTEST_LOG_(ERROR) << toString(std::move(create_error)); - return false; - } + if (auto create_error = create_or_error.takeError()) + return create_error; m_process_info = *create_or_error; - return true; + return Error::success(); } -bool TestClient::ListThreadsInStopReply() { +Error TestClient::ListThreadsInStopReply() { return SendMessage("QListThreadsInStopReply"); } -bool TestClient::SetBreakpoint(unsigned long address) { - std::stringstream command; - command << "Z0," << std::hex << address << ",1"; - return SendMessage(command.str()); +Error TestClient::SetBreakpoint(unsigned long address) { + return SendMessage(formatv("Z0,{0:x-},1", address).str()); } -bool TestClient::ContinueAll() { return Continue("vCont;c"); } +Error TestClient::ContinueAll() { return Continue("vCont;c"); } -bool TestClient::ContinueThread(unsigned long thread_id) { +Error TestClient::ContinueThread(unsigned long thread_id) { return Continue(formatv("vCont;c:{0:x-}", thread_id).str()); } @@ -155,7 +147,7 @@ const ProcessInfo &TestClient::GetProcessInfo() { return *m_process_info; } Optional TestClient::GetJThreadsInfo() { std::string response; - if (!SendMessage("jThreadsInfo", response)) + if (SendMessage("jThreadsInfo", response)) return llvm::None; auto creation = JThreadsInfo::Create(response, m_process_info->GetEndian()); if (auto create_error = creation.takeError()) { @@ -170,36 +162,37 @@ const StopReply &TestClient::GetLatestStopReply() { return m_stop_reply.getValue(); } -bool TestClient::SendMessage(StringRef message) { +Error TestClient::SendMessage(StringRef message) { std::string dummy_string; return SendMessage(message, dummy_string); } -bool TestClient::SendMessage(StringRef message, std::string &response_string) { - if (!SendMessage(message, response_string, PacketResult::Success)) - return false; - else if (response_string[0] == 'E') { - GTEST_LOG_(ERROR) << "Error " << response_string - << " while sending message: " << message.str(); - return false; +Error TestClient::SendMessage(StringRef message, std::string &response_string) { + if (Error E = SendMessage(message, response_string, PacketResult::Success)) + return E; + if (response_string[0] == 'E') { + return make_error( + formatv("Error `{0}` while sending message: {1}", response_string, + message) + .str(), + inconvertibleErrorCode()); } - - return true; + return Error::success(); } -bool TestClient::SendMessage(StringRef message, std::string &response_string, - PacketResult expected_result) { +Error TestClient::SendMessage(StringRef message, std::string &response_string, + PacketResult expected_result) { StringExtractorGDBRemote response; GTEST_LOG_(INFO) << "Send Packet: " << message.str(); PacketResult result = SendPacketAndWaitForResponse(message, response, false); response.GetEscapedBinaryData(response_string); GTEST_LOG_(INFO) << "Read Packet: " << response_string; - if (result != expected_result) { - GTEST_LOG_(ERROR) << FormatFailedResult(message, result); - return false; - } + if (result != expected_result) + return make_error( + formatv("Error sending message `{0}`: {1}", message, result).str(), + inconvertibleErrorCode()); - return true; + return Error::success(); } unsigned int TestClient::GetPcRegisterId() { @@ -209,7 +202,7 @@ unsigned int TestClient::GetPcRegisterId() { for (unsigned int register_id = 0;; register_id++) { std::string message = formatv("qRegisterInfo{0:x-}", register_id).str(); std::string response; - if (!SendMessage(message, response)) { + if (SendMessage(message, response)) { GTEST_LOG_(ERROR) << "Unable to query register ID for PC register."; return UINT_MAX; } @@ -231,23 +224,18 @@ unsigned int TestClient::GetPcRegisterId() { return m_pc_register; } -bool TestClient::Continue(StringRef message) { - if (!m_process_info.hasValue()) { - GTEST_LOG_(ERROR) << "Continue() called before m_process_info initialized."; - return false; - } +Error TestClient::Continue(StringRef message) { + assert(m_process_info.hasValue()); std::string response; - if (!SendMessage(message, response)) - return false; + if (Error E = SendMessage(message, response)) + return E; auto creation = StopReply::Create(response, m_process_info->GetEndian()); - if (auto create_error = creation.takeError()) { - GTEST_LOG_(ERROR) << toString(std::move(create_error)); - return false; - } + if (Error E = creation.takeError()) + return E; m_stop_reply = std::move(*creation); - return true; + return Error::success(); } std::string TestClient::GenerateLogFileName(const ArchSpec &arch) const { @@ -267,42 +255,4 @@ std::string TestClient::GenerateLogFileName(const ArchSpec &arch) const { return log_file.str(); } -std::string TestClient::FormatFailedResult(const std::string &message, - PacketResult result) { - std::string formatted_error; - raw_string_ostream error_stream(formatted_error); - error_stream << "Failure sending message: " << message << " Result: "; - - switch (result) { - case PacketResult::ErrorSendFailed: - error_stream << "ErrorSendFailed"; - break; - case PacketResult::ErrorSendAck: - error_stream << "ErrorSendAck"; - break; - case PacketResult::ErrorReplyFailed: - error_stream << "ErrorReplyFailed"; - break; - case PacketResult::ErrorReplyTimeout: - error_stream << "ErrorReplyTimeout"; - break; - case PacketResult::ErrorReplyInvalid: - error_stream << "ErrorReplyInvalid"; - break; - case PacketResult::ErrorReplyAck: - error_stream << "ErrorReplyAck"; - break; - case PacketResult::ErrorDisconnected: - error_stream << "ErrorDisconnected"; - break; - case PacketResult::ErrorNoSequenceLock: - error_stream << "ErrorNoSequenceLock"; - break; - default: - error_stream << "Unknown Error"; - } - - error_stream.str(); - return formatted_error; -} } // namespace llgs_tests diff --git a/lldb/unittests/tools/lldb-server/tests/TestClient.h b/lldb/unittests/tools/lldb-server/tests/TestClient.h index 378e9c2..b228c92 100644 --- a/lldb/unittests/tools/lldb-server/tests/TestClient.h +++ b/lldb/unittests/tools/lldb-server/tests/TestClient.h @@ -27,27 +27,25 @@ public: TestClient(const std::string &test_name, const std::string &test_case_name); virtual ~TestClient(); - LLVM_NODISCARD bool StartDebugger(); - LLVM_NODISCARD bool StopDebugger(); - LLVM_NODISCARD bool SetInferior(llvm::ArrayRef inferior_args); - LLVM_NODISCARD bool ListThreadsInStopReply(); - LLVM_NODISCARD bool SetBreakpoint(unsigned long address); - LLVM_NODISCARD bool ContinueAll(); - LLVM_NODISCARD bool ContinueThread(unsigned long thread_id); + llvm::Error StartDebugger(); + llvm::Error StopDebugger(); + llvm::Error SetInferior(llvm::ArrayRef inferior_args); + llvm::Error ListThreadsInStopReply(); + llvm::Error SetBreakpoint(unsigned long address); + llvm::Error ContinueAll(); + llvm::Error ContinueThread(unsigned long thread_id); const ProcessInfo &GetProcessInfo(); llvm::Optional GetJThreadsInfo(); const StopReply &GetLatestStopReply(); - LLVM_NODISCARD bool SendMessage(llvm::StringRef message); - LLVM_NODISCARD bool SendMessage(llvm::StringRef message, - std::string &response_string); - LLVM_NODISCARD bool SendMessage(llvm::StringRef message, - std::string &response_string, - PacketResult expected_result); + llvm::Error SendMessage(llvm::StringRef message); + llvm::Error SendMessage(llvm::StringRef message, + std::string &response_string); + llvm::Error SendMessage(llvm::StringRef message, std::string &response_string, + PacketResult expected_result); unsigned int GetPcRegisterId(); private: - LLVM_NODISCARD bool Continue(llvm::StringRef message); - LLVM_NODISCARD bool GenerateConnectionAddress(std::string &address); + llvm::Error Continue(llvm::StringRef message); std::string GenerateLogFileName(const lldb_private::ArchSpec &arch) const; std::string FormatFailedResult( const std::string &message, diff --git a/lldb/unittests/tools/lldb-server/tests/ThreadIdsInJstopinfoTest.cpp b/lldb/unittests/tools/lldb-server/tests/ThreadIdsInJstopinfoTest.cpp index 961b0a3..a97df44 100644 --- a/lldb/unittests/tools/lldb-server/tests/ThreadIdsInJstopinfoTest.cpp +++ b/lldb/unittests/tools/lldb-server/tests/ThreadIdsInJstopinfoTest.cpp @@ -8,6 +8,7 @@ //===----------------------------------------------------------------------===// #include "TestClient.h" +#include "llvm/Testing/Support/Error.h" #include "gtest/gtest.h" #include @@ -27,10 +28,10 @@ TEST_F(ThreadsInJstopinfoTest, TestStopReplyContainsThreadPcsLlgs) { auto test_info = ::testing::UnitTest::GetInstance()->current_test_info(); TestClient client(test_info->name(), test_info->test_case_name()); - ASSERT_TRUE(client.StartDebugger()); - ASSERT_TRUE(client.SetInferior(inferior_args)); - ASSERT_TRUE(client.ListThreadsInStopReply()); - ASSERT_TRUE(client.ContinueAll()); + ASSERT_THAT_ERROR(client.StartDebugger(), llvm::Succeeded()); + ASSERT_THAT_ERROR(client.SetInferior(inferior_args), llvm::Succeeded()); + ASSERT_THAT_ERROR(client.ListThreadsInStopReply(), llvm::Succeeded()); + ASSERT_THAT_ERROR(client.ContinueAll(), llvm::Succeeded()); unsigned int pc_reg = client.GetPcRegisterId(); ASSERT_NE(pc_reg, UINT_MAX); @@ -47,12 +48,11 @@ TEST_F(ThreadsInJstopinfoTest, TestStopReplyContainsThreadPcsLlgs) { unsigned long tid = stop_reply_pc.first; ASSERT_TRUE(thread_infos.find(tid) != thread_infos.end()) << "Thread ID: " << tid << " not in JThreadsInfo."; - uint64_t pc_value; - ASSERT_TRUE(thread_infos[tid].ReadRegisterAsUint64(pc_reg, pc_value)) - << "Failure reading ThreadInfo register " << pc_reg; - ASSERT_EQ(stop_reply_pcs[tid], pc_value) + auto pc_value = thread_infos[tid].ReadRegisterAsUint64(pc_reg); + ASSERT_THAT_EXPECTED(pc_value, llvm::Succeeded()); + ASSERT_EQ(stop_reply_pcs[tid], *pc_value) << "Mismatched PC for thread: " << tid; } - ASSERT_TRUE(client.StopDebugger()); + ASSERT_THAT_ERROR(client.StopDebugger(), llvm::Succeeded()); } -- 2.7.4