[intelpt] Refactoring instruction decoding for flexibility

[zrthxn]

Summary

Refactored (but not yet tested) version of the instruction decoding code

[wallace]

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

181

you need to have something like

std::unordered_map<uint64_t, llvm::Error> m_errors;

that way, you'll be able to quickly look for the error associated with an instruction index. The IntelPTInstruction int his case, instead of storing the Error, can just store one bit of information has_error = true/false;

[zrthxn]

Introduced unordered map for errors in DecodedThread

[jj10306]

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

181

nit: from https://llvm.org/docs/CodingStandards.html#c-standard-library
prefer llvm::DenseMap over std::map's unless there's a specific reason not to.

Don't forget to update CalculateApproximateMemoryUsage() as well! Also, besides for being inline with the coding standards I linked above, using llvm::DenseMap here has the actual advantage that it exposes its approximate size via getMemorySize(), whereas there is no easy way to get the size of std::map.

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

114

[zrthxn]

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

181

DenseMap looks interesting we should try that.

Yes i will update the mem calculation and make it more accurate so some refactor will be needed. That'll be the next small patch once this works.

[jj10306]

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

147

Return a reference here to avoid potential expensive copy when returning.

Something else to consider is if we need/want an API exposing all the entire error map or if something like:
llvm::Error GetErrorByInstructionIndex(uint64_t insn_index) const that allows the caller to specify the key into the map would make more sense?
This also has the advantage that it hides the implementation detail of what data type is being used under the hood to represent the error map!
@wallace @zrthxn wdyt?

[zrthxn]

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

147

Yea I did think about that. In my opinion returning a reference would be good since with that you can get the size and error at each index and we don't need to have functions for each operation which would have long clunky names making the code less readable...

[wallace]

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

102–104

remove this

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

147

ahh!! DenseMap is for sure the way to go. The getMemorySize() method will help us a good deal. Thanks @jj10306

Besides that, we shouldn't expose publicly the internal representation of an object whose data structure might change. For example, sooner than later we might prefer to store the errors in a different way. So, to prevent breaking callers if we change this data structure, let's go for what Jakob proposes

llvm::Error GetErrorByInstructionIndex(uint64_t insn_index) const

which returns Error::success() in case the instruction is not an actual error

149

Omit the thread, because you are appending to a DecodedThread. In any case, it's redundant to mention where you are appending to

152

you should be able to omit the insn_index by using m_instructions.size() + m_errors.size() as the index

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

103–105

update the documentation

107

don't call it thread, call it decoded_thread. The thread is the one passed as parameter

107

use make_shared here and return a DecodedThreadSP (alias for shared_ptr<DecodedThread>). That way, you'll avoid having to invoke shared_from_this() below

119

remove this

145

This will create a copy of the IntelPTInstruction before storing it in the vector. Instead, you should use the same semantics as vector::emplace_back(), which uses paratemer packs/variadic templates. You can even rename Append to Emplace in this case

175–207

see below. We don't need to pass the raw_buffer size because it can be gotten from the buffer itself. Also, return the DecodedThreadSP directly

211–224

Now we don't need to return the raw_trace_size as an out parameter because we are creating the decoded thread deeper in the stack.

214–215

we don't need raw_trace_size to be an out parameter anymore. We can directly use it deep inside our decoding logic

249

remove raw_trace_size as an out parameter

255–256

the trace_size doesn't need to be passed

[zrthxn]

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

145

Yea I was doing that before, the idea was to send those variadic args to emplace_back but that wasnt working. I introduced this to avoid having 2 Appends since we already have 2 constructors which fulfill that requirement, and I can change this to std::move to avoid copies if thats a concern

[wallace]

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

145

Yes, use the same pattern than emolace_back uses with a parameter pack

[zrthxn]

A few changes to remove redundant things

[wallace]

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

102–104

Errors can only be copied, that's why we need to create a new instance of the error that is a copy of the original one. We can draw inspiration from IntelPTInstruction::ToError(), which can now be deleted

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

143–146

we can make the documentation clearer

150

this has to use the new parameter pack semantics, so that you can pass either {pt_insn} or {pt_insn, timestamp} without having to create copies of the IntelPTInstruction class

152

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

107

make this a shared pointer since the beginning. Use make_shared here

[wallace]

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

145

ideally you should be able to do decoded_thread.AppendInstruction({insn}) here

[wallace]

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

211

don't use expected. The DecodedThread object already can store errors. Just return a DecodedThreadSP and assign it a single error and return it. If you need to return the failed DecodedThread before you know the size of the buffer, just pass 0 as size

214

remove the *, because we will make DecodeLiveThread not return an Expected

[zrthxn]

Error gettting method

[wallace]

there are many comments from the previous versions of this diff that you didn't apply. Go through all of them first :)

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

85–87

delete it. We don't want to leave old code as comments

117

