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

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

I'm not sure if I'll have time to go through this again, but it seems ok. One question inline, though.

In D89283#2348815 <https://reviews.llvm.org/D89283#2348815>, @wallace wrote:

> The diff is the now ready for review. There are a few updates, including some design decisions after some chats with Greg.
>
> - Now the dump command includes disassembly information and symbol context information whenever relevant, e.g.

I should've said something earlier, but I think the decision to the actual disassembling in a separate patch was a good one. This patch was big enough as it was.

> - I tried to use yaml to represent the binary fails, but it doesn't work. The yaml representation doesn't have all the required information and the decoder complaints about missing memory. At this point it's better to just include the binary and have strong tests.

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...



================
Comment at: lldb/packages/Python/lldbsuite/test/lldbtest.py:2111
         # Fail fast if 'cmd' is not meaningful.
-        if not cmd or len(cmd) == 0:
+        if not allowEmptyCmd and (not cmd or len(cmd) == 0):
             raise Exception("Bad 'cmd' parameter encountered")
----------------
I think that we could just drop the len(cmd)==0 check. It's pretty unlikely that anyone will do that by accident (and have all of his checks will still pass)...


================
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;
+}
----------------
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.)


================
Comment at: lldb/source/Plugins/Trace/intel-pt/TraceIntelPT.cpp:109
+  else {
+    consumeError(decoded_thread.takeError());
+    return 0;
----------------
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...


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

https://reviews.llvm.org/D89283