Add StructuredData unit tests; move packet processing into delegate.

[tfiala]

Summary

This change does the following:

• Changes the signature for the continuation delegate method that handles async structured data from accepting an already-parsed structured data element to taking just the packet contents.
• Moves the conversion of the JSON-async: packet contents from GDBRemoteClientBase to the continuation delegate method.
• Adds a new unit test for verifying that the $JSON-asyc: packets get decoded and that the decoded packets get forwarded on to the delegate for further processing. Thanks to Pavel for making that whole section of code easily unit testable!
• Tightens up the packet verification on reception of a $JSON-async: packet contents. The code prior to this change is susceptible to a segfault if a packet is carefully crafted that starts with $J but has a total length shorter than the length of "$JSON-async:".

[labath]

Thank you for writing the tests. I have two stylistic comments, but otherwise looks great.

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

113

I'd like to move this code to a separate function. The main job of SendContinuePacketAndWaitForResponse is dealing with the thread synchronization issues, which is tricky enough without having json parsing in the middle of it.

unittests/Process/gdb-remote/GDBRemoteClientBaseTest.cpp

371

Please put the "expected" values first (ASSERT_NE(expected, actual)). Otherwise the error message will come out wrong when the comparison fails. The same issue is present in a number of other checks.

[clayborg]

We should probably move over to using llvm::StrringRef instead of "const char *". It the JSON parser isn't using llvm::StringRef already it probably should.

[tfiala]

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

113

Sure, sounds like a good change.

unittests/Process/gdb-remote/GDBRemoteClientBaseTest.cpp

371

Okay. That's somewhat unfortunate that Python unittest's messages are geared for the reverse:
http://bugs.python.org/issue10573

I will reverse these, thanks for catching that!

[clayborg]

Actually after looking at the JSON parser I see that it isn't using llvm::StringRef throughout. So I vote to keep the "const char *" for now since we need to guarantee that the JSON is null terminated. If we take a llvm::StringRef, then we will need to call llvm::StringRef::str() to make a std::string so we can guarantee that the JSON is null terminated which we do not want to have to do.

Either we include changing the JSON parser over to using llvm::StringRef throughout and change the constructors, or we leave things as "const char *" for now. I am assuming Todd doesn't want to take on the conversion over to llvm::StringRef so I am going to say I am ok with this change.

[tfiala]

<nevermind>

[clayborg]

Looking closer at the JSON parser llvm::StringRef isn't the right thing to use when parsing JSON. The parser will often remove desensitizing characters from say a string like:

"hello \"world\""

And when parsing a token in JSONParser::GetToken, we can't just hand out a llvm::StringRef do this data in memory. The function prototype is:

JSONParser::Token
JSONParser::GetToken (std::string &value)

This would mean we would need "value" to be able to have a new backing store in case the values don't match what is in the JSON text buffer which would defeat the purpose of using llvm::StringRef. We could make a struct like:

struct ParseString
{
    llvm::StringRef value;
    std::string backing_store;
};

But then the code becomes more complex for no real benefit.

[zturner]

I'm not sure I follow. StringRef is literally just a wrapper around a const char*, so if const char* works, why doesn't StringRef? JSONParser constructor is already taking a const char* in this patch, so it's not like it is modifying the contents of the string.

Checking the length and/or getting a substring or trimming values from the beginning and end is going to be a far more common operation than converting it into something that has to be null terminated (why does it have to be null terminated for that matter anyway?), so the benefit of having the length stored with the string outweighs the con of having to call str() when you want something null terminated, IMO

[zturner]

Actually it looks like JSONParser constructor takes a StringRef and then converts it to a std::string internally. Why can't it just store the StringRef internally? It doesn't modify the buffer, and incidentally it also keeps a uint64_t representing the "offset" into the string. That's what a StringRef is. an immutable buffer and an offset. Each time you extract something, you just say m_buffer = m_buffer.drop_front(n).

There is the SetFilePos() method, but it looks like it's only being used for two purposes: 1) To move forward, and 2) To reset the position to 0 after setting a new string in the extractor. The first one is still possible with a StringRef by writing extractor.skip(n), and the second one is still possible by writing extractor.SetString(NewStringRef).

