labath created this revision.
Herald added a subscriber: mgorny.
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.
https://reviews.llvm.org/D39790
Files:
source/Plugins/Process/gdb-remote/GDBRemoteCommunication.cpp
source/Plugins/Process/gdb-remote/GDBRemoteCommunication.h
unittests/tools/lldb-server/tests/CMakeLists.txt
unittests/tools/lldb-server/tests/MessageObjects.cpp
unittests/tools/lldb-server/tests/MessageObjects.h
unittests/tools/lldb-server/tests/TestClient.cpp
unittests/tools/lldb-server/tests/TestClient.h
unittests/tools/lldb-server/tests/ThreadIdsInJstopinfoTest.cpp
Index: unittests/tools/lldb-server/tests/ThreadIdsInJstopinfoTest.cpp
===================================================================
--- unittests/tools/lldb-server/tests/ThreadIdsInJstopinfoTest.cpp
+++ 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 <string>
@@ -27,10 +28,10 @@
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 @@
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());
}
Index: unittests/tools/lldb-server/tests/TestClient.h
===================================================================
--- unittests/tools/lldb-server/tests/TestClient.h
+++ unittests/tools/lldb-server/tests/TestClient.h
@@ -27,27 +27,25 @@
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<std::string> 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<std::string> 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<JThreadsInfo> 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,
Index: unittests/tools/lldb-server/tests/TestClient.cpp
===================================================================
--- unittests/tools/lldb-server/tests/TestClient.cpp
+++ unittests/tools/lldb-server/tests/TestClient.cpp
@@ -41,7 +41,7 @@
TestClient::~TestClient() {}
-bool TestClient::StartDebugger() {
+llvm::Error TestClient::StartDebugger() {
const ArchSpec &arch_spec = HostInfo::GetArchitecture();
Args args;
args.AppendArgument(LLDB_SERVER);
@@ -56,13 +56,11 @@
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),
@@ -72,12 +70,9 @@
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",
@@ -87,25 +82,26 @@
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.
PacketResult result =
IsDebugServer() ? PacketResult::Success : PacketResult::ErrorDisconnected;
return SendMessage("k", response, result);
}
-bool TestClient::SetInferior(llvm::ArrayRef<std::string> inferior_args) {
+Error TestClient::SetInferior(llvm::ArrayRef<std::string> 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<StringError>(
+ formatv("Failed to set environment variable: {0}", env[i]).str(),
+ inconvertibleErrorCode());
}
}
std::stringstream command;
@@ -117,44 +113,40 @@
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());
}
const ProcessInfo &TestClient::GetProcessInfo() { return *m_process_info; }
Optional<JThreadsInfo> 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()) {
@@ -169,36 +161,37 @@
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<StringError>(
+ 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<StringError>(
+ formatv("Error sending message `{0}`: {1}", message, result).str(),
+ inconvertibleErrorCode());
- return true;
+ return Error::success();
}
unsigned int TestClient::GetPcRegisterId() {
@@ -208,7 +201,7 @@
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;
}
@@ -230,23 +223,18 @@
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 {
@@ -266,42 +254,4 @@
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
Index: unittests/tools/lldb-server/tests/MessageObjects.h
===================================================================
--- unittests/tools/lldb-server/tests/MessageObjects.h
+++ unittests/tools/lldb-server/tests/MessageObjects.h
@@ -48,7 +48,7 @@
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<uint64_t> ReadRegisterAsUint64(unsigned int register_id) const;
private:
std::string m_name;
Index: unittests/tools/lldb-server/tests/MessageObjects.cpp
===================================================================
--- unittests/tools/lldb-server/tests/MessageObjects.cpp
+++ unittests/tools/lldb-server/tests/MessageObjects.cpp
@@ -65,19 +65,16 @@
return m_registers.lookup(register_id);
}
-bool ThreadInfo::ReadRegisterAsUint64(unsigned int register_id,
- uint64_t &value) const {
+Expected<uint64_t>
+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 ==========================================================
Index: unittests/tools/lldb-server/tests/CMakeLists.txt
===================================================================
--- unittests/tools/lldb-server/tests/CMakeLists.txt
+++ unittests/tools/lldb-server/tests/CMakeLists.txt
@@ -10,6 +10,8 @@
lldbTarget
lldbPluginPlatformLinux
lldbPluginProcessGDBRemote
+
+ LLVMTestingSupport
LINK_COMPONENTS
Support
)
Index: source/Plugins/Process/gdb-remote/GDBRemoteCommunication.h
===================================================================
--- source/Plugins/Process/gdb-remote/GDBRemoteCommunication.h
+++ source/Plugins/Process/gdb-remote/GDBRemoteCommunication.h
@@ -290,4 +290,14 @@
} // 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_
Index: source/Plugins/Process/gdb-remote/GDBRemoteCommunication.cpp
===================================================================
--- source/Plugins/Process/gdb-remote/GDBRemoteCommunication.cpp
+++ source/Plugins/Process/gdb-remote/GDBRemoteCommunication.cpp
@@ -1376,3 +1376,39 @@
}
}
}
+
+void llvm::format_provider<GDBRemoteCommunication::PacketResult>::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;
+ }
+}
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
