[trace] Create a top level ThreadTrace class

[wallace]

Summary

We want to start doing some analisis at instruction level, including stack trace reconstruction, trace summarization and security analysis. For this we need to provide a basic and efficient way to access instruction information without the symbolication bits for quick analysis. This class should be at Trace.h level abstracting plug-in dependent information.

[clayborg]

So it would be nice to try and not encode errors into the TraceInstruction class and deal with any errors at decode time.

lldb/include/lldb/Target/Trace.h

32–33

Can't we just avoid including any errors in this TraceInstruction class? And have the function that generates these return a:

llvm::Expected<TraceInstruction>

?

76

It would be nice to avoid trying to encode errors into this class.

lldb/include/lldb/lldb-enumerations.h

798

[wallace]

lldb/include/lldb/Target/Trace.h

32–33

not really, because these errors don't mean that the entire decoding failed, they instead mean that some specific chunks of the trace were not able to be decoded.
For example, you can have this decoded trace:

a.out`(none)
    [ 4] 0x0000000000400510    pushq  0x200af2(%rip)            ; _GLOBAL_OFFSET_TABLE_ + 8
    [ 5] 0x0000000000400516    jmpq   *0x200af4(%rip)           ; _GLOBAL_OFFSET_TABLE_ + 16
    [ 6] 0x00007ffff7df1950    error: no memory mapped at this address
    ...missing instructions
  a.out`main + 20 at main.cpp:10
    [ 7] 0x0000000000400674    movl   %eax, -0xc(%rbp)

Instruction 6 is an error. We have an associated address but the decoder couldn't decode it and then it skipped the trace until the next synchronization point, which is instruction 7 in this case.

Not all decoder errors have an associated address, though, only some. So I prefer just to have any errors be represented as a strings for now.

Another option is to create a new class that contains a sequential list of valid instructions terminated by 0 or more contiguous errors. But that seems a little bit too much, as there's not a lot that we can do when we see errors, we either ignore them or we stop the current iteration.

[wallace]

address comments

[wallace]

lldb/include/lldb/Target/Trace.h

32–33

It should be possible. I wonder about memory consumption.
llvm::Expected has these members:

union {
    AlignedCharArrayUnion<storage_type> TStorage;
    AlignedCharArrayUnion<error_type> ErrorStorage;
  };
  bool HasError : 1;
#if LLVM_ENABLE_ABI_BREAKING_CHECKS
  bool Unchecked : 1;
#endif

that means that it would end up using 3 * 8 bytes in total, right?

32–33

disregard this comment, I understood your remark incorrectly

[clayborg]

One quick either documentation update for the error string, or switch to ConstString... Probably best to the the error string docs as if we use fancy C++ union stuff it might not compile for everyone on all C++ compilers..

lldb/include/lldb/Target/Trace.h

76

I would be great if this could actually be a ConstString object. I haven't played with C++ unions with actual objects in them, but I believe it is possible. If you can't make a comment stating this string will be from a ConstString and doesn't need to be freed.

[wallace]

I tried to use a ConstString instead of a const char *, but that's not trivial because
the compilers wants a safe destructor all the way down, and it seems that this is non-standard
across compilers, except for C++17 and onwards, which has the std::variant type.

So I'm just adding a comment as suggested.

[vsk]

I'm quite concerned about the design decision to represent a trace as a vector of TraceInstruction. This bakes considerations that appear to be specific to Facebook's usage of IntelPT way too deep into lldb's APIs, and isn't workable for our use cases.

At Apple, we use lldb to navigate instruction traces that contain billions of instructions. Allocating 16 bytes per instruction simply will not scale for our workflows. We require the in-memory overhead to be approximately 1-2 bits per instruction. I'm not familiar with how large IntelPT traces can get, but presumably (for long enough traces) you will encounter the same scaling problem.

What alternatives to the vector<TraceInstruction> representation have you considered? One idea might be to implement your program analyses on top of a generic interface for navigating forward/backward through a trace and extracting info about the instruction via a set of API calls; this leaves the actual in-memory representation of "TraceInstruction" unspecified.

[wallace]

At Apple, we use lldb to navigate instruction traces that contain billions of instructions. Allocating 16 bytes per instruction simply will not scale for our workflows. We require the in-memory overhead to be approximately 1-2 bits per instruction. I'm not familiar with how large IntelPT traces can get, but presumably (for long enough traces) you will encounter the same scaling problem.

I have thought about that and I'm taking it into account. We implemented a TraverseInstructions API in Trace.h that receives a callback (see https://lldb.llvm.org/cpp_reference/classlldb__private_1_1Trace.html#a02e346b117c15cef1cb0568c343f7e1c). The idea is that the intel pt plugin can decode the instructions on the fly as part of the iteration.
In terms of memory, there are two kinds of trace objects: one is the raw undecoded instruction trace that uses 1 bit per instruction in avg. That one is decoded in https://github.com/llvm/llvm-project/blob/main/lldb/source/Plugins/Trace/intel-pt/IntelPTDecoder.cpp#L163 into TraceInstructions, which effectively use 16 bytes in the duration of their lives. Right now we haven't implemented lazy decoding; we are decoding the entire trace. But that's just because so far we have been working with small traces. As we progress in this work, we'll start working with larger traces and we'll have to do implement the lazy decoding, for which the TraverseInstructions API will come handy.

What alternatives to the vector<TraceInstruction> representation have you considered? One idea might be to implement your program analyses on top of a generic interface for navigating forward/backward through a trace and extracting info about the instruction via a set of API calls; this leaves the actual in-memory representation of "TraceInstruction" unspecified.

That's exactly what I described as the TraverseInstructions method :) The intel-pt plugin or any other trace plugin could have an internal representation for traces, but we still need a generic TraceInstruction object for consumers that want to be agnostic of the trace technology.

[vsk]

In D103588#2806512, @wallace wrote:

Right now we haven't implemented lazy decoding; we are decoding the entire trace. But that's just because so far we have been working with small traces. As we progress in this work, we'll start working with larger traces and we'll have to do implement the lazy decoding, for which the TraverseInstructions API will come handy.

This approach - of prototyping trace analyses on a vector<TraceInstruction> representation and then rewriting them later when scaling issues arise - doesn't sound good to me. Even for something simple, like finding a backtrace, the window of instructions needed for analysis can easily grow to a point where the full vector<TraceInstruction> can't be maintained in memory.

To clarify: I think that's a perfectly reasonable way to prototype trace analyses, but I don't think that kind of prototyping should be done at the SB API level, as there are extra bincompat & portability concerns here.

What alternatives to the vector<TraceInstruction> representation have you considered? One idea might be to implement your program analyses on top of a generic interface for navigating forward/backward through a trace and extracting info about the instruction via a set of API calls; this leaves the actual in-memory representation of "TraceInstruction" unspecified.

That's exactly what I described as the TraverseInstructions method :) The intel-pt plugin or any other trace plugin could have an internal representation for traces, but we still need a generic TraceInstruction object for consumers that want to be agnostic of the trace technology.

It's not at all clear to me that lldb needs to surface a generic TraceInstruction object, with metadata like m_byte_size, m_inst_type, etc. stored inline. It seems more flexible to instead vend a cursor (just an integer) that indicates a position in the trace; a separate set of APIs could answer queries given a cursor, e.g. 'getInstTypeAtCursor(u64 cursor)'.

[wallace]

This approach - of prototyping trace analyses on a vector<TraceInstruction> representation and then rewriting them later when scaling issues arise - doesn't sound good to me. Even for something simple, like finding a backtrace, the window of instructions needed for analysis can easily grow to a point where the full vector<TraceInstruction> can't be maintained in memory.

It's worth noticing that the vector<TraceInstruction> is internal to the Intel PT plugin. It's not exposed at the Trace.h interface level, so any consumers can't make any assumptions on how the data is stored. We will improve the intel pt plugin itself as we make progress on this, but we are ensuring that the interfaces are correct for the future.

To clarify: I think that's a perfectly reasonable way to prototype trace analyses, but I don't think that kind of prototyping should be done at the SB API level, as there are extra bincompat & portability concerns here.

I know, and I want to stress out that we are not exposing any of this instruction information at the SB API level and probably we won't for a while, i.e. I don't plan on creating a SBTraceInstruction class, as there's too much boilerplate needed to expose an SBInstruction object. For now, we want to have some code that processes instructions and that exists at the Trace.h level, outside of the intel-pt plugin, as we want to make it generic enough to be utilized by different tracing technologies.

It's not at all clear to me that lldb needs to surface a generic TraceInstruction object, with metadata like m_byte_size, m_inst_type, etc. stored inline. It seems more flexible to instead vend a cursor (just an integer) that indicates a position in the trace; a separate set of APIs could answer queries given a cursor, e.g. 'getInstTypeAtCursor(u64 cursor)'.

There's an advantage. Decoding intel-pt instructions is very slow and we are able to produce correct indices only if we decode the raw trace sequentially, either backwards (for negative indices) or forwards (for positive indices). If we start from the middle, we don't know which instruction we are at. If we allow the user to query properties of arbitrary indices, we might need to decode a big prefix or a big suffix of the trace until we get to that index. It seems safer to me to expose right away the properties of these instructions as soon as they are decoded, so that we don't need any further decoding if the user invokes a query method with a possibly wrong index.

