lldb-server tests: Add support for testing debugserver

[labath]

Summary

This adds support for running the lldb-server test suite (currently consisting
of only one test) against the debugserver. Currently, the choice which binary
to test is based on the host system. This will need to be replaced by something
more elaborate if/when lldb-server starts supporting debugging on darwin.

I need to make a couple of tweaks to the test client to work with debugserver:

• debugserver has different command-line arguments - launching code adjusted to handle that
• debugserver sends duplicate "medata" fields in the stop reply packet - adjusted stop-reply parsing code to handle that
• debugserver replies to the k packet instead of just dropping the connection - stopping code adjusted, although we should probably consider aligning the behavior of the two stubs in this case

[labath]

@beanz: It looks like you're doing some debugserver work. :) Any thoughts on this?

[jingham]

This isn't about this patch, but replying to:

debugserver replies to the k packet instead of just dropping the connection - stopping code adjusted, although we should probably consider aligning the behavior of the two stubs in this case

Especially when dealing with flakey connections, knowing that you are done with a target was really handy. People what to hit "rerun" in some UI and have the rerun happen instantly, but if you have no way of knowing when the former process actually died, you have to introduce delays, which slows this down. This is particularly true when talking to an OS that really wants only one copy of an "app" running at a time (e.g. iOS) but is more generally useful for responsiveness. And if something goes wrong with killing the target, we could even use the new error reply strings to give more info about this.

[labath]

In D35311#814941, @jingham wrote:

This isn't about this patch, but replying to:

debugserver replies to the k packet instead of just dropping the connection - stopping code adjusted, although we should probably consider aligning the behavior of the two stubs in this case

Especially when dealing with flakey connections, knowing that you are done with a target was really handy. People what to hit "rerun" in some UI and have the rerun happen instantly, but if you have no way of knowing when the former process actually died, you have to introduce delays, which slows this down. This is particularly true when talking to an OS that really wants only one copy of an "app" running at a time (e.g. iOS) but is more generally useful for responsiveness. And if something goes wrong with killing the target, we could even use the new error reply strings to give more info about this.

Yes, I can totally believe that the response to that packet can be useful. I'm not actually sure that changing the debugserver behavior is the best solution here, I just think that we should pick one behavior and have both of the stubs act the same way. It's a bit unfortunate that this is in violation of the gdb-remote protocol, but I guess if a client is expecting the connection to be dropped, it is unlikely it will get confused by an OK response (OTOH, maybe we could use the fact that the connection was dropped as a signal that the kill has completed).

[beanz]

This all looks fine to me. The one thing I'd like you to consider is that someday (when lldb-server is supported on Darwin) we will want to run these tests against both debugserver and lldb-server. I'm not asking you to make those changes to this patch, but if there are things you would change about how you're doing things to make that easier to support I'd ask that you consider that.

I have one specific comment below that is along those lines. Otherwise, this LGTM.

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

32

Instead of making this ifdef'd can you just check to see if LLDB_SERVER is debugserver or lldb-server?

One of the items on my todo list is to get rid of all the places where #ifdef __APPLE__ equates to using debugserver so that we can start evaluating making lldb-server work on Darwin.

[labath]

In D35311#830779, @beanz wrote:

This all looks fine to me. The one thing I'd like you to consider is that someday (when lldb-server is supported on Darwin) we will want to run these tests against both debugserver and lldb-server. I'm not asking you to make those changes to this patch, but if there are things you would change about how you're doing things to make that easier to support I'd ask that you consider that.

I have one specific comment below that is along those lines. Otherwise, this LGTM.

