[lldb][intelpt] Remove `IntelPTInstruction` and move methods to `DecodedThread`

[zrthxn]

Summary

This is to reduce the size of the trace further and has appreciable results.

[zrthxn]

Change in memory use is appreciable.

thread #1: tid = 40371
  Raw trace size: 4 KiB
  Total number of instructions: 145011
  Total approximate memory usage: 1840.96 KiB
  Average memory usage per instruction: 13.00 bytes

  Number of TSC decoding errors: 0

Tests still need to be updated.

[zrthxn]

Updated tests

[wallace]

only mostly cosmetic changes needed. Thanks for this. I'm glad that we are bringing the usage down

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

46–47

54–56

let's give better names to these variables

60

we can stop using next_load_address because we now have access to the entire trace

167

you also need to include m_instruction_timestamps, probably m_instruction_timestamps.size() * (sizeof(size_t) + sizeof(uint64_t))

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

127

lowercase tsc because it's a variable

129–136

I regret having created this method because it forces us to have a certain interface for the storage of the instructions. Let's just delete this method and create one new method

size_t GetInstructionsCount() const;

which should be able to replace all the external calls to this method

136

140–151

Now that DecodedThread knows everything, we can simplify this method a bit. First, we can remove the parameter next_load_address, because it can easily be gotten by checking the instruction at index insn_index + 1

201–207

+1

218

220

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

23–30

[zrthxn]

Address all comments

[wallace]

just remove a small comment and good to go!

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

143–146

remove this

[zrthxn]

Removed unnecessary comment

[wallace]

great job!

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

public:
 
   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)
-      : m_pt_insn(pt_insn), m_is_error(false) {}
-
-  /// Error constructor
-  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 an instance of this class
-  static size_t GetMemoryUsage();
-
-  /// Get the \a lldb::TraceInstructionControlFlowType categories of the
-  /// instruction.
-  ///
-  /// \param[in] next_load_address
-  ///     The address of the next instruction in the trace or \b
-  ///     LLDB_INVALID_ADDRESS if not available.
-  ///
-  /// \return
-  ///     The control flow categories, or \b 0 if the instruction is an error.
-  lldb::TraceInstructionControlFlowType
-  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
-  // IntelPTInstruction::GetMemoryUsage() if needed.
-  pt_insn m_pt_insn;
-  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:
   void AppendInstruction(const pt_insn &instruction);
 
   /// Append a sucessfully decoded instruction with an associated TSC timestamp.
   void AppendInstruction(const pt_insn &instruction, uint64_t tsc);
 
   /// Append a decoding error (i.e. an instruction that failed to be decoded).
   void AppendError(llvm::Error &&error);
 
-  /// Get the instructions from the decoded trace. Some of them might indicate
-  /// errors (i.e. gaps) in the trace. For an instruction error, you can access
-  /// its underlying error message with the \a GetErrorByInstructionIndex()
-  /// method.
+  /// Get the instruction pointers from the decoded trace. Some of them might
+  /// indicate 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;
+  llvm::ArrayRef<const pt_insn *> GetInstructions() const;
+
+  /// \return
+  ///     The instruction pointer address, or \a LLDB_INVALID_ADDRESS if it is
+  ///     an error.
+  lldb::addr_t GetInstructionLoadAddress(size_t insn_index) const;
+
+  /// Get the \a lldb::TraceInstructionControlFlowType categories of the
+  /// instruction.
+  ///
+  /// \param[in] next_load_address
+  ///     The address of the next instruction in the trace or \b
+  ///     LLDB_INVALID_ADDRESS if not available.
+  ///
+  /// \return
+  ///     The control flow categories, or \b 0 if the instruction is an error.
+  lldb::TraceInstructionControlFlowType
+  GetInstructionControlFlowType(size_t insn_index,
+                                lldb::addr_t next_load_address) const;
 
   /// Construct the TSC range that covers the given instruction index.
   /// This operation is O(logn) and should be used sparingly.
   /// If the trace was collected with TSC support, all the instructions of
   /// the trace will have associated TSCs. This means that this method will
   /// only return \b llvm::None if there are no TSCs whatsoever in the trace.
   llvm::Optional<TscRange> CalculateTscRange(size_t insn_index) const;
 
