[Lldb-commits] [PATCH] D124858: [trace][intelpt] Support system-wide tracing [4] - Support per core tracing on lldb-server

[Jakob Johnson via Phabricator via lldb-commits]

jj10306 accepted this revision.
jj10306 added a comment.
This revision is now accepted and ready to land.

couple minor things, but looks good overall



================
Comment at: lldb/docs/lldb-gdb-remote.txt:465-474
 //      "tid": <decimal integer>,
 //      "binaryData": [
 //        {
 //          "kind": <string>,
 //              Identifier for some binary data related to this thread to
 //              fetch with the jLLDBTraceGetBinaryData packet.
 //          "size": <decimal integer>,
----------------
wallace wrote:
> wallace wrote:
> > jj10306 wrote:
> > > Should the thread info be optional now that we have an optional `cores`? If not, can you explain how this output works in the case that you are doing per core tracing? Will both the tid binary data and the cores binary data section be populated?
> > I'll make this field optional and include more documentation below
> in the end i didn't make this optional but instead forced the per-core case to return all threads 
so the tid field of a tracedThread object will be populated but the binaryData will not be populated in the tracedThread object but instead in the cores section?


================
Comment at: lldb/source/Plugins/Process/Linux/Perf.h:119
   ///
   /// \param[in] group_fd
+  ///     File descriptor of the group leader. \b -1 indicated that this
----------------
nit: we could make this optional as well and just default to passing in -1 to be consistent with cpu and pid


================
Comment at: lldb/source/Utility/TraceGDBRemotePackets.cpp:124
+json::Value toJSON(const TraceCoreState &packet) {
+  return json::Value(Object{{"coreId", static_cast<int64_t>(packet.core_id)},
+                            {"binaryData", packet.binary_data}});
----------------
isn't uint64_t serialization and deserialization now supported?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D124858/new/

https://reviews.llvm.org/D124858