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

[Greg Clayton via Phabricator via llvm-commits]

clayborg added a comment.

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

> Addressed almost all changes.
>
> What's left to discuss:
>
> - I tried to use obj2yaml, but it creates files much larger than the binaries, so in terms of space, I'd rather keep the binaries. Besides, I removed ld.so, and the binaries that are left are tiny. I imagine that the binaries that will be committed in the future will also be tiny ones depicting specific edge cases.
> - Showing the disassembly of the instructions, requested by Greg, is not yet done.
> - I'm representing now all decoding errors as instructions, which in fact simplifies the API a good deal. However, I'm still not happy with the API, right now we have
>
>   class Trace { void TraverseInstructions(Thread, callback, ...); int GetInstructionsCount(Thread...); }

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

> Right now these methods are not doing anything (or returning 0 in the case of the instructions count) if the provided Thread is not traced by the Trace instance.

If we get rid of

  int Trace::GetInstructionsCount(Thread...);

Then the first call to TraverseInstructions can return an appropriate error to the callback in the Expected<addr_t> right?

> Besides, eventually we'll add methods like
>
>   vector<addr_t> GetBacktrace(Thread, position)

We might want the thread to be able to produce real stack frames so we can re-use the current StackFrame classes. Each stack frame will probably only contain the PC. This call could be used to create the StackFrames in the trace thread class.

>   position ReverseNext(Thread, position)
>   position ReverseSingleStep(Thread, position)

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?

> We could leave the code as it is, which results in a simple API with some undefined behavior if an invalid Thread is passed.

I like the current API. Great if we can remove the Trace::GetInstructionsCount() API to keep things as simple as possible and allow us to lazily fetch instructions as needed as we eventually step and continue around.

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.

> Another option is to have
>
>   class Trace {
>     TracedThreadSP GetTraceForThread(Thread ...); 
>   }
>   
>   class TracedThread {
>     void TraverseInstructions(callback, ...);
>     int GetInstructionsCount()
>    
>     position ReverseNext(Thread, position)
>     position ReverseSingleStep(Thread, position)
>   }
>
> This creates a level of indirection that forces the caller to check if the returned TracedTrace is a valid pointer or not. If the pointer is invalid, the caller can show an error message or fail silently, otherwise, TraverseInstructions and GetInstructionsCount will perform valid operations.

I don't think we need this. I think we have enough API here to implement all of this stuff through the lldb_private::Thread class as it can call through to the Trace APIs with the thread class pointer.

> This might become useful to avoid errors in complex scenarios as the API grows. But then, how often will someone invoke these methods with the wrong thread? And this TracedThread object would have to be passed around a lot if the developer doesn't want to if the thread is invalid in each callsite.
>
> The first form of the API looks simpler without that level of indirection, which is cool. But the second one adds some boilerplate in each callsite, with some safety benefits.
>
> What do you guys think?

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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89283