[Lldb-commits] [lldb] r281121 - async structured data packet handling improvements

Todd Fiala via lldb-commits lldb-commits at lists.llvm.org
Fri Sep 9 17:06:30 PDT 2016


Author: tfiala
Date: Fri Sep  9 19:06:29 2016
New Revision: 281121

URL: http://llvm.org/viewvc/llvm-project?rev=281121&view=rev
Log:
async structured data packet handling improvements

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:".

Reviewers: labath, clayborg, zturner

Differential Revision: https://reviews.llvm.org/D23884

Modified:
    lldb/trunk/source/Plugins/Process/gdb-remote/GDBRemoteClientBase.cpp
    lldb/trunk/source/Plugins/Process/gdb-remote/GDBRemoteClientBase.h
    lldb/trunk/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
    lldb/trunk/source/Plugins/Process/gdb-remote/ProcessGDBRemote.h
    lldb/trunk/unittests/Process/gdb-remote/GDBRemoteClientBaseTest.cpp

Modified: lldb/trunk/source/Plugins/Process/gdb-remote/GDBRemoteClientBase.cpp
URL: http://llvm.org/viewvc/llvm-project/lldb/trunk/source/Plugins/Process/gdb-remote/GDBRemoteClientBase.cpp?rev=281121&r1=281120&r2=281121&view=diff
==============================================================================
--- lldb/trunk/source/Plugins/Process/gdb-remote/GDBRemoteClientBase.cpp (original)
+++ lldb/trunk/source/Plugins/Process/gdb-remote/GDBRemoteClientBase.cpp Fri Sep  9 19:06:29 2016
@@ -103,38 +103,9 @@ StateType GDBRemoteClientBase::SendConti
       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());
+      break;
     case 'T':
     case 'S':
       // Do this with the continue lock held.

Modified: lldb/trunk/source/Plugins/Process/gdb-remote/GDBRemoteClientBase.h
URL: http://llvm.org/viewvc/llvm-project/lldb/trunk/source/Plugins/Process/gdb-remote/GDBRemoteClientBase.h?rev=281121&r1=281120&r2=281121&view=diff
==============================================================================
--- lldb/trunk/source/Plugins/Process/gdb-remote/GDBRemoteClientBase.h (original)
+++ lldb/trunk/source/Plugins/Process/gdb-remote/GDBRemoteClientBase.h Fri Sep  9 19:06:29 2016
@@ -25,14 +25,13 @@ public:
     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);

Modified: lldb/trunk/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
URL: http://llvm.org/viewvc/llvm-project/lldb/trunk/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp?rev=281121&r1=281120&r2=281121&view=diff
==============================================================================
--- lldb/trunk/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp (original)
+++ lldb/trunk/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp Fri Sep  9 19:06:29 2016
@@ -4808,9 +4808,49 @@ void ProcessGDBRemote::HandleStopReply()
   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 {

Modified: lldb/trunk/source/Plugins/Process/gdb-remote/ProcessGDBRemote.h
URL: http://llvm.org/viewvc/llvm-project/lldb/trunk/source/Plugins/Process/gdb-remote/ProcessGDBRemote.h?rev=281121&r1=281120&r2=281121&view=diff
==============================================================================
--- lldb/trunk/source/Plugins/Process/gdb-remote/ProcessGDBRemote.h (original)
+++ lldb/trunk/source/Plugins/Process/gdb-remote/ProcessGDBRemote.h Fri Sep  9 19:06:29 2016
@@ -415,8 +415,7 @@ private:
   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.

Modified: lldb/trunk/unittests/Process/gdb-remote/GDBRemoteClientBaseTest.cpp
URL: http://llvm.org/viewvc/llvm-project/lldb/trunk/unittests/Process/gdb-remote/GDBRemoteClientBaseTest.cpp?rev=281121&r1=281120&r2=281121&view=diff
==============================================================================
--- lldb/trunk/unittests/Process/gdb-remote/GDBRemoteClientBaseTest.cpp (original)
+++ lldb/trunk/unittests/Process/gdb-remote/GDBRemoteClientBaseTest.cpp Fri Sep  9 19:06:29 2016
@@ -20,6 +20,7 @@
 #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"
 
@@ -34,14 +35,14 @@ struct MockDelegate : public GDBRemoteCl
   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);
   }
 };
 
@@ -321,6 +322,37 @@ TEST_F(GDBRemoteClientBaseTest, SendCont
   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;




More information about the lldb-commits mailing list