wallace added inline comments.
================
Comment at: lldb/docs/lldb-gdb-remote.txt:372-374
+// This limit applies to the sum of the sizes of all trace and core
+// buffers for the current process, excluding the ones started with
+// "thread tracing".
----------------
jj10306 wrote:
> Not sure what is trying to be communicated hear, so maybe my suggestion
> doesn't make sense, but I was confused by the wording here
you are right!
================
Comment at: lldb/docs/lldb-gdb-remote.txt:465-474
// "tid": <decimal integer>,
// "binaryData": [
// {
// "kind": <string>,
// Identifier for some binary data related to this thread to
// fetch with the jLLDBTraceGetBinaryData packet.
// "size": <decimal integer>,
----------------
jj10306 wrote:
> Should the thread info be optional now that we have an optional `cores`? If
> not, can you explain how this output works in the case that you are doing per
> core tracing? Will both the tid binary data and the cores binary data section
> be populated?
I'll make this field optional and include more documentation below
================
Comment at: lldb/include/lldb/lldb-types.h:92
typedef uint64_t queue_id_t;
+typedef int core_id_t; // CPU core id
} // namespace lldb
----------------
jj10306 wrote:
> nit: this should be an unsigned int of some sort?
I wasn't sure if int would be correct for all platforms, but in any case, I
should go for unsigned, as you suggest, which is a more restrictive type, and
if in the future someone needs a signed int, they could change this type
================
Comment at: lldb/source/Plugins/Process/Linux/IntelPTCollector.cpp:228-234
+ return m_per_thread_process_trace_up->TraceStart(tid);
}
Error IntelPTCollector::OnThreadDestroyed(lldb::tid_t tid) {
- if (IsProcessTracingEnabled() && m_process_trace->TracesThread(tid))
- return m_process_trace->TraceStop(tid);
+ if (IsProcessTracingEnabled() &&
+ m_per_thread_process_trace_up->TracesThread(tid))
+ return m_per_thread_process_trace_up->TraceStop(tid);
----------------
jj10306 wrote:
> Do we not need to check if `m_per_thread_process_trace_up` is null here (aka
> per core tracing is enabled)?
I need to check that m_per_thread_process_trace_up exists
================
Comment at: lldb/source/Plugins/Process/Linux/IntelPTCollector.cpp:260
+ }
+ if (m_per_core_process_trace_up) {
+ state.cores.emplace();
----------------
jj10306 wrote:
> Should this be an "else if" if these two are mutually exclusive?
good idea
================
Comment at: lldb/source/Plugins/Process/Linux/IntelPTCollector.h:63-64
+
+/// Manages a "process trace" instance by tracing each thread individually.
+class IntelPTPerThreadProcessTrace {
public:
----------------
jj10306 wrote:
> Why is this named "PerThread" if it handles both the per thread and per core
> cases for "process trace"? Perhaps I'm missing something
this doesn't handle the per core case. This is handling exclusively the case of
"process trace" without the "per-cpu" option. This class effectively creates a
perf event per thread. Even its Start method asks for tids
================
Comment at: lldb/source/Plugins/Process/Linux/IntelPTCollector.h:152-156
+ IntelPTPerThreadProcessTraceUP m_per_thread_process_trace_up;
+ /// Cores traced due to per-core "process tracing". Only one active
+ /// "process tracing" instance is allowed for a single process.
+ /// This might be \b nullptr.
+ IntelPTMultiCoreTraceUP m_per_core_process_trace_up;
----------------
jj10306 wrote:
> So these are mutually exclusive tight?
yes, I'll leave a comment about that here
================
Comment at: lldb/source/Plugins/Process/Linux/IntelPTMultiCoreTrace.cpp:20-22
+ int64_t required = cores.size() * request.trace_buffer_size;
+ int64_t limit = request.process_buffer_size_limit.getValueOr(
+ std::numeric_limits<int64_t>::max());
----------------
jj10306 wrote:
> nit: why are we using signed integers here?
yes, this is bad, i'm changing to unsigned
================
Comment at: lldb/source/Plugins/Process/Linux/Perf.h:111-115
/// \param[in] pid
- /// The process to be monitored by the event.
+ /// The process or thread to be monitored by the event.
///
/// \param[in] cpu
/// The cpu to be monitored by the event.
----------------
jj10306 wrote:
> Maybe add explanation of what is passed to the syscall for pid and cpu when
> they are `None` here?
makes sense
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D124858/new/
https://reviews.llvm.org/D124858
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