[vsk]

In D103588#2806611, @wallace wrote:

This approach - of prototyping trace analyses on a vector<TraceInstruction> representation and then rewriting them later when scaling issues arise - doesn't sound good to me. Even for something simple, like finding a backtrace, the window of instructions needed for analysis can easily grow to a point where the full vector<TraceInstruction> can't be maintained in memory.

It's worth noticing that the vector<TraceInstruction> is internal to the Intel PT plugin. It's not exposed at the Trace.h interface level, so any consumers can't make any assumptions on how the data is stored. We will improve the intel pt plugin itself as we make progress on this, but we are ensuring that the interfaces are correct for the future.

This doesn't quite alleviate my concern. We'd like for the generic trace infrastructure (outside of the PT plugin) to support different tracing technologies as well. It's not clear that the TraceInstruction concept can do that.

Two questions that have remained unanswered here: (1) how would TraceInstruction be used by non-PT plugins? (2) how do we guarantee the generic analyses you're planning to write will scale?

To clarify: I think that's a perfectly reasonable way to prototype trace analyses, but I don't think that kind of prototyping should be done at the SB API level, as there are extra bincompat & portability concerns here.

I know, and I want to stress out that we are not exposing any of this instruction information at the SB API level and probably we won't for a while, i.e. I don't plan on creating a SBTraceInstruction class, as there's too much boilerplate needed to expose an SBInstruction object. For now, we want to have some code that processes instructions and that exists at the Trace.h level, outside of the intel-pt plugin, as we want to make it generic enough to be utilized by different tracing technologies.

lldb's SB interfaces have always been relatively thin wrappers around classes in include/lldb (SBTarget -> Target, SBProcess -> Process, etc.). So it's really worth spending the extra time to get the generic interfaces right, even if they're not SB API yet, since that's ultimately the direction in which we'll want to go.

It's not at all clear to me that lldb needs to surface a generic TraceInstruction object, with metadata like m_byte_size, m_inst_type, etc. stored inline. It seems more flexible to instead vend a cursor (just an integer) that indicates a position in the trace; a separate set of APIs could answer queries given a cursor, e.g. 'getInstTypeAtCursor(u64 cursor)'.

There's an advantage. Decoding intel-pt instructions is very slow and we are able to produce correct indices only if we decode the raw trace sequentially, either backwards (for negative indices) or forwards (for positive indices). If we start from the middle, we don't know which instruction we are at. If we allow the user to query properties of arbitrary indices, we might need to decode a big prefix or a big suffix of the trace until we get to that index. It seems safer to me to expose right away the properties of these instructions as soon as they are decoded, so that we don't need any further decoding if the user invokes a query method with a possibly wrong index.

I don't see how the fact that PT decoding is slow necessitates the TraceInstruction representation. I don't find that the issues you're ascribing to a cursor representation are inevitable, or all that different from the issues you'd run into with generating a large number of TraceInstructions for a subset of a trace (also not clear how this subset would be specified).

[wallace]

I've been thinking about what you said and I'm having second thoughts on my implementation. I'll share more context:

• I want to work in the short term on reverse debugging and reconstruction of stack traces, for that i'll need to know the instruction type of each instruction in the trace, which will be used as part of some heuristics to identify calls and returns between functions.
• A future application that we plan to work on is adding profiling information to the instructions
• Right now the intel-pt plugin is storing the decoded instructions in a vector, which works for small traces but wont' for gigantic traces. I imagine that I'll end up removing that vector and make the TraverseInstruction API decode instructions in place keeping one instruction in memory at a time within the intel pt plugin for a given traversal. For that I'll need accessors that can provide information of the current Instruction. As there could be two or more concurrent traversals happening at the same time (I'm trying to be generic here), it might make sense to create an abstract class TraceInstruction that can be extended by each plug-in and implement its getters.

I'm thinking about something like this

class TraceInstruction {
  virtual lldb::addr_t GetLoadAddress() = 0;
  virtual TraceInstructionType() GetInstructionType() = 0;
  virtual uint64_t GetTimestamp() = 0;
  ... anything that can appear in the future 
};

and have no members, leaving to each plug-in the decision of which of those methods to implement and how.

What do you think of this? I think this incorporates your feedback.

[vsk]

In D103588#2806894, @wallace wrote:

I've been thinking about what you said and I'm having second thoughts on my implementation. I'll share more context:

• I want to work in the short term on reverse debugging and reconstruction of stack traces, for that i'll need to know the instruction type of each instruction in the trace, which will be used as part of some heuristics to identify calls and returns between functions.
• A future application that we plan to work on is adding profiling information to the instructions
• Right now the intel-pt plugin is storing the decoded instructions in a vector, which works for small traces but wont' for gigantic traces. I imagine that I'll end up removing that vector and make the TraverseInstruction API decode instructions in place keeping one instruction in memory at a time within the intel pt plugin for a given traversal. For that I'll need accessors that can provide information of the current Instruction. As there could be two or more concurrent traversals happening at the same time (I'm trying to be generic here), it might make sense to create an abstract class TraceInstruction that can be extended by each plug-in and implement its getters.

I'm thinking about something like this

class TraceInstruction {
  virtual lldb::addr_t GetLoadAddress() = 0;
  virtual TraceInstructionType() GetInstructionType() = 0;
  virtual uint64_t GetTimestamp() = 0;
  ... anything that can appear in the future 
};

and have no members, leaving to each plug-in the decision of which of those methods to implement and how.

What do you think of this? I think this incorporates your feedback.

Thanks for the context.

I'm not convinced that retaining only 1 decoded instruction in memory at a time (under a TraverseInstructions callback) will be sufficient for the use cases you've outlined. Let's say the user sets a few breakpoints, then repeatedly does "rev-continue" followed by "bt". If lldb only holds a max of 1 decoded instruction in memory, it would need to repeatedly decode parts of the trace to evaluate those user commands, which would be slow as you pointed out earlier. OTOH there's not enough memory to retain all of the decoded instructions.

It seems like we need a way for generic trace analyses to request _batches_ of decoded trace data at a time, lazily demanding more from the trace plugin only if the analysis requires it to proceed, while freeing any stale decoded data it's not using.

Moreover, there doesn't seem to be a hard requirement that each trace plugin specialize a TraceInstruction class. For example, to support that breakpoint/rev-continue/bt workflow, you really only need a list of the places where _control flow changed_. So the generic layer could request that data from a trace plugin ('plugin->decodeControlFlowChanges(startPos, stopPos)'), then query more specific APIs that answer questions about how many control flow changes there were, and what the inst type + load address was at each control flow change point. Each plugin implementation can pick the most efficient way to implement those narrower APIs, instead of trying to cram all potentially useful information into a single TraceInst class.

The reason why I believe this to be such a crucial part of the design is because of the scaling problem I mentioned earlier. A single 1.5GHz core running at IPC=2 yields 3*10^9 instructions/second, so we hit "gigantic" all too quickly!

lldb/source/Plugins/Trace/intel-pt/IntelPTDecoder.cpp

104

Are all of these instruction types both necessary and generic? E.g. does having FarCall in addition to Call assist with backtrace reconstruction, and if so, what does FarCall mean precisely?

[wallace]

Btw, thanks for the conversation. This is being really helpful.

I'm not convinced that retaining only 1 decoded instruction in memory at a time (under a TraverseInstructions callback) will be sufficient for the use cases you've outlined. Let's say the user sets a few breakpoints, then repeatedly does "rev-continue" followed by "bt". If lldb only holds a max of 1 decoded instruction in memory, it would need to repeatedly decode parts of the trace to evaluate those user commands, which would be slow as you pointed out earlier. OTOH there's not enough memory to retain all of the decoded instructions.

It seems like we need a way for generic trace analyses to request _batches_ of decoded trace data at a time, lazily demanding more from the trace plugin only if the analysis requires it to proceed, while freeing any stale decoded data it's not using.

Moreover, there doesn't seem to be a hard requirement that each trace plugin specialize a TraceInstruction class. For example, to support that breakpoint/rev-continue/bt workflow, you really only need a list of the places where _control flow changed_. So the generic layer could request that data from a trace plugin ('plugin->decodeControlFlowChanges(startPos, stopPos)'), then query more specific APIs that answer questions about how many control flow changes there were, and what the inst type + load address was at each control flow change point. Each plugin implementation can pick the most efficient way to implement those narrower APIs, instead of trying to cram all potentially useful information into a single TraceInst class.

The reason why I believe this to be such a crucial part of the design is because of the scaling problem I mentioned earlier. A single 1.5GHz core running at IPC=2 yields 3*10^9 instructions/second, so we hit "gigantic" all too quickly!

Yes, I agree with everything you said. It will really make the implementation simpler and more generic if we have these accessors based on indices instead of a struct/class, and we let each plug-in manage its data and its cache their own way.
decodeControlFlowChanges is definitely a good idea, and other similar interfaces could be created following the same approach.

[wallace]

Following @vsk 's feedback, I made the following changes:

• Create a ThreadTrace class, that contains the trace for a specific thread and offers the TraverseInstructions method and other getters like GetInstructionType and GetInstructionSize. This effectively moves some code from the Trace.h class to this new class, which has a few advantages:
  • Trace.h now has the method GetThreadTrace(thread) which can return an Error if the thread couldn't be decoded or wasn't traced. This is a safer way to know if things are correctly set up for a given thread.
  • It's easier to expose an SBAPI for instructions and its data storage with a corresponding SBThreadTrace class. Having only SBTrace is not enough for that.
  • A ThreadTrace can potentially outlive a stop id in the case of a live process, which could be useful if the user wants to compare traces or two different points in the execution of a process.
• I'm keeping the vector<IntelPTInstruction> class for now as an internal representation, but now there's more freedom to change it because the top level API in ThreadTrace makes no assumptions on the data, except that it's indexed.
• I simplified the TraceInstructionType enum. It was more complex than needed.
• I removed some deprecated code and improved some comments.

As a side note, I was chatting with some teammates and we'll explore storing decoded intel pt traces on disk, which later lldb will mmap and use as its storage. The accessors exposed in this diff are generic enough to handle that case as well.

[wallace]

nits

[vsk]

I think this is moving in the right direction! However, the way ThreadTrace is specified still raises the same scalability concerns (see inline). The "instruction index" concept doesn't seem sufficient. Decoding needs to be done in two stages: a separate "trace index" concept would help with that. In the first stage, lldb identifies which subset of the trace it needs decoded by specifying start/stop trace indices. This should be fast, as counting the number of available trace bytes/"chunks" is O(1). Nothing needs to be decoded at this point. The second stage starts after a trace subset is decoded, and is where instruction indices are useful.

(I'll be OOO next week, but hope to pick up review on 6/21.)

lldb/include/lldb/Target/ThreadTrace.h

68

GetInstructionCount requires decoding the whole ThreadTrace (if not for PT, then for other tracing technologies). I don't think we can take the O(n) hit up front.

77

How does a a ThreadTrace client query what the valid range for indices is?

lldb/include/lldb/lldb-enumerations.h

791

Do we have an example of a generic trace analysis that needs to differentiate between conditional & unconditional branches?

[clayborg]

I mostly commented on ThreadTrace.h and we should resolve the questions in there before reviewing the rest of this patch.

lldb/include/lldb/Target/ThreadTrace.h

44–45

Should zero mean oldest when using TraceDirection::Forwards and newest when using TraceDirection::Backwards? Otherwise how would a person easily go backwards from the end of the thread trace?

61

Maybe name this just "Traverse" and add an additional parameter like:

enum class TraceUnit {
  Instruction, ///< Call the callback with the address each every instruction in a program.
  Branch, ///< Call the callback with the address of each branch that causes program flow to change.
  Return, ///< Call the callback only for branches that return from a function and removes a function from the thread's stack.
  Call, ///< Call the callback only for a function calls that add a function to the thread's stack.
};

Then this would make this traverse function very versatile as it could be used for making backtraces or stepping through the trace (step in, out, over etc). The enums could alternatively define bits in a bitfield that could be combined.

68

This could be very expensive to calculate. Do we want to omit this to avoid having to traverse all of the data and also disassemble everything to find out instruction counts? Since our trace might have 1000 trace branches, but millions of instructions that are traversed between each branch

77

Why not just put this information in the callback? It seems like a storage issue or performance issue if you can only find the address out at callback time.

83

Can we put this info in the callback? Ditto from above comment.

[wallace]

I'll redo this in a different patch

lldb/include/lldb/Target/ThreadTrace.h

+//===-- ThreadTrace.h -------------------------------------------*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLDB_TARGET_THREAD_TRACE_H
+#define LLDB_TARGET_THREAD_TRACE_H
+
+#include "llvm/Support/Error.h"
+
+#include "lldb/lldb-private.h"
+
+namespace lldb_private {
+
+/// Helper enumeration for traversing the instructions of a processor trace.
+enum class TraceDirection {
+  Forwards = 0,
+  Backwards,
+};
+
+/// Class that represents a trace for a specific thread.
+///
+/// This class attempts to be a generic interface for accessing the instructions
+/// of the trace so that each Trace plug-in can reconstruct, represent and store
+/// the instruction data in the best possible way for the given technology.
+///
+/// Consumers of this class should make no assumptions on whether the trace is
+/// fully loaded on memory or not, as the plug-in might load or reconstruct
+/// instruction lazily.
+class ThreadTrace {
+public:
+  virtual ~ThreadTrace() = default;
+
+  /// Run the provided callback on the instructions of the trace.
+  ///
+  /// The instructions will be traversed starting at the given \a start_position
+  /// sequentially until the callback returns \b false, in which case no more
+  /// instructions are inspected.
+  ///
+  /// \param[in] start_position
+  ///     The instruction position to start iterating on, where 0 corresponds to
+  ///     the oldest instruction chronologically.
+  ///
+  /// \param[in] direction
+  ///     If \b TraceDirection::Forwards, then then instructions will be
+  ///     traversed forwards chronologically, i.e. with incrementing indices. If
+  ///     \b TraceDirection::Backwards, the traversal is done backwards
+  ///     chronologically, i.e. with decrementing indices.
+  ///
+  /// \param[in] callback
+  ///     The callback to execute on each instruction. If it returns \b false,
+  ///     the iteration stops.
+  ///
+  ///     If the \b load_address input of this callback is an \a llvm::Error,
+  ///     then that means that the instruction failed to be reconstructed.
+  ///     Otherwise, the load address of that instruction is returned.
+  virtual void TraverseInstructions(
+      size_t position, TraceDirection direction,
+      std::function<bool(size_t index,
+                         llvm::Expected<lldb::addr_t> load_address)>
+          callback) = 0;
+
+  /// \return
+  ///       The number of available instructions in the trace.
+  virtual size_t GetInstructionCount() = 0;
+
+  /// Return an architecture-agnostic category for the given instruction.
+  ///
+  /// \param[in] index
+  ///      The index of the instruction in question. It must be valid.
+  ///
+  /// \return
+  ///      The instruction type.
+  virtual lldb::TraceInstructionType GetInstructionType(size_t index) = 0;
+
+  /// Return the size in bytes of a given instruction.
+  ///
+  /// \param[in] index
+  ///      The index of the instruction in question. It must be valid.
+  virtual size_t GetInstructionSize(size_t index) = 0;
+};
+
+} // namespace lldb_private
+
+#endif // LLDB_TARGET_THREAD_TRACE_H

lldb/include/lldb/Target/Trace.h

 #define LLDB_TARGET_TRACE_H
 
 #include <unordered_map>
 
 #include "llvm/Support/JSON.h"
 
 #include "lldb/Core/PluginInterface.h"
 #include "lldb/Target/Thread.h"
+#include "lldb/Target/ThreadTrace.h"
 #include "lldb/Utility/ArchSpec.h"
 #include "lldb/Utility/TraceGDBRemotePackets.h"
 #include "lldb/Utility/UnimplementedError.h"
 #include "lldb/lldb-private.h"
 
 namespace lldb_private {
 
 /// \class Trace Trace.h "lldb/Target/Trace.h"
 /// A plug-in interface definition class for trace information.
 ///
 /// Trace plug-ins allow processor trace information to be loaded into LLDB so
 /// that the data can be dumped, used for reverse and forward stepping to allow
 /// introspection into the reason your process crashed or found its way to its
 /// current state.
 ///
 /// Trace information can be loaded into a target without a process to allow
 /// introspection of the trace information during post mortem analysis, such as
 /// when loading core files.
 ///
 /// Processor trace information can also be fetched through the process
 /// interfaces during a live debug session if your process supports gathering
 /// this information.
 ///
 /// In order to support live tracing, the name of the plug-in should match the
 /// name of the tracing type returned by the gdb-remote packet
 /// \a jLLDBTraceSupported.
 class Trace : public PluginInterface,
               public std::enable_shared_from_this<Trace> {
 public:
-  enum class TraceDirection {
-    Forwards = 0,
-    Backwards,
-  };
-
   /// Dump the trace data that this plug-in has access to.
   ///
   /// This function will dump all of the trace data for all threads in a user
   /// readable format. Options for dumping can be added as this API is iterated
   /// on.
   ///
   /// \param[in] s
   ///     A stream object to dump the information to.
public:
   ///     - core trace data
   ///     - thread events or related information
   ///   - shared library load information to use for this trace data that
   ///     allows a target to be created so the trace information can be
   ///     symbolicated so that the trace information can be displayed to the
   ///     user
   ///     - shared library path
   ///     - load address
   ///     - information on how to fetch the shared library
   ///       - path to locally cached file on disk
   ///       - URL to download the file
   ///   - Any information needed to load the trace file
   ///     - CPU information
   ///     - Custom plug-in information needed to decode the trace information
   ///       correctly.
   ///
   /// \param[in] debugger
public:
   /// Get the command handle for the "thread trace start" command.
   virtual lldb::CommandObjectSP
   GetThreadTraceStartCommand(CommandInterpreter &interpreter) = 0;
 
   /// \return
   ///     The JSON schema of this Trace plug-in.
   virtual llvm::StringRef GetSchema() = 0;
 
-  /// Each decoded thread contains a cursor to the current position the user is
-  /// stopped at. When reverse debugging, each operation like reverse-next or
-  /// reverse-continue will move this cursor, which is then picked by any
-  /// subsequent dump or reverse operation.
-  ///
-  /// The initial position for this cursor is the last element of the thread,
-  /// which is the most recent chronologically.
-  ///
-  /// \return
-  ///     The current position of the thread's trace or \b 0 if empty.
-  virtual size_t GetCursorPosition(Thread &thread) = 0;
-
   /// Dump \a count instructions of the given thread's trace ending at the
   /// given \a end_position position.
   ///
   /// The instructions are printed along with their indices or positions, which
   /// are increasing chronologically. This means that the \a index 0 represents
   /// the oldest instruction of the trace chronologically.
   ///
   /// \param[in] thread
public:
   ///     The position of the last instruction to print.
   ///
   /// \param[in] raw
   ///     Dump only instruction addresses without disassembly nor symbol
   ///     information.
   void DumpTraceInstructions(Thread &thread, Stream &s, size_t count,
                              size_t end_position, bool raw);
 
-  /// Run the provided callback on the instructions of the trace of the given
-  /// thread.
-  ///
-  /// The instructions will be traversed starting at the given \a position
-  /// sequentially until the callback returns \b false, in which case no more
-  /// instructions are inspected.
-  ///
-  /// The purpose of this method is to allow inspecting traced instructions
-  /// without exposing the internal representation of how they are stored on
-  /// memory.
-  ///
-  /// \param[in] thread
-  ///     The thread whose trace will be traversed.
-  ///
-  /// \param[in] position
-  ///     The instruction position to start iterating on.
-  ///
-  /// \param[in] direction
-  ///     If \b TraceDirection::Forwards, then then instructions will be
-  ///     traversed forwards chronologically, i.e. with incrementing indices. If
-  ///     \b TraceDirection::Backwards, the traversal is done backwards
-  ///     chronologically, i.e. with decrementing indices.
-  ///
-  /// \param[in] callback
-  ///     The callback to execute on each instruction. If it returns \b false,
-  ///     the iteration stops.
-  virtual void TraverseInstructions(
-      Thread &thread, size_t position, TraceDirection direction,
-      std::function<bool(size_t index, llvm::Expected<lldb::addr_t> load_addr)>
-          callback) = 0;
-
-  /// Get the number of available instructions in the trace of the given thread.
-  ///
-  /// \param[in] thread
-  ///     The thread whose trace will be inspected.
-  ///
-  /// \return
-  ///     The total number of instructions in the trace, or \a llvm::None if the
-  ///     thread is not being traced.
-  virtual llvm::Optional<size_t> GetInstructionCount(Thread &thread) = 0;
-
-  /// Check if a thread is currently traced by this object.
+  /// Get the trace corresponding to a specific thread.
   ///
   /// \param[in] thread
-  ///     The thread in question.
   ///
   /// \return
-  ///     \b true if the thread is traced by this instance, \b false otherwise.
-  virtual bool IsTraced(const Thread &thread) = 0;
+  ///     The requested trace or an \a llvm::Error if the thread is not being
+  ///     traced or if there was an error loading or fetching the trace.
+  virtual llvm::Expected<lldb::ThreadTraceSP>
+  GetThreadTrace(Thread &thread) = 0;
 
   /// \return
   ///     A description of the parameters to use for the \a Trace::Start method.
   virtual const char *GetStartConfigurationHelp() = 0;
 
   /// Start tracing a live process.
   ///
   /// \param[in] configuration
protected:
   llvm::Expected<std::string> GetLiveProcessState();
 
   /// Method to be overriden by the plug-in to refresh its own state.
   ///
   /// This is invoked by RefreshLiveProcessState when a new state is found.
   ///
   /// \param[in] state
   ///     The jLLDBTraceGetState response.
-  virtual void
-  DoRefreshLiveProcessState(llvm::Expected<TraceGetStateResponse> state) = 0;
+  virtual llvm::Error
+  DoRefreshLiveProcessState(const TraceGetStateResponse &state) = 0;
 
   /// Method to be invoked by the plug-in to refresh the live process state.
   ///
   /// The result is cached through the same process stop.
-  void RefreshLiveProcessState();
+  llvm::Error RefreshLiveProcessState();
 
   /// Process traced by this object if doing live tracing. Otherwise it's null.
   int64_t m_stop_id = -1;
   Process *m_live_process = nullptr;
   /// tid -> data kind -> size
   std::map<lldb::tid_t, std::unordered_map<std::string, size_t>>
       m_live_thread_data;
   /// data kind -> size
   std::unordered_map<std::string, size_t> m_live_process_data;
+  /// Cached error if \a Trace::RefreshLiveProcessState fails.
+  llvm::Optional<std::string> m_live_process_refresh_error;
 };
 
 } // namespace lldb_private
 
 #endif // LLDB_TARGET_TRACE_H

lldb/include/lldb/lldb-enumerations.h

 //===-- lldb-enumerations.h -------------------------------------*- C++ -*-===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef LLDB_LLDB_ENUMERATIONS_H
 #define LLDB_LLDB_ENUMERATIONS_H
 
+#include <cstdint>
 #include <type_traits>
 
 #ifndef SWIG
 // Macro to enable bitmask operations on an enum.  Without this, Enum | Enum
 // gets promoted to an int, so you have to say Enum a = Enum(eFoo | eBar).  If
 // you mark Enum with LLDB_MARK_AS_BITMASK_ENUM(Enum), however, you can simply
 // write Enum a = eFoo | eBar.
 // Unfortunately, swig<3.0 doesn't recognise the constexpr keyword, so remove
enum BasicType {
   eBasicTypeLongDoubleComplex,
   eBasicTypeObjCID,
   eBasicTypeObjCClass,
   eBasicTypeObjCSel,
   eBasicTypeNullPtr,
   eBasicTypeOther
 };
 
-/// Deprecated
-enum TraceType {
-  eTraceTypeNone = 0,
-
-  /// Intel Processor Trace
-  eTraceTypeProcessorTrace
+/// Architecture-agnostic categorization of instructions. Useful for doing
+/// analysis on traces.
+enum TraceInstructionType : uint8_t {
+  /// The instruction is not recognized by LLDB
+  eTraceInstructionUnknown = 0,
+  /// The instruction is something not listed below
+  eTraceInstructionOther,
+  /// The instruction is a (function) call
+  eTraceInstructionCall,
+  /// The instruction is a (function) return
+  eTraceInstructionReturn,
+  /// The instruction is a unconditional jump
+  eTraceInstructionJump,
+  /// The instruction is a conditional jump
+  eTraceInstructionCondJump,
+  /// The instruction writes custom data to the trace, e.g. Intel's PTWRITE.
+  eTraceInstructionTraceWrite,
 };
 
 enum StructuredDataType {
   eStructuredDataTypeInvalid = -1,
   eStructuredDataTypeNull = 0,
   eStructuredDataTypeGeneric,
   eStructuredDataTypeArray,
   eStructuredDataTypeInteger,
   eStructuredDataTypeFloat,
   eStructuredDataTypeBoolean,
   eStructuredDataTypeString,
   eStructuredDataTypeDictionary
 };

lldb/include/lldb/lldb-forward.h

 class ThreadPlanBase;
 class ThreadPlanRunToAddress;
 class ThreadPlanStepInstruction;
 class ThreadPlanStepOut;
 class ThreadPlanStepOverBreakpoint;
 class ThreadPlanStepRange;
 class ThreadPlanStepThrough;
 class ThreadPlanTracer;
-class ThreadSpec;
 class ThreadPostMortemTrace;
+class ThreadSpec;
+class ThreadTrace;
 class Trace;
 class TraceSessionFileParser;
 class Type;
 class TypeAndOrName;
 class TypeCategoryImpl;
 class TypeCategoryMap;
 class TypeEnumMemberImpl;
 class TypeEnumMemberListImpl;

 typedef std::shared_ptr<lldb_private::Thread> ThreadSP;
 typedef std::weak_ptr<lldb_private::Thread> ThreadWP;
 typedef std::shared_ptr<lldb_private::ThreadCollection> ThreadCollectionSP;
 typedef std::shared_ptr<lldb_private::ThreadPlan> ThreadPlanSP;
 typedef std::shared_ptr<lldb_private::ThreadPostMortemTrace>
     ThreadPostMortemTraceSP;
 typedef std::weak_ptr<lldb_private::ThreadPlan> ThreadPlanWP;
 typedef std::shared_ptr<lldb_private::ThreadPlanTracer> ThreadPlanTracerSP;
+typedef std::shared_ptr<lldb_private::ThreadTrace> ThreadTraceSP;
 typedef std::shared_ptr<lldb_private::Trace> TraceSP;
 typedef std::shared_ptr<lldb_private::Type> TypeSP;
 typedef std::weak_ptr<lldb_private::Type> TypeWP;
 typedef std::shared_ptr<lldb_private::TypeCategoryImpl> TypeCategoryImplSP;
 typedef std::shared_ptr<lldb_private::TypeImpl> TypeImplSP;
 typedef std::shared_ptr<lldb_private::TypeMemberFunctionImpl>
     TypeMemberFunctionImplSP;
 typedef std::shared_ptr<lldb_private::TypeEnumMemberImpl> TypeEnumMemberImplSP;

lldb/source/Commands/CommandObjectThread.cpp

protected:
   bool IsRepeatCommand() {
     return !m_repeat_command.empty() && !m_create_repeat_command_just_invoked;
   }
 
   bool HandleOneThread(lldb::tid_t tid, CommandReturnObject &result) override {
     const TraceSP &trace_sp = m_exe_ctx.GetTargetSP()->GetTrace();
     ThreadSP thread_sp =
         m_exe_ctx.GetProcessPtr()->GetThreadList().FindThreadByID(tid);
-
+    if (llvm::Expected<ThreadTraceSP> thread_trace_sp =
+            trace_sp->GetThreadTrace(*thread_sp)) {
       size_t count = m_options.m_count;
-    ssize_t position = m_options.m_position.getValueOr(
-                           trace_sp->GetCursorPosition(*thread_sp)) -
-                       m_consecutive_repetitions * count;
+      ssize_t position =
+          m_options.m_position.getValueOr(
+              (ssize_t)thread_trace_sp.get()->GetInstructionCount() - 1) -
       if (position < 0)
         result.SetError("error: no more data");
       else
         trace_sp->DumpTraceInstructions(*thread_sp, result.GetOutputStream(),
                                         count, position, m_options.m_raw);
+    } else {
+      result.GetOutputStream().Printf(
+          "thread #%u: tid = %" PRIu64 ", %s", thread_sp->GetIndexID(),
+          thread_sp->GetID(),
+          llvm::toString(thread_trace_sp.takeError()).c_str());
+    }
+
     return true;
   }
 
   CommandOptions m_options;
 
   // Repeat command helpers
   std::string m_repeat_command;
   bool m_create_repeat_command_just_invoked;

lldb/source/Plugins/Trace/intel-pt/CMakeLists.txt

 find_library(LIBIPT_LIBRARY ipt PATHS ${LIBIPT_LIBRARY_PATH} REQUIRED)
 
 lldb_tablegen(TraceIntelPTCommandOptions.inc -gen-lldb-option-defs
   SOURCE TraceIntelPTOptions.td
   TARGET TraceIntelPTOptionsGen)
 
 add_lldb_library(lldbPluginTraceIntelPT PLUGIN
   CommandObjectTraceStartIntelPT.cpp
-  DecodedThread.cpp
+  ThreadTraceIntelPT.cpp
   IntelPTDecoder.cpp
   TraceIntelPT.cpp
   TraceIntelPTSessionFileParser.cpp
 
   LINK_LIBS
     lldbCore
     lldbSymbol
     lldbTarget
     ${LIBIPT_LIBRARY}
   LINK_COMPONENTS
     Support
   )
 
 
 add_dependencies(lldbPluginTraceIntelPT TraceIntelPTOptionsGen)

lldb/source/Plugins/Trace/intel-pt/DecodedThread.cpp

-//===-- DecodedThread.cpp -------------------------------------------------===//
-//
-// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
-// See https://llvm.org/LICENSE.txt for license information.
-// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
-//
-//===----------------------------------------------------------------------===//
-
-#include "DecodedThread.h"
-
-#include "lldb/Utility/StreamString.h"
-
-using namespace lldb_private;
-using namespace lldb_private::trace_intel_pt;
-using namespace llvm;
-
-char IntelPTError::ID;
-
-IntelPTError::IntelPTError(int libipt_error_code, lldb::addr_t address)
-    : m_libipt_error_code(libipt_error_code), m_address(address) {
-  assert(libipt_error_code < 0);
-}
-
-void IntelPTError::log(llvm::raw_ostream &OS) const {
-  const char *libipt_error_message = pt_errstr(pt_errcode(m_libipt_error_code));
-  if (m_address != LLDB_INVALID_ADDRESS && m_address > 0) {
-    write_hex(OS, m_address, HexPrintStyle::PrefixLower, 18);
-    OS << "    ";
-  }
-  OS << "error: " << libipt_error_message;
-}
-
-bool IntelPTInstruction::IsError() const { return (bool)m_error; }
-
-Expected<lldb::addr_t> IntelPTInstruction::GetLoadAddress() const {
-  if (IsError())
-    return ToError();
-  return m_pt_insn.ip;
-}
-
-Error IntelPTInstruction::ToError() const {
-  if (!IsError())
-    return Error::success();
-
-  if (m_error->isA<IntelPTError>())
-    return make_error<IntelPTError>(static_cast<IntelPTError &>(*m_error));
-  return make_error<StringError>(m_error->message(),
-                                 m_error->convertToErrorCode());
-}
-
-size_t DecodedThread::GetLastPosition() const {
-  return m_instructions.empty() ? 0 : m_instructions.size() - 1;
-}
-
-ArrayRef<IntelPTInstruction> DecodedThread::GetInstructions() const {
-  return makeArrayRef(m_instructions);
-}
-
-size_t DecodedThread::GetCursorPosition() const { return m_position; }
-
-size_t DecodedThread::SetCursorPosition(size_t new_position) {
-  m_position = std::min(new_position, GetLastPosition());
-  return m_position;
-}
-
-DecodedThread::DecodedThread(Error error) {
-  m_instructions.emplace_back(std::move(error));
-  m_position = GetLastPosition();
-}

lldb/source/Plugins/Trace/intel-pt/IntelPTDecoder.h

 //===-- IntelPTDecoder.h --======--------------------------------*- C++ -*-===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef LLDB_SOURCE_PLUGINS_TRACE_INTEL_PT_DECODER_H
 #define LLDB_SOURCE_PLUGINS_TRACE_INTEL_PT_DECODER_H
 
 #include "intel-pt.h"
 
-#include "DecodedThread.h"
+#include "ThreadTraceIntelPT.h"
 #include "forward-declarations.h"
 #include "lldb/Target/Process.h"
 #include "lldb/Utility/FileSpec.h"
 
 namespace lldb_private {
 namespace trace_intel_pt {
 
 /// Base class that handles the decoding of a thread and caches the result.
 class ThreadDecoder {
 public:
   virtual ~ThreadDecoder() = default;
 
   ThreadDecoder() = default;
 
   /// Decode the thread and store the result internally, to avoid
   /// recomputations.
   ///
   /// \return
   ///     A \a DecodedThread instance.
-  const DecodedThread &Decode();
+  llvm::Expected<ThreadTraceIntelPTSP> Decode();
 
   ThreadDecoder(const ThreadDecoder &other) = delete;
   ThreadDecoder &operator=(const ThreadDecoder &other) = delete;
 
 protected:
   /// Decode the thread.
   ///
   /// \return
   ///     A \a DecodedThread instance.
-  virtual DecodedThread DoDecode() = 0;
+  virtual llvm::Expected<ThreadTraceIntelPTSP> DoDecode() = 0;
 
-  llvm::Optional<DecodedThread> m_decoded_thread;
+  llvm::Optional<ThreadTraceIntelPTSP> m_decoded_thread;
 };
 
 /// Decoder implementation for \a lldb_private::ThreadPostMortemTrace, which are
 /// non-live processes that come trace session files.
 class PostMortemThreadDecoder : public ThreadDecoder {
 public:
   /// \param[in] trace_thread
   ///     The thread whose trace file will be decoded.
   ///
   /// \param[in] trace
   ///     The main Trace object who owns this decoder and its data.
   PostMortemThreadDecoder(const lldb::ThreadPostMortemTraceSP &trace_thread,
                           TraceIntelPT &trace);
 
 private:
-  DecodedThread DoDecode() override;
+  llvm::Expected<ThreadTraceIntelPTSP> DoDecode() override;
 
   lldb::ThreadPostMortemTraceSP m_trace_thread;
   TraceIntelPT &m_trace;
 };
 
 class LiveThreadDecoder : public ThreadDecoder {
 public:
   /// \param[in] thread
   ///     The thread whose traces will be decoded.
   ///
   /// \param[in] trace
   ///     The main Trace object who owns this decoder and its data.
   LiveThreadDecoder(Thread &thread, TraceIntelPT &trace);
 
 private:
-  DecodedThread DoDecode() override;
+  llvm::Expected<ThreadTraceIntelPTSP> DoDecode() override;
 
   lldb::ThreadSP m_thread_sp;
   TraceIntelPT &m_trace;
 };
 
 } // namespace trace_intel_pt
 } // namespace lldb_private
 
 #endif // LLDB_SOURCE_PLUGINS_TRACE_INTEL_PT_DECODER_H

lldb/source/Plugins/Trace/intel-pt/IntelPTDecoder.cpp

 //===-- IntelPTDecoder.cpp --======----------------------------------------===//
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include "IntelPTDecoder.h"
 
 #include "llvm/Support/MemoryBuffer.h"
+#include <intel-pt.h>
 
 #include "TraceIntelPT.h"
 #include "lldb/Core/Module.h"
 #include "lldb/Core/Section.h"
 #include "lldb/Target/Target.h"
 #include "lldb/Target/ThreadPostMortemTrace.h"
-#include "lldb/Utility/StringExtractor.h"
 
 using namespace lldb;
 using namespace lldb_private;
 using namespace lldb_private::trace_intel_pt;
 using namespace llvm;
 
 /// Move the decoder forward to the next synchronization point (i.e. next PSB
 /// packet).

 /// Error codes returned by libipt while decoding are:
 /// - negative: actual errors
 /// - positive or zero: not an error, but a list of bits signaling the status of
 /// the decoder
 ///
 /// \param[in] decoder
 ///   A configured libipt \a pt_insn_decoder.
 ///
 /// \return
 ///   The decoded instructions.
 static std::vector<IntelPTInstruction>
 DecodeInstructions(pt_insn_decoder &decoder) {
   std::vector<IntelPTInstruction> instructions;
 
   while (true) {
     int errcode = FindNextSynchronizationPoint(decoder);
     if (errcode == -pte_eos)
if (!buffer)
     return buffer.takeError();
   if (Expected<pt_cpu> cpu_info = trace.GetCPUInfo())
     return DecodeInMemoryTrace(*thread.GetProcess(), trace,
                                MutableArrayRef<uint8_t>(*buffer));
   else
     return cpu_info.takeError();
 }
 
-const DecodedThread &ThreadDecoder::Decode() {
-  if (!m_decoded_thread.hasValue())
-    m_decoded_thread = DoDecode();
+Expected<ThreadTraceIntelPTSP> ThreadDecoder::Decode() {
+  if (!m_decoded_thread.hasValue()) {
+    if (Expected<ThreadTraceIntelPTSP> thread_trace_sp = DoDecode()) {
+      m_decoded_thread = *thread_trace_sp;
+    } else {
+      return thread_trace_sp.takeError();
+    }
+  }
   return *m_decoded_thread;
 }
 
 PostMortemThreadDecoder::PostMortemThreadDecoder(
     const lldb::ThreadPostMortemTraceSP &trace_thread, TraceIntelPT &trace)
     : m_trace_thread(trace_thread), m_trace(trace) {}
 
-DecodedThread PostMortemThreadDecoder::DoDecode() {
+Expected<ThreadTraceIntelPTSP> PostMortemThreadDecoder::DoDecode() {
   if (Expected<std::vector<IntelPTInstruction>> instructions =
           DecodeTraceFile(*m_trace_thread->GetProcess(), m_trace,
                           m_trace_thread->GetTraceFile()))
-    return DecodedThread(std::move(*instructions));
+    return std::make_shared<ThreadTraceIntelPT>(std::move(*instructions));
   else
-    return DecodedThread(instructions.takeError());
+    return instructions.takeError();
 }
 
 LiveThreadDecoder::LiveThreadDecoder(Thread &thread, TraceIntelPT &trace)
     : m_thread_sp(thread.shared_from_this()), m_trace(trace) {}
 
-DecodedThread LiveThreadDecoder::DoDecode() {
+Expected<ThreadTraceIntelPTSP> LiveThreadDecoder::DoDecode() {
   if (Expected<std::vector<IntelPTInstruction>> instructions =
           DecodeLiveThread(*m_thread_sp, m_trace))
-    return DecodedThread(std::move(*instructions));
+    return std::make_shared<ThreadTraceIntelPT>(std::move(*instructions));
   else
-    return DecodedThread(instructions.takeError());
+    return instructions.takeError();
 }

lldb/source/Plugins/Trace/intel-pt/ThreadTraceIntelPT.h

-//===-- DecodedThread.h -----------------------------------------*- C++ -*-===//
+//===-- ThreadTraceIntelPT.h ------------------------------------*- C++ -*-===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
-#ifndef LLDB_SOURCE_PLUGINS_TRACE_INTEL_PT_DECODEDTHREAD_H
-#define LLDB_SOURCE_PLUGINS_TRACE_INTEL_PT_DECODEDTHREAD_H
+#ifndef LLDB_SOURCE_PLUGINS_TRACE_INTEL_PT_THREAD_TRACE_H
+#define LLDB_SOURCE_PLUGINS_TRACE_INTEL_PT_THREAD_TRACE_H
 
 #include <vector>
 
-#include "llvm/Support/Errc.h"
 #include "llvm/Support/Error.h"
 
 #include "lldb/Target/Trace.h"
 #include "lldb/Utility/TraceIntelPTGDBRemotePackets.h"
 
 #include "intel-pt.h"
 
 namespace lldb_private {
public:
   ///     Whether this object represents an error.
   bool IsError() const;
 
   /// \return
   ///     The instruction pointer address, or an \a llvm::Error if it is an
   ///     error.
   llvm::Expected<lldb::addr_t> GetLoadAddress() const;
 
+  lldb::TraceInstructionType GetInstructionType() const;
+
+  size_t GetInstructionSize() const;
+
   /// \return
   ///     An \a llvm::Error object if this class corresponds to an Error, or an
   ///     \a llvm::Error::success otherwise.
   llvm::Error ToError() const;
 
   IntelPTInstruction(IntelPTInstruction &&other) = default;
 
 private:
   IntelPTInstruction(const IntelPTInstruction &other) = delete;
   const IntelPTInstruction &operator=(const IntelPTInstruction &other) = delete;
 
   pt_insn m_pt_insn;
   std::unique_ptr<llvm::ErrorInfoBase> m_error;
 };
 
-/// \class DecodedThread
-/// Class holding the instructions and function call hierarchy obtained from
-/// decoding a trace, as well as a position cursor used when reverse debugging
-/// the trace.
-///
-/// Each decoded thread contains a cursor to the current position the user is
-/// stopped at. See \a Trace::GetCursorPosition for more information.
-class DecodedThread {
+class ThreadTraceIntelPT : public ThreadTrace {
 public:
-  DecodedThread(std::vector<IntelPTInstruction> &&instructions)
-      : m_instructions(std::move(instructions)), m_position(GetLastPosition()) {
-  }
+  ThreadTraceIntelPT(std::vector<IntelPTInstruction> &&instructions)
+      : m_instructions(std::move(instructions)) {}
 
-  /// Constructor with a single error signaling a complete failure of the
-  /// decoding process.
-  DecodedThread(llvm::Error error);
+  void TraverseInstructions(
+      size_t start_position, TraceDirection direction,
+      std::function<bool(size_t index,
+                         llvm::Expected<lldb::addr_t> load_address)>
+          callback) override;
 
-  /// Get the instructions from the decoded trace. Some of them might indicate
-  /// errors (i.e. gaps) in the trace.
-  ///
-  /// \return
-  ///   The instructions of the trace.
-  llvm::ArrayRef<IntelPTInstruction> GetInstructions() const;
+  size_t GetInstructionCount() override;
 
-  /// \return
-  ///   The current position of the cursor of this trace, or 0 if there are no
-  ///   instructions.
-  size_t GetCursorPosition() const;
+  lldb::TraceInstructionType GetInstructionType(size_t index) override;
 
-  /// Change the position of the cursor of this trace. If this value is to high,
-  /// the new position will be set as the last instruction of the trace.
-  ///
-  /// \return
-  ///     The effective new position.
-  size_t SetCursorPosition(size_t new_position);
-  /// \}
+  size_t GetInstructionSize(size_t index) override;
 
 private:
-  /// \return
-  ///     The index of the last element of the trace, or 0 if empty.
-  size_t GetLastPosition() const;
-
   std::vector<IntelPTInstruction> m_instructions;
-  size_t m_position;
 };
 
+using ThreadTraceIntelPTSP = std::shared_ptr<ThreadTraceIntelPT>;
+
 } // namespace trace_intel_pt
 } // namespace lldb_private
 
-#endif // LLDB_SOURCE_PLUGINS_TRACE_INTEL_PT_DECODEDTHREAD_H
+#endif // LLDB_SOURCE_PLUGINS_TRACE_INTEL_PT_THREAD_TRACE_H

lldb/source/Plugins/Trace/intel-pt/ThreadTraceIntelPT.cpp

+//===-- ThreadTraceIntelPT.cpp --------------------------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#include "ThreadTraceIntelPT.h"
+
+#include "lldb/Utility/StreamString.h"
+
+using namespace lldb;
+using namespace lldb_private;
+using namespace lldb_private::trace_intel_pt;
+using namespace llvm;
+
+char IntelPTError::ID;
+
+IntelPTError::IntelPTError(int libipt_error_code, lldb::addr_t address)
+    : m_libipt_error_code(libipt_error_code), m_address(address) {
+  assert(libipt_error_code < 0);
+}
+
+void IntelPTError::log(llvm::raw_ostream &OS) const {
+  const char *libipt_error_message = pt_errstr(pt_errcode(m_libipt_error_code));
+  if (m_address != LLDB_INVALID_ADDRESS && m_address > 0) {
+    write_hex(OS, m_address, HexPrintStyle::PrefixLower, 18);
+    OS << "    ";
+  }
+  OS << "error: " << libipt_error_message;
+}
+
+bool IntelPTInstruction::IsError() const { return (bool)m_error; }
+
+Expected<lldb::addr_t> IntelPTInstruction::GetLoadAddress() const {
+  if (IsError())
+    return ToError();
+  return m_pt_insn.ip;
+}
+
+Error IntelPTInstruction::ToError() const {
+  if (!IsError())
+    return Error::success();
+
+  if (m_error->isA<IntelPTError>())
+    return make_error<IntelPTError>(static_cast<IntelPTError &>(*m_error));
+  return make_error<StringError>(m_error->message(),
+                                 m_error->convertToErrorCode());
+}
+
+TraceInstructionType IntelPTInstruction::GetInstructionType() const {
+  switch (m_pt_insn.iclass) {
+  case ptic_other:
+    return lldb::eTraceInstructionOther;
+  case ptic_call:
+  case ptic_far_call:
+    return lldb::eTraceInstructionCall;
+  case ptic_return:
+  case ptic_far_return:
+    return lldb::eTraceInstructionReturn;
+  case ptic_jump:
+  case ptic_far_jump:
+    return lldb::eTraceInstructionJump;
+  case ptic_cond_jump:
+    return lldb::eTraceInstructionCondJump;
+  case ptic_ptwrite:
+    return lldb::eTraceInstructionTraceWrite;
+  default:
+    return lldb::eTraceInstructionUnknown;
+  }
+}
+
+size_t IntelPTInstruction::GetInstructionSize() const { return m_pt_insn.size; }
+
+void ThreadTraceIntelPT::TraverseInstructions(
+    size_t start_position, TraceDirection direction,
+    std::function<bool(size_t index, llvm::Expected<lldb::addr_t> load_address)>
+        callback) {
+  ssize_t delta = direction == TraceDirection::Forwards ? 1 : -1;
+  for (ssize_t i = start_position; i < (ssize_t)m_instructions.size() && i >= 0;
+       i += delta)
+    if (!callback(i, m_instructions[i].GetLoadAddress()))
+      break;
+}
+
+size_t ThreadTraceIntelPT::GetInstructionCount() {
+  return m_instructions.size();
+}
+
+lldb::TraceInstructionType
+ThreadTraceIntelPT::GetInstructionType(size_t index) {
+  return m_instructions[index].GetInstructionType();
+}
+
+size_t ThreadTraceIntelPT::GetInstructionSize(size_t index) {
+  return m_instructions[index].GetInstructionSize();
+}

lldb/source/Plugins/Trace/intel-pt/TraceIntelPT.h

public:
   lldb::CommandObjectSP
   GetProcessTraceStartCommand(CommandInterpreter &interpreter) override;
 
   lldb::CommandObjectSP
   GetThreadTraceStartCommand(CommandInterpreter &interpreter) override;
 
   llvm::StringRef GetSchema() override;
 
-  void TraverseInstructions(
-      Thread &thread, size_t position, TraceDirection direction,
-      std::function<bool(size_t index, llvm::Expected<lldb::addr_t> load_addr)>
-          callback) override;
+  llvm::Error
+  DoRefreshLiveProcessState(const TraceGetStateResponse &state) override;
 
-  llvm::Optional<size_t> GetInstructionCount(Thread &thread) override;
-
-  size_t GetCursorPosition(Thread &thread) override;
-
-  void DoRefreshLiveProcessState(
-      llvm::Expected<TraceGetStateResponse> state) override;
-
-  bool IsTraced(const Thread &thread) override;
+  llvm::Expected<lldb::ThreadTraceSP> GetThreadTrace(Thread &thread) override;
 
   const char *GetStartConfigurationHelp() override;
 
   /// Start tracing a live process.
   ///
   /// \param[in] thread_buffer_size
   ///     Trace size per thread in bytes.
   ///
private:
   TraceIntelPT(
       const pt_cpu &cpu_info,
       const std::vector<lldb::ThreadPostMortemTraceSP> &traced_threads);
 
   /// Constructor for live processes
   TraceIntelPT(Process &live_process)
       : Trace(live_process), m_thread_decoders(){};
 
-  /// Decode the trace of the given thread that, i.e. recontruct the traced
-  /// instructions. That trace must be managed by this class.
-  ///
-  /// \param[in] thread
-  ///     If \a thread is a \a ThreadTrace, then its internal trace file will be
-  ///     decoded. Live threads are not currently supported.
+  /// Decode the trace of the given thread, i.e. recontruct the traced
+  /// instructions.
   ///
   /// \return
-  ///     A \a DecodedThread instance if decoding was successful, or a \b
-  ///     nullptr if the thread's trace is not managed by this class.
-  const DecodedThread *Decode(Thread &thread);
+  ///     A \a ThreadTraceIntelPT instance if decoding was successful, or an \a
+  ///     llvm::Error if the thread's trace is not managed by this class or if
+  ///     there was an error setting up the the decoder or the raw trace data.
+  llvm::Expected<ThreadTraceIntelPTSP> Decode(Thread &thread);
 
   /// It is provided by either a session file or a live process' "cpuInfo"
   /// binary data.
   llvm::Optional<pt_cpu> m_cpu_info;
   std::map<const Thread *, std::unique_ptr<ThreadDecoder>> m_thread_decoders;
-  /// Dummy DecodedThread used when decoding threads after there were errors
-  /// when refreshing the live process state.
-  llvm::Optional<DecodedThread> m_failed_live_threads_decoder;
 };
 
 } // namespace trace_intel_pt
 } // namespace lldb_private
 
 #endif // LLDB_SOURCE_PLUGINS_TRACE_INTEL_PT_TRACEINTELPT_H

lldb/source/Plugins/Trace/intel-pt/TraceIntelPT.cpp

TraceIntelPT::TraceIntelPT(
     const pt_cpu &cpu_info,
     const std::vector<ThreadPostMortemTraceSP> &traced_threads)
     : m_cpu_info(cpu_info) {
   for (const ThreadPostMortemTraceSP &thread : traced_threads)
     m_thread_decoders.emplace(
         thread.get(), std::make_unique<PostMortemThreadDecoder>(thread, *this));
 }
 
-const DecodedThread *TraceIntelPT::Decode(Thread &thread) {
-  RefreshLiveProcessState();
-  if (m_failed_live_threads_decoder.hasValue())
-    return &*m_failed_live_threads_decoder;
-
-  auto it = m_thread_decoders.find(&thread);
-  if (it == m_thread_decoders.end())
-    return nullptr;
-  return &it->second->Decode();
-}
-
-size_t TraceIntelPT::GetCursorPosition(Thread &thread) {
-  const DecodedThread *decoded_thread = Decode(thread);
-  if (!decoded_thread)
-    return 0;
-  return decoded_thread->GetCursorPosition();
+llvm::Expected<ThreadTraceSP> TraceIntelPT::GetThreadTrace(Thread &thread) {
+  return Decode(thread);
 }
 
-void TraceIntelPT::TraverseInstructions(
-    Thread &thread, size_t position, TraceDirection direction,
-    std::function<bool(size_t index, Expected<lldb::addr_t> load_addr)>
-        callback) {
-  const DecodedThread *decoded_thread = Decode(thread);
-  if (!decoded_thread)
-    return;
-
-  ArrayRef<IntelPTInstruction> instructions = decoded_thread->GetInstructions();
-
-  ssize_t delta = direction == TraceDirection::Forwards ? 1 : -1;
-  for (ssize_t i = position; i < (ssize_t)instructions.size() && i >= 0;
-       i += delta)
-    if (!callback(i, instructions[i].GetLoadAddress()))
-      break;
-}
+llvm::Expected<ThreadTraceIntelPTSP> TraceIntelPT::Decode(Thread &thread) {
+  if (llvm::Error err = RefreshLiveProcessState())
+    return std::move(err);
 
-Optional<size_t> TraceIntelPT::GetInstructionCount(Thread &thread) {
-  if (const DecodedThread *decoded_thread = Decode(thread))
-    return decoded_thread->GetInstructions().size();
-  else
-    return None;
+  auto it = m_thread_decoders.find(&thread);
+  if (it == m_thread_decoders.end())
+    return createStringError(inconvertibleErrorCode(), "thread not traced");
+  return it->second->Decode();
 }
 
 Expected<pt_cpu> TraceIntelPT::GetCPUInfoForLiveProcess() {
   Expected<std::vector<uint8_t>> cpu_info = GetLiveProcessBinaryData("cpuInfo");
   if (!cpu_info)
     return cpu_info.takeError();
 
   int64_t cpu_family = -1;
if (!m_cpu_info) {
     if (llvm::Expected<pt_cpu> cpu_info = GetCPUInfoForLiveProcess())
       m_cpu_info = *cpu_info;
     else
       return cpu_info.takeError();
   }
   return *m_cpu_info;
 }
 
-void TraceIntelPT::DoRefreshLiveProcessState(
-    Expected<TraceGetStateResponse> state) {
+Error TraceIntelPT::DoRefreshLiveProcessState(
+    const TraceGetStateResponse &state) {
   m_thread_decoders.clear();
-
-  if (!state) {
-    m_failed_live_threads_decoder = DecodedThread(state.takeError());
-    return;
-  }
-
-  for (const TraceThreadState &thread_state : state->tracedThreads) {
+  for (const TraceThreadState &thread_state : state.tracedThreads) {
     Thread &thread =
         *m_live_process->GetThreadList().FindThreadByID(thread_state.tid);
     m_thread_decoders.emplace(
         &thread, std::make_unique<LiveThreadDecoder>(thread, *this));
   }
-}
-
-bool TraceIntelPT::IsTraced(const Thread &thread) {
-  RefreshLiveProcessState();
-  return m_thread_decoders.count(&thread);
+  return Error::success();
 }
 
 const char *TraceIntelPT::GetStartConfigurationHelp() {
   return R"(Parameters:
 
   Note: If a parameter is not specified, a default value will be used.
 
   - int threadBufferSize (defaults to 4096 bytes):

lldb/source/Target/Trace.cpp

 /// \param[in] symbol_scope
 ///     If not \b 0, then the \a InstructionSymbolInfo will have its
 ///     SymbolContext calculated up to that level.
 ///
 /// \param[in] include_disassembler
 ///     If \b true, then the \a InstructionSymbolInfo will have the
 ///     \a disassembler and \a instruction objects calculated.
 static void TraverseInstructionsWithSymbolInfo(
-    Trace &trace, Thread &thread, size_t position,
-    Trace::TraceDirection direction, SymbolContextItem symbol_scope,
+    ThreadTraceSP thread_trace_sp, Thread &thread, size_t position,
+    TraceDirection direction, SymbolContextItem symbol_scope,
     bool include_disassembler,
     std::function<bool(size_t index, Expected<InstructionSymbolInfo> insn)>
         callback) {
   InstructionSymbolInfo prev_insn;
 
   Target &target = thread.GetProcess()->GetTarget();
   ExecutionContext exe_ctx;
   target.CalculateExecutionContext(exe_ctx);
DisassemblerSP disassembler =
         Disassembler::DisassembleRange(arch, /*plugin_name*/ nullptr,
                                        /*flavor*/ nullptr, target, range);
     return std::make_tuple(disassembler,
                            disassembler ? disassembler->GetInstructionList()
                                               .GetInstructionAtAddress(address)
                                         : InstructionSP());
   };
 
-  trace.TraverseInstructions(
-      thread, position, direction,
+  thread_trace_sp->TraverseInstructions(
+      position, direction,
       [&](size_t index, Expected<lldb::addr_t> load_address) -> bool {
         if (!load_address)
           return callback(index, load_address.takeError());
 
         InstructionSymbolInfo insn;
         insn.load_address = *load_address;
         insn.exe_ctx = exe_ctx;
         insn.address.SetLoadAddress(*load_address, &target);
insn.instruction->Dump(&s, /*show_address*/ false, /*show_bytes*/ false,
                          /*max_opcode_byte_size*/ 0, &insn.exe_ctx, &insn.sc,
                          /*prev_sym_ctx*/ nullptr,
                          /*disassembly_addr_format*/ nullptr,
                          /*max_address_text_size*/ 0);
 }
 
 void Trace::DumpTraceInstructions(Thread &thread, Stream &s, size_t count,
                                   size_t end_position, bool raw) {
-  Optional<size_t> instructions_count = GetInstructionCount(thread);
-  if (!instructions_count) {
-    s.Printf("thread #%u: tid = %" PRIu64 ", not traced\n", thread.GetIndexID(),
-             thread.GetID());
-    return;
-  }
+  s.Printf("thread #%u: tid = %" PRIu64, thread.GetIndexID(), thread.GetID());
 
-  s.Printf("thread #%u: tid = %" PRIu64 ", total instructions = %zu\n",
-           thread.GetIndexID(), thread.GetID(), *instructions_count);
+  if (Expected<ThreadTraceSP> thread_trace_sp = GetThreadTrace(thread)) {
+    size_t instructions_count = thread_trace_sp.get()->GetInstructionCount();
+    s.Printf(", total instructions = %zu\n", instructions_count);
 
-  if (count == 0 || end_position >= *instructions_count)
+    if (count == 0 || end_position >= instructions_count)
       return;
 
     int digits_count = GetNumberOfDigits(end_position);
     size_t start_position =
         end_position + 1 < count ? 0 : end_position + 1 - count;
     auto printInstructionIndex = [&](size_t index) {
       s.Printf("    [%*zu] ", digits_count, index);
     };
 
     bool was_prev_instruction_an_error = false;
     Optional<InstructionSymbolInfo> prev_insn;
 
     TraverseInstructionsWithSymbolInfo(
-      *this, thread, start_position, TraceDirection::Forwards,
+        *thread_trace_sp, thread, start_position, TraceDirection::Forwards,
         eSymbolContextEverything, /*disassembler*/ true,
         [&](size_t index, Expected<InstructionSymbolInfo> insn) -> bool {
           if (!insn) {
             printInstructionIndex(index);
             s << toString(insn.takeError());
 
             prev_insn = None;
             was_prev_instruction_an_error = true;
           } else {
             if (was_prev_instruction_an_error)
               s.Printf("    ...missing instructions\n");
 
             if (!raw)
               DumpInstructionSymbolContext(s, prev_insn, *insn);
 
             printInstructionIndex(index);
             s.Printf("0x%016" PRIx64, insn->load_address);
 
             if (!raw)
               DumpInstructionDisassembly(s, *insn);
 
             prev_insn = *insn;
             was_prev_instruction_an_error = false;
           }
 
           s.Printf("\n");
           return index < end_position;
         });
+
+  } else {
+    s.Printf(", %s\n", llvm::toString(thread_trace_sp.takeError()).c_str());
+  }
 }
 
 Error Trace::Start(const llvm::json::Value &request) {
   if (!m_live_process)
     return createStringError(inconvertibleErrorCode(),
                              "Tracing requires a live process.");
   return m_live_process->TraceStart(request);
 }
return createStringError(
         inconvertibleErrorCode(),
         "Tracing data \"%s\" is not available for the process.", kind.data());
 
   TraceGetBinaryDataRequest request{GetPluginName().AsCString(), kind.str(),
                                     None, 0, static_cast<int64_t>(*size)};
   return m_live_process->TraceGetBinaryData(request);
 }
 
-void Trace::RefreshLiveProcessState() {
+Error Trace::RefreshLiveProcessState() {
   if (!m_live_process)
-    return;
+    return Error::success();
 
   uint32_t new_stop_id = m_live_process->GetStopID();
-  if (new_stop_id == m_stop_id)
-    return;
+  if (new_stop_id == m_stop_id) {
+    if (m_live_process_refresh_error)
+      return createStringError(inconvertibleErrorCode(),
+                               *m_live_process_refresh_error);
+    else
+      return Error::success();
+  }
 
   m_stop_id = new_stop_id;
   m_live_thread_data.clear();
+  m_live_process_refresh_error = None;
 
   Expected<std::string> json_string = GetLiveProcessState();
   if (!json_string) {
-    DoRefreshLiveProcessState(json_string.takeError());
-    return;
+    m_live_process_refresh_error = toString(json_string.takeError());
+    return createStringError(inconvertibleErrorCode(),
+                             *m_live_process_refresh_error);
   }
   Expected<TraceGetStateResponse> live_process_state =
       json::parse<TraceGetStateResponse>(*json_string, "TraceGetStateResponse");
   if (!live_process_state) {
-    DoRefreshLiveProcessState(live_process_state.takeError());
-    return;
+    m_live_process_refresh_error = toString(live_process_state.takeError());
+    return createStringError(inconvertibleErrorCode(),
+                             *m_live_process_refresh_error);
   }
 
   for (const TraceThreadState &thread_state :
        live_process_state->tracedThreads) {
     for (const TraceBinaryData &item : thread_state.binaryData)
       m_live_thread_data[thread_state.tid][item.kind] = item.size;
   }
 
   for (const TraceBinaryData &item : live_process_state->processBinaryData)
     m_live_process_data[item.kind] = item.size;
 
-  DoRefreshLiveProcessState(std::move(live_process_state));
+  return DoRefreshLiveProcessState(*live_process_state);
 }

lldb/test/API/commands/trace/TestTraceDumpInstructions.py

def testWrongImage(self):
             substrs=['''thread #1: tid = 3842849, total instructions = 2
     [0] 0x0000000000400511    error: no memory mapped at this address
     [1] 0x0000000000400518    error: no memory mapped at this address'''])
 
     def testWrongCPU(self):
         self.expect("trace load " +
             os.path.join(self.getSourceDir(), "intelpt-trace", "trace_wrong_cpu.json"))
         self.expect("thread trace dump instructions",
-            substrs=['''thread #1: tid = 3842849, total instructions = 1
-    [0] error: unknown cpu'''])
+            substrs=['''thread #1: tid = 3842849, error: unknown cpu'''])
 
     def testMultiFileTraceWithMissingModule(self):
         self.expect("trace load " +
             os.path.join(self.getSourceDir(), "intelpt-trace-multi-file", "multi-file-no-ld.json"))
 
         # This instructions in this test covers the following flow:
         #
         # - The trace starts with a call to libfoo, which triggers the dynamic