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

[walter erquinigo via Phabricator via lldb-commits]

wallace added a comment.

thanks for the gotchas



================
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.
----------------
jj10306 wrote:
> "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.
> 
> 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.

Not necessarily. A possibility is that the constructor doesn't accept a DecodedThread. Instead, we could create a new method `DecodeUntilThreadIsScheduledOut(decoded_thread, end_tsc_for_this_thread)`. If we have something like that, we ask this decoder to put all the instructions into the given DecodedThread until a TSC mark is reached, which signals the end of a continuous execution of this thread.


================
Comment at: lldb/source/Plugins/Trace/intel-pt/LibiptDecoder.h:23
+/// library underneath.
+void DecodeInMemoryTrace(DecodedThread &decoded_thread,
+                         TraceIntelPT &trace_intel_pt,
----------------
jj10306 wrote:
> 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?
DecodeTrace is definitely a better name because now we are not showing anymore whether the data comes from a file or just from memory


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123106