[Lldb-commits] [PATCH] D125897: [trace][intelpt] Support system-wide tracing [9] - Collect and return context switch traces

[Jakob Johnson via Phabricator via lldb-commits]

jj10306 added inline comments.


================
Comment at: lldb/source/Plugins/Process/Linux/IntelPTMultiCoreTrace.cpp:122-129
 void IntelPTMultiCoreTrace::ForEachCore(
     std::function<void(core_id_t core_id, IntelPTSingleBufferTrace &core_trace)>
         callback) {
   for (auto &it : m_traces_per_core)
-    callback(it.first, *it.second);
+    callback(it.first, it.second.first);
 }
 
----------------
Is there a way to consolidate these two methods?


================
Comment at: lldb/source/Plugins/Process/Linux/IntelPTSingleBufferTrace.h:43
+  /// \param[in] disabled
+  ///     Whether to start the tracing paused.
   ///
----------------
This wording is a bit confusing - maybe provide some additional context here about this flag controlling the whether the perf event is enabled/disabled at perf_event_open time


================
Comment at: lldb/source/Plugins/Process/Linux/Perf.cpp:161
   if (Expected<resource_handle::MmapUP> mmap_metadata_data =
-          DoMmap(nullptr, mmap_size, PROT_WRITE, MAP_SHARED, 0,
+          DoMmap(nullptr, mmap_size, PROT_READ | PROT_WRITE, MAP_SHARED, 0,
                  "metadata and data buffer")) {
----------------
curious what adding the PROT_READ does here, given that this code was working with just PROT_WRITE it seems like PROT_WRITE implies PROT_READ


================
Comment at: lldb/source/Plugins/Process/Linux/Perf.cpp:226
+Expected<std::vector<uint8_t>>
+PerfEvent::ReadFlushedOutAuxCyclicBuffer(size_t offset, size_t size) {
+  CollectionState previous_state = m_collection_state;
----------------
It's worth noting that this function should only be called when the aux buffer is configured as a ring buffer (ie the aux buffer was mmaped with PROT_READ). if the aux buffer mmaped with PROT_WRITE then the behavior of the head is different and the consumer is required to update the aux_tail when reading.
Perhaps we should check this before doing the reading or make it very clear in the docs that this should only be called on perf events with that specific configuration


================
Comment at: lldb/source/Plugins/Process/Linux/Perf.h:101
 class PerfEvent {
+  enum class CollectionState {
+    Enabled,
----------------
is an enum needed for this? Since the perf event will only ever be in two states, feels like a bool would suffice


================
Comment at: lldb/source/Plugins/Process/Linux/Perf.h:238
+  llvm::Expected<std::vector<uint8_t>>
+  ReadFlushedOutAuxCyclicBuffer(size_t offset, size_t size);
+
----------------
nit: This is wordy. imo the name needs not mention that it's flushed out, just put that in the docs


================
Comment at: lldb/test/API/commands/trace/multiple-threads/TestTraceStartStopMultipleThreads.py:226-229
+            self.assertTrue(context_switch_size is not None)
+            self.assertTrue(trace_buffer_size is not None)
+            if context_switch_size > 0:
+                found_non_empty_context_switch = True
----------------
nice test!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125897