[Lldb-commits] [PATCH] D122293: [wip][intelpt] Refactoring instruction decoding for flexibility

[walter erquinigo via Phabricator via lldb-commits]

wallace added inline comments.


================
Comment at: lldb/source/Plugins/Trace/intel-pt/DecodedThread.cpp:103-105
+// llvm::Error DecodedThread::GetError(uint64_t idx) const {
+//   return m_errors.at(idx);
+// }
----------------
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


================
Comment at: lldb/source/Plugins/Trace/intel-pt/DecodedThread.h:148-151
+  /// Get the error at some instruction index from the decoded trace.
+  ///
+  /// \return
+  ///   The error of the trace.
----------------
we can make the documentation clearer


================
Comment at: lldb/source/Plugins/Trace/intel-pt/DecodedThread.h:155
+  /// Append a successfully decoded instruction.
+  void AppendInstruction(IntelPTInstruction ins);
+
----------------
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



================
Comment at: lldb/source/Plugins/Trace/intel-pt/DecodedThread.h:157
+
+  /// Append an error of instruction decoding.
+  void AppendError(llvm::Error err);
----------------



================
Comment at: lldb/source/Plugins/Trace/intel-pt/IntelPTDecoder.cpp:110
+                                        const size_t raw_trace_size) {
+  DecodedThread decoded_thread = DecodedThread(
+      thread_sp, std::vector<IntelPTInstruction>(), raw_trace_size);
----------------
make this a shared pointer since the beginning. Use make_shared here


================
Comment at: lldb/source/Plugins/Trace/intel-pt/IntelPTDecoder.cpp:148
+        decoded_thread.AppendError(make_error<IntelPTError>(time_error, insn.ip));
+        decoded_thread.AppendInstruction(IntelPTInstruction(insn));
         break;
----------------
ideally you should be able to do `decoded_thread.AppendInstruction({insn})` here


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122293