public:
   /// Return the number of TSC decoding errors that happened. A TSC error
   /// is not a fatal error and doesn't create gaps in the trace. Instead
   /// we only keep track of them as a statistic.
   ///
   /// \return
   ///   The number of TSC decoding errors.
   const LibiptErrors &GetTscErrors() const;
 
   /// The approximate size in bytes used by this instance,
   /// including all the already decoded instructions.
   size_t CalculateApproximateMemoryUsage() const;
 
   lldb::ThreadSP GetThread();
 
 private:
   /// Notify this class that the last added instruction or error has
   /// an associated TSC.
   void RecordTscForLastInstruction(uint64_t tsc);
 
   /// When adding new members to this class, make sure
   /// to update \a CalculateApproximateMemoryUsage() accordingly.
   lldb::ThreadSP m_thread_sp;
   /// The low level storage of all instruction addresses. Each instruction has
   /// an index in this vector and it will be used in other parts of the code.
-  std::vector<IntelPTInstruction> m_instructions;
+  std::vector<const pt_insn *> m_instructions;
   /// This map contains the TSCs of the decoded instructions. It maps
   /// `instruction index -> TSC`, where `instruction index` is the first index
   /// at which the mapped TSC appears. We use this representation because TSCs
   /// are sporadic and we can think of them as ranges. If TSCs are present in
   /// the trace, all instructions will have an associated TSC, including the
   /// first one. Otherwise, this map will be empty.
   std::map<size_t, uint64_t> m_instruction_timestamps;
   /// This is the chronologically last TSC that has been added.
   llvm::Optional<uint64_t> m_last_tsc = llvm::None;
   // This variables stores the messages of all the error instructions in the
   // trace. It maps `instruction index -> error message`.

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

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() {
-  m_pt_insn.ip = LLDB_INVALID_ADDRESS;
-  m_pt_insn.iclass = ptic_error;
-  m_is_error = true;
-}
-
-bool IntelPTInstruction::IsError() const { return m_is_error; }
-
-lldb::addr_t IntelPTInstruction::GetLoadAddress() const { return m_pt_insn.ip; }
-
-size_t IntelPTInstruction::GetMemoryUsage() {
-  return sizeof(IntelPTInstruction);
-}
-
 Optional<size_t> DecodedThread::GetRawTraceSize() const {
   return m_raw_trace_size;
 }
 
