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

[walter erquinigo via Phabricator via lldb-commits]

wallace added a comment.

> Do we need to know the instruction count? Again, won't this be really expensive to calculate?

It is expensive, but today I realized that it might be necessary to decode the entire trace if you want things to work nicely.

Let's take, for example, a very common future use case: "you stop at a breakpoint, and then you do reverse-next and print the backtrace". Getting the backtrace requires decoding all the instructions, as potentially the first instruction in the trace corresponds to the oldest frame of the backtrace of your breakpoint position (which would be the case if you start tracing at main). You won't know all the frames of your backtrace unless you decode it all. In general, you can't have the backtrace of the i-th position unless you decode everything up to that position. And without the backtrace you really don't know where you are, it's hard for the user to make sense of the state of the program without it.

Also, if would be difficult to provide comprehensive position offsets to the user in the "dump instructions" command unless we also show the maximum index, which is equivalent to the instruction count.

> Lets not do this extra indirection, I like the current API with or without GetInstructionsCount

That's reassuring. I'm also of that opinion.

> If an invalid thread is passed, then TraverseInstructions can just return an error on the first callback. If we still need GetInstructionCount(), we can have it return 0 if the thread has no trace instructions.

Sure

> We should be able to implement these in Trace.cpp and have it only use TraverseInstructions right? Do we also need ReverseContinue() to be able to backup until we hit a breakpoint or the start of the trace data?

Yes, definitely. We'll be adding more stuff to it like ReverseContinue.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89283