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

[Jakob Johnson via Phabricator via lldb-commits]

jj10306 accepted this revision.
jj10306 added a comment.
This revision is now accepted and ready to land.

I'm assuming the changes to make the single buffer trace class generic for threads and cpu buffers is done in the next diff so stamping this, feel free to update with those changes if that is not the case



================
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>
----------------
wallace wrote:
> 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.
What you suggested makes sense, do you plan make this change in this diff or is this in one of the future ones?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124648