wallace added inline comments.
================
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.
----------------
jj10306 wrote:
> 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?
the jLLDBGetBinaryData packet has, for example, an optional tid argument that
indicates the data correspond to a specific tid. I'll add a new optional
parameter called core_id, so that we can associate a data chunk with a core_id
================
Comment at: lldb/source/Plugins/Process/Linux/IntelPTCollector.h:136
-
/// Manages a list of thread traces.
class IntelPTThreadTraceCollection {
----------------
jj10306 wrote:
> update doc since this is no longer tied to thread's traces
good catch!
================
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));
----------------
jj10306 wrote:
> The PerfEvent logic will need to be updated to support per CPU or per thread
> as it currently only supports per thread
yes, definitely
================
Comment at: lldb/source/Plugins/Process/Linux/IntelPTSingleBufferTrace.h:1
+//===-- IntelPTSingleBufferTrace.h ---------------------------- -*- C++
-*-===//
+//
----------------
jj10306 wrote:
> nit: thoughts on the name `IntelPTTraceBuffer` instead of
> `IntelPTSingleBufferTrace`? The current name is a bit wordy imo
this is intentional because the next file will be IntelPTMultiCoreTrace.h.
I also don't want to call this IntelPTTraceBuffer because it'll eventually have
itrace data so the object won't be just a trace buffer.
But I'm open to any suggestions because I also don't like super verbose names
================
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:
> 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.
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
