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

[walter erquinigo via Phabricator via lldb-commits]

wallace marked 7 inline comments as done.
wallace added a comment.

> That makes me sad, but I am not going to hold this patch over that. I would encourage you to find and implement the missing bits in yaml2obj though...

I'll do that later as a way to learn yaml2obj.



================
Comment at: lldb/source/Plugins/Trace/intel-pt/DecodedThread.cpp:53-56
+size_t DecodedThread::SetPosition(size_t new_position) {
+  m_position = std::min(new_position, GetLastPosition());
+  return m_position;
+}
----------------
labath wrote:
> I don't see this used anywhere. And if it's not used, how is the dump command implemented?
> 
> (I'm guessing this is used to implement per-thread "last dumped instruction" positions. I'm not sure if that feature is worth it (the `list` command doesn't have that), but if that's the case maybe the name should also be more specific, as that position is unlikely to be useful for anything else.)
I'm not using it in this patch, but I added it along with the GetCurrentPosition. I plan to use it for the reverse debugging case, in which a reverse-next would move this value to a different position, that would get picked by any subsequent dump or reverse command. I'll improve the documentation and the function name


================
Comment at: lldb/source/Plugins/Trace/intel-pt/TraceIntelPT.cpp:109
+  else {
+    consumeError(decoded_thread.takeError());
+    return 0;
----------------
labath wrote:
> wallace wrote:
> > 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.
> Sounds good.
> 
> I am slightly worried about the emphasis on sequential instruction numbers in this design. It seems like it'd be hard to avoid decoding the entire trace if one needs to assign a sequential id to each instruction. But let's see how it goes...
I think that the actual problem is we can avoid decoding the entire trace in the first place. This will be unavoidable if we want to show backtraces, as the frames are scattered throughout the trace and there's no way to know where they are unless you decode it all. I don't know of any other efficient tracing mechanism that doesn't have this problem. When I implement the backtrace reconstruction we can have a much better picture of what's possible and what not.


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

https://reviews.llvm.org/D89283