[Lldb-commits] [PATCH] D89283: [trace][intel-pt] Implement the basic decoding functionality

[Vedant Kumar via Phabricator via lldb-commits]

vsk added a comment.

@wallace thanks for winnowing the test objects. I left an inline suggestion about simplifying the error-handling in IntelPTInstruction. Other than that, mechanically this is looking good. Thanks!



================
Comment at: lldb/source/Plugins/Trace/intel-pt/DecodedThread.h:36
+
+  IntelPTInstruction(const pt_insn &pt_insn, int libipt_error_code = 0)
+      : m_pt_insn(pt_insn), m_libipt_error_code(libipt_error_code),
----------------
Does this IntelPTInstruction constructor ever get called with a non-zero libipt error code?


================
Comment at: lldb/source/Plugins/Trace/intel-pt/DecodedThread.h:40
+
+  IntelPTInstruction(int libipt_error_code)
+      : m_pt_insn(), m_libipt_error_code(libipt_error_code), m_custom_error() {}
----------------
It might be a bit cleaner to just have two IntelPTInstruction constructors: one that accepts a `const pt_insn &` and another that accepts an `llvm::Error`. To do that, you'd need to introduce a PT-specific ErrorInfo (`class IntelPTError : public ErrorInfo<IntelPTError> { ... }`). This can wrap some sort of generic error (as a std::string, perhaps), or a libipt-specific error.

It'd be a little more up front work, but the benefit is that it simplifies error handling (there's just one type of error, only one error value to check in IsError, etc).


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

https://reviews.llvm.org/D89283