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

[Jakob Johnson via Phabricator via lldb-commits]

jj10306 requested changes to this revision.
jj10306 added inline comments.
This revision now requires changes to proceed.


================
Comment at: lldb/source/Plugins/Trace/intel-pt/DecodedThread.h:172
   std::vector<IntelPTInstruction> m_instructions;
+  std::vector<llvm::Error> m_errors;
+
----------------
wallace wrote:
> you need to have something like
>   std::unordered_map<uint64_t, llvm::Error> m_errors;
> 
> that way, you'll be able to quickly look for the error associated with an instruction index. The IntelPTInstruction int his case, instead of storing the Error, can just store one bit of information has_error = true/false;
nit: from https://llvm.org/docs/CodingStandards.html#c-standard-library 
prefer `llvm::DenseMap` over `std::map`'s unless there's a specific reason not to.

Don't forget to update `CalculateApproximateMemoryUsage()` as well! Also, besides for being inline with the coding standards I linked above, using `llvm::DenseMap` here has the actual advantage that it exposes its approximate size via  `getMemorySize()`, whereas there is no easy way to get the size of `std::map`.


================
Comment at: lldb/source/Plugins/Trace/intel-pt/IntelPTDecoder.cpp:120
+      thread.AppendError(make_error<IntelPTError>(errcode));
+      // instructions.emplace_back(make_error<IntelPTError>(errcode));
       break;
----------------



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122293