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

[walter erquinigo via Phabricator via lldb-commits]

wallace added inline comments.


================
Comment at: lldb/source/Plugins/Process/Linux/Perf.cpp:188
+
+  bool was_enabled = m_enabled;
   if (Error err = DisableWithIoctl())
----------------
jj10306 wrote:
> why this intermediate `was_enabled`  variable?
it stores the original state of the perf event, so that after pausing it and reading, the perf event is returned to that state. We need to pause the perf event before reading so that the CPU flushed out its internal buffer. We also want to prevent the cpu from writing to the buffer while we read, which would give us wrong data


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


================
Comment at: lldb/source/Plugins/Process/Linux/Perf.cpp:347-349
+  attr.exclude_kernel = 1;
+  attr.sample_id_all = 1;
+  attr.exclude_hv = 1;
----------------
jj10306 wrote:
> 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?
not necessarily. Maybe you want context switches that include kernel but you want your intel pt trace to be user only. That could give you an interesting visualization


================
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.
----------------
jj10306 wrote:
> use llvm::DenseMap instead?
I've fixed that in a later diff, but good catch!


================
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;
----------------
jj10306 wrote:
> pid is really a int32_t in the kernel. should these be int32_t or pid_t/tid_t instead?
On OSX it's 64 bit IIRC, so I'm just adhering to that extreme case. lldb::pid_t and lldb::tid_t are 64 bits to be able to support all systems


================
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;
----------------
jj10306 wrote:
> why are we storing a pointer now if the constructor is still passing in a ref?
We have two options: store a pointer or a a weak pointer. We can't use a reference variable because the new version of the code requires the multicore decoder to have a copy constructor (Optional is the one asking for that).
So I'm changing this now to use weak_pointers to make it more c++ idiomatic.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D127456