[lldb] [gdb-remote client] Refactor handling qSupported

[mgorny]

Summary

Refactor the qSupported handler to split the reply into an array,
and identify features within the array rather than searching the string
for partial matches. While at it, use StringRef.split() to process
the compression list instead of reinventing the wheel.

Switch the arguments to MaybeEnableCompression() to use an ArrayRef
of StringRefs to simplify parameter passing from GetRemoteQSupported().

[rovka]

LGTM

[labath]

looks great, just fix the build errors :)

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

375

not a big deal, but this probably shouldn't be auto.

[mgorny]

In D100146#2684942, @labath wrote:

looks great, just fix the build errors :)

Yeah, I'm trying to see if I can reproduce them when building with Clang.

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

375

Could you explain a bit? I thought auto is convenient here since the actual type is visible three lines higher.

[labath]

In D100146#2685026, @mgorny wrote:

In D100146#2684942, @labath wrote:

looks great, just fix the build errors :)

Yeah, I'm trying to see if I can reproduce them when building with Clang.

I believe StringRef is not implicitly convertible to std::string these days, but all the conversions are in conditionally compiled code. Maybe you don't have any of it enabled?

My guess is it would be sufficient to change avail_name to a StringRef (and fix the errors caused by that).

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

375

Well.. my reasoning was something like this:

• llvm guidelines say that auto should be used (only?) when the type is obvious from context
• "context" is not exactly specified, but I would take that to mean the enclosing expression/statement (not the statement two lines up). The use of cast<Foo> in the example supports this.
• the actual type is not that complicated (llvm::StringRef), so it e.g. won't cause the line to be split in two
• given the above, and the generally cautious approach to auto in llvm, its better to err on the side of spelling out the type

But like I said, it's not a big deal...

[mgorny]

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

375

Yeah, this makes sense.

lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.h

protected:
   bool m_qXfer_memory_map_loaded;
 
   bool GetCurrentProcessInfo(bool allow_lazy_pid = true);
 
   bool GetGDBServerVersion();
 
   // Given the list of compression types that the remote debug stub can support,
   // possibly enable compression if we find an encoding we can handle.
