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 ↗

(On Diff #69269)

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 ↗

(On Diff #69269)

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 ↗

(On Diff #69269)

Sure, sounds like a good change.

unittests/Process/gdb-remote/GDBRemoteClientBaseTest.cpp

371 ↗

(On Diff #69269)

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 ↗

(On Diff #70909)

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 ↗

(On Diff #70909)

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

4821–4838 ↗

(On Diff #70909)

This entire block becomes:

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

4856 ↗

(On Diff #70909)

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 ↗

(On Diff #70909)

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 ↗

(On Diff #70909)

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 ↗

(On Diff #70909)

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 ↗

(On Diff #70909)

Yep, that looks great. Thanks!

unittests/Process/gdb-remote/GDBRemoteClientBaseTest.cpp

38 ↗

(On Diff #70909)

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 ↗

(On Diff #70909)

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 ↗

(On Diff #70909)

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

38 ↗

(On Diff #70931)

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.

lldb/trunk/source/Plugins/Process/gdb-remote/GDBRemoteClientBase.h

 class GDBRemoteClientBase : public GDBRemoteCommunication {
 public:
   struct ContinueDelegate {
     virtual ~ContinueDelegate();
     virtual void HandleAsyncStdout(llvm::StringRef out) = 0;
     virtual void HandleAsyncMisc(llvm::StringRef data) = 0;
     virtual void HandleStopReply() = 0;
 
-    //
-    /// Processes async structured data.
+    // =========================================================================
+    /// Process asynchronously-received structured data.
     ///
-    /// @return
-    ///    true if the data was handled; otherwise, false.
-    //
-    virtual bool
-    HandleAsyncStructuredData(const StructuredData::ObjectSP &object_sp) = 0;
+    /// @param[in] data
+    ///   The complete data packet, expected to start with JSON-async.
+    // =========================================================================
+    virtual void HandleAsyncStructuredDataPacket(llvm::StringRef data) = 0;
   };
 
   GDBRemoteClientBase(const char *comm_name, const char *listener_name);
 
   bool SendAsyncSignal(int signo);
 
   bool Interrupt();
 

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

case 'O': {
       response.GetHexByteString(inferior_stdout);
       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.
-      {
-        // Parse the content into a StructuredData instance.
-        auto payload_index = strlen("JSON-async:");
-        StructuredData::ObjectSP json_sp = StructuredData::ParseJSON(
-            response.GetStringRef().substr(payload_index));
-        if (log) {
-          if (json_sp)
-            log->Printf("GDBRemoteCommmunicationClientBase::%s() "
-                        "received Async StructuredData packet: %s",
-                        __FUNCTION__,
-                        response.GetStringRef().substr(payload_index).c_str());
-          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
-        // to it from there.
-        bool routed = delegate.HandleAsyncStructuredData(json_sp);
-        if (log)
-          log->Printf("GDBRemoteCommmunicationClientBase::%s()"
-                      " packet %s",
-                      __FUNCTION__, routed ? "handled" : "not handled");
-        break;
-      }
+      delegate.HandleAsyncStructuredDataPacket(response.GetStringRef());
     case 'T':
     case 'S':
       // Do this with the continue lock held.
       const bool should_stop = ShouldStop(signals, response);
       response.SetFilePos(0);
 
       // The packet we should resume with. In the future
       // we should check our thread list and "do the right thing"

lldb/trunk/source/Plugins/Process/gdb-remote/ProcessGDBRemote.h

static bool NewThreadNotifyBreakpointHit(void *baton,
                                            lldb::user_id_t break_loc_id);
 
   //------------------------------------------------------------------
   // ContinueDelegate interface
   //------------------------------------------------------------------
   void HandleAsyncStdout(llvm::StringRef out) override;
   void HandleAsyncMisc(llvm::StringRef data) override;
   void HandleStopReply() override;
-  bool
-  HandleAsyncStructuredData(const StructuredData::ObjectSP &object_sp) override;
+  void HandleAsyncStructuredDataPacket(llvm::StringRef data) override;
 
   using ModuleCacheKey = std::pair<std::string, std::string>;
   // KeyInfo for the cached module spec DenseMap.
   // The invariant is that all real keys will have the file and architecture
   // set.
   // The empty key has an empty file and an empty arch.
   // The tombstone key has an invalid arch and an empty file.
   // The comparison and hash functions take the file name and architecture

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

void ProcessGDBRemote::HandleStopReply() {
   if (GetID() == LLDB_INVALID_PROCESS_ID) {
     lldb::pid_t pid = m_gdb_comm.GetCurrentProcessID();
     if (pid != LLDB_INVALID_PROCESS_ID)
       SetID(pid);
   }
   BuildDynamicRegisterInfo(true);
 }
 
-bool ProcessGDBRemote::HandleAsyncStructuredData(
-    const StructuredData::ObjectSP &object_sp) {
-  return RouteAsyncStructuredData(object_sp);
+static const char *const s_async_json_packet_prefix = "JSON-async:";
+
+static StructuredData::ObjectSP
+ParseStructuredDataPacket(llvm::StringRef packet) {
+  Log *log(ProcessGDBRemoteLog::GetLogIfAllCategoriesSet(GDBR_LOG_PROCESS));
+
+  if (!packet.consume_front(s_async_json_packet_prefix)) {
+    if (log) {
+      log->Printf(
+          "GDBRemoteCommmunicationClientBase::%s() received $J packet "
+          "but was not a StructuredData packet: packet starts with "
+          "%s",
+          __FUNCTION__,
+          packet.slice(0, strlen(s_async_json_packet_prefix)).str().c_str());
+    }
+    return StructuredData::ObjectSP();
+  }
+
+  // This is an asynchronous JSON packet, destined for a
+  // StructuredDataPlugin.
+  StructuredData::ObjectSP json_sp = StructuredData::ParseJSON(packet);
+  if (log) {
+    if (json_sp) {
+      StreamString json_str;
+      json_sp->Dump(json_str);
+      json_str.Flush();
+      log->Printf("ProcessGDBRemote::%s() "
+                  "received Async StructuredData packet: %s",
+                  __FUNCTION__, json_str.GetString().c_str());
+    } else {
+      log->Printf("ProcessGDBRemote::%s"
+                  "() received StructuredData packet:"
+                  " parse failure",
+                  __FUNCTION__);
+    }
+  }
+  return json_sp;
+}
+
+void ProcessGDBRemote::HandleAsyncStructuredDataPacket(llvm::StringRef data) {
+  auto structured_data_sp = ParseStructuredDataPacket(data);
+  if (structured_data_sp)
+    RouteAsyncStructuredData(structured_data_sp);
 }
 
 class CommandObjectProcessGDBRemoteSpeedTest : public CommandObjectParsed {
 public:
   CommandObjectProcessGDBRemoteSpeedTest(CommandInterpreter &interpreter)
       : CommandObjectParsed(interpreter, "process plugin packet speed-test",
                             "Tests packet speeds of various sizes to determine "
                             "the performance characteristics of the GDB remote "

lldb/trunk/unittests/Process/gdb-remote/GDBRemoteClientBaseTest.cpp

 #endif
 #include <future>
 
 #include "GDBRemoteTestUtils.h"
 
 #include "Plugins/Process/Utility/LinuxSignals.h"
 #include "Plugins/Process/gdb-remote/GDBRemoteClientBase.h"
 #include "Plugins/Process/gdb-remote/GDBRemoteCommunicationServer.h"
+#include "lldb/Core/StreamGDBRemote.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 {
 
 struct MockDelegate : public GDBRemoteClientBase::ContinueDelegate {
   std::string output;
   std::string misc_data;
   unsigned stop_reply_called = 0;
+  std::vector<std::string> structured_data_packets;
 
   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;
+  void HandleAsyncStructuredDataPacket(llvm::StringRef data) {
+    structured_data_packets.push_back(data);
   }
 };
 
 struct TestClient : public GDBRemoteClientBase {
   TestClient() : GDBRemoteClientBase("test.client", "test.client.listener") {
     m_send_acks = false;
   }
 };
TEST_F(GDBRemoteClientBaseTest, SendContinueDelegateInterface) {
   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_payload =
+      "{ \"type\": \"MyFeatureType\", "
+      "  \"elements\": [ \"entry1\", \"entry2\" ] }";
+  const std::string json_packet = "JSON-async:" + json_payload;
+
+  // Escape it properly for transit.
+  StreamGDBRemote stream;
+  stream.PutEscapedBytes(json_packet.c_str(), json_packet.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(1, fix.delegate.structured_data_packets.size());
+
+  // Verify the packet contents.  It should have been unescaped upon packet
+  // reception.
+  ASSERT_EQ(json_packet, fix.delegate.structured_data_packets[0]);
+}
+
 TEST_F(GDBRemoteClientBaseTest, InterruptNoResponse) {
   StringExtractorGDBRemote continue_response, response;
   ContinueFixture fix;
   if (HasFailure())
     return;
 
   // Continue. After the run packet is sent, send an interrupt.
   std::future<StateType> continue_state = std::async(