-TraceInstructionControlFlowType
-IntelPTInstruction::GetControlFlowType(lldb::addr_t next_load_address) const {
-  if (IsError())
+lldb::addr_t DecodedThread::GetInstructionLoadAddress(size_t insn_index) const {
+  m_instructions[insn_index]->ip;
+}
+
+TraceInstructionControlFlowType DecodedThread::GetInstructionControlFlowType(
+    size_t insn_index, lldb::addr_t next_load_address) const {
+  if (IsInstructionAnError(insn_index))
     return (TraceInstructionControlFlowType)0;
 
   TraceInstructionControlFlowType mask =
       eTraceInstructionControlFlowTypeInstruction;
 
-  switch (m_pt_insn.iclass) {
+  pt_insn insn = *m_instructions[insn_index];
+  switch (insn.iclass) {
   case ptic_cond_jump:
   case ptic_jump:
   case ptic_far_jump:
     mask |= eTraceInstructionControlFlowTypeBranch;
-    if (m_pt_insn.ip + m_pt_insn.size != next_load_address)
+    if (insn.ip + insn.size != next_load_address)
       mask |= eTraceInstructionControlFlowTypeTakenBranch;
     break;
   case ptic_return:
   case ptic_far_return:
     mask |= eTraceInstructionControlFlowTypeReturn;
     break;
   case ptic_call:
   case ptic_far_call:
if (!m_last_tsc || *m_last_tsc != tsc) {
     size_t start_index =
         m_instruction_timestamps.empty() ? 0 : m_instructions.size() - 1;
     m_instruction_timestamps.emplace(start_index, tsc);
     m_last_tsc = tsc;
   }
 }
 
 void DecodedThread::AppendInstruction(const pt_insn &insn) {
-  m_instructions.emplace_back(insn);
+  m_instructions.emplace_back(&insn);
 }
 
 void DecodedThread::AppendInstruction(const pt_insn &insn, uint64_t tsc) {
   AppendInstruction(insn);
   RecordTscForLastInstruction(tsc);
 }
 
 void DecodedThread::AppendError(llvm::Error &&error) {

 void DecodedThread::RecordTscError(int libipt_error_code) {
   m_tsc_errors.RecordError(libipt_error_code);
 }
 
 const DecodedThread::LibiptErrors &DecodedThread::GetTscErrors() const {
   return m_tsc_errors;
 }
 
-ArrayRef<IntelPTInstruction> DecodedThread::GetInstructions() const {
+ArrayRef<const pt_insn *> DecodedThread::GetInstructions() const {
   return makeArrayRef(m_instructions);
 }
 
 Optional<DecodedThread::TscRange>
 DecodedThread::CalculateTscRange(size_t insn_index) const {
   auto it = m_instruction_timestamps.upper_bound(insn_index);
   if (it == m_instruction_timestamps.begin())
     return None;
 
   return TscRange(--it, *this);
 }
 
 bool DecodedThread::IsInstructionAnError(size_t insn_idx) const {
-  return m_instructions[insn_idx].IsError();
+  return m_instructions[insn_idx]->ip == LLDB_INVALID_ADDRESS;
 }
 
 const char *DecodedThread::GetErrorByInstructionIndex(size_t insn_idx) {
   auto it = m_errors.find(insn_idx);
   if (it == m_errors.end())
     return nullptr;
 
   return it->second.c_str();
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 IntelPTInstruction::GetMemoryUsage() * m_instructions.size() +
-         m_errors.getMemorySize();
+  return sizeof(pt_insn *) * m_instructions.size() + m_errors.getMemorySize();
 }
 
 DecodedThread::TscRange::TscRange(std::map<size_t, uint64_t>::const_iterator it,
                                   const DecodedThread &decoded_thread)
     : m_it(it), m_decoded_thread(&decoded_thread) {
   auto next_it = m_it;
   ++next_it;
   m_end_index = (next_it == m_decoded_thread->m_instruction_timestamps.end())

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

 using namespace lldb;
 using namespace lldb_private;
 using namespace lldb_private::trace_intel_pt;
 using namespace llvm;
 
 TraceCursorIntelPT::TraceCursorIntelPT(ThreadSP thread_sp,
                                        DecodedThreadSP decoded_thread_sp)
     : TraceCursor(thread_sp), m_decoded_thread_sp(decoded_thread_sp) {
   assert(!m_decoded_thread_sp->GetInstructions().empty() &&
          "a trace should have at least one instruction or error");
   m_pos = m_decoded_thread_sp->GetInstructions().size() - 1;
   m_tsc_range = m_decoded_thread_sp->CalculateTscRange(m_pos);
 }
 
 size_t TraceCursorIntelPT::GetInternalInstructionSize() {
   return m_decoded_thread_sp->GetInstructions().size();
 }
 
 bool TraceCursorIntelPT::Next() {
   auto canMoveOne = [&]() {
     if (IsForwards())
       return m_pos + 1 < GetInternalInstructionSize();
     return m_pos > 0;
   };
case TraceCursor::SeekType::End:
       return last_index - m_pos;
     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);
     }
   };
-  
-  int64_t dist = FindDistanceAndSetPos();
+  int64_t dist = FindDistanceAndSetPos();
   m_tsc_range = m_decoded_thread_sp->CalculateTscRange(m_pos);
   return dist;
 }
 
 bool TraceCursorIntelPT::IsError() {
   return m_decoded_thread_sp->IsInstructionAnError(m_pos);
 }
 
 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();
+  return m_decoded_thread_sp->GetInstructionLoadAddress(m_pos);
 }
 
 Optional<uint64_t>
 TraceCursorIntelPT::GetCounter(lldb::TraceCounter counter_type) {
   switch (counter_type) {
   case lldb::eTraceCounterTSC:
     if (m_tsc_range)
       return m_tsc_range->GetTsc();
     else
       return llvm::None;
   }
 }
 
 TraceInstructionControlFlowType
 TraceCursorIntelPT::GetInstructionControlFlowType() {
   lldb::addr_t next_load_address =
       m_pos + 1 < GetInternalInstructionSize()
-          ? m_decoded_thread_sp->GetInstructions()[m_pos + 1].GetLoadAddress()
+          ? m_decoded_thread_sp->GetInstructionLoadAddress(m_pos + 1)
           : LLDB_INVALID_ADDRESS;
-  return m_decoded_thread_sp->GetInstructions()[m_pos].GetControlFlowType(
+  return m_decoded_thread_sp->GetInstructionControlFlowType(m_pos,
                                                             next_load_address);
 }