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

[walter erquinigo via Phabricator via lldb-commits]

wallace added inline comments.


================
Comment at: lldb/source/Plugins/Trace/intel-pt/DecodedThread.h:26
+public:
+  IntelPTError() = default;
+
----------------
vsk wrote:
> Do we need a default constructor for this pure-virtual class?
The compiler forces me to do it because I declare below a deleted copy constructor.


================
Comment at: lldb/source/Plugins/Trace/intel-pt/DecodedThread.h:116
+  pt_insn m_pt_insn;
+  std::shared_ptr<IntelPTError> m_error;
+};
----------------
vsk wrote:
> Still doesn't feel like this is aligning with the design of llvm::Error - they're supposed to be unique and lightweight. Specifically:
> 
> - Why is the error instance shareable? Do multiple threads need to handle the same error?
> - For that matter, why is IntelPTInstruction copyable?
> - How much are we gaining by storing the libipt_error_code vs. storing a `bool m_is_gap` field?
> 
> Imo it seems a bit too complicated to have three different ways to construct an IntelPTInstruction with an error. Would be nice to just have one?
Will try to fix that now. 

Definitely I shouldn't make it copyable, which will simplify the code. 

Regarding the error, it's hard to receive an llvm::Error in the constructor, because every time the user iterates on each instruction, that Error will need to be returned, but Errors are not copyable, only movable. That's why I found it easier to create a custom error class that can create Error's on demand.


================
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),
----------------
vsk wrote:
> vsk wrote:
> > Does this IntelPTInstruction constructor ever get called with a non-zero libipt error code?
> Still wondering about this: why do we need to construct an IntelPTInstruction using a pt_insn as well as an error code?
Indeed, there are cases in which the libipt decoder library fails to decode an instruction, but contains some information about it, like the address (among other info). Then you have a partially correct pt_insn object along with an error code


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89283