[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:116
+  pt_insn m_pt_insn;
+  std::shared_ptr<IntelPTError> m_error;
+};
----------------
wallace wrote:
> 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.
> How much are we gaining by storing the libipt_error_code vs. storing a bool m_is_gap field?

Quite a lot. If we can report to the user why decoding failed, then the user can try to fix that accordingly. For example, if an address section is missing, we can report it and the user can provide that missing library and retry decoding.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89283