[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/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);
----------------
wallace wrote:
> 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
Yes, I meant "is *not* null here", my bad (-:


================
Comment at: lldb/source/Plugins/Process/Linux/IntelPTCollector.h:63-64
+
+/// Manages a "process trace" instance by tracing each thread individually.
+class IntelPTPerThreadProcessTrace {
 public:
----------------
wallace wrote:
> 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
ahhh yes I got messed up by the diff view and thought that the fields of IntelPTCollector were being stored on this per thread structure, thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124858