now that you changed this, could show share in the description of this diff the difference in byte size between the old and new code when tracing the same number of instructions?

143–146

you didn't apply these changes

147

or GetErrorByInstructionIndex

152

same here

[zrthxn]

Incorporate other comments

[zrthxn]

Resolved many runtime errors.
One small thing still remains, with post mortem decoder

[zrthxn]

Refactor to use more templates and param packs

[wallace]

much closer! I'm glad you are starting to understand the patterns we use for this kind of code

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

13

delete this

14

you don't need to import it. Maybe your c++ vscode extension has been autoimporting this one, in which case it's better to disable that feature.

128–131

in order to have correct formatting all along, you need to use git clang-format: https://llvm.org/docs/Contributing.html#format-patches

follow that guide. Whenever you are going to submit a patch, first run git clang-format and it will format your code correctly, so that you never again have to lose time doing that. It can even format comments

129–130

here you also need to ask for the size of the DenseMap

136–139

emplace will prevent unnecessary copies and also doesn't need you to pass a pair

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

43–46

delete commented code

72

Pass the error by const reference, because we don't modify it

135–137

146

151

remove this one. We need the version that accepts a parameter pack, as we discussed offline

153–154

same here

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

108–109

the magic of make_shared is that it uses parameter packs, so that it only constructs the object right where it'll store it, thus preventing unnecessary copies

175

don't pass the process, because we can get it from the thread_sp object

199–201

if the compiler complains mentioning that the Process pointer you are passing is const, you can do an unsafe cast that removes the const qualifier and write a comment here. I hope you don't need it anyway

206–207

use make_shared instead of shared_from_this. That will be much more performant

210

don't pass the process, it's not necessary anymore

214–218

same here

245–246

just return a direct DecodedThreadSP that holds any errors you might find here, same like LiveThreadDecoder::DoDecode()

256–261

this will become simpler once DecodeTraceFile returns directly a DecodedThreadSP

[zrthxn]

Before refactor

thread #1: tid = 37275
  Raw trace size: 4 KiB
  Total number of instructions: 21
  Total approximate memory usage: 5.38 KiB

After refactor

(lldb) thread trace dump info
Trace technology: intel-pt

thread #1: tid = 13690
  Raw trace size: 4 KiB
  Total number of instructions: 20
  Total approximate memory usage: 5.34 KiB

[zrthxn]

Incoporate more feedback,
Only parameter pack isnt done yet

[zrthxn]

Finalize diff

[zrthxn]

Added average memory per instruction

[jj10306]

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

38

