[lldb] Add helper class for dealing with key:value; GDB responses

[DavidSpickett]

Summary

The current handling of qHostInfo is a bunch of if-else,
one for each key we think we might find. This works fine,
but we end up repeating parsing calls and extending it
gets tricky.

To improve this I'm adding KeyValueExtractorGDBRemote.
This is a class that takes the packet response and splits it up
into the key-value pairs. Where a pair is "<key>:<value>;".

Then you can do Get<T>(<key>) to get an Optional<T>.
This Optional is empty if the key was missing or its value
didn't parse as the T you asked for.

Or you can GetWithDefault<T>(<key>, <default value>)
which always gives you a T but if search/parse fails
then you get <default value>.

Plus specific methods to get LazyBool and hex encoded
string keys.

This improves the packet handling code by:

• Putting the type, key name and default value on the same line.
• Reducing the number of repeated parsing calls. (fewer "!value.getAsInteger(0, ...)" means fewer chances to forget a "!")
• Removing the need for num_keys_decoded, which was essentially a boolean but was treated as a running total. (replaced with HadValidKey)

We do pay an up front cost in building the map of keys up
front. However these packets are infrequent and the increase
in readability makes up for it.

This change adds the class and applies it to qHostInfo, keeping
the original logic intact. (future patches will apply it to
other packets and refactor post decode logic)

[DavidSpickett]

I realise it's a lot to review but if you have any initial comments on the usability that'd be great. My main point here was to make the if-else chain a bit "safer" and more easily extended. The parsing logic itself I did not change, to keep things clear (though I would like to improve it later).

I've also put up https://reviews.llvm.org/D93226 where you can see how it changes qProcessInfo parsing.

There are a few more packets that could benefit to varying degrees. (some are just 1/2 keys)

• qGDBServerVersion
• qMemoryRegionInfo
• qWatchpointSupportInfo
• qProcessInfo again (in GetCurrentProcessInfo)
• qLaunchGDBServer
• qModuleInfo

A quick look over them doesn't show anything that would prevent KeyValueExtractorGDBRemote from applying there too if it proves useful here.

[labath]

I think that streamlining the packet (de)serialization process is badly needing. I'm not so sure about the approach though. On one hand, it's definitely an improvement. On the other, it does not handle the deserialization process, and I was hoping we could have a solution which handles both, for two reasons:

• it makes things simpler
• it allows us to avoid the need for tests which explicitly test serialization and deserialization of each packet, as the things is implemented (and tested) once, centrally.

Have you looked at the how llvm YAML and JSON libraries handle (de)serialization? I was hoping that we could implement something similar to that...

[DavidSpickett]

Have you looked at the how llvm YAML and JSON libraries handle (de)serialization? I was hoping that we could implement something similar to that...

Good point, I was only looking at half the issue. I'll see how those libraries do things.

lldb/include/lldb/Utility/KeyValueExtractorGDBRemote.h