[clayborg]

In D23884#526385, @zturner wrote:

I'm not sure I follow. StringRef is literally just a wrapper around a const char*, so if const char* works, why doesn't StringRef?

"const char *" assumed null termination. StringRef can be a reference to part of a C string:

const char *cstr = "\"valid json string\"and some other junk";
llvm::StringRef json_text(cstr, 19);

Not that the last character after the ending quote isn't a '\0' charcter it is a 'a'.

So if we want to actually use the "json_text" to parse the JSON, we need make a std::string so that it becomes null terminated. We could switch the parsing code to use a new class StringRefExtractor instead of StringExtractor in which case we can accept the data as a llvm::StringRef from the constructor. But all internal code would need to use std::string (GetToken() would still need to use std::string).

JSONParser constructor is already taking a const char* in this patch, so it's not like it is modifying the contents of the string.

You are correct. It does make a contract that used to not be there where the caller must guarantee the data it is parsing doesn't go away, but I don't think that will be a problem.

Checking the length and/or getting a substring or trimming values from the beginning and end is going to be a far more common operation than converting it into something that has to be null terminated (why does it have to be null terminated for that matter anyway?)

Currently it is so the parser knows when to stop, but it doesn't have to be if we switch to using a new StringRefExtractor class.

So the benefit of having the length stored with the string outweighs the con of having to call str() when you want something null terminated, IMO

Yes it it fine as long as you convert the parsing code.

[clayborg]

In D23884#526390, @zturner wrote:

Actually it looks like JSONParser constructor takes a StringRef and then converts it to a std::string internally. Why can't it just store the StringRef internally? It doesn't modify the buffer, and incidentally it also keeps a uint64_t representing the "offset" into the string. That's what a StringRef is. an immutable buffer and an offset. Each time you extract something, you just say m_buffer = m_buffer.drop_front(n).

Yep, we just need to create a StringRefExtractor class that uses a llvm::StringRef class internally instead of a std::string. If you do something about this, don't change StringExtractor since all of the GDB remote packets use these and actually use the std::string as the backing store for all of its incoming packets.

There is the SetFilePos() method, but it looks like it's only being used for two purposes: 1) To move forward, and 2) To reset the position to 0 after setting a new string in the extractor. The first one is still possible with a StringRef by writing extractor.skip(n), and the second one is still possible by writing extractor.SetString(NewStringRef).

You don't want to modify the string in any way. StringRefExtractor would contain a llvm::StringRef + the current offset position just like StringExtractor. No need to change anything functionality wise.

Again, if we switch the parser over to use an llvm::StringRef then this make sense, otherwise it doesn't.

[zturner]

You mention a StringRefExtractor. But nothing about StringExtractor that I can tell depends on the fact that it's null terminated. It only needs to know where to stop. A StringRef contains a length, so you stop when it gets to the end of the StringRef. It seems to me like we don't need a StringRefExtractor, but rather that StringExtractor itself can just be updated to use a StringRef instead of a std::string.

Anyway, I will try to whip up a patch that converts StringExtractor to store a StringRef internally. I'll put it up for review when I'm done just so you can see what I have in mind. If it works great, if it doesn't oh well :)

[tfiala]

Again, if we switch the parser over to use an llvm::StringRef then this make sense, otherwise it doesn't.

I don't want to conflate this change with switching the parser over to a string ref. The only reason I changed the entry to a const char* was to avoid creating a couple unnecessary temporaries.

We can consider the parser conversion a separate logical change that can be done independently.

[tfiala]

In D23884#526419, @tfiala wrote:

Again, if we switch the parser over to use an llvm::StringRef then this make sense, otherwise it doesn't.

I don't want to conflate this change with switching the parser over to a string ref. The only reason I changed the entry to a const char* was to avoid creating a couple unnecessary temporaries.

We can consider the parser conversion a separate logical change that can be done independently.

Greg and I talked about this a bit.

I'm going to reverse out the change to the JSON parsing signature, and just add a move-enabled one to avoid the temporary I was concerned about.