-  void MaybeEnableCompression(std::vector<std::string> supported_compressions);
+  void MaybeEnableCompression(
+      llvm::ArrayRef<llvm::StringRef> supported_compressions);
 
   bool DecodeProcessInfoResponse(StringExtractorGDBRemote &response,
                                  ProcessInstanceInfo &process_info);
 
   void OnRunPacketSent(bool first) override;
 
   PacketResult SendThreadSpecificPacketAndWaitForResponse(
       lldb::tid_t tid, StreamString &&payload,

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

void GDBRemoteCommunicationClient::GetRemoteQSupported() {
   // Clear out any capabilities we expect to see in the qSupported response
   m_supports_qXfer_auxv_read = eLazyBoolNo;
   m_supports_qXfer_libraries_read = eLazyBoolNo;
   m_supports_qXfer_libraries_svr4_read = eLazyBoolNo;
   m_supports_augmented_libraries_svr4_read = eLazyBoolNo;
   m_supports_qXfer_features_read = eLazyBoolNo;
   m_supports_qXfer_memory_map_read = eLazyBoolNo;
   m_supports_multiprocess = eLazyBoolNo;
+  m_supports_qEcho = eLazyBoolNo;
+  m_supports_QPassSignals = eLazyBoolNo;
+
   m_max_packet_size = UINT64_MAX; // It's supposed to always be there, but if
                                   // not, we assume no limit
 
   // build the qSupported packet
   std::vector<std::string> features = {"xmlRegisters=i386,arm,mips,arc",
                                        "multiprocess+"};
   StreamString packet;
   packet.PutCString("qSupported");
   for (uint32_t i = 0; i < features.size(); ++i) {
     packet.PutCString(i == 0 ? ":" : ";");
     packet.PutCString(features[i]);
   }
 
   StringExtractorGDBRemote response;
   if (SendPacketAndWaitForResponse(packet.GetString(), response,
                                    /*send_async=*/false) ==
       PacketResult::Success) {
-    const char *response_cstr = response.GetStringRef().data();
-
     // Hang on to the qSupported packet, so that platforms can do custom
     // configuration of the transport before attaching/launching the process.
-    m_qSupported_response = response_cstr;
+    m_qSupported_response = response.GetStringRef().str();
 
-    if (::strstr(response_cstr, "qXfer:auxv:read+"))
+    llvm::SmallVector<llvm::StringRef, 16> server_features;
+    response.GetStringRef().split(server_features, ';');
+
+    for (auto x : server_features) {
+      if (x == "qXfer:auxv:read+")
         m_supports_qXfer_auxv_read = eLazyBoolYes;
-    if (::strstr(response_cstr, "qXfer:libraries-svr4:read+"))
+      else if (x == "qXfer:libraries-svr4:read+")
         m_supports_qXfer_libraries_svr4_read = eLazyBoolYes;
-    if (::strstr(response_cstr, "augmented-libraries-svr4-read")) {
+      else if (x == "augmented-libraries-svr4-read") {
         m_supports_qXfer_libraries_svr4_read = eLazyBoolYes; // implied
         m_supports_augmented_libraries_svr4_read = eLazyBoolYes;
-    }
-    if (::strstr(response_cstr, "qXfer:libraries:read+"))
+      } else if (x == "qXfer:libraries:read+")
         m_supports_qXfer_libraries_read = eLazyBoolYes;
-    if (::strstr(response_cstr, "qXfer:features:read+"))
+      else if (x == "qXfer:features:read+")
         m_supports_qXfer_features_read = eLazyBoolYes;
-    if (::strstr(response_cstr, "qXfer:memory-map:read+"))
+      else if (x == "qXfer:memory-map:read+")
         m_supports_qXfer_memory_map_read = eLazyBoolYes;
-
-    // Look for a list of compressions in the features list e.g.
-    // qXfer:features:read+;PacketSize=20000;qEcho+;SupportedCompressions=zlib-
-    // deflate,lzma
-    const char *features_list = ::strstr(response_cstr, "qXfer:features:");
-    if (features_list) {
-      const char *compressions =
-          ::strstr(features_list, "SupportedCompressions=");
-      if (compressions) {
-        std::vector<std::string> supported_compressions;
-        compressions += sizeof("SupportedCompressions=") - 1;
-        const char *end_of_compressions = strchr(compressions, ';');
-        if (end_of_compressions == nullptr) {
-          end_of_compressions = strchr(compressions, '\0');
-        }
-        const char *current_compression = compressions;
-        while (current_compression < end_of_compressions) {
-          const char *next_compression_name = strchr(current_compression, ',');
-          const char *end_of_this_word = next_compression_name;
-          if (next_compression_name == nullptr ||
-              end_of_compressions < next_compression_name) {
-            end_of_this_word = end_of_compressions;
-          }
-
-          if (end_of_this_word) {
-            if (end_of_this_word == current_compression) {
-              current_compression++;
-            } else {
-              std::string this_compression(
-                  current_compression, end_of_this_word - current_compression);
-              supported_compressions.push_back(this_compression);
-              current_compression = end_of_this_word + 1;
-            }
-          } else {
-            supported_compressions.push_back(current_compression);
-            current_compression = end_of_compressions;
-          }
-        }
-
-        if (supported_compressions.size() > 0) {
-          MaybeEnableCompression(supported_compressions);
-        }
-      }
-    }
-
-    if (::strstr(response_cstr, "qEcho"))
+      else if (x == "qEcho")
         m_supports_qEcho = eLazyBoolYes;
-    else
-      m_supports_qEcho = eLazyBoolNo;
-
-    if (::strstr(response_cstr, "QPassSignals+"))
+      else if (x == "QPassSignals+")
         m_supports_QPassSignals = eLazyBoolYes;
-    else
-      m_supports_QPassSignals = eLazyBoolNo;
-
-    if (::strstr(response_cstr, "multiprocess+"))
+      else if (x == "multiprocess+")
         m_supports_multiprocess = eLazyBoolYes;
-    else
-      m_supports_multiprocess = eLazyBoolNo;
-
-    const char *packet_size_str = ::strstr(response_cstr, "PacketSize=");
-    if (packet_size_str) {
-      StringExtractorGDBRemote packet_response(packet_size_str +
-                                               strlen("PacketSize="));
+      // Look for a list of compressions in the features list e.g.
+      // qXfer:features:read+;PacketSize=20000;qEcho+;SupportedCompressions=zlib-
+      // deflate,lzma
+      else if (x.consume_front("SupportedCompressions=")) {
+        llvm::SmallVector<llvm::StringRef, 4> compressions;
+        x.split(compressions, ',');
+        if (!compressions.empty())
+          MaybeEnableCompression(compressions);
+      } else if (x.consume_front("PacketSize=")) {
+        StringExtractorGDBRemote packet_response(x);
         m_max_packet_size =
             packet_response.GetHexMaxU64(/*little_endian=*/false, UINT64_MAX);
         if (m_max_packet_size == 0) {
           m_max_packet_size = UINT64_MAX; // Must have been a garbled response
           Log *log(
               ProcessGDBRemoteLog::GetLogIfAllCategoriesSet(GDBR_LOG_PROCESS));
           LLDB_LOGF(log, "Garbled PacketSize spec in qSupported response");
         }
       }
     }
   }
+}
 
 bool GDBRemoteCommunicationClient::GetThreadSuffixSupported() {
   if (m_supports_thread_suffix == eLazyBoolCalculate) {
     StringExtractorGDBRemote response;
     m_supports_thread_suffix = eLazyBoolNo;
     if (SendPacketAndWaitForResponse("QThreadSuffixSupported", response,
                                      false) == PacketResult::Success) {
       if (response.IsOKResponse())
if (SendPacketAndWaitForResponse("qGDBServerVersion", response, false) ==
           m_qGDBServerVersion_is_valid = eLazyBoolYes;
       }
     }
   }
   return m_qGDBServerVersion_is_valid == eLazyBoolYes;
 }
 
 void GDBRemoteCommunicationClient::MaybeEnableCompression(
-    std::vector<std::string> supported_compressions) {
+    llvm::ArrayRef<llvm::StringRef> supported_compressions) {
   CompressionType avail_type = CompressionType::None;
   std::string avail_name;
 
 #if defined(HAVE_LIBCOMPRESSION)
   if (avail_type == CompressionType::None) {
     for (auto compression : supported_compressions) {
       if (compression == "lzfse") {
         avail_type = CompressionType::LZFSE;