+//===-- KeyValueExtractorGDBRemote.h ----------------------------*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLDB_UTILITY_KEYVALUEEXTRACTORGDBREMOTE_H
+#define LLDB_UTILITY_KEYVALUEEXTRACTORGDBREMOTE_H
+
+#include "lldb/Utility/StringExtractorGDBRemote.h"
+#include "lldb/lldb-private-enumerations.h"
+#include "llvm/ADT/Optional.h"
+#include "llvm/Support/VersionTuple.h"
+#include <map>
+#include <string>
+
+/// This class is used to handle GDB protocol packet responses
+/// of the form: <key>:<value>;<key2>:<value2>;<...>
+/// (e.g. qHostInfo and qProcessInfo)
+
+class KeyValueExtractorGDBRemote {
+public:
+  /// Construct from a packet response. Splits into key value pairs
+  /// but does not attempt to parse them.
+  explicit KeyValueExtractorGDBRemote(StringExtractorGDBRemote response);
+
+  /// Try to lookup and parse the given key.
+  /// If the key is present and parses as the given T
+  /// (Parse<T> succeeds), you get a valid Optional<T>.
+  /// If the key is not present or does not parse, you
+  /// get an empty Optional<T>.
+  ///
+  /// Type specific notes:
+  /// For std::string we allow empty values.
+  /// So "abc:;" will return a valid Optional with value "".
+  ///
+  /// For lldb::ByteOrder we do not return eByteOrderInvalid,
+  /// just an empty Optional.
+  ///
+  /// To add support for a new T, define a Parse function for it.
+  template <typename T> llvm::Optional<T> Get(llvm::StringRef key) {
+    return GetImpl(key, Parse<T>);
+  }
+
+  /// Like Get but returns default_value if the key is missing or did
+  /// not parse. Returning default_value is not counted as having found
+  /// a valid key.
+  template <typename T> T GetWithDefault(llvm::StringRef key, T default_value) {
+    llvm::Optional<T> got = Get<T>(key);
+    return got ? *got : default_value;
+  }
+
+  /// Try to lookup a key whose value is a hex encoded string.
+  /// Returns an empty Optional if:
+  /// * the key does not exist
+  /// * the value is not a hex byte string
+  /// * the value is empty
+  ///
+  /// Returns a valid optional if the key exists and some part
+  /// of the beginning of the value parses as hex bytes.
+  /// Meaning "a:41zzz;" returns "A", the rest is ignored.
+  llvm::Optional<std::string> GetHexByteString(llvm::StringRef key);
+
+  /// Try to lookup a key whose value maps to a lazy bool.
+  /// Where yes_string maps to LazyBoolYes and no_string maps to LazyBoolNo.
+  /// Returns a valid Optional if we find either of those exact values.
+  /// Otherwise returns an empty optional. (does not return LazyBoolCalculate)
+  llvm::Optional<lldb_private::LazyBool> GetLazyBool(llvm::StringRef key,
+                                                     llvm::StringRef yes_string,
+                                                     llvm::StringRef no_string);
+
+  /// Returns true if at least one Get<...> method has found and parsed
+  /// a key.
+  /// For example if we have "a:abc;" and do Get<uint32_t>("a") this method
+  /// will return false. Since we found the key but it failed to parse.
+  /// If we had "a:1;" instead, it would return true.
+  /// This is sticky so remains true once at least one Get<...> succeeds.
+  bool HadValidKey() const { return m_parsed_once; }
+
+private:
+  /// Generic logic to attempt to parse and record valid keys.
+  /// Note that GetLazyBool does not use this.
+  template <typename T>
+  llvm::Optional<T> GetImpl(llvm::StringRef key,
+                            llvm::Optional<T> (*parser)(llvm::StringRef)) {
+    auto key_value = m_key_values.find(key);
+    if (key_value == m_key_values.end())
+      return llvm::None;
+
+    llvm::Optional<T> got = parser(key_value->second);
+    if (got)
+      m_parsed_once = true;
+
+    return got;
+  }
+
+  template <typename T>
+  static llvm::Optional<
+      typename std::enable_if_t<std::is_integral<T>::value, T>>
+  Parse(llvm::StringRef s) {
+    T result;
+    // getAsInteger returns false on success
+    if (!s.getAsInteger(/*auto radix*/ 0, result))
+      return result;
+    return llvm::None;
+  }
+
+  template <typename T>
+  static llvm::Optional<
+      typename std::enable_if_t<std::is_same<llvm::VersionTuple, T>::value, T>>
+  Parse(llvm::StringRef s) {
+    llvm::VersionTuple ver;
+    // tryParse returns false on success
+    if (!ver.tryParse(s))
+      return ver;
+    return llvm::None;
+  }
+
+  template <typename T>
+  static llvm::Optional<
+      typename std::enable_if_t<std::is_same<lldb::ByteOrder, T>::value, T>>
+  Parse(llvm::StringRef s) {
+    lldb::ByteOrder byte_order = llvm::StringSwitch<lldb::ByteOrder>(s)
+                                     .Case("little", lldb::eByteOrderLittle)
+                                     .Case("big", lldb::eByteOrderBig)
+                                     .Case("pdp", lldb::eByteOrderPDP)
+                                     .Default(lldb::eByteOrderInvalid);
+    if (byte_order == lldb::eByteOrderInvalid)
+      return llvm::None;
+    return byte_order;
+  }
+
+  template <typename T>
+  static llvm::Optional<
+      typename std::enable_if_t<std::is_same<std::string, T>::value, T>>
+  Parse(llvm::StringRef s) {
+    return s.str();
+  }
+
+  // There is only one string format but writing it this way lets
+  // us reuse GetImpl for HexByteStrings.
+  enum class StringFormat {
+    HexBytes,
+  };
+  template <typename T, StringFormat F>
+  static llvm::Optional<T> Parse(llvm::StringRef value);
+
+  // Whether at least one key was correctly parsed in response to a Get<...>Key
+  // call
+  bool m_parsed_once;
+  // Inital map of key value pairs as their original strings
+  std::map<llvm::StringRef, llvm::StringRef> m_key_values;
+  StringExtractorGDBRemote m_response;
+};
+
+#endif // LLDB_UTILITY_KEYVALUEEXTRACTORGDBREMOTE_H

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

 //===-- GDBRemoteCommunicationClient.cpp ----------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 

 #include "lldb/Utility/LLDBAssert.h"
 #include "lldb/Utility/Log.h"
 #include "lldb/Utility/State.h"
 #include "lldb/Utility/StreamString.h"
 
 #include "ProcessGDBRemote.h"
 #include "ProcessGDBRemoteLog.h"
 #include "lldb/Host/Config.h"