Feels kinda weird that the err param isn't being used? Not sure if it would be preferred but another option would be to make the default constructor construct an error instruction.
Passing the error is more clear that this constructor will create an error instruction, but then it feels a bit unnecessary to pass it since it's now unused. I'm a little torn so would be curious to hear others opinions (:

41

Is this boolean necessary? In the case of an error, the other two fields also indicate an error so this doesn't seem to add much information.
If we remove it, you can just update IsError to check the other two fields accordingly.

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

152

Should we be using std::forward() here? Same question for the AppendError function

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

77

nit: should we update this to use the error map? I don't think there's a significant difference performance wise, but the code would be a little cleaner imo and consistent with how GetError() works.

[zrthxn]

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

77

That would sort of look like this I think

if (m_decoded_thread_sp->GetErrorByInstructionIndex(m_pos).isA<ErrorSuccess>())
  return false;
else true;

[zrthxn]

Updated tests

[jj10306]

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

77

What about
return (bool)m_decoded_thread_sp->GetErrorByInstructionIndex(m_pos);
Another idea is to just remove the IsError() function entirely since calling GetError() tells you if it's an error. iirc all error checks actually use GetError except for the checks inside of TraceHtr which is soon going to be deleted by @wallace in new patches, so you could just change those couple instances of IsError and remove it all together. Definitely not necessary, just spitballing ideas (:
@wallace what do you think?

[wallace]

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

25

101–107

let's improve this method a bit

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

85–87

good

151

just call it args or instruction_args instead of __args

152

the goal is to avoid creating explicitly a IntelPTInstruction, so you should be able to achieve something like

m_instructions.emplace_back(args...);

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

199–201

delete this

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

122–123

print it in bytes instead

[wallace]

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

77

I don't think that's a good idea. The problem is calling GetErrorByInstructionIndex is that you then have an Error object that you need to consume. There's also the cost of creating this object even if you just want to know if there's an error or not and you don't want to do anything with the actual error message. It's better then to create the Error object only when needed.

[zrthxn]

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

25

This is needed at DecodedThread.h:66, not here

[zrthxn]

Test program info, (llvm-project/lldb/test/API/commands/trace/intelpt-trace/a.out)

(lldb) thread trace dump info
Trace technology: intel-pt

thread #1: tid = 3842849
  Raw trace size: 4 KiB
  Total number of instructions: 22
  Total approximate memory usage: 6.38 KiB
  Average memory usage per instruction: 296 bytes

[zrthxn]

Clean up and finalize

[wallace]

• make tests pass
• simplified the error handling. In fact, using Error objects might be too expensive and potentially provides little

value in the API, because the user needs to consume the Error forcefully. Besides that, once we expose this python,
the error will be a plain string, therefore, I'm now storing the error as a string. Error won't be that frequent,
so the cost of that is okay.

lldb/include/lldb/Target/TraceCursor.h

public:
   /// \return
   ///     Whether the cursor points to an error or not.
   virtual bool IsError() = 0;
 
   /// Get the corresponding error message if the cursor points to an error in
   /// the trace.
   ///
   /// \return
-  ///     \b llvm::Error::success if the cursor is not pointing to an error in
-  ///     the trace. Otherwise return an \a llvm::Error describing the issue.
-  virtual llvm::Error GetError() = 0;
+  ///     \b nullptr if the cursor is not pointing to an error in
+  ///     the trace. Otherwise return the actual error message.
+  virtual const char *GetError() = 0;
 
   /// \return
   ///     The load address of the instruction the cursor is pointing at. If the
   ///     cursor points to an error in the trace, return \b
   ///     LLDB_INVALID_ADDRESS.
   virtual lldb::addr_t GetLoadAddress() = 0;
 
   /// Get the hardware counter of a given type associated with the current

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

 //===-- DecodedThread.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
 
+#include <utility>
 #include <vector>
 
 #include "llvm/Support/Errc.h"
 #include "llvm/Support/Error.h"
 
 #include "lldb/Target/Trace.h"
 #include "lldb/Utility/TraceIntelPTGDBRemotePackets.h"
 
public:
   /// \param[in] address
   ///     Optional instruction address. When decoding an individual instruction,
   ///     its address might be available in the \a pt_insn object, and should be
   ///     passed to this constructor. Other errors don't have an associated
   ///     address.
   IntelPTError(int libipt_error_code,
                lldb::addr_t address = LLDB_INVALID_ADDRESS);
 
   std::error_code convertToErrorCode() const override {
     return llvm::errc::not_supported;
   }
 
   void log(llvm::raw_ostream &OS) const override;
 
 private:
   int m_libipt_error_code;
   lldb::addr_t m_address;
 };
 
 /// \class IntelPTInstruction
 /// An instruction obtained from decoding a trace. It is either an actual
 /// instruction or an error indicating a gap in the trace.
 ///
 /// Gaps in the trace can come in a few flavors:
 ///   - tracing gaps (e.g. tracing was paused and then resumed)
 ///   - tracing errors (e.g. buffer overflow)
 ///   - decoding errors (e.g. some memory region couldn't be decoded)
 /// As mentioned, any gap is represented as an error in this class.
 class IntelPTInstruction {
 public:
   IntelPTInstruction(const pt_insn &pt_insn, uint64_t timestamp)
-      : m_pt_insn(pt_insn), m_timestamp(timestamp) {}
+      : m_pt_insn(pt_insn), m_timestamp(timestamp), m_is_error(false) {}
 
-  IntelPTInstruction(const pt_insn &pt_insn) : m_pt_insn(pt_insn) {}
+  IntelPTInstruction(const pt_insn &pt_insn)
+      : m_pt_insn(pt_insn), m_is_error(false) {}
 
   /// Error constructor
-  ///
-  /// libipt errors should use the underlying \a IntelPTError class.
-  IntelPTInstruction(llvm::Error err);
+  IntelPTInstruction();
 
   /// Check if this object represents an error (i.e. a gap).
   ///
   /// \return
   ///     Whether this object represents an error.
   bool IsError() const;
 
   /// \return
   ///     The instruction pointer address, or \a LLDB_INVALID_ADDRESS if it is
   ///     an error.
   lldb::addr_t GetLoadAddress() const;
-  
-  /// Get the size in bytes of a non-error instance of this class
-  static size_t GetNonErrorMemoryUsage();
-
-  /// \return
-  ///     An \a llvm::Error object if this class corresponds to an Error, or an
-  ///     \a llvm::Error::success otherwise.
-  llvm::Error ToError() const;
+  /// Get the size in bytes of an instance of this class
+  static size_t GetMemoryUsage();
 
   /// Get the timestamp associated with the current instruction. The timestamp
   /// is similar to what a rdtsc instruction would return.
   ///
   /// \return
   ///     The timestamp or \b llvm::None if not available.
   llvm::Optional<uint64_t> GetTimestampCounter() const;
 
   /// Get the \a lldb::TraceInstructionControlFlowType categories of the
public:
   GetControlFlowType(lldb::addr_t next_load_address) const;
 
   IntelPTInstruction(IntelPTInstruction &&other) = default;
 
 private:
   IntelPTInstruction(const IntelPTInstruction &other) = delete;
   const IntelPTInstruction &operator=(const IntelPTInstruction &other) = delete;
 
-  // When adding new members to this class, make sure to update 
+  // When adding new members to this class, make sure to update
   // IntelPTInstruction::GetNonErrorMemoryUsage() if needed.
   pt_insn m_pt_insn;
   llvm::Optional<uint64_t> m_timestamp;
-  std::unique_ptr<llvm::ErrorInfoBase> m_error;
+  bool m_is_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 : public std::enable_shared_from_this<DecodedThread> {
 public:
-  DecodedThread(lldb::ThreadSP thread_sp,
-                std::vector<IntelPTInstruction> &&instructions,
-                size_t raw_trace_size);
-
-  /// Constructor with a single error signaling a complete failure of the
-  /// decoding process.
-  DecodedThread(lldb::ThreadSP thread_sp, llvm::Error error);
+  DecodedThread(lldb::ThreadSP thread_sp);
+
+  /// Utility constructor that initializes the trace with a provided error.
+  DecodedThread(lldb::ThreadSP thread_sp, llvm::Error &&err);
 
   /// Get the instructions from the decoded trace. Some of them might indicate
-  /// errors (i.e. gaps) in the trace.
+  /// errors (i.e. gaps) in the trace. For an instruction error, you can access
+  /// its underlying error message with the \a GetErrorByInstructionIndex()
+  /// method.
   ///
   /// \return
   ///   The instructions of the trace.
   llvm::ArrayRef<IntelPTInstruction> GetInstructions() const;
 
+  /// Get the error associated with a given instruction index.
+  ///
+  /// \return
+  ///   The error message of \b nullptr if the given index
+  ///   points to a valid instruction.
+  const char *GetErrorByInstructionIndex(uint64_t ins_idx);
+
+  /// Append a successfully decoded instruction.
+  template <typename... Ts> void AppendInstruction(Ts... instruction_args) {
+    m_instructions.emplace_back(instruction_args...);
+  }
+
+  /// Append a decoding error (i.e. an instruction that failed to be decoded).
+  void AppendError(llvm::Error &&error);
+
   /// Get a new cursor for the decoded thread.
   lldb::TraceCursorUP GetCursor();
 
-  /// Get the size in bytes of the corresponding Intel PT raw trace
+  /// Set the size in bytes of the corresponding Intel PT raw trace.
+  void SetRawTraceSize(size_t size);
+
+  /// Get the size in bytes of the corresponding Intel PT raw trace.
   ///
   /// \return
-  ///   The size of the trace.
-  size_t GetRawTraceSize() const;
+  ///   The size of the trace, or \b llvm::None if not available.
+  llvm::Optional<size_t> GetRawTraceSize() const;
 
-  /// The approximate size in bytes used by this instance, 
+  /// The approximate size in bytes used by this instance,
   /// including all the already decoded instructions.
   size_t CalculateApproximateMemoryUsage() const;
 
+  lldb::ThreadSP GetThread();
+
 private:
-  /// When adding new members to this class, make sure 
+  /// When adding new members to this class, make sure
   /// to update \a CalculateApproximateMemoryUsage() accordingly.
   lldb::ThreadSP m_thread_sp;
   std::vector<IntelPTInstruction> m_instructions;
-  size_t m_raw_trace_size;
+  llvm::DenseMap<uint64_t, std::string> m_errors;
+  llvm::Optional<size_t> m_raw_trace_size;
 };
 
 using DecodedThreadSP = std::shared_ptr<DecodedThread>;
 
 } // namespace trace_intel_pt
 } // namespace lldb_private
 
 #endif // LLDB_SOURCE_PLUGINS_TRACE_INTEL_PT_DECODEDTHREAD_H

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 <intel-pt.h>
 #include <memory>
 
 #include "TraceCursorIntelPT.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;
 }
 
