Fix compatibility with OpenOCD debug stub.

[vadimcn]

Summary

OpenOCD sends register classes as two separate <feature> nodes, fixed parser to process both of them.

OpenOCD returns "l" in response to "qfThreadInfo", so IsUnsupportedResponse() was false and we were ending up without any threads in the process. I think it's reasonable to assume that there's always at least one thread.

[clayborg]

The multiple feature fix is fine.

As for the qfThreadInfo fix, do you not have control over the OpenOCD GDB server? I would be nice to modify it to return something valid in response to qfThreadInfo? If you don't control or have access to modify the OpenOCD GDB server, we should at least verify that you sent qfThreadInfo and got "l" as the only response. Seems weird to say we will accept any kind of input.

[vadimcn]

This is what I see in the log:

<  16> send packet: $qfThreadInfo#bb
<   5> read packet: $l#6c

And here's code that generates it: https://github.com/gnu-mcu-eclipse/openocd/blob/b21ab1d683aaee501d45fe8a509a2043123f16fd/src/rtos/rtos.c#L370

I agree, they should have responded with "not supported", but I've never heard of a system that allows processes without threads, and openocd is pretty popular in embedded, so I think it would make sense to just support this quirk. That's what gdb does anyways...

[clayborg]

As long as everyone agrees that no threads from qfThreadInfo means there is one thread whose thread ID is 1 then this is ok.

[clayborg]

If we get a response of "l" for the "qfThreadInfo", it might be worth trying to send a "H1" packet (select thread ID 1 for subsequent continues and steps) to see if the server knows about thread 1 before proceeding? This patch also broke other platforms that respond with "OK" to the "qfThreadInfo". So we really need to do some extra verification that "l" was the only things received in response to the "qfThreadInfo".

[tatyana-krasnukha]

This obvious solution works well for me and seems safe.

auto isEmptyList = response.IsNormalResponse() && response.GetStringRef() == "l";

if ((response.IsUnsupportedResponse() || isEmptyList) ...

[clayborg]

I like the solution detailed above by tatyana-krasnukha

[vadimcn]

Sorry, for not catching this regression! I've checked that attaching to a standard gdbserver still worked, but was not aware of the other scenarios.

Does lldb-server return 'OK' string in response to 'qfThreadInfo'? According to this, 'OK' is not a valid response to qfThreadInfo. (Is there a better reference for gdb-remote protocol?)

Also, Tamas Berghammer has already submitted a fix, does is still not resolve this issue?

[vadimcn]

Maybe LLDB should use qAttached to determine if there's an active process? OpenOCD seems to implement that one correctly.

source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp

if (lock) {
     /*
      * Connected bare-iron target (like YAMON gdb-stub) may not have support for
      * qProcessInfo, qC and qfThreadInfo packets. The reply from '?' packet
      * could
      * be as simple as 'S05'. There is no packet which can give us pid and/or
      * tid.
      * Assume pid=tid=1 in such cases.
     */
-    if (response.IsUnsupportedResponse() && thread_ids.size() == 0 &&
-        IsConnected()) {
+    if (thread_ids.size() == 0 && IsConnected()) {
       thread_ids.push_back(1);
     }
   } else {
 #if !defined(LLDB_CONFIGURATION_DEBUG)
     Log *log(ProcessGDBRemoteLog::GetLogIfAnyCategoryIsSet(GDBR_LOG_PROCESS |
                                                            GDBR_LOG_PACKETS));
     if (log)
       log->Printf("error: failed to get packet sequence mutex, not sending "

source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp

 
 typedef std::map<uint32_t, RegisterSetInfo> RegisterSetMap;
 
 struct GdbServerTargetInfo {
   std::string arch;
   std::string osabi;
   stringVec includes;
   RegisterSetMap reg_set_map;
-  XMLNode feature_node;
 };
 
 bool ParseRegisters(XMLNode feature_node, GdbServerTargetInfo &target_info,
                     GDBRemoteDynamicRegisterInfo &dyn_reg_info, ABISP abi_sp,
                     uint32_t &cur_reg_num, uint32_t &reg_offset) {
   if (!feature_node)
     return false;
 
bool ProcessGDBRemote::GetGDBServerRegisterInfo(ArchSpec &arch_to_use) {
 
   XMLDocument xml_document;
 
   if (xml_document.ParseMemory(raw.c_str(), raw.size(), "target.xml")) {
     GdbServerTargetInfo target_info;
 
     XMLNode target_node = xml_document.GetRootElement("target");
     if (target_node) {
-      XMLNode feature_node;
-      target_node.ForEachChildElement([&target_info, &feature_node](
+      std::vector<XMLNode> feature_nodes;
+      target_node.ForEachChildElement([&target_info, &feature_nodes](
                                           const XMLNode &node) -> bool {
         llvm::StringRef name = node.GetName();
         if (name == "architecture") {
           node.GetElementText(target_info.arch);
         } else if (name == "osabi") {
           node.GetElementText(target_info.osabi);
         } else if (name == "xi:include" || name == "include") {
           llvm::StringRef href = node.GetAttributeValue("href");
           if (!href.empty())
             target_info.includes.push_back(href.str());
         } else if (name == "feature") {
-          feature_node = node;
+          feature_nodes.push_back(node);
         } else if (name == "groups") {
           node.ForEachChildElementWithName(
               "group", [&target_info](const XMLNode &node) -> bool {
                 uint32_t set_id = UINT32_MAX;
                 RegisterSetInfo set_info;
 
                 node.ForEachAttribute(
                     [&set_id, &set_info](const llvm::StringRef &name,
if (target_node) {
       uint32_t cur_reg_num = 0;
       uint32_t reg_offset = 0;
 
       // Don't use Process::GetABI, this code gets called from DidAttach, and in
       // that context we haven't
       // set the Target's architecture yet, so the ABI is also potentially
       // incorrect.
       ABISP abi_to_use_sp = ABI::FindPlugin(shared_from_this(), arch_to_use);
-      if (feature_node) {
+      for (auto feature_node : feature_nodes) {
         ParseRegisters(feature_node, target_info, this->m_register_info,
                        abi_to_use_sp, cur_reg_num, reg_offset);
       }
 
       for (const auto &include : target_info.includes) {
         // request register file
         std::string xml_data;
         if (!comm.ReadExtFeature(ConstString("features"), ConstString(include),