[Lldb-commits] [PATCH] D127456: [trace][intelpt] Support system-wide tracing [17] - Some improvements

[Jakob Johnson via Phabricator via lldb-commits]

jj10306 requested changes to this revision.
jj10306 added a comment.
This revision now requires changes to proceed.
Herald added a subscriber: Michael137.

Looks great overall, thanks for making these improvements - just a couple minor things



================
Comment at: lldb/include/lldb/Target/Trace.h:524
+  /// process data refreshers.
+  struct Storage {
+    /// Portmortem processes traced by this object if doing non-live tracing.
----------------
nice


================
Comment at: lldb/source/Plugins/Process/Linux/Perf.cpp:188
+
+  bool was_enabled = m_enabled;
   if (Error err = DisableWithIoctl())
----------------
why this intermediate `was_enabled`  variable?


================
Comment at: lldb/source/Plugins/Process/Linux/Perf.cpp:248
+
+  bool was_enabled = m_enabled;
   if (Error err = DisableWithIoctl())
----------------
same question as above


================
Comment at: lldb/source/Plugins/Process/Linux/Perf.cpp:347-349
+  attr.exclude_kernel = 1;
+  attr.sample_id_all = 1;
+  attr.exclude_hv = 1;
----------------
it may not make a difference since this is a context switch event, but should these values be set based on the parent's perf event?


================
Comment at: lldb/source/Plugins/Trace/intel-pt/TraceIntelPT.cpp:319
+
+TraceIntelPT::Storage &TraceIntelPT::GetUpdatedStorage() {
   RefreshLiveProcessState();
----------------
this is much cleaner now (:


================
Comment at: lldb/source/Plugins/Trace/intel-pt/TraceIntelPT.h:205
+    /// These decoders are used for the non-per-core case
+    std::map<lldb::tid_t, std::unique_ptr<ThreadDecoder>> thread_decoders;
+    /// Helper variable used to track long running operations for telemetry.
----------------
use llvm::DenseMap instead?


================
Comment at: lldb/source/Plugins/Trace/intel-pt/TraceIntelPTJSONStructs.h:33-38
+  uint64_t tid;
   llvm::Optional<std::string> trace_buffer;
 };
 
 struct JSONProcess {
+  uint64_t pid;
----------------
pid is really a int32_t in the kernel. should these be int32_t or pid_t/tid_t instead?


================
Comment at: lldb/source/Plugins/Trace/intel-pt/TraceIntelPTMultiCoreDecoder.h:65
 
-  TraceIntelPT &m_trace;
-  std::set<lldb::core_id_t> m_cores;
+  TraceIntelPT *m_trace;
   std::set<lldb::tid_t> m_tids;
----------------
why are we storing a pointer now if the constructor is still passing in a ref?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D127456/new/

https://reviews.llvm.org/D127456