jj10306 requested changes to this revision.
jj10306 added inline comments.
This revision now requires changes to proceed.
================
Comment at: lldb/docs/lldb-gdb-remote.txt:498
// Binary data kinds:
-// - threadTraceBuffer: trace buffer for a thread.
+// - traceBuffer: trace buffer for a thread.
// - procFsCpuInfo: contents of the /proc/cpuinfo file.
----------------
If perCore tracing is enabled, how will this packet work since currently it
requires a tid, but in perCore mode the trace data will contain all activity on
that core, not just the specified thread?
================
Comment at: lldb/source/Plugins/Process/Linux/IntelPTCollector.h:136
-
/// Manages a list of thread traces.
class IntelPTThreadTraceCollection {
----------------
update doc since this is no longer tied to thread's traces
================
Comment at:
lldb/source/Plugins/Process/Linux/IntelPTSingleBufferTrace.cpp:292-298
+ if (Expected<PerfEvent> perf_event = PerfEvent::Init(*attr, tid)) {
+ if (Error mmap_err = perf_event->MmapMetadataAndBuffers(buffer_numpages,
+ buffer_numpages)) {
+ return std::move(mmap_err);
+ }
+ return IntelPTSingleBufferTraceUP(
+ new IntelPTSingleBufferTrace(std::move(*perf_event), tid));
----------------
The PerfEvent logic will need to be updated to support per CPU or per thread as
it currently only supports per thread
================
Comment at: lldb/source/Plugins/Process/Linux/IntelPTSingleBufferTrace.h:1
+//===-- IntelPTSingleBufferTrace.h ---------------------------- -*- C++
-*-===//
+//
----------------
nit: thoughts on the name `IntelPTTraceBuffer` instead of
`IntelPTSingleBufferTrace`? The current name is a bit wordy imo
================
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>
----------------
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
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
