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 _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits