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