[Lldb-commits] [PATCH] D125503: [trace][intelpt] Support system-wide tracing [7] - Create a base IntelPTProcessTrace class

[walter erquinigo via Phabricator via lldb-commits]

wallace added inline comments.


================
Comment at: lldb/source/Plugins/Process/Linux/IntelPTCollector.h:73
+  /// The target process.
   NativeProcessProtocol &m_process;
   /// Threads traced due to "thread tracing"
----------------
jj10306 wrote:
> Could this be moved to be a part of the new `IntelPTProcessTrace` abstract class or is this also needed for handling `thread trace`?
i can't because you the reason you mention: thread trace


================
Comment at: lldb/source/Plugins/Process/Linux/IntelPTMultiCoreTrace.cpp:36
 
-Expected<IntelPTMultiCoreTraceUP> IntelPTMultiCoreTrace::StartOnAllCores(
-    const TraceIntelPTStartRequest &request) {
+Expected<IntelPTProcessTraceUP>
+IntelPTMultiCoreTrace::StartOnAllCores(const TraceIntelPTStartRequest &request,
----------------
jj10306 wrote:
> If this now returns `IntelPTProcessTraceUP` instead of an instance of itself, then we are basically forcing any uses of this class to be done using dynamic polymorphism (behind the indirection of a pointer to the super class). Should this instead return an instance of itself and then leave it up to the caller to decide if they want to use the object directly or behind a pointer?
> I know that currently the only use of this is behind the `IntelPTProcessTraceUP` (stored on the collector), but maybe in the future we would want to use it directly.
> wdyt?
> 
good call


================
Comment at: lldb/source/Plugins/Process/Linux/IntelPTMultiCoreTrace.cpp:119-125
+  // All the process' threads are being traced automatically.
+  return (bool)m_process.GetThreadByID(tid);
+}
+
+llvm::Error IntelPTMultiCoreTrace::TraceStart(lldb::tid_t tid) {
+  // This instance is already tracing all threads automatically.
+  return llvm::Error::success();
----------------
jj10306 wrote:
> In `TracesThread` you check that the tid is a thread of `m_process` but in `TraceStart` you return success for all threads without checking if it's a part of the process.
> I don't think it particularly matters if we do the check or not, but I do think that the behavior should be consistent between these functions.
ahh you are right


================
Comment at: lldb/source/Plugins/Process/Linux/IntelPTPerThreadProcessTrace.cpp:58
+
+Expected<IntelPTProcessTraceUP>
+IntelPTPerThreadProcessTrace::Start(const TraceIntelPTStartRequest &request,
----------------
jj10306 wrote:
> same comment here as above on the other subclasss
+1


================
Comment at: lldb/source/Plugins/Process/Linux/IntelPTProcessTrace.h:19
+
+// Abstract class to be inherited by all the process tracing strategies.
+class IntelPTProcessTrace {
----------------
jj10306 wrote:
> 
+1


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125503