[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: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".
----------------
jj10306 wrote:
> 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
you are right!


================
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:
> 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


================
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
----------------
jj10306 wrote:
> nit: this should be an unsigned int of some sort?
I wasn't sure if int would be correct for all platforms, but in any case, I should go for unsigned, as you suggest, which is a more restrictive type, and if in the future someone needs a signed int, they could change this type


================
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);
----------------
jj10306 wrote:
> Do we not need to check if `m_per_thread_process_trace_up` is null here (aka per core tracing is enabled)?
I need to check that m_per_thread_process_trace_up exists


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


================
Comment at: lldb/source/Plugins/Process/Linux/IntelPTCollector.h:63-64
+
+/// Manages a "process trace" instance by tracing each thread individually.
+class IntelPTPerThreadProcessTrace {
 public:
----------------
jj10306 wrote:
> Why is this named "PerThread" if it handles both the per thread and per core cases for "process trace"? Perhaps I'm missing something
this doesn't handle the per core case. This is handling exclusively the case of "process trace" without the "per-cpu" option. This class effectively creates a perf event per thread. Even its Start method asks for tids


================
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:
> So these are mutually exclusive tight?
yes, I'll leave a comment about that here


================
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());
----------------
jj10306 wrote:
> nit: why are we using signed integers here?
yes, this is bad, i'm changing to unsigned


================
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.
----------------
jj10306 wrote:
> Maybe add explanation of what is passed to the syscall for pid and cpu when they are `None` here?
makes sense


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124858