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

[Jakob Johnson via Phabricator via lldb-commits]

jj10306 requested changes to this revision.
jj10306 added inline comments.
This revision now requires changes to proceed.


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


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


================
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));
----------------
The PerfEvent logic will need to be updated to support per CPU or per thread as it currently only supports per thread


================
Comment at: lldb/source/Plugins/Process/Linux/IntelPTSingleBufferTrace.h:1
+//===-- IntelPTSingleBufferTrace.h ---------------------------- -*- C++ -*-===//
+//
----------------
nit: thoughts on the name  `IntelPTTraceBuffer` instead of `IntelPTSingleBufferTrace`? The current name is a bit wordy imo


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124648