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

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

Thank you for taking the time to write the test. Just a couple of more comments on things I noticed when going through this again:



================
Comment at: source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp:3172
+  if (custom_params)
+    json_packet.AddItem("jparams", custom_params);
+
----------------
I don't think we need the "j" in the "jparams" now that the whole packet is json.


================
Comment at: source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp:3288
+
+      json_dict->GetValueForKeyAsInteger<uint64_t>("type", type);
+      options.setType(static_cast<lldb::TraceType>(type));
----------------
I'm wondering whether we should be sending the trace type as an opaque number -- it's much easier to allocate IDs and manage compatibility if this is a string field. Greg, what do you think?


================
Comment at: source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp:1142
+  StructuredData::ObjectSP custom_params_sp = json_dict->GetValueForKey("jparams");
+  options.setTraceParams(static_pointer_cast<StructuredData::Dictionary> (custom_params_sp));
+
----------------
What if jparams **isn't** a dictionary?


================
Comment at: source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp:1298
+  std::vector<uint8_t> buffer(byte_count, '\0');
+  if (buffer.empty())
+    return SendErrorResponse(0x78);
----------------
this is dead code. empty will never return true here. The process will just get killed if it fails to allocate.

However, you are right that we should check the allocation as we are allocating a buffer of user-provided size. I recommend explicitly allocating a uint8_t[] using nothrow new. It doesn't stop us from getting killed if the kernel overcommits, but it's better than nothing.

Alternatively you could also change the API to leave the memory allocation to the callee, which will be in a better position do determine whether the size is reasonable.


https://reviews.llvm.org/D32585