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 _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
