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

[walter erquinigo via Phabricator via lldb-commits]

wallace added inline comments.


================
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;
----------------
jj10306 wrote:
> 
ahh good one


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


================
Comment at: lldb/source/Plugins/Trace/intel-pt/LibiptDecoder.cpp:206
+      }
+      m_next_infinite_decoding_loop_threshold *= 2;
+    }
----------------
jj10306 wrote:
> can you explain why we are increasing the threshold?
the idea is to check for infinite loops sporadically without making the total checks in O(N^2) and instead do it in O(N)

If we first do a linear check in the trace, which is O(T) after T instructions are appended and there are no loops, we might want to check again in the future. We could wait for the next T instructions and then run another check, and if we fail, wait for the next T and so on. This result in a total time spent of O(T + 2T + 3T + 4T + ... + N) which is O(N^2). Instead, we can run the check after 2T, and then after 4T and then after 8T and so on. This gives us a geometric progression of (N + N/2 + N / 4 + ... + T) which is amortized total O(N). A similar algorithm is vector::push_back (https://cs.stackexchange.com/questions/9380/why-is-push-back-in-c-vectors-constant-amortized) which is total O(N) using a similar approach.


================
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;
+      }
----------------
jj10306 wrote:
> 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
I like your idea. I think I can simplify the code


================
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) {
----------------
jj10306 wrote:
> help me understand this please. I thought `pt_insn_get_offset` would always return a new, increasing offset every time this function is called.
Not really. pt_insn_get_offset returns the offset of the last packet that was processed, and that single could lead to many individual sequential instructions until the next packet is needed.
 
Let's imagine that you have this trace

PSB with starting address of 0xAAA
TNT with 4 bits
TIP with address 0xFFF

What the decoder will do is to first read the PSB and start at IP 0xAAA. It'll then decode sequential instructions until it reaches the first branch or jump. It then needs to read the next packet, which is the TNT with 4 bits, so it will help decode the next 4 branches but not the fifth one. So the decoder will change the offset and resume decoding instructions sequentially until that fifth branch (or jump) is reached. Then the decoder will read the next packet, which is a TIP and tells the decoder to jump to address 0xFFF.
So this means that with the PSB, the decoder produced, let's say, 10 instructions, and with the TNT maybe 1000 were produced,, and then the decoder moved to the offset of the TIP for the next instruction.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136557