[tfiala]

Getting back to this change.

I'm going to rebase for code reformatting, remove the signature change on the JSON parsing, adjust my call sites for it, refactor the structured data parsing in the gdb-remote reception to a separate function, and then put this back up for review.

[zturner]

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

4812

How about just a global llvm::StringRef, or even a StringRef at local scope? Doesn't seem worth using a function local static for this.

4817

Change the function parameter to an llvm::StringRef, and then you can do the following:

4821–4838

This entire block becomes:

if (!packet.consume_front("JSON-async:")) {
   // print the log statement
}
auto json_sp = StructuredData::ParseJSON(packet);

4856

This is doing a string copy since you're going from a StringRef to a std::string. Use StringRef all the way down.

unittests/Process/gdb-remote/GDBRemoteClientBaseTest.cpp

38

This can be a vector of StringRefs as well, unless there's some reason you need to throw away the memory backing the StringRef, which it doesn't appear you do.

Also, if you have a rough idea of how many StringRefs there's going to be ahead of time, or at least an upper bound, then an llvm::SmallVector<StringRef> will be more efficient.

335

Would be nice to see PutEscapedBytes updated to take a StringRef. Every occurrence of passing const char * str, int len should be replaced with StringRef as we find occurrences of it.

[tfiala]

I'll make a few more adjustments here based on Zachary's feedback.

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

4812

If llvm::StringRef at global scope does not incur a global constructor, that's fine. If it does, we are prevented from adding global constructors within our products except on a special-case basis. This would not pass that mark.

Easy enough for me to try, though. I'll check.

4821–4838

Yep, that looks great. Thanks!

unittests/Process/gdb-remote/GDBRemoteClientBaseTest.cpp

38

It's not clear to me what the lifetime is of the packet message content and how that interplays with a StringRef. I was under the impression that a StringRef will assume the backing store hangs around. That sounds like a recipe for potential invalid memory access? I'll check the code, though. It may be totally fine based on how the packet content is handled.

335

Yep. I'd prefer to not make that part of this change, though.

[tfiala]

In D23884#539142, @tfiala wrote:

I'll make a few more adjustments here based on Zachary's feedback.

Heh, ahem, that was meant to be "global *destructor*" above, not global constructor.

[zturner]

unittests/Process/gdb-remote/GDBRemoteClientBaseTest.cpp

335

Yea you are right, since the packets are coming in asynchronously I don't think we can make any assumptions about how long it will live.

[tfiala]

Re-uploaded last patch with full context.

[tfiala]

unittests/Process/gdb-remote/GDBRemoteClientBaseTest.cpp

68

Yeah, I did try a std::vector<llvm::StringRef> but that actually blew up since the backing storage did disappear.

For the rest of the interface, I was able to pipe through the llvm::StringRef, though, since that's all serial up to the JSON parser, which itself then makes a copy that it requires. So I think that's all for the better. Thanks for the suggestion!

[tfiala]

I'm going to go ahead with this since I think the biggest concerns have been addressed. @labath we can adjust more later if you still have concerns.

include/lldb/Core/StructuredData.h

public:
 
         void Dump(Stream &s, bool pretty_print = true) const override;
 
     private:
         void *m_object;
     };
 
     static ObjectSP
-    ParseJSON (std::string json_text);
+    ParseJSON (const char *json_text);
 };
 
 } // namespace lldb_private
 
 #endif // liblldb_StructuredData_h_

source/Core/StructuredData.cpp

switch (token)
         default:
             break;
     }
     return StructuredData::ObjectSP();
 
 }
 
 StructuredData::ObjectSP
