[Lldb-commits] [PATCH] D136557: [trace][intel pt] Simple detection of infinite decoding loops

[Jakob Johnson via Phabricator via lldb-commits]

jj10306 requested changes to this revision.
jj10306 added a comment.
This revision now requires changes to proceed.

looks good overall, mainly some questions and a few nits



================
Comment at: lldb/source/Plugins/Trace/intel-pt/DecodedThread.h:123
     /// of a DenseMap because DenseMap can't understand enums.
-    std::unordered_map<lldb::TraceEvent, size_t> events_counts;
-    size_t total_count = 0;
+    std::unordered_map<lldb::TraceEvent, uint64_t> events_counts;
+    uint64_t total_count = 0;
----------------



================
Comment at: lldb/source/Plugins/Trace/intel-pt/DecodedThread.h:130
+  // Struct holding counts for errors
+  struct ErrorStats {
+    /// The following counters are mutually exclusive
----------------
nice, I was about to add this as part of my diff (:


================
Comment at: lldb/source/Plugins/Trace/intel-pt/LibiptDecoder.cpp:206
+      }
+      m_next_infinite_decoding_loop_threshold *= 2;
+    }
----------------
can you explain why we are increasing the threshold?


================
Comment at: lldb/source/Plugins/Trace/intel-pt/LibiptDecoder.cpp:235-241
+    if (m_decoded_thread.GetInstructionLoadAddress(last_insn_index) !=
+        eTraceItemKindInstruction) {
+      if (Optional<uint64_t> prev_index = prev_insn_index(last_insn_index)) {
+        last_insn_index = *prev_index;
+      } else {
+        return None;
+      }
----------------
if you move the `--item_index` in `prev_insn_index` lambda, would that allow you to remove this duplicated `eTraceItemKindInstruction` check and instead unconditionally call `prev_insn_index`?
or would this not work because the intention of the lamda is to skip the current event even if it's already an instruction


================
Comment at: lldb/source/Plugins/Trace/intel-pt/LibiptDecoder.cpp:281
+    lldb::addr_t new_packet_offset;
+    if (!IsLibiptError(pt_insn_get_offset(&m_decoder, &new_packet_offset)) &&
+        new_packet_offset != m_last_packet_offset) {
----------------
help me understand this please. I thought `pt_insn_get_offset` would always return a new, increasing offset every time this function is called.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136557