-IntelPTInstruction::IntelPTInstruction(llvm::Error err) {
-  llvm::handleAllErrors(std::move(err),
-                        [&](std::unique_ptr<llvm::ErrorInfoBase> info) {
-                          m_error = std::move(info);
-                        });
+IntelPTInstruction::IntelPTInstruction() {
   m_pt_insn.ip = LLDB_INVALID_ADDRESS;
   m_pt_insn.iclass = ptic_error;
+  m_is_error = true;
 }
 
-bool IntelPTInstruction::IsError() const { return (bool)m_error; }
+bool IntelPTInstruction::IsError() const { return m_is_error; }
 
 lldb::addr_t IntelPTInstruction::GetLoadAddress() const { return m_pt_insn.ip; }
 
-size_t IntelPTInstruction::GetNonErrorMemoryUsage() { return sizeof(IntelPTInstruction); }
+size_t IntelPTInstruction::GetMemoryUsage() {
+  return sizeof(IntelPTInstruction);
+}
 
 Optional<uint64_t> IntelPTInstruction::GetTimestampCounter() const {
   return m_timestamp;
 }
 
-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());
+Optional<size_t> DecodedThread::GetRawTraceSize() const {
+  return m_raw_trace_size;
 }
-size_t DecodedThread::GetRawTraceSize() const { return m_raw_trace_size; }
 
 TraceInstructionControlFlowType
 IntelPTInstruction::GetControlFlowType(lldb::addr_t next_load_address) const {
   if (IsError())
     return (TraceInstructionControlFlowType)0;
 
   TraceInstructionControlFlowType mask =
       eTraceInstructionControlFlowTypeInstruction;
case ptic_far_call:
     break;
   default:
     break;
   }
 
   return mask;
 }
 