-StructuredData::ParseJSON (std::string json_text)
+StructuredData::ParseJSON (const char *json_text)
 {
-    JSONParser json_parser(json_text.c_str());
-    StructuredData::ObjectSP object_sp = ParseJSONValue(json_parser);
+    StructuredData::ObjectSP object_sp;
+
+    if (json_text && json_text[0])
+    {
+        JSONParser json_parser(json_text);
+        object_sp = ParseJSONValue(json_parser);
+    }
     return object_sp;
 }
 
 StructuredData::ObjectSP
 StructuredData::Object::GetObjectForDotSeparatedPath (llvm::StringRef path)
 {
     if (this->GetType() == Type::eTypeDictionary)
     {

source/Host/macosx/Host.mm

if (launch_info.GetFlags().Test(eLaunchFlagShellExpandArguments))
         RunShellCommand(expand_command, cwd, &status, nullptr, &output, 10);
         
         if (status != 0)
         {
             error.SetErrorStringWithFormat("lldb-argdumper exited with error %d", status);
             return error;
         }
         
-        auto data_sp = StructuredData::ParseJSON(output);
+        auto data_sp = StructuredData::ParseJSON(output.c_str());
         if (!data_sp)
         {
             error.SetErrorString("invalid JSON");
             return error;
         }
         
         auto dict_sp = data_sp->GetAsDictionary();
         if (!data_sp)

source/Plugins/Platform/gdb-server/PlatformRemoteGDBServer.cpp

PlatformRemoteGDBServer::GetRemoteUnixSignals()
     StringExtractorGDBRemote response;
     auto result = m_gdb_client.SendPacketAndWaitForResponse(
             packet, strlen(packet), response, false);
 
     if (result != decltype(result)::Success ||
             response.GetResponseType() != response.eResponse)
         return m_remote_signals_sp;
 
-    auto object_sp = StructuredData::ParseJSON(response.GetStringRef());
+    auto object_sp = StructuredData::ParseJSON(response.GetStringRef().c_str());
     if (!object_sp || !object_sp->IsValid())
         return m_remote_signals_sp;
 
     auto array_sp = object_sp->GetAsArray();
     if (!array_sp || !array_sp->IsValid())
         return m_remote_signals_sp;
 
     auto remote_signals_sp = std::make_shared<lldb_private::GDBRemoteSignals>();

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

 #include "ProcessGDBRemoteLog.h"
 
 using namespace lldb;
 using namespace lldb_private;
 using namespace lldb_private::process_gdb_remote;
 
 static const std::chrono::seconds kInterruptTimeout(5);
 
+static const std::string &
+GetStructuredDataPacketPrefix()
+{
+    static const std::string prefix("JSON-async:");
+    return prefix;
+}
+
 /////////////////////////
 // GDBRemoteClientBase //
 /////////////////////////
 
 GDBRemoteClientBase::ContinueDelegate::~ContinueDelegate() = default;
 
 GDBRemoteClientBase::GDBRemoteClientBase(const char *comm_name, const char *listener_name)
     : GDBRemoteCommunication(comm_name, listener_name), m_async_count(0), m_is_running(false), m_should_stop(false)
for (;;)
                 delegate.HandleAsyncStdout(inferior_stdout);
                 break;
             }
             case 'A':
                 delegate.HandleAsyncMisc(llvm::StringRef(response.GetStringRef()).substr(1));
                 break;
 
             case 'J':
-                // Asynchronous JSON packet, destined for a
-                // StructuredDataPlugin.
             {
+                // Verify this J packet is a JSON-async: packet.
+                const std::string &expected_prefix =
+                    GetStructuredDataPacketPrefix();
+                const std::string &packet = response.GetStringRef();
+                const std::string packet_prefix =
+                    packet.substr(0, expected_prefix.length());
+                if (packet_prefix != expected_prefix)
+                {
+                    if (log)
+                    {
+                        log->Printf("GDBRemoteCommmunicationClientBase::%s() "
+                                    "received $J packet but was not a "
+                                    "StructuredData packet: packet starts with "
+                                    "%s",
+                                    __FUNCTION__,
+                                    packet_prefix.c_str());
+                    }
+                    break;
+                }
+                
+                // This is an asynchronous JSON packet, destined for a
+                // StructuredDataPlugin.
+
                 // Parse the content into a StructuredData instance.
-                auto payload_index = strlen("JSON-async:");
+                const char *const encoded_json =
+                    packet.c_str() + expected_prefix.length();
+
                 StructuredData::ObjectSP json_sp =
-                    StructuredData::ParseJSON(response.GetStringRef()
-                                              .substr(payload_index));
+                    StructuredData::ParseJSON(encoded_json);
                 if (log)
                 {
                     if (json_sp)
                         log->Printf(
                                     "GDBRemoteCommmunicationClientBase::%s() "
                                     "received Async StructuredData packet: %s",
                                     __FUNCTION__,
-                                    response.GetStringRef().
-                                    substr(payload_index).c_str());
+                                    encoded_json);
                     else
                         log->Printf("GDBRemoteCommmunicationClientBase::%s"
                                     "() received StructuredData packet:"
                                     " parse failure", __FUNCTION__);
                 }
 
                 // Pass the data to the process to route to the
                 // appropriate plugin.  The plugin controls what happens

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

if (m_supports_jThreadsInfo)
         if (SendPacketAndWaitForResponse("jThreadsInfo", response, false) == PacketResult::Success)
         {
             if (response.IsUnsupportedResponse())
             {
                 m_supports_jThreadsInfo = false;
             }
             else if (!response.Empty())
             {
-                object_sp = StructuredData::ParseJSON (response.GetStringRef());
+                object_sp = StructuredData::ParseJSON (response.GetStringRef().c_str());
             }
         }
     }
     return object_sp;
 }
 
 
 bool

 GDBRemoteCommunicationClient::QueryGDBServer (std::vector<std::pair<uint16_t, std::string>>& connection_urls)
 {
     connection_urls.clear();
 
     StringExtractorGDBRemote response;
     if (SendPacketAndWaitForResponse("qQueryGDBServer", response, false) != PacketResult::Success)
         return 0;
 
-    StructuredData::ObjectSP data = StructuredData::ParseJSON(response.GetStringRef());
+    StructuredData::ObjectSP data = StructuredData::ParseJSON(response.GetStringRef().c_str());
     if (!data)
         return 0;
 
     StructuredData::Array* array = data->GetAsArray();
     if (!array)
         return 0;
 
     for (size_t i = 0, count = array->GetSize(); i < count; ++i)
if (!m_supported_async_json_packets_is_valid)
 
         // Poll it now.
         StringExtractorGDBRemote response;
         const bool send_async = false;
         if (SendPacketAndWaitForResponse("qStructuredDataPlugins", response,
                                          send_async) == PacketResult::Success)
         {
             m_supported_async_json_packets_sp = StructuredData::ParseJSON(
-                response.GetStringRef());
+                response.GetStringRef().c_str());
             if (m_supported_async_json_packets_sp &&
                 !m_supported_async_json_packets_sp->GetAsArray())
             {
                 // We were returned something other than a JSON array.  This
                 // is invalid.  Clear it out.
                 if (log)
                     log->Printf("GDBRemoteCommunicationClient::%s(): "
                                 "QSupportedAsyncJSONPackets returned invalid "

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

case 'S':
                     StringExtractor json_extractor;
                     // Swap "value" over into "name_extractor"
                     json_extractor.GetStringRef().swap(value);
                     // Now convert the HEX bytes into a string value
                     json_extractor.GetHexByteString (value);
 
                     // This JSON contains thread IDs and thread stop info for all threads.
                     // It doesn't contain expedited registers, memory or queue info.
-                    m_jstopinfo_sp = StructuredData::ParseJSON (value);
+                    m_jstopinfo_sp = StructuredData::ParseJSON (value.c_str());
                 }
                 else if (key.compare("hexname") == 0)
                 {
                     StringExtractor name_extractor;
                     // Swap "value" over into "name_extractor"
                     name_extractor.GetStringRef().swap(value);
                     // Now convert the HEX bytes into a string value
                     name_extractor.GetHexByteString (value);
if (m_gdb_comm.GetThreadExtendedInfoSupported())
         response.SetResponseValidatorToJSON();
         if (m_gdb_comm.SendPacketAndWaitForResponse(packet.GetData(), packet.GetSize(), response, false) == GDBRemoteCommunication::PacketResult::Success)
         {
             StringExtractorGDBRemote::ResponseType response_type = response.GetResponseType();
             if (response_type == StringExtractorGDBRemote::eResponse)
             {
                 if (!response.Empty())
                 {
-                    object_sp = StructuredData::ParseJSON (response.GetStringRef());
+                    object_sp = StructuredData::ParseJSON (response.GetStringRef().c_str());
                 }
             }
         }
     }
     return object_sp;
 }
 
 StructuredData::ObjectSP
if (m_gdb_comm.GetLoadedDynamicLibrariesInfosSupported())
         response.SetResponseValidatorToJSON();
         if (m_gdb_comm.SendPacketAndWaitForResponse(packet.GetData(), packet.GetSize(), response, false) == GDBRemoteCommunication::PacketResult::Success)
         {
             StringExtractorGDBRemote::ResponseType response_type = response.GetResponseType();
             if (response_type == StringExtractorGDBRemote::eResponse)
             {
                 if (!response.Empty())
                 {
-                    object_sp = StructuredData::ParseJSON (response.GetStringRef());
+                    object_sp = StructuredData::ParseJSON (response.GetStringRef().c_str());
                 }
             }
         }
     }
     return object_sp;
 }
 
 
if (m_gdb_comm.GetSharedCacheInfoSupported())
         response.SetResponseValidatorToJSON();
         if (m_gdb_comm.SendPacketAndWaitForResponse(packet.GetData(), packet.GetSize(), response, false) == GDBRemoteCommunication::PacketResult::Success)
         {
             StringExtractorGDBRemote::ResponseType response_type = response.GetResponseType();
             if (response_type == StringExtractorGDBRemote::eResponse)
             {
                 if (!response.Empty())
                 {
-                    object_sp = StructuredData::ParseJSON (response.GetStringRef());
+                    object_sp = StructuredData::ParseJSON (response.GetStringRef().c_str());
                 }
             }
         }
     }
     return object_sp;
 }
 
 Error

 }
 
 Error
 ProcessGDBRemote::GetLoadedModuleList (LoadedModuleInfoList & list)
 {
     // Make sure LLDB has an XML parser it can use first
     if (!XMLDocument::XMLEnabled())
         return Error (0, ErrorType::eErrorTypeGeneric);
 
     Log *log = GetLogIfAnyCategoriesSet (LIBLLDB_LOG_PROCESS);
     if (log)
         log->Printf ("ProcessGDBRemote::%s", __FUNCTION__);
 
     GDBRemoteCommunicationClient & comm = m_gdb_comm;
 
     // check that we have extended feature read support
     if (comm.GetQXferLibrariesSVR4ReadSupported ()) {
         list.clear ();
 
         // request the loaded library list
         std::string raw;
         lldb_private::Error lldberr;
 
         if (!comm.ReadExtFeature (ConstString ("libraries-svr4"), ConstString (""), raw, lldberr))
           return Error (0, ErrorType::eErrorTypeGeneric);
 
         // parse the xml file in memory
         if (log)
             log->Printf ("parsing: %s", raw.c_str());
         XMLDocument doc;
 
         if (!doc.ParseMemory(raw.c_str(), raw.size(), "noname.xml"))
             return Error (0, ErrorType::eErrorTypeGeneric);
 
         XMLNode root_element = doc.GetRootElement("library-list-svr4");
         if (!root_element)
             return Error();
 
         // main link map structure
         llvm::StringRef main_lm = root_element.GetAttributeValue("main-lm");
         if (!main_lm.empty())
         {
             list.m_link_map = StringConvert::ToUInt64(main_lm.data(), LLDB_INVALID_ADDRESS, 0);
         }
 
         root_element.ForEachChildElementWithName("library", [log, &list](const XMLNode &library) -> bool {
 
             LoadedModuleInfoList::LoadedModuleInfo module;
 
             library.ForEachAttribute([log, &module](const llvm::StringRef &name, const llvm::StringRef &value) -> bool {
 
                 if (name == "name")
                     module.set_name (value.str());
                 else if (name == "lm")
                 {
                     // the address of the link_map struct.
                     module.set_link_map(StringConvert::ToUInt64(value.data(), LLDB_INVALID_ADDRESS, 0));
                 }
                 else if (name == "l_addr")
                 {
                     // the displacement as read from the field 'l_addr' of the link_map struct.

unittests/Process/gdb-remote/GDBRemoteClientBaseTest.cpp

 // exceptions are disabled.
 #include <eh.h>
 #endif
 #include <future>
 
 #include "GDBRemoteTestUtils.h"
 #include "gtest/gtest.h"
 
+#include "lldb/Core/StreamGDBRemote.h"
 #include "Plugins/Process/Utility/LinuxSignals.h"
 #include "Plugins/Process/gdb-remote/GDBRemoteClientBase.h"
 #include "Plugins/Process/gdb-remote/GDBRemoteCommunicationServer.h"
 
 #include "llvm/ADT/STLExtras.h"
 
 using namespace lldb_private::process_gdb_remote;
 using namespace lldb_private;
 using namespace lldb;
 typedef GDBRemoteCommunication::PacketResult PacketResult;
 
 namespace
 {
+using StructuredDataVector = std::vector<StructuredData::ObjectSP>;
     
 struct MockDelegate : public GDBRemoteClientBase::ContinueDelegate
 {
     std::string output;
     std::string misc_data;
     unsigned stop_reply_called = 0;
+    StructuredDataVector structured_data_entries;
 
     void
     HandleAsyncStdout(llvm::StringRef out)
     {
         output += out;
     }
     void
     HandleAsyncMisc(llvm::StringRef data)
     {
         misc_data += data;
     }
     void
     HandleStopReply()
     {
         ++stop_reply_called;
     }
 
     bool
     HandleAsyncStructuredData(const StructuredData::ObjectSP
                               &object_sp)
     {
-        // TODO work in a test here after I fix the gtest breakage.
-        return true;
+        if (object_sp)
+            structured_data_entries.push_back(object_sp);
+        return object_sp != nullptr;
     }
 
 };
 
 struct TestClient : public GDBRemoteClientBase
 {
     TestClient() : GDBRemoteClientBase("test.client", "test.client.listener") { m_send_acks = false; }
 };
 
 struct ContinueFixture
 {
TEST_F(GDBRemoteClientBaseTest, SendContinueDelegateInterface)
     if (HasFailure())
         return;
 
     // Continue. We'll have the server send a bunch of async packets before it stops.
     ASSERT_EQ(PacketResult::Success, fix.server.SendPacket("O4142"));
     ASSERT_EQ(PacketResult::Success, fix.server.SendPacket("Apro"));
     ASSERT_EQ(PacketResult::Success, fix.server.SendPacket("O4344"));
     ASSERT_EQ(PacketResult::Success, fix.server.SendPacket("Afile"));
     ASSERT_EQ(PacketResult::Success, fix.server.SendPacket("T01"));
     ASSERT_EQ(eStateStopped, fix.SendCPacket(response));
     ASSERT_EQ("T01", response.GetStringRef());
     ASSERT_EQ(PacketResult::Success, fix.server.GetPacket(response));
     ASSERT_EQ("c", response.GetStringRef());
 
     EXPECT_EQ("ABCD", fix.delegate.output);
     EXPECT_EQ("profile", fix.delegate.misc_data);
     EXPECT_EQ(1u, fix.delegate.stop_reply_called);
 }
 
+TEST_F(GDBRemoteClientBaseTest, SendContinueDelegateStructuredDataReceipt)
+{
+    // Build the plain-text version of the JSON data we will have the
+    // server send.
+    const std::string json_data =
+        "{ \"type\": \"MyFeatureType\", "
+        "  \"elements\": [ \"entry1\", \"entry2\" ] }";
+
+    // Escape it properly for transit.
+    StreamGDBRemote stream;
+    stream.PutCString("JSON-async:");
+    stream.PutEscapedBytes(json_data.c_str(), json_data.length());
+    stream.Flush();
+    
+    // Set up the
+    StringExtractorGDBRemote response;
+    ContinueFixture fix;
+    if (HasFailure())
+        return;
+    
+    // Send async structured data packet, then stop.
+    ASSERT_EQ(PacketResult::Success, fix.server.SendPacket(stream.GetData()));
+    ASSERT_EQ(PacketResult::Success, fix.server.SendPacket("T01"));
+    ASSERT_EQ(eStateStopped, fix.SendCPacket(response));
+    ASSERT_EQ("T01", response.GetStringRef());
+    ASSERT_EQ(fix.delegate.structured_data_entries.size(), 1);
+    
+    // Verify the structured data.
+    auto object_sp = fix.delegate.structured_data_entries[0];
+    ASSERT_NE(object_sp, nullptr);
+    
+    auto dictionary = object_sp->GetAsDictionary();
+    ASSERT_NE(dictionary, nullptr);
+    
+    std::string type_value;
+    dictionary->GetValueForKeyAsString("type", type_value);
+    ASSERT_EQ(type_value, "MyFeatureType");
+    
+    StructuredData::Array *elements = nullptr;
+    dictionary->GetValueForKeyAsArray("elements", elements);
+    ASSERT_NE(elements, nullptr);
+    ASSERT_EQ(elements->GetSize(), 2);
+    
+    std::string element_value;
+    elements->GetItemAtIndexAsString(0, element_value);
+    ASSERT_EQ(element_value, "entry1");
+
+    elements->GetItemAtIndexAsString(1, element_value);
+    ASSERT_EQ(element_value, "entry2");
+}
+
+TEST_F(GDBRemoteClientBaseTest, SendContinueDelegateJPacketStructuredDataBadPrefix)
+{
+    // Build the plain-text version of the JSON data we will have the
+    // server send.
+    const std::string json_data =
+        "{ \"type\": \"MyFeatureType\", "
+        "  \"elements\": [ \"entry1\", \"entry2\" ] }";
+    
+    // Escape it properly for transit.
+    StreamGDBRemote stream;
+    stream.PutCString("Jfoo:");
+    stream.PutEscapedBytes(json_data.c_str(), json_data.length());
+    stream.Flush();
+    
+    // Set up the
+    StringExtractorGDBRemote response;
+    ContinueFixture fix;
+    if (HasFailure())
+        return;
+    
+    // Send async structured data packet, then stop.
+    ASSERT_EQ(PacketResult::Success, fix.server.SendPacket(stream.GetData()));
+    ASSERT_EQ(PacketResult::Success, fix.server.SendPacket("T01"));
+    ASSERT_EQ(eStateStopped, fix.SendCPacket(response));
+    ASSERT_EQ("T01", response.GetStringRef());
+    ASSERT_EQ(fix.delegate.structured_data_entries.size(), 0);
+}
+
+TEST_F(GDBRemoteClientBaseTest, SendContinueDelegateJPacketStructuredDataBadJSON)
+{
+    // Build the plain-text version of the JSON data we will have the
+    // server send.
+    const std::string json_data = "{ \"type\": \"MyFeatureType\", ";
+    
+    // Escape it properly for transit.
+    StreamGDBRemote stream;
+    stream.PutCString("JSON-async:");
+    stream.PutEscapedBytes(json_data.c_str(), json_data.length());
+    stream.Flush();
+    
+    // Set up the
+    StringExtractorGDBRemote response;
+    ContinueFixture fix;
+    if (HasFailure())
+        return;
+    
+    // Send async structured data packet, then stop.
+    ASSERT_EQ(PacketResult::Success, fix.server.SendPacket(stream.GetData()));
+    ASSERT_EQ(PacketResult::Success, fix.server.SendPacket("T01"));
+    ASSERT_EQ(eStateStopped, fix.SendCPacket(response));
+    ASSERT_EQ("T01", response.GetStringRef());
+    ASSERT_EQ(fix.delegate.structured_data_entries.size(), 0);
+}
+
 TEST_F(GDBRemoteClientBaseTest, InterruptNoResponse)
 {
     StringExtractorGDBRemote continue_response, response;
     ContinueFixture fix;
     if (HasFailure())
         return;
 
     // Continue. After the run packet is sent, send an interrupt.