[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 added inline comments.


================
Comment at: lldb/docs/lldb-gdb-remote.txt:372-374
+//         This limit applies to the sum of the sizes of all trace and core
+//         buffers for the current process, excluding the ones started with
+//         "thread tracing".
----------------
Not sure what is trying to be communicated hear, so maybe my suggestion doesn't make sense, but I was confused by the wording here


================
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>,
----------------
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?


================
Comment at: lldb/include/lldb/Utility/TraceGDBRemotePackets.h:157-160
+  std::vector<TraceThreadState> traced_threads;
+  std::vector<TraceBinaryData> process_binary_data;
+  llvm::Optional<std::vector<TraceCoreState>> cores;
 };
----------------
Similar question here as above on the GetState documentation - how do the cores and traced_threads options play together?


================
Comment at: lldb/include/lldb/lldb-types.h:92
 typedef uint64_t queue_id_t;
+typedef int core_id_t; // CPU core id
 } // namespace lldb
----------------
nit: this should be an unsigned int of some sort?


================
Comment at: lldb/source/Plugins/Process/Linux/IntelPTCollector.cpp:228-234
+  return m_per_thread_process_trace_up->TraceStart(tid);
 }
 
 Error IntelPTCollector::OnThreadDestroyed(lldb::tid_t tid) {
-  if (IsProcessTracingEnabled() && m_process_trace->TracesThread(tid))
-    return m_process_trace->TraceStop(tid);
+  if (IsProcessTracingEnabled() &&
+      m_per_thread_process_trace_up->TracesThread(tid))
+    return m_per_thread_process_trace_up->TraceStop(tid);
----------------
Do we not need to check if `m_per_thread_process_trace_up` is null here (aka per core tracing is enabled)?


================
Comment at: lldb/source/Plugins/Process/Linux/IntelPTCollector.cpp:260
+  }
+  if (m_per_core_process_trace_up) {
+    state.cores.emplace();
----------------
Should this be an "else if" if these two are mutually exclusive?


================
Comment at: lldb/source/Plugins/Process/Linux/IntelPTCollector.h:63-64
+
+/// Manages a "process trace" instance by tracing each thread individually.
+class IntelPTPerThreadProcessTrace {
 public:
----------------
Why is this named "PerThread" if it handles both the per thread and per core cases for "process trace"? Perhaps I'm missing something


================
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;
----------------
So these are mutually exclusive tight?


================
Comment at: lldb/source/Plugins/Process/Linux/IntelPTMultiCoreTrace.cpp:20-22
+  int64_t required = cores.size() * request.trace_buffer_size;
+  int64_t limit = request.process_buffer_size_limit.getValueOr(
+      std::numeric_limits<int64_t>::max());
----------------
nit: why are we using signed integers here?


================
Comment at: lldb/source/Plugins/Process/Linux/Perf.h:111-115
   /// \param[in] pid
-  ///     The process to be monitored by the event.
+  ///     The process or thread to be monitored by the event.
   ///
   /// \param[in] cpu
   ///     The cpu to be monitored by the event.
----------------
Maybe add explanation of what is passed to the syscall for pid and cpu when they are `None` here?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124858