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 _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