+ThreadSP DecodedThread::GetThread() { return m_thread_sp; }
+
+void DecodedThread::AppendError(llvm::Error &&error) {
+  m_errors.try_emplace(m_instructions.size(), toString(std::move(error)));
+  m_instructions.emplace_back();
+}
+
 ArrayRef<IntelPTInstruction> DecodedThread::GetInstructions() const {
   return makeArrayRef(m_instructions);
 }
 
-DecodedThread::DecodedThread(ThreadSP thread_sp, Error error)
-    : m_thread_sp(thread_sp) {
-  m_instructions.emplace_back(std::move(error));
+const char *DecodedThread::GetErrorByInstructionIndex(uint64_t idx) {
+  auto it = m_errors.find(idx);
+  if (it == m_errors.end())
+    return nullptr;
+
+  return it->second.c_str();
 }
 
-DecodedThread::DecodedThread(ThreadSP thread_sp,
-                             std::vector<IntelPTInstruction> &&instructions,
-                             size_t raw_trace_size)
-    : m_thread_sp(thread_sp), m_instructions(std::move(instructions)),
-      m_raw_trace_size(raw_trace_size) {
-  if (m_instructions.empty())
-    m_instructions.emplace_back(
-        createStringError(inconvertibleErrorCode(), "empty trace"));
+DecodedThread::DecodedThread(ThreadSP thread_sp) : m_thread_sp(thread_sp) {}
+
+DecodedThread::DecodedThread(ThreadSP thread_sp, Error &&error)
+    : m_thread_sp(thread_sp) {
+  AppendError(std::move(error));
 }
 
+void DecodedThread::SetRawTraceSize(size_t size) { m_raw_trace_size = size; }
+
 lldb::TraceCursorUP DecodedThread::GetCursor() {
+  // We insert a fake error signaling an empty trace if needed becasue the
+  // TraceCursor requires non-empty traces.
+  if (m_instructions.empty())
+    AppendError(createStringError(inconvertibleErrorCode(), "empty trace"));
   return std::make_unique<TraceCursorIntelPT>(m_thread_sp, shared_from_this());
 }
 
 size_t DecodedThread::CalculateApproximateMemoryUsage() const {
-  return m_raw_trace_size
-    + IntelPTInstruction::GetNonErrorMemoryUsage() * m_instructions.size()
-    + sizeof(DecodedThread);
+  return m_raw_trace_size.getValueOr(0) +
+         IntelPTInstruction::GetMemoryUsage() * m_instructions.size() +
+         m_errors.getMemorySize();
 }

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

 
 #include "../common/ThreadPostMortemTrace.h"
 #include "DecodedThread.h"
 #include "TraceIntelPT.h"
 #include "lldb/Core/Module.h"
 #include "lldb/Core/Section.h"
 #include "lldb/Target/Target.h"
 #include "lldb/Utility/StringExtractor.h"
+#include <utility>
 
 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).

 /// https://github.com/intel/libipt/blob/master/doc/howto_libipt.md#the-instruction-flow-decode-loop
 /// but with some relaxation to allow for gaps in the trace.
 ///
 /// 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;
-
+static void DecodeInstructions(pt_insn_decoder &decoder,
+                               DecodedThreadSP &decoded_thread_sp) {
   while (true) {
     int errcode = FindNextSynchronizationPoint(decoder);
     if (errcode == -pte_eos)
       break;
 
     if (errcode < 0) {
-      instructions.emplace_back(make_error<IntelPTError>(errcode));
+      decoded_thread_sp->AppendError(make_error<IntelPTError>(errcode));
       break;
     }
 
     // We have synchronized, so we can start decoding
     // instructions and events.
     while (true) {
       errcode = ProcessPTEvents(decoder, errcode);
       if (errcode < 0) {
-        instructions.emplace_back(make_error<IntelPTError>(errcode));
+        decoded_thread_sp->AppendError(make_error<IntelPTError>(errcode));
         break;
       }
-      pt_insn insn;
 
+      pt_insn insn;
       errcode = pt_insn_next(&decoder, &insn, sizeof(insn));
       if (errcode == -pte_eos)
         break;
 
       if (errcode < 0) {
-        instructions.emplace_back(make_error<IntelPTError>(errcode, insn.ip));
+        decoded_thread_sp->AppendError(
+            make_error<IntelPTError>(errcode, insn.ip));
         break;
       }
 
       uint64_t time;
       int time_error = pt_insn_time(&decoder, &time, nullptr, nullptr);
       if (time_error == -pte_invalid) {
         // This happens if we invoke the pt_insn_time method incorrectly,
         // but the instruction is good though.
-        instructions.emplace_back(
+        decoded_thread_sp->AppendError(
             make_error<IntelPTError>(time_error, insn.ip));
-        instructions.emplace_back(insn);
+        decoded_thread_sp->AppendInstruction(insn);
         break;
       }
+
       if (time_error == -pte_no_time) {
         // We simply don't have time information, i.e. None of TSC, MTC or CYC
         // was enabled.
-        instructions.emplace_back(insn);
+        decoded_thread_sp->AppendInstruction(insn);
       } else {
-        instructions.emplace_back(insn, time);
+        decoded_thread_sp->AppendInstruction(insn, time);
       }
     }
   }
-
-  return instructions;
 }
 
 /// Callback used by libipt for reading the process memory.
 ///
 /// More information can be found in
 /// https://github.com/intel/libipt/blob/master/doc/man/pt_image_set_callback.3.md
 static int ReadProcessMemory(uint8_t *buffer, size_t size,
                              const pt_asid * /* unused */, uint64_t pc,
                              void *context) {
   Process *process = static_cast<Process *>(context);
 
   Status error;
   int bytes_read = process->ReadMemory(pc, buffer, size, error);
   if (error.Fail())
     return -pte_nomap;
   return bytes_read;
 }
 
-static Expected<std::vector<IntelPTInstruction>>
-DecodeInMemoryTrace(Process &process, TraceIntelPT &trace_intel_pt,
+static void DecodeInMemoryTrace(DecodedThreadSP &decoded_thread_sp,
+                                TraceIntelPT &trace_intel_pt,
                                 MutableArrayRef<uint8_t> buffer) {
   Expected<pt_cpu> cpu_info = trace_intel_pt.GetCPUInfo();
-  if (!cpu_info)
-    return cpu_info.takeError();
+  if (!cpu_info) {
+    return decoded_thread_sp->AppendError(cpu_info.takeError());
+  }
 
   pt_config config;
   pt_config_init(&config);
   config.cpu = *cpu_info;
 
   if (int errcode = pt_cpu_errata(&config.errata, &config.cpu))
-    return make_error<IntelPTError>(errcode);
+    return decoded_thread_sp->AppendError(make_error<IntelPTError>(errcode));
 
   config.begin = buffer.data();
   config.end = buffer.data() + buffer.size();
 
   pt_insn_decoder *decoder = pt_insn_alloc_decoder(&config);
   if (!decoder)
-    return make_error<IntelPTError>(-pte_nomem);
+    return decoded_thread_sp->AppendError(make_error<IntelPTError>(-pte_nomem));
 
   pt_image *image = pt_insn_get_image(decoder);
 
-  int errcode = pt_image_set_callback(image, ReadProcessMemory, &process);
+  int errcode =
+      pt_image_set_callback(image, ReadProcessMemory,
+                            decoded_thread_sp->GetThread()->GetProcess().get());
   assert(errcode == 0);
   (void)errcode;
 
-  std::vector<IntelPTInstruction> instructions = DecodeInstructions(*decoder);
-
+  DecodeInstructions(*decoder, decoded_thread_sp);
   pt_insn_free_decoder(decoder);
-  return instructions;
 }
+// ---------------------------
 
-static Expected<std::vector<IntelPTInstruction>>
-DecodeTraceFile(Process &process, TraceIntelPT &trace_intel_pt,
-                const FileSpec &trace_file, size_t &raw_trace_size) {
-  ErrorOr<std::unique_ptr<MemoryBuffer>> trace_or_error =
-      MemoryBuffer::getFile(trace_file.GetPath());
-  if (std::error_code err = trace_or_error.getError())
-    return errorCodeToError(err);
-
-  MemoryBuffer &trace = **trace_or_error;
-  MutableArrayRef<uint8_t> trace_data(
-      // The libipt library does not modify the trace buffer, hence the
-      // following cast is safe.
-      reinterpret_cast<uint8_t *>(const_cast<char *>(trace.getBufferStart())),
-      trace.getBufferSize());
-  raw_trace_size = trace_data.size();
-  return DecodeInMemoryTrace(process, trace_intel_pt, trace_data);
+DecodedThreadSP ThreadDecoder::Decode() {
+  if (!m_decoded_thread.hasValue())
+    m_decoded_thread = DoDecode();
+  return *m_decoded_thread;
 }
 
-static Expected<std::vector<IntelPTInstruction>>
-DecodeLiveThread(Thread &thread, TraceIntelPT &trace, size_t &raw_trace_size) {
+// LiveThreadDecoder ====================
+
+LiveThreadDecoder::LiveThreadDecoder(Thread &thread, TraceIntelPT &trace)
+    : m_thread_sp(thread.shared_from_this()), m_trace(trace) {}
+
+DecodedThreadSP LiveThreadDecoder::DoDecode() {
+  DecodedThreadSP decoded_thread_sp =
+      std::make_shared<DecodedThread>(m_thread_sp);
+
   Expected<std::vector<uint8_t>> buffer =
-      trace.GetLiveThreadBuffer(thread.GetID());
-  if (!buffer)
-    return buffer.takeError();
-  raw_trace_size = buffer->size();
-  if (Expected<pt_cpu> cpu_info = trace.GetCPUInfo())
-    return DecodeInMemoryTrace(*thread.GetProcess(), trace,
-                               MutableArrayRef<uint8_t>(*buffer));
-  else
-    return cpu_info.takeError();
+      m_trace.GetLiveThreadBuffer(m_thread_sp->GetID());
+  if (!buffer) {
+    decoded_thread_sp->AppendError(buffer.takeError());
+    return decoded_thread_sp;
   }
 
-DecodedThreadSP ThreadDecoder::Decode() {
-  if (!m_decoded_thread.hasValue())
-    m_decoded_thread = DoDecode();
-  return *m_decoded_thread;
+  decoded_thread_sp->SetRawTraceSize(buffer->size());
+  DecodeInMemoryTrace(decoded_thread_sp, m_trace,
+                      MutableArrayRef<uint8_t>(*buffer));
+  return decoded_thread_sp;
 }
 
+// PostMortemThreadDecoder =======================
+
 PostMortemThreadDecoder::PostMortemThreadDecoder(
     const lldb::ThreadPostMortemTraceSP &trace_thread, TraceIntelPT &trace)
     : m_trace_thread(trace_thread), m_trace(trace) {}
 
 DecodedThreadSP PostMortemThreadDecoder::DoDecode() {
-  size_t raw_trace_size = 0;
-  if (Expected<std::vector<IntelPTInstruction>> instructions =
-          DecodeTraceFile(*m_trace_thread->GetProcess(), m_trace,
-                          m_trace_thread->GetTraceFile(), raw_trace_size))
-    return std::make_shared<DecodedThread>(m_trace_thread->shared_from_this(),
-                                           std::move(*instructions),
-                                           raw_trace_size);
-  else
-    return std::make_shared<DecodedThread>(m_trace_thread->shared_from_this(),
-                                           instructions.takeError());
+  DecodedThreadSP decoded_thread_sp =
+      std::make_shared<DecodedThread>(m_trace_thread);
+
+  ErrorOr<std::unique_ptr<MemoryBuffer>> trace_or_error =
+      MemoryBuffer::getFile(m_trace_thread->GetTraceFile().GetPath());
+  if (std::error_code err = trace_or_error.getError()) {
+    decoded_thread_sp->AppendError(errorCodeToError(err));
+    return decoded_thread_sp;
   }
 
-LiveThreadDecoder::LiveThreadDecoder(Thread &thread, TraceIntelPT &trace)
-    : m_thread_sp(thread.shared_from_this()), m_trace(trace) {}
+  MemoryBuffer &trace = **trace_or_error;
+  MutableArrayRef<uint8_t> trace_data(
+      // The libipt library does not modify the trace buffer, hence the
+      // following cast is safe.
+      reinterpret_cast<uint8_t *>(const_cast<char *>(trace.getBufferStart())),
+      trace.getBufferSize());
+  decoded_thread_sp->SetRawTraceSize(trace_data.size());
 
-DecodedThreadSP LiveThreadDecoder::DoDecode() {
-  size_t raw_trace_size = 0;
-  if (Expected<std::vector<IntelPTInstruction>> instructions =
-          DecodeLiveThread(*m_thread_sp, m_trace, raw_trace_size))
-    return std::make_shared<DecodedThread>(
-        m_thread_sp, std::move(*instructions), raw_trace_size);
-  else
-    return std::make_shared<DecodedThread>(m_thread_sp,
-                                           instructions.takeError());
+  DecodeInMemoryTrace(decoded_thread_sp, m_trace, trace_data);
+  return decoded_thread_sp;
 }

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

 public:
   TraceCursorIntelPT(lldb::ThreadSP thread_sp,
                      DecodedThreadSP decoded_thread_sp);
 
   size_t Seek(int64_t offset, SeekType origin) override;
 
   virtual bool Next() override;
 
-  llvm::Error GetError() override;
+  const char *GetError() override;
 
   lldb::addr_t GetLoadAddress() override;
 
   llvm::Optional<uint64_t> GetCounter(lldb::TraceCounter counter_type) override;
 
   lldb::TraceInstructionControlFlowType
   GetInstructionControlFlowType() override;
 

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

case TraceCursor::SeekType::Current:
     int64_t new_pos = fitPosToBounds(offset + m_pos);
     int64_t dist = m_pos - new_pos;
     m_pos = new_pos;
     return std::abs(dist);
   }
 }
 
 bool TraceCursorIntelPT::IsError() {
   return m_decoded_thread_sp->GetInstructions()[m_pos].IsError();
 }
 
-Error TraceCursorIntelPT::GetError() {
-  return m_decoded_thread_sp->GetInstructions()[m_pos].ToError();
+const char *TraceCursorIntelPT::GetError() {
+  return m_decoded_thread_sp->GetErrorByInstructionIndex(m_pos);
 }
 
 lldb::addr_t TraceCursorIntelPT::GetLoadAddress() {
   return m_decoded_thread_sp->GetInstructions()[m_pos].GetLoadAddress();
 }
 
 Optional<uint64_t> TraceCursorIntelPT::GetCounter(lldb::TraceCounter counter_type) {
   switch (counter_type) {

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

void TraceIntelPT::DumpTraceInfo(Thread &thread, Stream &s, bool verbose) {
   Optional<size_t> raw_size = GetRawTraceSize(thread);
   s.Printf("\nthread #%u: tid = %" PRIu64, thread.GetIndexID(), thread.GetID());
   if (!raw_size) {
     s.Printf(", not traced\n");
     return;
   }
   s.Printf("\n");
 
+  size_t insn_len = Decode(thread)->GetInstructions().size();
   size_t mem_used = Decode(thread)->CalculateApproximateMemoryUsage();
+
   s.Printf("  Raw trace size: %zu KiB\n", *raw_size / 1024);
-  s.Printf("  Total number of instructions: %zu\n",
-           Decode(thread)->GetInstructions().size());
+  s.Printf("  Total number of instructions: %zu\n", insn_len);
   s.Printf("  Total approximate memory usage: %0.2lf KiB\n",
            (double)mem_used / 1024);
+  s.Printf("  Average memory usage per instruction: %zu bytes\n",
+           mem_used / insn_len);
   return;
 }
 
 Optional<size_t> TraceIntelPT::GetRawTraceSize(Thread &thread) {
   if (IsTraced(thread.GetID()))
     return Decode(thread)->GetRawTraceSize();
   else
     return None;

lldb/source/Target/TraceInstructionDumper.cpp

void TraceInstructionDumper::DumpInstructions(Stream &s, size_t count) {
   };
 
   for (size_t i = 0; i < count; i++) {
     if (!HasMoreData()) {
       s.Printf("    no more data\n");
       break;
     }
 
-    if (Error err = m_cursor_up->GetError()) {
+    if (const char *err = m_cursor_up->GetError()) {
       if (!m_cursor_up->IsForwards() && !was_prev_instruction_an_error)
         printMissingInstructionsMessage();
 
       was_prev_instruction_an_error = true;
 
       printInstructionIndex();
-      s << toString(std::move(err));
+      s << err;
     } else {
       if (m_cursor_up->IsForwards() && was_prev_instruction_an_error)
         printMissingInstructionsMessage();
 
       was_prev_instruction_an_error = false;
 
       InstructionSymbolInfo insn_info;
 

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

def testDumpRawTraceSize(self):
         substrs=["intel-pt"])
 
         self.expect("thread trace dump info",
             substrs=['''Trace technology: intel-pt
 
 thread #1: tid = 3842849
   Raw trace size: 4 KiB
   Total number of instructions: 21
-  Total approximate memory usage: 5.38 KiB'''])
+  Total approximate memory usage: 5.31 KiB
+  Average memory usage per instruction: 259 bytes'''])

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

def testLoadTrace(self):
         # check that the Process and Thread objects were created correctly
         self.expect("thread info", substrs=["tid = 3842849"])
         self.expect("thread list", substrs=["Process 1234 stopped", "tid = 3842849"])
         self.expect("thread trace dump info", substrs=['''Trace technology: intel-pt
 
 thread #1: tid = 3842849
   Raw trace size: 4 KiB
   Total number of instructions: 21
-  Total approximate memory usage: 5.38 KiB'''])
+  Total approximate memory usage: 5.31 KiB
+  Average memory usage per instruction: 259 bytes'''])
 
     def testLoadInvalidTraces(self):
         src_dir = self.getSourceDir()
         # We test first an invalid type
         self.expect("trace load -v " + os.path.join(src_dir, "intelpt-trace", "trace_bad.json"), error=True,
           substrs=['''error: expected object at traceSession.processes[0]
 
 Context: