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

[Greg Clayton via Phabricator via lldb-commits]

clayborg requested changes to this revision.
clayborg added a comment.
This revision now requires changes to proceed.

Close, some minor fixes.



================
Comment at: source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp:3293
+      if (custom_params_sp) {
+        if (custom_params_sp->GetType() != StructuredData::Type::eTypeDictionary) {
+          error.SetErrorString("Invalid Configuration obtained");
----------------
Might be simpler to just do:

```
if (!custom_params_sp->GetAsDictionary())
```



================
Comment at: source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp:188
+      StringExtractorGDBRemote::eServerPacketType_jTraceStart,
+      &GDBRemoteCommunicationServerLLGS::Handle_jTrace_start);
+  RegisterMemberFunctionHandler(
----------------
Rename to match previous inline comments


================
Comment at: source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp:191
+      StringExtractorGDBRemote::eServerPacketType_jTraceBufferRead,
+      &GDBRemoteCommunicationServerLLGS::Handle_jTrace_read);
+  RegisterMemberFunctionHandler(
----------------
Rename to match previous inline comments


================
Comment at: source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp:194
+      StringExtractorGDBRemote::eServerPacketType_jTraceMetaRead,
+      &GDBRemoteCommunicationServerLLGS::Handle_jTrace_read);
+  RegisterMemberFunctionHandler(
----------------
Rename to match previous inline comments


================
Comment at: source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp:197
+      StringExtractorGDBRemote::eServerPacketType_jTraceStop,
+      &GDBRemoteCommunicationServerLLGS::Handle_jTrace_stop);
+  RegisterMemberFunctionHandler(
----------------
Rename to match previous inline comments


================
Comment at: source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp:200
+      StringExtractorGDBRemote::eServerPacketType_jTraceConfigRead,
+      &GDBRemoteCommunicationServerLLGS::Handle_jTrace_conf_read);
+
----------------
Rename to match previous inline comments


================
Comment at: source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp:1141
+
+  StructuredData::ObjectSP custom_params_sp = json_dict->GetValueForKey("jparams");
+  options.setTraceParams(static_pointer_cast<StructuredData::Dictionary> (custom_params_sp));
----------------
verify this is a dictionary first before the static cast below.


================
Comment at: source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.h:192
 
+  PacketResult Handle_jTrace_start(StringExtractorGDBRemote &packet);
+
----------------
"Handle_jTraceStart" to make it match the packet name of "jTraceStart"


================
Comment at: source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.h:194
+
+  PacketResult Handle_jTrace_read(StringExtractorGDBRemote &packet);
+
----------------
"Handle_jTraceRead"


================
Comment at: source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.h:196
+
+  PacketResult Handle_jTrace_stop(StringExtractorGDBRemote &packet);
+
----------------
Handle_jTraceStop


================
Comment at: source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.h:198
+
+  PacketResult Handle_jTrace_conf_read(StringExtractorGDBRemote &packet);
+
----------------
Handle_ jTraceConfigRead


================
Comment at: unittests/Process/gdb-remote/GDBRemoteCommunicationClientTest.cpp:400
+
+  HandlePacket(server, "jTraceStart:{\"buffersize\" : 8192,\"jparams\" : {\"psb\" : 1,\"tracetech\" : \"intel-pt\"},\"metabuffersize\" : 8192,\"threadid\" : 35,\"type\" : 1}", "1");
+  ASSERT_EQ(result.get(),1);
----------------
Use the R"( so you don't have to desensitize the double quotes and so you can format nicely:

```
const char *json = R"(
jTraceStart: {
  "buffersize" : 8192,
  "metabuffersize" : 8192,
  "threadid" : 35,
  "type" : 1,
  "jparams" : {
    "psb" : 1,
    "tracetech" : "intel-pt"
  }
})";
HandlePacket(server, json, "1");
```


================
Comment at: unittests/Process/gdb-remote/GDBRemoteCommunicationClientTest.cpp:409
+
+  HandlePacket(server, "jTraceStart:{\"buffersize\" : 8192,\"jparams\" : {\"psb\" : 1,\"tracetech\" : \"intel-pt\"},\"metabuffersize\" : 8192,\"threadid\" : 35,\"type\" : 1}", "E23");
+  ASSERT_EQ(result.get(), LLDB_INVALID_UID);
----------------
Ditto


================
Comment at: unittests/Process/gdb-remote/GDBRemoteCommunicationClientTest.cpp:428
+
+  HandlePacket(server, "jTraceStop:{\"threadid\" : 35,\"traceid\" : 3}", "OK");
+  ASSERT_TRUE(result.get().Success());
----------------
Ditto, but inline the R"(...)"


================
Comment at: unittests/Process/gdb-remote/GDBRemoteCommunicationClientTest.cpp:435
+
+  HandlePacket(server, "jTraceStop:{\"threadid\" : 35,\"traceid\" : 3}", "E23");
+  ASSERT_FALSE(result.get().Success());
----------------
Ditto, but inline the R"(...)"


================
Comment at: unittests/Process/gdb-remote/GDBRemoteCommunicationClientTest.cpp:457
+
+  StringRef expected("jTraceBufferRead:{\"buffersize\" : 32,\"offset\" : 0,\"threadid\" : 35,\"traceid\" : 3}");
+  HandlePacket(server, expected, "123456");
----------------
Ditto, but inline the R"(...)"


================
Comment at: unittests/Process/gdb-remote/GDBRemoteCommunicationClientTest.cpp:493
+
+  StringRef expected("jTraceMetaRead:{\"buffersize\" : 32,\"offset\" : 0,\"threadid\" : 35,\"traceid\" : 3}");
+  HandlePacket(server, expected, "123456");
----------------
Ditto, but inline the R"(...)"


================
Comment at: unittests/Process/gdb-remote/GDBRemoteCommunicationClientTest.cpp:527
+
+  HandlePacket(server, "jTraceConfigRead:{\"threadid\" : 35,\"traceid\" : 3}",
+                       "{\"buffersize\" : 8192,\"jparams\" : {\"psb\" : 1,\"tracetech\" : \"intel-pt\"}],\"metabuffersize\" : 8192,\"threadid\" : 35,\"type\" : 1}]");
----------------
Use the R"( 


================
Comment at: unittests/Process/gdb-remote/GDBRemoteCommunicationClientTest.cpp:551
+
+  HandlePacket(server, "jTraceConfigRead:{\"threadid\" : 35,\"traceid\" : 3}",
+                       "E23");
----------------
Ditto, but inline the R"(...)"


================
Comment at: unittests/Process/gdb-remote/GDBRemoteCommunicationClientTest.cpp:560
+
+  HandlePacket(server, "jTraceConfigRead:{\"threadid\" : 35,\"traceid\" : 3}",
+                       "\"buffersize\" : 8192,\"jparams\" : {\"psb\" : 1,\"tracetech\" : \"intel-pt\"}],\"metabuffersize\" : 8192,\"threadid\" : 35,\"type\" : 1}]");
----------------
Use the R"( 


================
Comment at: unittests/Process/gdb-remote/GDBRemoteCommunicationClientTest.cpp:569
+
+  HandlePacket(server, "jTraceConfigRead:{\"threadid\" : 35,\"traceid\" : 3}",
+                       "{\"buffersize\" : 8192,\"jparams\" : \"psb\" : 1,\"tracetech\" : \"intel-pt\"}],\"metabuffersize\" : 8192,\"threadid\" : 35,\"type\" : 1}]");
----------------
Use the R"( 


https://reviews.llvm.org/D32585