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