jj10306 requested changes to this revision.
jj10306 added inline comments.
This revision now requires changes to proceed.
================
Comment at: lldb/include/lldb/Utility/TraceIntelPTGDBRemotePackets.h:83-85
+ uint64_t ToNanos(uint64_t tsc) const;
+ uint64_t ToTSC(uint64_t nanos) const;
----------------
why get rid of chronos here?
================
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; }
};
----------------
nit: Is there a reason these classes/structs are not declared in the header
file?
================
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.
----------------
is this what you intended to say here?
================
Comment at: lldb/source/Plugins/Trace/intel-pt/LibiptDecoder.cpp:353
+ auto variant = execution.thread_execution.variant;
+ // If we haven't seen a PSB yet, then it's fine not to show errors
+ if (has_seen_psbs) {
----------------
Why is this the case?
================
Comment at: lldb/source/Plugins/Trace/intel-pt/LibiptDecoder.h:21-24
+struct IntelPTThreadSubtrace {
+ uint64_t tsc;
+ uint64_t psb_offset;
+};
----------------
docs
================
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,
----------------
docs
================
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;
----------------
why not put these declarations in the header file?
================
Comment at: lldb/source/Plugins/Trace/intel-pt/PerfContextSwitchDecoder.cpp:227
+
+#include <fstream>
+
----------------
what's this doing here?
================
Comment at: lldb/source/Plugins/Trace/intel-pt/PerfContextSwitchDecoder.cpp:239
+
+ auto do_decode = [&]() -> Error {
+ Optional<ContextSwitchRecord> prev_record;
----------------
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
================
Comment at: lldb/source/Plugins/Trace/intel-pt/PerfContextSwitchDecoder.h:1
+//===-- PerfContextSwitchDecoder.h --======----------------------*- C++
-*-===//
+//
----------------
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?
================
Comment at: lldb/source/Plugins/Trace/intel-pt/PerfContextSwitchDecoder.h:124
+
+/// Decodes a context switch trace gotten with perf_event_open.
+///
----------------
================
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());
----------------
================
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;
+
----------------
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.
================
Comment at:
lldb/source/Plugins/Trace/intel-pt/TraceIntelPTMultiCoreDecoder.cpp:84
+ auto on_new_thread_execution =
+ [&](ThreadContinuousExecution thread_execution) {
+ IntelPTThreadContinousExecution execution(thread_execution);
----------------
================
Comment at:
lldb/source/Plugins/Trace/intel-pt/TraceIntelPTMultiCoreDecoder.cpp:93
+ } else {
+ m_unattributed_intelpt_subtraces++;
+ }
----------------
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**?
================
Comment at:
lldb/source/Plugins/Trace/intel-pt/TraceIntelPTMultiCoreDecoder.cpp:121
Error TraceIntelPTMultiCoreDecoder::DecodeContextSwitchTraces() {
if (m_setup_error)
----------------
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)?
================
Comment at: lldb/source/Target/Trace.cpp:415
+llvm::Error
+Trace::OnCoresBinaryDataRead(const std::set<lldb::core_id_t> core_ids,
+ llvm::StringRef kind,
----------------
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D126394/new/
https://reviews.llvm.org/D126394
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
