[Lldb-commits] [PATCH] D122991: [lldb][intelpt] Remove `IntelPTInstruction` and move methods to `DecodedThread`

[walter erquinigo via Phabricator via lldb-commits]

wallace requested changes to this revision.
wallace added a comment.
This revision now requires changes to proceed.

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



================
Comment at: lldb/source/Plugins/Trace/intel-pt/DecodedThread.cpp:46-47
+
+TraceInstructionControlFlowType DecodedThread::GetInstructionControlFlowType(
+    size_t insn_index, lldb::addr_t next_load_address) const {
+  if (IsInstructionAnError(insn_index))
----------------



================
Comment at: lldb/source/Plugins/Trace/intel-pt/DecodedThread.cpp:54-55
 
-  switch (m_pt_insn.iclass) {
+  lldb::addr_t ip = m_instruction_ips[insn_index];
+  uint8_t size = m_instruction_sizes[insn_index];
+  pt_insn_class iclass = m_instruction_classes[insn_index];
----------------
let's give better names to these variables


================
Comment at: lldb/source/Plugins/Trace/intel-pt/DecodedThread.cpp:62
     mask |= eTraceInstructionControlFlowTypeBranch;
-    if (m_pt_insn.ip + m_pt_insn.size != next_load_address)
+    if (ip + size != next_load_address)
       mask |= eTraceInstructionControlFlowTypeTakenBranch;
----------------
we can stop using next_load_address because we now have access to the entire trace


================
Comment at: lldb/source/Plugins/Trace/intel-pt/DecodedThread.cpp:176
+         sizeof(pt_insn::iclass) * m_instruction_classes.size() +
          m_errors.getMemorySize();
 }
----------------
you also need to include `m_instruction_timestamps`, probably `m_instruction_timestamps.size() * (sizeof(size_t) + sizeof(uint64_t))`


================
Comment at: lldb/source/Plugins/Trace/intel-pt/DecodedThread.h:127
+  /// Append a decoding error with a corresponding TSC.
+  void AppendError(llvm::Error &&error, uint64_t TSC);
+
----------------
lowercase tsc because it's a variable


================
Comment at: lldb/source/Plugins/Trace/intel-pt/DecodedThread.h:129-136
+  /// 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.
----------------
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


================
Comment at: lldb/source/Plugins/Trace/intel-pt/DecodedThread.h:139
+  /// \return
+  ///     The instruction pointer address, or \a LLDB_INVALID_ADDRESS if it is
+  ///     an error.
----------------



================
Comment at: lldb/source/Plugins/Trace/intel-pt/DecodedThread.h:143-154
+  /// 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.
+  ///
----------------
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`


================
Comment at: lldb/source/Plugins/Trace/intel-pt/DecodedThread.h:193-199
+  /// Record an error decoding a TSC timestamp.
+  ///
+  /// See \a GetTscErrors() for more documentation.
+  ///
+  /// \param[in] libipt_error_code
+  ///   An error returned by the libipt library.
+  void RecordTscError(int libipt_error_code);
----------------
+1


================
Comment at: lldb/source/Plugins/Trace/intel-pt/DecodedThread.h:218
+  std::vector<lldb::addr_t> m_instruction_ips;
+  /// The low level storage of all instruction sizes.
+  std::vector<uint8_t> m_instruction_sizes;
----------------



================
Comment at: lldb/source/Plugins/Trace/intel-pt/DecodedThread.h:220
+  std::vector<uint8_t> m_instruction_sizes;
+  /// The low level storage of all instruction classes.
+  std::vector<pt_insn_class> m_instruction_classes;
----------------



================
Comment at: lldb/source/Plugins/Trace/intel-pt/TraceCursorIntelPT.cpp:23-30
+  assert(!m_decoded_thread_sp->GetInstructionPointers().empty() &&
          "a trace should have at least one instruction or error");
-  m_pos = m_decoded_thread_sp->GetInstructions().size() - 1;
+  m_pos = m_decoded_thread_sp->GetInstructionPointers().size() - 1;
   m_tsc_range = m_decoded_thread_sp->CalculateTscRange(m_pos);
 }
 
 size_t TraceCursorIntelPT::GetInternalInstructionSize() {
----------------



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122991