[Lldb-commits] [PATCH] D124648: [trace][intelpt] Support system-wide tracing [3] - Refactor IntelPTThreadTrace

[walter erquinigo via Phabricator via lldb-commits]

wallace added inline comments.


================
Comment at: lldb/docs/lldb-gdb-remote.txt:498
 //  Binary data kinds:
-//    - threadTraceBuffer: trace buffer for a thread.
+//    - traceBuffer: trace buffer for a thread.
 //    - procFsCpuInfo: contents of the /proc/cpuinfo file.
----------------
jj10306 wrote:
> If perCore tracing is enabled, how will this packet work since currently it requires a tid, but in perCore mode the trace data will contain all activity on that core, not just the specified thread?
the jLLDBGetBinaryData packet has, for example, an optional tid argument that indicates the data correspond to a specific tid. I'll add a new optional parameter called core_id, so that we can associate a data chunk with a core_id


================
Comment at: lldb/source/Plugins/Process/Linux/IntelPTCollector.h:136
-
 /// Manages a list of thread traces.
 class IntelPTThreadTraceCollection {
----------------
jj10306 wrote:
> update doc since this is no longer tied to thread's traces
good catch!


================
Comment at: lldb/source/Plugins/Process/Linux/IntelPTSingleBufferTrace.cpp:292-298
+  if (Expected<PerfEvent> perf_event = PerfEvent::Init(*attr, tid)) {
+    if (Error mmap_err = perf_event->MmapMetadataAndBuffers(buffer_numpages,
+                                                            buffer_numpages)) {
+      return std::move(mmap_err);
+    }
+    return IntelPTSingleBufferTraceUP(
+        new IntelPTSingleBufferTrace(std::move(*perf_event), tid));
----------------
jj10306 wrote:
> The PerfEvent logic will need to be updated to support per CPU or per thread as it currently only supports per thread
yes, definitely


================
Comment at: lldb/source/Plugins/Process/Linux/IntelPTSingleBufferTrace.h:1
+//===-- IntelPTSingleBufferTrace.h ---------------------------- -*- C++ -*-===//
+//
----------------
jj10306 wrote:
> nit: thoughts on the name  `IntelPTTraceBuffer` instead of `IntelPTSingleBufferTrace`? The current name is a bit wordy imo
this is intentional because the next file will be IntelPTMultiCoreTrace.h. 

I also don't want to call this IntelPTTraceBuffer because it'll eventually have itrace data so the object won't be just a trace buffer.

But I'm open to any suggestions because I also don't like super verbose names


================
Comment at: lldb/source/Plugins/Process/Linux/IntelPTSingleBufferTrace.h:41-48
+  /// \param[in] tid
+  ///     The tid of the thread to be traced.
+  ///
+  /// \return
+  ///   A \a IntelPTSingleBufferTrace instance if tracing was successful, or
+  ///   an \a llvm::Error otherwise.
+  static llvm::Expected<IntelPTSingleBufferTraceUP>
----------------
jj10306 wrote:
> Shouldn't this structure be general and have no notion of a tid since it could represent the buffer of a single thread or the buffer of a single CPU?
> The way I see it, this structure simply wraps the buffer of a specific perf_event but has no notion of if it's for a specific tid or cpu.
> Then you could have two subclasses, one for thread one for cpu, that inherit from this and have the additional context about the buffer. The inheritance may be overkill, but point I'm trying to make is that this structure should be agnostic to what "unit's" (thread or cpu) trace data it contains
what I was thinking is to change this in a later diff to be like 
  Start(const TraceIntelPTStartRequest &request, Optional<lldb::tid_t> tid, Optional<int> core_id);

which seems to me generic enough for all possible use cases. LLDB couldn't support cgroups, so we are limited to tids and core_ids. With this, it seems to me that having two child classes it's a little bit too much because the only difference are just two lines of code where we specify the perf_event configuration for tid and core_id.

But if you insist in the clarify of the subclasses, I for sure can do it that way.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124648