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

[walter erquinigo via Phabricator via lldb-commits]

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