[Lldb-commits] [PATCH] D126394: [trace][intelpt] Support system-wide tracing [14] - Decode per cpu
walter erquinigo via Phabricator via lldb-commits
lldb-commits at lists.llvm.org
Tue Jun 14 15:26:14 PDT 2022
wallace added inline comments.
Herald added a subscriber: Michael137.
================
Comment at: lldb/include/lldb/Utility/TraceIntelPTGDBRemotePackets.h:83-85
+ uint64_t ToNanos(uint64_t tsc) const;
+ uint64_t ToTSC(uint64_t nanos) const;
----------------
jj10306 wrote:
> why get rid of chronos here?
because chronos uses signed integers and we lose precision by using it
================
Comment at: lldb/source/Plugins/Trace/intel-pt/LibiptDecoder.cpp:22-38
struct TscInfo {
uint64_t tsc = 0;
LazyBool has_tsc = eLazyBoolCalculate;
explicit operator bool() const { return has_tsc == eLazyBoolYes; }
};
----------------
jj10306 wrote:
> nit: Is there a reason these classes/structs are not declared in the header file?
they are not used by anyone else, so just that
================
Comment at: lldb/source/Plugins/Trace/intel-pt/LibiptDecoder.cpp:100-102
+ /// If \b true, decoding
+ /// An optional offset to a given PSB. Decoding stops if a different PSB is
+ /// reached.
----------------
jj10306 wrote:
> is this what you intended to say here?
hehe yes
================
Comment at: lldb/source/Plugins/Trace/intel-pt/LibiptDecoder.h:21-24
+struct IntelPTThreadSubtrace {
+ uint64_t tsc;
+ uint64_t psb_offset;
+};
----------------
jj10306 wrote:
> docs
+1
================
Comment at: lldb/source/Plugins/Trace/intel-pt/LibiptDecoder.h:47-53
+void DecodeTrace(
+ DecodedThread &decoded_thread, TraceIntelPT &trace_intel_pt,
+ const llvm::DenseMap<lldb::core_id_t, llvm::ArrayRef<uint8_t>> &buffers,
+ const std::vector<IntelPTThreadContinousExecution> &executions);
+
+llvm::Expected<std::vector<IntelPTThreadSubtrace>>
+SplitTraceInContinuousExecutions(TraceIntelPT &trace_intel_pt,
----------------
jj10306 wrote:
> docs
+1
================
Comment at: lldb/source/Plugins/Trace/intel-pt/PerfContextSwitchDecoder.cpp:15-81
+/// Copied from <linux/perf_event.h> to avoid depending on perf_event.h on
+/// non-linux platforms.
+/// \{
+struct perf_event_header {
+ uint32_t type;
+ uint16_t misc;
+ uint16_t size;
----------------
jj10306 wrote:
> why not put these declarations in the header file?
they are not used externally, so I rather not expose them until we really need to
================
Comment at: lldb/source/Plugins/Trace/intel-pt/PerfContextSwitchDecoder.cpp:227
+
+#include <fstream>
+
----------------
jj10306 wrote:
> what's this doing here?
lol, forgive this poor man
================
Comment at: lldb/source/Plugins/Trace/intel-pt/PerfContextSwitchDecoder.cpp:239
+
+ auto do_decode = [&]() -> Error {
+ Optional<ContextSwitchRecord> prev_record;
----------------
jj10306 wrote:
> does this need to be a lambda? iiuc this is only called once (at the end of this function), so it seems like this could just be placed inline instead of being in a lambda
I'm using this lambda trick to capture any possible error returned by the complex logic in it and then append some text to this error. This would be equivalent to moving the lambda to its own function. I'm fine with creating another function for this case, but the lambda seems fine to me
================
Comment at: lldb/source/Plugins/Trace/intel-pt/PerfContextSwitchDecoder.h:1
+//===-- PerfContextSwitchDecoder.h --======----------------------*- C++ -*-===//
+//
----------------
jj10306 wrote:
> do the structures/logic contained in this file (and its .cpp) belong with the other Perf logic of LLDB or should does this belong with the intelpt specific code?
the other Perf.h file is lldb-server only, so they can't be together nor merged :(
================
Comment at: lldb/source/Plugins/Trace/intel-pt/PerfContextSwitchDecoder.h:124
+
+/// Decodes a context switch trace gotten with perf_event_open.
+///
----------------
jj10306 wrote:
>
+1
================
Comment at: lldb/source/Plugins/Trace/intel-pt/TraceIntelPTMultiCoreDecoder.cpp:43
+ m_cores, IntelPTDataKinds::kTraceBuffer,
+ [&](const DenseMap<core_id_t, ArrayRef<uint8_t>> buffers) -> Error {
+ auto it = m_continuous_executions_per_thread->find(thread.GetID());
----------------
jj10306 wrote:
>
owch, thank you
================
Comment at: lldb/source/Plugins/Trace/intel-pt/TraceIntelPTMultiCoreDecoder.cpp:64
+ for (core_id_t core_id : m_cores) {
+ std::vector<IntelPTThreadSubtrace> intel_pt_executions;
+
----------------
jj10306 wrote:
> I think naming this subtraces would help as it makes it makes the distinction between the subtraces and thread executions more clear while reading the code below.
+1
================
Comment at: lldb/source/Plugins/Trace/intel-pt/TraceIntelPTMultiCoreDecoder.cpp:84
+ auto on_new_thread_execution =
+ [&](ThreadContinuousExecution thread_execution) {
+ IntelPTThreadContinousExecution execution(thread_execution);
----------------
jj10306 wrote:
>
thanks
================
Comment at: lldb/source/Plugins/Trace/intel-pt/TraceIntelPTMultiCoreDecoder.cpp:93
+ } else {
+ m_unattributed_intelpt_subtraces++;
+ }
----------------
jj10306 wrote:
> perhaps I'm misunderstanding the intention of this counter, but won't this be incremented for all subtraces that don't belong to **the specific ThreadContinousExecution currently being operated** on instead of being incremented for the subtraces that don't belong to **any ThreadContinousExecution**?
not really. In this case it is any ThreadContinousExecution. You need to notice a few things:
- the intel pt subtraces (or executions), are sorted by time
- on_new_thread_execution is invoked sorted by time
when we process a new thread execution, we look for the intel pt subtraces that we haven't processed yet that happened no later than our current execution. If we find a subtrace that happened before the current execution, then that means that we don't have any thread execution for this subtrace, regardless of which thread it was
================
Comment at: lldb/source/Plugins/Trace/intel-pt/TraceIntelPTMultiCoreDecoder.cpp:121
Error TraceIntelPTMultiCoreDecoder::DecodeContextSwitchTraces() {
if (m_setup_error)
----------------
jj10306 wrote:
> Should this be named differently since this is doing much more than decoding the context switches (it splits the intel pt traces and correlates them with the context switch executions)?
+1
================
Comment at: lldb/source/Target/Trace.cpp:415
+llvm::Error
+Trace::OnCoresBinaryDataRead(const std::set<lldb::core_id_t> core_ids,
+ llvm::StringRef kind,
----------------
jj10306 wrote:
>
i've gotten rid of the set
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D126394/new/
https://reviews.llvm.org/D126394
More information about the lldb-commits
mailing list