[Lldb-commits] [PATCH] D126394: [trace][intelpt] Support system-wide tracing [14] - Decode per cpu

[Jakob Johnson via Phabricator via lldb-commits]

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