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

[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/source/Plugins/Process/Linux/IntelPTCollector.h:73
+  /// The target process.
   NativeProcessProtocol &m_process;
   /// Threads traced due to "thread tracing"
----------------
Could this be moved to be a part of the new `IntelPTProcessTrace` abstract class or is this also needed for handling `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,
----------------
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?



================
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();
----------------
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.


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


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



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125503