[Lldb-commits] [PATCH] D32585: Implementation of remote packets for Trace data.

[Greg Clayton via Phabricator via lldb-commits]

clayborg added a comment.

So my question is: if we go the jTrace packet route, I would like to not use GDB remote key/value pairs, and just go full JSON. You can make a new JSON dict and pass all of the GDB remote key/value pairs inside and then you can add a "jparams" key whose value is the JSON from the SB layer. JSON packets start with lower case j, not sure why some were upper cased?



================
Comment at: docs/lldb-gdb-remote.txt:212
 //----------------------------------------------------------------------
+// JTrace:start:
+//
----------------
I think JSON packets start with lower case 'j'


================
Comment at: docs/lldb-gdb-remote.txt:217-218
+//  parameters (mandatory and optional) should be appended to the packet
+//  although there is no specific order imposed. The parameters need to
+//  formatted as a semi-colon seperated list of "Name:Value" pairs.
+//  Different tracing types could require different custom parameters.
----------------
Need to remove this bit about semicolon separates key/value pairs as everything is now in the JSON


================
Comment at: docs/lldb-gdb-remote.txt:239
+//
+//  threadid        The id of the thread to start tracing    O
+//                  on.
----------------
Maybe state that all threads will be traced if this is not supplied?


================
Comment at: docs/lldb-gdb-remote.txt:256
+
+send packet: JTrace:start:type:<type>;buffersize:<buffersize>;
+read packet: <trace id>/E<error code>
----------------
So I am not sure why we are mixing GDB remote key:value; with JSON here? I was thinking the packet would be jTrace:XXXXX where XXXX is the JSON that contains all key value pairs and it also might have an extra jparams key whose value is the JSON from the SB interface.

If we need more than 1 jTrace command it could be "jTraceStart:XXXX" and "jTraceStop:XXXX"


================
Comment at: source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp:187-188
+  RegisterMemberFunctionHandler(
+      StringExtractorGDBRemote::eServerPacketType_JTrace_Start,
+      &GDBRemoteCommunicationServerLLGS::Handle_JTrace_start);
+  RegisterMemberFunctionHandler(
----------------
lower case j


================
Comment at: source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp:196-197
+  RegisterMemberFunctionHandler(
+      StringExtractorGDBRemote::eServerPacketType_JTrace_Stop,
+      &GDBRemoteCommunicationServerLLGS::Handle_JTrace_stop);
+  RegisterMemberFunctionHandler(
----------------
lower case j


================
Comment at: source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp:199-200
+  RegisterMemberFunctionHandler(
+      StringExtractorGDBRemote::eServerPacketType_jTraceConfigRead,
+      &GDBRemoteCommunicationServerLLGS::Handle_jTrace_conf_read);
+
----------------
lower case j


================
Comment at: source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp:1110
+
+  if (!packet.Consume_front("JTrace:start:"))
+    return SendIllFormedResponse(packet, "JTrace:start: Ill formed packet ");
----------------
lower case j


================
Comment at: source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.h:192
 
+  PacketResult Handle_JTrace_start(StringExtractorGDBRemote &packet);
+
----------------
lower case j


================
Comment at: source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.h:196
+
+  PacketResult Handle_JTrace_stop(StringExtractorGDBRemote &packet);
+
----------------
lower case j


================
Comment at: source/Utility/StringExtractorGDBRemote.cpp:282
 
+  case 'J':
+    if (PACKET_STARTS_WITH("JTrace:start:"))
----------------
lower case j


================
Comment at: source/Utility/StringExtractorGDBRemote.cpp:283
+  case 'J':
+    if (PACKET_STARTS_WITH("JTrace:start:"))
+      return eServerPacketType_JTrace_Start;
----------------
lower case j


================
Comment at: source/Utility/StringExtractorGDBRemote.cpp:285
+      return eServerPacketType_JTrace_Start;
+    if (PACKET_STARTS_WITH("JTrace:stop:"))
+      return eServerPacketType_JTrace_Stop;
----------------
lower case j


https://reviews.llvm.org/D32585