[Lldb-commits] [PATCH] D123106: [trace][intel pt] Create a class for the libipt decoder wrapper

[Jakob Johnson via Phabricator via lldb-commits]

jj10306 added a comment.

Overall looks good, just a couple cosmetic things and comments about the plan for multi-buffer, single thread decoding



================
Comment at: lldb/source/Plugins/Trace/intel-pt/LibiptDecoder.cpp:41
+
+/// Class that decodes a raw buffer for a single thread using the low level
+/// libipt library.
----------------
"for a single thread"
thinking ahead for the multi-CPU buffer case - this class will be responsible for decoding a single trace buffer, not necessarily a single thread's buffer, right? Also, come multi-buffer time, the notion of `DecodeThread` in this class will need to be changed to `DecodedBuffer` or something similar that decouples the decoder from a particular thread.

No need to change this now but wanted to make sure I'm understanding the plan correctly.



================
Comment at: lldb/source/Plugins/Trace/intel-pt/LibiptDecoder.cpp:103
+  ///   The libipt decoder status after moving to the next PSB. Negative if
+  ///   not PSB was found.
+  int FindNextSynchronizationPoint() {
----------------



================
Comment at: lldb/source/Plugins/Trace/intel-pt/LibiptDecoder.cpp:112
+
+    if (status != -pte_eos && IsLibiptError(status)) {
+      uint64_t decoder_offset = 0;
----------------



================
Comment at: lldb/source/Plugins/Trace/intel-pt/LibiptDecoder.cpp:152
+      status = pt_insn_event(&m_decoder, &event, sizeof(event));
+      if (status < 0)
+        break;
----------------



================
Comment at: lldb/source/Plugins/Trace/intel-pt/LibiptDecoder.cpp:253
+using PtInsnDecoderUP =
+    std::unique_ptr<pt_insn_decoder, decltype(DecoderDeleter)>;
+
----------------
nice custom deleter (:


================
Comment at: lldb/source/Plugins/Trace/intel-pt/LibiptDecoder.h:23
+/// library underneath.
+void DecodeInMemoryTrace(DecodedThread &decoded_thread,
+                         TraceIntelPT &trace_intel_pt,
----------------
If this file is just a wrapper around Libipt and doesn’t have any context about how LLDB gets a trace (from the aux buffer or a file), should we rename it to ‘DecodeTrace’ and the caller is responsible for providing the trace data in a byte buffer?
In the case of in memory trace, the caller already has the byte buffer. In the case of a file, they could mmap the file and cast it to the appropriate type?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123106