llgs-tests: Replace the "log+return false" pattern with llvm::Error

[labath]

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.

lldb/trunk/source/Plugins/Process/gdb-remote/GDBRemoteCommunication.h

private:
   std::string m_listen_url;
 
   DISALLOW_COPY_AND_ASSIGN(GDBRemoteCommunication);
 };
 
 } // 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_

lldb/trunk/source/Plugins/Process/gdb-remote/GDBRemoteCommunication.cpp

if (type == PacketType::Notify) {
 
       // as the communication class, we are a broadcaster and the
       // async thread is tuned to listen to us
       BroadcastEvent(eBroadcastBitGdbReadThreadGotNotify,
                      new EventDataBytes(pdata));
     }
   }
 }
+
+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/trunk/unittests/tools/lldb-server/tests/CMakeLists.txt

 add_lldb_unittest(LLDBServerTests
   TestClient.cpp
   MessageObjects.cpp
   ThreadIdsInJstopinfoTest.cpp
 
   LINK_LIBS
     lldbHost
     lldbCore
     lldbInterpreter
     lldbTarget
     lldbPluginPlatformLinux
     lldbPluginProcessGDBRemote
+
+    LLVMTestingSupport
   LINK_COMPONENTS
     Support
   )

lldb/trunk/unittests/tools/lldb-server/tests/MessageObjects.h

 
 class ThreadInfo {
 public:
   ThreadInfo() = default;
   ThreadInfo(llvm::StringRef name, llvm::StringRef reason,
              const RegisterMap &registers, 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;
   std::string m_reason;
   RegisterMap m_registers;
   unsigned int m_signal;
 };
 

lldb/trunk/unittests/tools/lldb-server/tests/MessageObjects.cpp

ThreadInfo::ThreadInfo(StringRef name, StringRef reason,
                        const RegisterMap &registers, unsigned int signal)
     : m_name(name.str()), m_reason(reason.str()), m_registers(registers),
       m_signal(signal) {}
 
 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<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 ==========================================================
 Expected<JThreadsInfo> JThreadsInfo::Create(StringRef response,
                                             endianness endian) {
   JThreadsInfo jthreads_info;
 
   StructuredData::ObjectSP json = StructuredData::ParseJSON(response);

lldb/trunk/unittests/tools/lldb-server/tests/TestClient.h

class TestClient
     : public lldb_private::process_gdb_remote::GDBRemoteCommunicationClient {
 public:
   static void Initialize();
   static bool IsDebugServer();
   static bool IsLldbServer();
 
   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,
+  llvm::Error SendMessage(llvm::StringRef message, std::string &response_string,
   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,
       lldb_private::process_gdb_remote::GDBRemoteCommunication::PacketResult
           result);
 
   llvm::Optional<ProcessInfo> m_process_info;
   llvm::Optional<StopReply> m_stop_reply;
   lldb_private::ProcessLaunchInfo m_server_process_info;
   std::string m_test_name;
   std::string m_test_case_name;
   unsigned int m_pc_register;
 };
 } // namespace llgs_tests

lldb/trunk/unittests/tools/lldb-server/tests/TestClient.cpp

 
 TestClient::TestClient(const std::string &test_name,
                        const std::string &test_case_name)
     : m_test_name(test_name), m_test_case_name(test_case_name),
       m_pc_register(UINT_MAX) {}
 
 TestClient::~TestClient() {}
 
-bool TestClient::StartDebugger() {
+llvm::Error TestClient::StartDebugger() {
   const ArchSpec &arch_spec = HostInfo::GetArchitecture();
   Args args;
   args.AppendArgument(LLDB_SERVER);
   if (IsLldbServer()) {
     args.AppendArgument("gdbserver");
     args.AppendArgument("--log-channels=gdb-remote packets");
   } else {
     args.AppendArgument("--log-flags=0x800000");
   }
   args.AppendArgument("--reverse-connect");
   std::string log_file_name = GenerateLogFileName(arch_spec);
   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),
            "localhost:%u", listen_socket.GetLocalPortNumber());
 
   args.AppendArgument(connect_remote_address);
 
   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",
            connect_remote_address);
   Socket *accept_socket;
   listen_socket.Accept(accept_socket);
   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;
   command << "A";
   for (size_t i = 0; i < inferior_args.size(); i++) {
     if (i > 0)
       command << ',';
     std::string hex_encoded = toHex(inferior_args[i]);
     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()) {
     GTEST_LOG_(ERROR) << toString(std::move(create_error));
     return llvm::None;
   }
 
   return std::move(*creation);
 }
 
 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<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,
+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() {
   if (m_pc_register != UINT_MAX)
     return m_pc_register;
 
   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;
     }
 
     auto elements_or_error = SplitUniquePairList("GetPcRegisterId", response);
     if (auto split_error = elements_or_error.takeError()) {
       GTEST_LOG_(ERROR) << "GetPcRegisterId: Error splitting response: "
                         << response;
       return UINT_MAX;
     }
 
     auto elements = *elements_or_error;
     if (elements["alt-name"] == "pc" || elements["generic"] == "pc") {
       m_pc_register = register_id;
       break;
     }
   }
 
   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 {
   char *log_directory = getenv("LOG_FILE_DIRECTORY");
   if (!log_directory)
     return "";
 
   if (!llvm::sys::fs::is_directory(log_directory)) {
     GTEST_LOG_(WARNING) << "Cannot access log directory: " << log_directory;
     return "";
   }
 
   std::string log_file_name;
   raw_string_ostream log_file(log_file_name);
   log_file << log_directory << "/lldb-" << m_test_case_name << '-'
            << m_test_name << '-' << arch.GetArchitectureName() << ".log";
   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

lldb/trunk/unittests/tools/lldb-server/tests/ThreadIdsInJstopinfoTest.cpp

 //===-- ThreadsInJstopinfoTest.cpp ------------------------------*- C++ -*-===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 
 #include "TestClient.h"
+#include "llvm/Testing/Support/Error.h"
 #include "gtest/gtest.h"
 #include <string>
 
 using namespace llgs_tests;
 
 class ThreadsInJstopinfoTest : public ::testing::Test {
 protected:
   virtual void SetUp() { TestClient::Initialize(); }
 };
 
 TEST_F(ThreadsInJstopinfoTest, TestStopReplyContainsThreadPcsLlgs) {
   std::vector<std::string> inferior_args;
   // This inferior spawns N threads, then forces a break.
   inferior_args.push_back(THREAD_INFERIOR);
   inferior_args.push_back("4");
 
   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);
 
   auto jthreads_info = client.GetJThreadsInfo();
   ASSERT_TRUE(jthreads_info);
 
   auto stop_reply = client.GetLatestStopReply();
   auto stop_reply_pcs = stop_reply.GetThreadPcs();
   auto thread_infos = jthreads_info->GetThreadInfos();
   ASSERT_EQ(stop_reply_pcs.size(), thread_infos.size())
       << "Thread count mismatch.";
 
   for (auto stop_reply_pc : stop_reply_pcs) {
     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());
 }