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
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