+#include "lldb/Utility/KeyValueExtractorGDBRemote.h"
 #include "lldb/Utility/StringExtractorGDBRemote.h"
 
 #include "llvm/ADT/StringSwitch.h"
 #include "llvm/Support/JSON.h"
 
 #if defined(HAVE_LIBCOMPRESSION)
 #include <compression.h>
 #endif
if (force || m_qHostInfo_is_valid == eLazyBoolCalculate) {
     // host info computation can require DNS traffic and shelling out to external processes.
     // Increase the timeout to account for that.
     ScopedTimeout timeout(*this, seconds(10));
     m_qHostInfo_is_valid = eLazyBoolNo;
     StringExtractorGDBRemote response;
     if (SendPacketAndWaitForResponse("qHostInfo", response, false) ==
         PacketResult::Success) {
       if (response.IsNormalResponse()) {
-        llvm::StringRef name;
-        llvm::StringRef value;
-        uint32_t cpu = LLDB_INVALID_CPUTYPE;
-        uint32_t sub = 0;
-        std::string arch_name;
-        std::string os_name;
-        std::string environment;
-        std::string vendor_name;
-        std::string triple;
-        std::string distribution_id;
-        uint32_t pointer_byte_size = 0;
-        ByteOrder byte_order = eByteOrderInvalid;
-        uint32_t num_keys_decoded = 0;
-        while (response.GetNameColonValue(name, value)) {
-          if (name.equals("cputype")) {
-            // exception type in big endian hex
-            if (!value.getAsInteger(0, cpu))
-              ++num_keys_decoded;
-          } else if (name.equals("cpusubtype")) {
-            // exception count in big endian hex
-            if (!value.getAsInteger(0, sub))
-              ++num_keys_decoded;
-          } else if (name.equals("arch")) {
-            arch_name = std::string(value);
-            ++num_keys_decoded;
-          } else if (name.equals("triple")) {
-            StringExtractor extractor(value);
-            extractor.GetHexByteString(triple);
-            ++num_keys_decoded;
-          } else if (name.equals("distribution_id")) {
-            StringExtractor extractor(value);
-            extractor.GetHexByteString(distribution_id);
-            ++num_keys_decoded;
-          } else if (name.equals("os_build")) {
-            StringExtractor extractor(value);
-            extractor.GetHexByteString(m_os_build);
-            ++num_keys_decoded;
-          } else if (name.equals("hostname")) {
-            StringExtractor extractor(value);
-            extractor.GetHexByteString(m_hostname);
-            ++num_keys_decoded;
-          } else if (name.equals("os_kernel")) {
-            StringExtractor extractor(value);
-            extractor.GetHexByteString(m_os_kernel);
-            ++num_keys_decoded;
-          } else if (name.equals("ostype")) {
-            ParseOSType(value, os_name, environment);
-            ++num_keys_decoded;
-          } else if (name.equals("vendor")) {
-            vendor_name = std::string(value);
-            ++num_keys_decoded;
-          } else if (name.equals("endian")) {
-            byte_order = llvm::StringSwitch<lldb::ByteOrder>(value)
-                             .Case("little", eByteOrderLittle)
-                             .Case("big", eByteOrderBig)
-                             .Case("pdp", eByteOrderPDP)
-                             .Default(eByteOrderInvalid);
-            if (byte_order != eByteOrderInvalid)
-              ++num_keys_decoded;
-          } else if (name.equals("ptrsize")) {
-            if (!value.getAsInteger(0, pointer_byte_size))
-              ++num_keys_decoded;
-          } else if (name.equals("os_version") ||
-                     name.equals(
-                         "version")) // Older debugserver binaries used the
-                                     // "version" key instead of
-                                     // "os_version"...
-          {
-            if (!m_os_version.tryParse(value))
-              ++num_keys_decoded;
-          } else if (name.equals("maccatalyst_version")) {
-            if (!m_maccatalyst_version.tryParse(value))
-              ++num_keys_decoded;
-          } else if (name.equals("watchpoint_exceptions_received")) {
-            m_watchpoints_trigger_after_instruction =
-                llvm::StringSwitch<LazyBool>(value)
-                    .Case("before", eLazyBoolNo)
-                    .Case("after", eLazyBoolYes)
-                    .Default(eLazyBoolCalculate);
-            if (m_watchpoints_trigger_after_instruction != eLazyBoolCalculate)
-              ++num_keys_decoded;
-          } else if (name.equals("default_packet_timeout")) {
-            uint32_t timeout_seconds;
-            if (!value.getAsInteger(0, timeout_seconds)) {
-              m_default_packet_timeout = seconds(timeout_seconds);
-              SetPacketTimeout(m_default_packet_timeout);
-              ++num_keys_decoded;
-            }
-          }
+        KeyValueExtractorGDBRemote extractor(response);
+
+        llvm::Optional<uint32_t> cpu = extractor.Get<uint32_t>("cputype");
+        uint32_t cpu_subtype =
+            extractor.GetWithDefault<uint32_t>("cpusubtype", 0);
+        llvm::Optional<std::string> arch_name =
+            extractor.Get<std::string>("arch");
+        llvm::Optional<std::string> triple =
+            extractor.GetHexByteString("triple");
+        llvm::Optional<std::string> distribution_id =
+            extractor.GetHexByteString("distribution_id");
+        llvm::Optional<std::string> os_build =
+            extractor.GetHexByteString("os_build");
+        llvm::Optional<std::string> hostname =
+            extractor.GetHexByteString("hostname");
+        llvm::Optional<std::string> os_kernel =
+            extractor.GetHexByteString("os_kernel");
+        llvm::Optional<std::string> os_name_value =
+            extractor.Get<std::string>("ostype");
+        llvm::Optional<std::string> vendor_name =
+            extractor.Get<std::string>("vendor");
+        llvm::Optional<lldb::ByteOrder> byte_order =
+            extractor.Get<lldb::ByteOrder>("endian");
+        llvm::Optional<uint32_t> pointer_byte_size =
+            extractor.Get<uint32_t>("ptrsize");
+        llvm::Optional<llvm::VersionTuple> os_version =
+            extractor.Get<llvm::VersionTuple>("os_version");
+        llvm::Optional<llvm::VersionTuple> version =
+            extractor.Get<llvm::VersionTuple>("version");
+        llvm::Optional<llvm::VersionTuple> maccatalyst_version =
+            extractor.Get<llvm::VersionTuple>("maccatalyst_version");
+        llvm::Optional<lldb_private::LazyBool> watchpoints_trigger =
+            extractor.GetLazyBool("watchpoint_exceptions_received", "after",
+                                  "before");
+        llvm::Optional<uint32_t> default_packet_timeout =
+            extractor.Get<uint32_t>("default_packet_timeout");
+
+        if (extractor.HadValidKey())
+          m_qHostInfo_is_valid = eLazyBoolYes;
+
+        if (os_build)
+          m_os_build = *os_build;
+        if (hostname)
+          m_hostname = *hostname;
+        if (os_kernel)
+          m_os_kernel = *os_kernel;
+
+        // Older debugserver binaries used the "version" key instead of
+        // "os_version"
+        if (os_version)
+          m_os_version = *os_version;
+        else if (version)
+          m_os_version = *version;
+
+        if (maccatalyst_version)
+          m_maccatalyst_version = *maccatalyst_version;
+
+        if (watchpoints_trigger)
+          m_watchpoints_trigger_after_instruction = *watchpoints_trigger;
+        else
+          m_watchpoints_trigger_after_instruction = eLazyBoolCalculate;
+
+        if (default_packet_timeout) {
+          m_default_packet_timeout = seconds(*default_packet_timeout);
         }
 
-        if (num_keys_decoded > 0)
-          m_qHostInfo_is_valid = eLazyBoolYes;
+        std::string os_name, environment;
+        if (os_name_value) {
+          ParseOSType(*os_name_value, os_name, environment);
+        }
 
-        if (triple.empty()) {
-          if (arch_name.empty()) {
-            if (cpu != LLDB_INVALID_CPUTYPE) {
-              m_host_arch.SetArchitecture(eArchTypeMachO, cpu, sub);
+        if (!triple) {
+          if (!arch_name) {
+            if (cpu) {
+              m_host_arch.SetArchitecture(eArchTypeMachO, *cpu, cpu_subtype);
               if (pointer_byte_size) {
-                assert(pointer_byte_size == m_host_arch.GetAddressByteSize());
+                assert(*pointer_byte_size == m_host_arch.GetAddressByteSize());
               }
-              if (byte_order != eByteOrderInvalid) {
-                assert(byte_order == m_host_arch.GetByteOrder());
+              if (byte_order) {
+                assert(*byte_order == m_host_arch.GetByteOrder());
               }
 
-              if (!vendor_name.empty())
+              if (vendor_name)
                 m_host_arch.GetTriple().setVendorName(
-                    llvm::StringRef(vendor_name));
-              if (!os_name.empty())
+                    llvm::StringRef(*vendor_name));
+
+              if (os_name.size())
                 m_host_arch.GetTriple().setOSName(llvm::StringRef(os_name));
-              if (!environment.empty())
+              if (environment.size())
                 m_host_arch.GetTriple().setEnvironmentName(environment);
             }
           } else {
-            std::string triple;
-            triple += arch_name;
-            if (!vendor_name.empty() || !os_name.empty()) {
-              triple += '-';
-              if (vendor_name.empty())
-                triple += "unknown";
+            std::string triple_str;
+            if (arch_name)
+              triple_str += *arch_name;
+
+            if (vendor_name || os_name.size()) {
+              triple_str += '-';
+              if (!vendor_name)
+                triple_str += "unknown";
               else
-                triple += vendor_name;
-              triple += '-';
+                triple_str += *vendor_name;
+              triple_str += '-';
               if (os_name.empty())
-                triple += "unknown";
+                triple_str += "unknown";
               else
-                triple += os_name;
+                triple_str += os_name;
             }
-            m_host_arch.SetTriple(triple.c_str());
+            m_host_arch.SetTriple(triple_str.c_str());
 
             llvm::Triple &host_triple = m_host_arch.GetTriple();
             if (host_triple.getVendor() == llvm::Triple::Apple &&
                 host_triple.getOS() == llvm::Triple::Darwin) {
               switch (m_host_arch.GetMachine()) {
               case llvm::Triple::aarch64:
               case llvm::Triple::aarch64_32:
               case llvm::Triple::arm:
               case llvm::Triple::thumb:
                 host_triple.setOS(llvm::Triple::IOS);
                 break;
               default:
                 host_triple.setOS(llvm::Triple::MacOSX);
                 break;
               }
             }
             if (pointer_byte_size) {
-              assert(pointer_byte_size == m_host_arch.GetAddressByteSize());
+              assert(*pointer_byte_size == m_host_arch.GetAddressByteSize());
             }
-            if (byte_order != eByteOrderInvalid) {
-              assert(byte_order == m_host_arch.GetByteOrder());
+            if (byte_order) {
+              assert(*byte_order == m_host_arch.GetByteOrder());
             }
           }
         } else {
-          m_host_arch.SetTriple(triple.c_str());
+          m_host_arch.SetTriple(triple->c_str());
+
           if (pointer_byte_size) {
-            assert(pointer_byte_size == m_host_arch.GetAddressByteSize());
+            assert(*pointer_byte_size == m_host_arch.GetAddressByteSize());
           }
-          if (byte_order != eByteOrderInvalid) {
-            assert(byte_order == m_host_arch.GetByteOrder());
+          if (byte_order) {
+            assert(*byte_order == m_host_arch.GetByteOrder());
           }
 
           LLDB_LOGF(log,
                     "GDBRemoteCommunicationClient::%s parsed host "
                     "architecture as %s, triple as %s from triple text %s",
                     __FUNCTION__,
                     m_host_arch.GetArchitectureName()
                         ? m_host_arch.GetArchitectureName()
                         : "<null-arch-name>",
                     m_host_arch.GetTriple().getTriple().c_str(),
-                    triple.c_str());
+                    triple->c_str());
         }
-        if (!distribution_id.empty())
-          m_host_arch.SetDistributionId(distribution_id.c_str());
+        if (distribution_id)
+          m_host_arch.SetDistributionId(distribution_id->c_str());
       }
     }
   }
   return m_qHostInfo_is_valid == eLazyBoolYes;
 }
 
 int GDBRemoteCommunicationClient::SendAttach(
     lldb::pid_t pid, StringExtractorGDBRemote &response) {

lldb/source/Utility/CMakeLists.txt

add_lldb_library(lldbUtility
   SelectHelper.cpp
   State.cpp
   Status.cpp
   Stream.cpp
   StreamCallback.cpp
   StreamString.cpp
   StringExtractor.cpp
   StringExtractorGDBRemote.cpp
+  KeyValueExtractorGDBRemote.cpp
   StringLexer.cpp
   StringList.cpp
   StructuredData.cpp
   TildeExpressionResolver.cpp
   Timer.cpp
   TraceOptions.cpp
   UnimplementedError.cpp
   UUID.cpp

lldb/source/Utility/KeyValueExtractorGDBRemote.cpp

+//===-- KeyValueExtractorGDBRemote.cpp ------------------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#include "lldb/Utility/KeyValueExtractorGDBRemote.h"
+#include "lldb/Utility/StringExtractor.h"
+#include "llvm/ADT/StringSwitch.h"
+
+template <>
+llvm::Optional<std::string> KeyValueExtractorGDBRemote::Parse<
+    std::string, KeyValueExtractorGDBRemote::StringFormat::HexBytes>(
+    llvm::StringRef s) {
+  StringExtractor extractor(s);
+  std::string result;
+  // Anything > 0 chars decoded is a success
+  size_t got = extractor.GetHexByteString(result);
+  if (got)
+    return result;
+  return llvm::None;
+}
+
+KeyValueExtractorGDBRemote::KeyValueExtractorGDBRemote(
+    StringExtractorGDBRemote response)
+    : m_parsed_once(false), m_response(response) {
+  llvm::StringRef key;
+  llvm::StringRef value;
+  while (m_response.GetNameColonValue(key, value)) {
+    m_key_values[key] = value;
+  }
+}
+
+llvm::Optional<std::string>
+KeyValueExtractorGDBRemote::GetHexByteString(llvm::StringRef key) {
+  return GetImpl(key, Parse<std::string, StringFormat::HexBytes>);
+}
+
+llvm::Optional<lldb_private::LazyBool>
+KeyValueExtractorGDBRemote::GetLazyBool(llvm::StringRef key,
+                                        llvm::StringRef yes_string,
+                                        llvm::StringRef no_string) {
+  // We don't use GetKeyImpl here because it would set m_parsed_once
+  // before we have tried to convert to LazyBool.
+  auto key_value = m_key_values.find(key);
+  if (key_value == m_key_values.end())
+    return llvm::None;
+
+  // Note that m_parsed_once is not set until we parse the string into LazyBool
+
+  // We would StringSwitch here but that needs string literals
+  lldb_private::LazyBool decoded = lldb_private::eLazyBoolCalculate;
+  if (key_value->second == yes_string)
+    decoded = lldb_private::eLazyBoolYes;
+  else if (key_value->second == no_string)
+    decoded = lldb_private::eLazyBoolNo;
+  else
+    return llvm::None;
+
+  m_parsed_once = true;
+  return decoded;
+}

lldb/unittests/Utility/CMakeLists.txt

 add_lldb_unittest(UtilityTests
   AnsiTerminalTest.cpp
   ArgsTest.cpp
   OptionsWithRawTest.cpp
   ArchSpecTest.cpp
   BroadcasterTest.cpp
   ConstStringTest.cpp
   CompletionRequestTest.cpp
   DataExtractorTest.cpp
   EnvironmentTest.cpp
   EventTest.cpp
   FileSpecTest.cpp
   FlagsTest.cpp
+  KeyValueExtractorGDBRemoteTest.cpp
   ListenerTest.cpp
   LogTest.cpp
   NameMatchesTest.cpp
   PredicateTest.cpp
   ProcessInfoTest.cpp
   ProcessInstanceInfoTest.cpp
   RangeMapTest.cpp
   RangeTest.cpp

lldb/unittests/Utility/KeyValueExtractorGDBRemoteTest.cpp

+//===-- KeyValueExtractorGDBRemoteTest.cpp --------------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#include "gmock/gmock.h"
+#include "gtest/gtest.h"
+
+#include "lldb/Utility/KeyValueExtractorGDBRemote.h"
+#include "llvm/Support/VersionTuple.h"
+
+TEST(KeyValueExtractorGDBRemoteTest, ParseNothing) {
+  StringExtractorGDBRemote response("");
+  KeyValueExtractorGDBRemote e(response);
+  ASSERT_FALSE(e.HadValidKey());
+  ASSERT_FALSE(e.Get<std::string>("test"));
+  ASSERT_FALSE(e.HadValidKey());
+}
+
+TEST(KeyValueExtractorGDBRemoteTest, ParseIncomplete) {
+  // Missing ending ';' so 'a' is not a valid key
+  StringExtractorGDBRemote response("a:abc");
+  KeyValueExtractorGDBRemote e(response);
+  ASSERT_FALSE(e.Get<std::string>("a"));
+}
+
+TEST(KeyValueExtractorGDBRemoteTest, GetWithDefaultHadValid) {
+  StringExtractorGDBRemote response("a:def;");
+  KeyValueExtractorGDBRemote e(response);
+
+  // HadValid not set if we returned the default value
+  uint32_t missing = e.GetWithDefault<uint32_t>("missing", 123);
+  ASSERT_EQ(missing, (uint32_t)123);
+  ASSERT_FALSE(e.HadValidKey());
+
+  // Not set if we found but failed to parse
+  uint32_t a_fails_parse = e.GetWithDefault<uint32_t>("a", 456);
+  ASSERT_EQ(a_fails_parse, (uint32_t)456);
+  ASSERT_FALSE(e.HadValidKey());
+
+  // Set if found and parsed, didn't use the default
+  std::string a_parsed = e.GetWithDefault<std::string>("a", "foo");
+  ASSERT_EQ(a_parsed, "def");
+  ASSERT_TRUE(e.HadValidKey());
+}
+
+TEST(KeyValueExtractorGDBRemoteTest, ParseUint32) {
+  StringExtractorGDBRemote response("valid:123;"
+                                    "invalid:abc;");
+  KeyValueExtractorGDBRemote e(response);
+
+  // Had valid is always false before any Get has been called
+  ASSERT_FALSE(e.HadValidKey());
+
+  ASSERT_FALSE(e.Get<uint32_t>("invalid"));
+  // Had valid is not set on parse failure
+  ASSERT_FALSE(e.HadValidKey());
+
+  llvm::Optional<uint32_t> valid = e.Get<uint32_t>("valid");
+  ASSERT_TRUE(valid);
+  ASSERT_EQ((uint32_t)123, *valid);
+
+  // Had valid now true because valid parsed successfully
+  ASSERT_TRUE(e.HadValidKey());
+
+  uint32_t get_default = e.GetWithDefault<uint32_t>("default", 99);
+  ASSERT_EQ((uint32_t)99, get_default);
+}
+
+TEST(KeyValueExtractorGDBRemoteTest, ParsePID) {
+  // This invokes the same path as uint32_t
+  // so this test just checks that we handle a wider
+  // type correctly.
+  StringExtractorGDBRemote response("invalid:???;"
+                                    "valid:0x1111222233334444;");
+  KeyValueExtractorGDBRemote e(response);
+
+  llvm::Optional<lldb::pid_t> invalid = e.Get<lldb::pid_t>("invalid");
+  ASSERT_FALSE(invalid);
+  ASSERT_FALSE(e.HadValidKey());
+
+  llvm::Optional<lldb::pid_t> valid = e.Get<lldb::pid_t>("valid");
+  ASSERT_TRUE(valid);
+  ASSERT_EQ((lldb::pid_t)0x1111222233334444, *valid);
+  ASSERT_TRUE(e.HadValidKey());
+}
+
+TEST(KeyValueExtractorGDBRemoteTest, ParseString) {
+  StringExtractorGDBRemote response("empty:;"
+                                    "valid:abc;"
+                                    "space:  ;");
+  KeyValueExtractorGDBRemote e(response);
+
+  // Empty strings are valid
+  llvm::Optional<std::string> empty = e.Get<std::string>("empty");
+  ASSERT_TRUE(empty);
+  ASSERT_EQ("", *empty);
+
+  ASSERT_TRUE(e.HadValidKey());
+
+  llvm::Optional<std::string> valid = e.Get<std::string>("valid");
+  ASSERT_TRUE(valid);
+  ASSERT_EQ("abc", *valid);
+
+  // All whitespace is valid
+  llvm::Optional<std::string> space = e.Get<std::string>("space");
+  ASSERT_TRUE(space);
+  ASSERT_EQ("  ", *space);
+
+  std::string get_default = e.GetWithDefault<std::string>("missing", "default");
+  ASSERT_EQ("default", get_default);
+}
+
+TEST(KeyValueExtractorGDBRemoteTest, ParseHexByteString) {
+  StringExtractorGDBRemote response("invalid:nothex;"
+                                    "invalid_end:68656cnothex;"
+                                    "valid:68656c6c6f;");
+  KeyValueExtractorGDBRemote e(response);
+
+  // Doesn't parse at all
+  llvm::Optional<std::string> invalid = e.GetHexByteString("invalid");
+  ASSERT_FALSE(invalid);
+  ASSERT_FALSE(e.HadValidKey());
+
+  // Decoding >0 chars is a success
+  llvm::Optional<std::string> invalid_end = e.GetHexByteString("invalid_end");
+  ASSERT_TRUE(invalid_end);
+  ASSERT_EQ("hel", *invalid_end);
+  ASSERT_TRUE(e.HadValidKey());
+
+  llvm::Optional<std::string> valid = e.GetHexByteString("valid");
+  ASSERT_TRUE(valid);
+  ASSERT_EQ("hello", *valid);
+
+  // Had valid is sticky so remains if a parse fails later
+  invalid = e.GetHexByteString("invalid");
+  ASSERT_TRUE(e.HadValidKey());
+}
+
+TEST(KeyValueExtractorGDBRemoteTest, ParseByteOrder) {
+  StringExtractorGDBRemote response("invalid:giant;"
+                                    "little:little;"
+                                    "big:big;"
+                                    "pdp:pdp;");
+  KeyValueExtractorGDBRemote e(response);
+
+  // We never return eByteOrderInvalid, just an empty optional
+  llvm::Optional<lldb::ByteOrder> invalid = e.Get<lldb::ByteOrder>("invalid");
+  ASSERT_FALSE(invalid);
+  ASSERT_FALSE(e.HadValidKey());
+
+  llvm::Optional<lldb::ByteOrder> little = e.Get<lldb::ByteOrder>("little");
+  ASSERT_TRUE(little);
+  ASSERT_EQ(lldb::eByteOrderLittle, *little);
+
+  ASSERT_TRUE(e.HadValidKey());
+
+  llvm::Optional<lldb::ByteOrder> big = e.Get<lldb::ByteOrder>("big");
+  ASSERT_TRUE(big);
+  ASSERT_EQ(lldb::eByteOrderBig, *big);
+
+  llvm::Optional<lldb::ByteOrder> pdp = e.Get<lldb::ByteOrder>("pdp");
+  ASSERT_TRUE(pdp);
+  ASSERT_EQ(lldb::eByteOrderPDP, *pdp);
+}
+
+TEST(KeyValueExtractorGDBRemoteTest, ParseVersionTuple) {
+  StringExtractorGDBRemote response("invalid:abc;"
+                                    "valid:1.2.3.4;");
+  KeyValueExtractorGDBRemote e(response);
+
+  // Usual parsing rules apply
+  llvm::Optional<llvm::VersionTuple> invalid =
+      e.Get<llvm::VersionTuple>("invalid");
+  ASSERT_FALSE(invalid);
+  ASSERT_FALSE(e.HadValidKey());
+
+  llvm::Optional<llvm::VersionTuple> valid = e.Get<llvm::VersionTuple>("valid");
+  ASSERT_TRUE(valid);
+  ASSERT_EQ(llvm::VersionTuple(1, 2, 3, 4), *valid);
+  ASSERT_TRUE(e.HadValidKey());
+}
+
+TEST(KeyValueExtractorGDBRemote, ParseLazyBoolKey) {
+  StringExtractorGDBRemote response("a_means_yes:a;"
+                                    "b_means_no:b;"
+                                    "c_is_invalid:c;");
+  KeyValueExtractorGDBRemote e(response);
+  std::string yes_value = "a";
+  std::string no_value = "b";
+
+  // We never return eLazyBoolCalculate, just an empty optional
+  llvm::Optional<lldb_private::LazyBool> c_is_invalid =
+      e.GetLazyBool("c_is_invalid", yes_value, no_value);
+  ASSERT_FALSE(c_is_invalid);
+  ASSERT_FALSE(e.HadValidKey());
+
+  llvm::Optional<lldb_private::LazyBool> a_means_yes =
+      e.GetLazyBool("a_means_yes", yes_value, no_value);
+  ASSERT_TRUE(a_means_yes);
+  ASSERT_EQ(lldb_private::eLazyBoolYes, *a_means_yes);
+  ASSERT_TRUE(e.HadValidKey());
+
+  llvm::Optional<lldb_private::LazyBool> b_means_no =
+      e.GetLazyBool("b_means_no", yes_value, no_value);
+  ASSERT_TRUE(b_means_no);
+  ASSERT_EQ(lldb_private::eLazyBoolNo, *b_means_no);
+}