wallace added inline comments.
================
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>,
----------------
jj10306 wrote:
> 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?
yes, exactly
================
Comment at: lldb/source/Plugins/Process/Linux/IntelPTCollector.h:152-156
+ IntelPTPerThreadProcessTraceUP m_per_thread_process_trace_up;
+ /// Cores traced due to per-core "process tracing". Only one active
+ /// "process tracing" instance is allowed for a single process.
+ /// This might be \b nullptr.
+ IntelPTMultiCoreTraceUP m_per_core_process_trace_up;
----------------
jj10306 wrote:
> wallace wrote:
> > jj10306 wrote:
> > > So these are mutually exclusive tight?
> > yes, I'll leave a comment about that here
> this could be overkill, but potentially we could use a union here to enforce
> this invariant and a boolean flag/enum to determine which process tracing
> "handler" is being used
> ```
> bool is_per_core_process_tracing_enabled; // used to determine how to
> interpret the union
> union {
> IntelPTPerThreadProcessTraceUP m_per_thread_process_trace_up;
> IntelPTMultiCoreTraceUP m_per_core_process_trace_up;
>
> };
> ```
> imo this is much more clearly expresses the invariant of only one of the
> process tracing "handles" being non null at a time.
> wdyt?
I tried this, but using unions with complex types requires some non-trivial
work. std::variant would be a good choice but is not available in the c++
version used by llvm. So in this case I think the current implementation is the
simpler. However, I'd rather create an interface and have one single instance
based on that interface. I'll try that
================
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
----------------
jj10306 wrote:
> nit: we could make this optional as well and just default to passing in -1 to
> be consistent with cpu and pid
good idea
================
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}});
----------------
jj10306 wrote:
> isn't uint64_t serialization and deserialization now supported?
good catch
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
