[PATCH] D89283: [trace][intel-pt] Implement the basic decoding functionality

[walter erquinigo via Phabricator via llvm-commits]

wallace marked 10 inline comments as done.
wallace added inline comments.


================
Comment at: lldb/source/Plugins/Trace/intel-pt/TraceIntelPT.cpp:109
+  else {
+    consumeError(decoded_thread.takeError());
+    return 0;
----------------
labath wrote:
> wallace wrote:
> > labath wrote:
> > > I'm having doubts about this "I have an thread but was not able to decode _anything_ about it" state is worth it. Having many different ways to report errors just increases the chance of something going wrong, and in `TraverseInstructions` you're already treating this state as a pseudo-instruction.
> > > 
> > > Maybe that representation should be used all the way down? Or (and this may be even better) we avoid creating such Threads in the first place (print an error/warning when the target is created).
> > > Maybe that representation should be used all the way down? 
> > 
> > I'll follow that path. This will create consistency through the code
> > 
> > 
> > > Or (and this may be even better) we avoid creating such Threads in the first place (print an error/warning when the target is created).
> >  
> > I wish I could do that, but decoding is very expensive and should be done lazily. According to Intel and my tests, if a thread was traced during T seconds, then decoding takes around 10T, which is a big amount of time if you were tracing 10 threads for 5 seconds, which would take 500 seconds to decode. At least for know we are not doing parallel decoding. I imagine at some point we'll have to work on that.
> If you can pull that off -- great. However, I have doubts (and judging by the other comments, you're starting to have some too) regarding how long will you be able to postpone the decoding. For example, lldb very much likes to know the PC value of each thread (can you really blame it?) -- so much that we've added a special field to the gdb-remote stop-reply packet which lists the pc values of all threads.
> 
> That leads me to believe that you'll need to decode at least the last block of each thread's trace very early on
Yes, I ended up understanding more of LLDB and it seems that it'll be as you describe. I'll try to limit the initial decoding to at most the PC of each thread, which would indeed be very beneficial, because we could catch early some critical errors. I'll do that early decoding in another diff.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89283