[Lldb-commits] [PATCH] D127752: [trace][intelpt] Support system-wide tracing [18] - some more 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, just a couple minor things.



================
Comment at: lldb/source/Plugins/Process/Linux/Perf.cpp:205-217
 
   if (data_head > data_size) {
     uint64_t actual_data_head = data_head % data_size;
     // The buffer has wrapped
-    for (uint64_t i = actual_data_head + offset;
-         i < data_size && output.size() < size; i++)
+    for (uint64_t i = actual_data_head; i < data_size; i++)
       output.push_back(data[i]);
 
----------------
can't this now be simplified to something like:
```
data_head %= data_size;
// Copy from the head to the end of the buffer.
output.insert(output.end(), data_head, data_size);
// Copy from the start of the buffer to the head.
output.insert(data.end(), data_start, data_head);
```
you will need to make the args to insert ptrs but the rough idea should be clear that you can unconditionally modulo the head no need to manually iterate any longer, just let insert handle that for you. you might need a check to see if the buffer is empty as well.


================
Comment at: lldb/source/Plugins/Process/Linux/Perf.cpp:257-261
+  for (uint64_t i = aux_head; i < aux_size; i++)
     output.push_back(data[i]);
 
-  // We need to find the starting position for the left part if the offset was
-  // too big
-  uint64_t left_part_start =
-      aux_head + offset >= aux_size ? aux_head + offset - aux_size : 0;
-  for (uint64_t i = left_part_start; i < aux_head && output.size() < size; i++)
+  for (uint64_t i = 0; i < aux_head; i++)
     output.push_back(data[i]);
----------------
same comment as above


================
Comment at: lldb/source/Plugins/Process/Linux/Perf.h:203-209
+  /// Read the aux buffer managed by this perf event assuming it was configured
+  /// with PROT_READ permissions only, which indicates that the buffer is
+  /// automatically wrapped and overwritten by the kernel or hardware. To ensure
+  /// that the data is up-to-date and is not corrupted by read-write race
+  /// conditions, the underlying perf_event is paused during read, and later
+  /// it's returned to its initial state. The returned data will be linear, i.e.
+  /// it will fix the circular wrapping the might exist in the buffer.
----------------
this is great, thanks for adding this


================
Comment at: lldb/source/Target/Trace.cpp:45
 
+/// Helper functions for fetching data in maps and returning Optionals or
+/// pointers instead of iterators for simplicity. It's worth mentioning that the
----------------
nit: maybe put these in a separate namespace, up to you though. 


================
Comment at: lldb/source/Target/Trace.cpp:67-75
+static Optional<V> Lookup2(DenseMap<K1, DenseMap<K2, V>> &map, K1 k1, K2 k2) {
+  auto it = map.find(k1);
+  if (it == map.end())
+    return None;
+  return Lookup(it->second, k2);
+}
+
----------------
nit: potentially better names on these?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D127752