[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/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:
> 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?
it's in a later diff


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124648