I did consider this to some depth while making this change. I think that the easiest way to make that happen would be to create two cmake targets (add_lldb_unittest(LLDBServerTests..., add_lldb_unittest(DebugServerTests...). The main difference between these would be the value of the LLDB_SERVER define, but other differences are possible as well (for example, if only one of the servers implements some feature). It means we will compile some things twice, but in return it will integrate nicely with the existing llvm unit testing infrastructure.

It may be possible to use the googletest's parameterized test mechanism for this, but I don't know that well enough to tell. In any case, this shouldn't interfere with this patch -- the only thing that will change is that the return value of isdebugserver() will be determined through some runtime mechanism.

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

32

I'll change this to detect the server type based on the file name.

unittests/tools/CMakeLists.txt

-if(CMAKE_SYSTEM_NAME MATCHES "Android|Linux|NetBSD")
+if(CMAKE_SYSTEM_NAME MATCHES "Android|Darwin|Linux|NetBSD")
   add_subdirectory(lldb-server)
 endif()

unittests/tools/lldb-server/CMakeLists.txt

 function(add_lldb_test_executable test_name)
   set(EXCLUDE_FROM_ALL ON)
   add_llvm_executable(${test_name} NO_INSTALL_RPATH ${ARGN})
   set(outdir ${CMAKE_CURRENT_BINARY_DIR}/${CMAKE_CFG_INTDIR})
   set_output_directory(${test_name} BINARY_DIR ${outdir} LIBRARY_DIR ${outdir})
 endfunction()
 
 add_lldb_test_executable(thread_inferior inferior/thread_inferior.cpp)
 
+if (CMAKE_SYSTEM_NAME MATCHES "Darwin")
+  add_definitions(-DLLDB_SERVER="$<TARGET_FILE:debugserver>")
+else()
   add_definitions(-DLLDB_SERVER="$<TARGET_FILE:lldb-server>")
+endif()
+
 add_definitions(-DTHREAD_INFERIOR="${CMAKE_CURRENT_BINARY_DIR}/thread_inferior")
 add_subdirectory(tests)
 add_dependencies(LLDBServerTests thread_inferior)

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

private:
   std::string m_name;
   U64Map m_thread_pcs;
   RegisterMap m_registers;
   std::string m_reason;
 };
 
 // Common functions for parsing packet data.
 llvm::Expected<llvm::StringMap<llvm::StringRef>>
-SplitPairList(llvm::StringRef caller, llvm::StringRef s);
+SplitUniquePairList(llvm::StringRef caller, llvm::StringRef s);
+
+llvm::StringMap<llvm::SmallVector<llvm::StringRef, 2>>
+SplitPairList(llvm::StringRef s);
 
 template <typename... Args>
 llvm::Error make_parsing_error(llvm::StringRef format, Args &&... args) {
   std::string error =
       "Unable to parse " +
       llvm::formatv(format.data(), std::forward<Args>(args)...).str();
   return llvm::make_error<llvm::StringError>(error,
                                              llvm::inconvertibleErrorCode());
 }
 } // namespace llgs_tests

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

 
 using namespace lldb_private;
 using namespace llvm;
 using namespace llvm::support;
 namespace llgs_tests {
 
 Expected<ProcessInfo> ProcessInfo::Create(StringRef response) {
   ProcessInfo process_info;
-  auto elements_or_error = SplitPairList("ProcessInfo", response);
+  auto elements_or_error = SplitUniquePairList("ProcessInfo", response);
   if (!elements_or_error)
     return elements_or_error.takeError();
 
   auto &elements = *elements_or_error;
   if (elements["pid"].getAsInteger(16, process_info.m_pid))
     return make_parsing_error("ProcessInfo: pid");
   if (elements["parent-pid"].getAsInteger(16, process_info.m_parent_pid))
     return make_parsing_error("ProcessInfo: parent-pid");
const ThreadInfoMap &JThreadsInfo::GetThreadInfos() const {
   return m_thread_infos;
 }
 
 //====== StopReply =============================================================
 const U64Map &StopReply::GetThreadPcs() const { return m_thread_pcs; }
 
 Expected<StopReply> StopReply::Create(StringRef response,
                                       llvm::support::endianness endian) {
+  if (response.size() < 3 || !response.consume_front("T"))
+    return make_parsing_error("StopReply: Invalid packet");
+
   StopReply stop_reply;
 
-  auto elements_or_error = SplitPairList("StopReply", response);
-  if (auto split_error = elements_or_error.takeError()) {
-    return std::move(split_error);
+  StringRef signal = response.take_front(2);
+  response = response.drop_front(2);
+  if (!llvm::to_integer(signal, stop_reply.m_signal, 16))
+    return make_parsing_error("StopReply: stop signal");
+
+  auto elements = SplitPairList(response);
+  for (StringRef field :
+       {"name", "reason", "thread", "threads", "thread-pcs"}) {
+    // This will insert an empty field if there is none. In the future, we
+    // should probably differentiate between these fields not being present and
+    // them being empty, but right now no tests depends on this.
+    if (elements.insert({field, {""}}).first->second.size() != 1)
+      return make_parsing_error(
+          "StopReply: got multiple responses for the {0} field", field);
   }
+  stop_reply.m_name = elements["name"][0];
+  stop_reply.m_reason = elements["reason"][0];
 
-  auto elements = *elements_or_error;
-  stop_reply.m_name = elements["name"];
-  stop_reply.m_reason = elements["reason"];
+  if (!llvm::to_integer(elements["thread"][0], stop_reply.m_thread, 16))
+    return make_parsing_error("StopReply: thread");
 
   SmallVector<StringRef, 20> threads;
   SmallVector<StringRef, 20> pcs;
-  elements["threads"].split(threads, ',');
-  elements["thread-pcs"].split(pcs, ',');
+  elements["threads"][0].split(threads, ',');
+  elements["thread-pcs"][0].split(pcs, ',');
   if (threads.size() != pcs.size())
     return make_parsing_error("StopReply: thread/PC count mismatch");
 
   for (size_t i = 0; i < threads.size(); i++) {
     lldb::tid_t thread_id;
     uint64_t pc;
     if (threads[i].getAsInteger(16, thread_id))
       return make_parsing_error("StopReply: thread ID at [{0}].", i);
     if (pcs[i].getAsInteger(16, pc))
       return make_parsing_error("StopReply: thread PC at [{0}].", i);
 
     stop_reply.m_thread_pcs[thread_id] = pc;
   }
 
   for (auto i = elements.begin(); i != elements.end(); i++) {
     StringRef key = i->getKey();
-    StringRef val = i->getValue();
-    if (key.size() >= 9 && key[0] == 'T' && key.substr(3, 6) == "thread") {
-      if (val.getAsInteger(16, stop_reply.m_thread))
-        return make_parsing_error("StopReply: thread id");
-      if (key.substr(1, 2).getAsInteger(16, stop_reply.m_signal))
-        return make_parsing_error("StopReply: stop signal");
-    } else if (key.size() == 2) {
+    const auto &val = i->getValue();
+    if (key.size() == 2) {
       unsigned int reg;
-      if (!key.getAsInteger(16, reg)) {
-        stop_reply.m_registers[reg] = val.str();
-      }
+      if (key.getAsInteger(16, reg))
+        continue;
+      if (val.size() != 1)
+        return make_parsing_error(
+            "StopReply: multiple entries for register field [{0:x}]", reg);
+
+      stop_reply.m_registers[reg] = val[0].str();
     }
   }
 
   return stop_reply;
 }
 
 //====== Globals ===============================================================
-Expected<StringMap<StringRef>> SplitPairList(StringRef caller, StringRef str) {
+Expected<StringMap<StringRef>> SplitUniquePairList(StringRef caller,
+                                                   StringRef str) {
   SmallVector<StringRef, 20> elements;
   str.split(elements, ';');
 
   StringMap<StringRef> pairs;
   for (StringRef s : elements) {
     std::pair<StringRef, StringRef> pair = s.split(':');
     if (pairs.count(pair.first))
       return make_parsing_error("{0}: Duplicate Key: {1}", caller, pair.first);
 
-    pairs.insert(s.split(':'));
+    pairs.insert(pair);
+  }
+
+  return pairs;
+}
+
+StringMap<SmallVector<StringRef, 2>> SplitPairList(StringRef str) {
+  SmallVector<StringRef, 20> elements;
+  str.split(elements, ';');
+
+  StringMap<SmallVector<StringRef, 2>> pairs;
+  for (StringRef s : elements) {
+    std::pair<StringRef, StringRef> pair = s.split(':');
+    pairs[pair.first].push_back(pair.second);
   }
 
   return pairs;
 }
 } // namespace llgs_tests

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

 
 namespace llgs_tests {
 // TODO: Make the test client an abstract base class, with different children
 // for different types of connections: llgs v. debugserver
 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();

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

 
 using namespace lldb;
 using namespace lldb_private;
 using namespace llvm;
 
 namespace llgs_tests {
 void TestClient::Initialize() { HostInfo::Initialize(); }
 
+bool TestClient::IsDebugServer() {
+#ifdef __APPLE__
+  return true;
+#else
+  return false;
+#endif
+}
+
+bool TestClient::IsLldbServer() { return !IsDebugServer(); }
+
 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() {
   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()) {
+  if (log_file_name.size())
     args.AppendArgument("--log-file=" + log_file_name);
-  }
 
   Status error;
   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;
   }
bool TestClient::StartDebugger() {
   SetConnection(new ConnectionFileDescriptor(accept_socket));
 
   SendAck(); // Send this as a handshake.
   return true;
 }
 
 bool TestClient::StopDebugger() {
   std::string response;
-  return SendMessage("k", response, PacketResult::ErrorDisconnected);
+  // 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) {
   std::stringstream command;
   command << "A";
   for (size_t i = 0; i < inferior_args.size(); i++) {
     if (i > 0)
       command << ',';
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)) {
       GTEST_LOG_(ERROR) << "Unable to query register ID for PC register.";
       return UINT_MAX;
     }
 
-    auto elements_or_error = SplitPairList("GetPcRegisterId", response);
+    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") {