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

[walter erquinigo via Phabricator via lldb-commits]

wallace requested changes to this revision.
wallace added inline comments.


================
Comment at: lldb/source/Plugins/Trace/intel-pt/DecodedThread.cpp:103-105
+std::unordered_map<uint64_t, llvm::Error> DecodedThread::GetErrors() const {
+  return m_errors;
+}
----------------
remove this


================
Comment at: lldb/source/Plugins/Trace/intel-pt/DecodedThread.h:152
+  ///   The errors of the trace.
+  std::unordered_map<uint64_t, llvm::Error> GetErrors() const;
+
----------------
zrthxn wrote:
> jj10306 wrote:
> > Return a reference here to avoid potential expensive copy when returning.
> > 
> > Something else to consider is if we need/want an API exposing all the entire error map or if something like:
> > `llvm::Error GetErrorByInstructionIndex(uint64_t insn_index) const` that allows the caller to specify the key into the map would make more sense? 
> > This also has the advantage that it hides the implementation detail of what data type is being used under the hood to represent the error map!
> > @wallace @zrthxn wdyt?
> Yea I did think about that. In my opinion returning a reference would be good since with that you can get the size and error at each index and we don't need to have functions for each operation which would have long clunky names making the code less readable...
ahh!! DenseMap is for sure the way to go. The getMemorySize() method will help us a good deal. Thanks @jj10306 

Besides that, we shouldn't expose publicly the internal representation of an object whose data structure might change. For example, sooner than later we might prefer to store the errors in a different way. So, to prevent breaking callers if we change this data structure, let's go for what Jakob proposes
  llvm::Error GetErrorByInstructionIndex(uint64_t insn_index) const
which returns Error::success() in case the instruction is not an actual error




================
Comment at: lldb/source/Plugins/Trace/intel-pt/DecodedThread.h:154
+
+  /// Append a successfully decoded instruction to thread.
+  void AppendInstruction(IntelPTInstruction ins);
----------------
Omit `the thread`, because you are appending to a `DecodedThread`. In any case, it's redundant to mention where you are appending to


================
Comment at: lldb/source/Plugins/Trace/intel-pt/DecodedThread.h:157
+
+  /// Append a error of instruction decoding to thread.
+  void AppendError(uint64_t insn_index, llvm::Error err);
----------------
you should be able to omit the insn_index by using `m_instructions.size() + m_errors.size()` as the index


================
Comment at: lldb/source/Plugins/Trace/intel-pt/IntelPTDecoder.cpp:102-104
 /// \param[in] decoder
 ///   A configured libipt \a pt_insn_decoder.
 ///
----------------
update the documentation


================
Comment at: lldb/source/Plugins/Trace/intel-pt/IntelPTDecoder.cpp:110
+                                        const size_t raw_trace_size) {
+  DecodedThread thread = DecodedThread(
+      thread_sp, std::vector<IntelPTInstruction>(), raw_trace_size);
----------------
don't call it thread, call it decoded_thread. The thread is the one passed as parameter


================
Comment at: lldb/source/Plugins/Trace/intel-pt/IntelPTDecoder.cpp:110
+                                        const size_t raw_trace_size) {
+  DecodedThread thread = DecodedThread(
+      thread_sp, std::vector<IntelPTInstruction>(), raw_trace_size);
----------------
wallace wrote:
> don't call it thread, call it decoded_thread. The thread is the one passed as parameter
use make_shared here and return a DecodedThreadSP (alias for shared_ptr<DecodedThread>). That way, you'll avoid having to invoke shared_from_this() below


================
Comment at: lldb/source/Plugins/Trace/intel-pt/IntelPTDecoder.cpp:125
     // instructions and events.
+    int insn_index = 0;
     while (true) {
----------------
remove this


================
Comment at: lldb/source/Plugins/Trace/intel-pt/IntelPTDecoder.cpp:149
+        thread.AppendError(insn_index, make_error<IntelPTError>(time_error, insn.ip));
+        thread.AppendInstruction(IntelPTInstruction(insn));
         break;
----------------
This will create a copy of the IntelPTInstruction before storing it in the vector. Instead, you should use the same semantics as vector::emplace_back(), which uses paratemer packs/variadic templates. You can even rename Append to Emplace in this case


================
Comment at: lldb/source/Plugins/Trace/intel-pt/IntelPTDecoder.cpp:184-216
+static Expected<DecodedThreadSP> DecodeInMemoryTrace(
+    const ThreadSP &thread_sp, Process &process, TraceIntelPT &trace_intel_pt,
+    MutableArrayRef<uint8_t> buffer, const size_t raw_trace_size) {
   Expected<pt_cpu> cpu_info = trace_intel_pt.GetCPUInfo();
   if (!cpu_info)
     return cpu_info.takeError();
 
----------------
see below. We don't need to pass the raw_buffer size because it can be gotten from the buffer itself. Also, return the DecodedThreadSP directly


================
Comment at: lldb/source/Plugins/Trace/intel-pt/IntelPTDecoder.cpp:230-243
+static Expected<DecodedThreadSP> DecodeLiveThread(const ThreadSP &thread_sp,
+                                                  TraceIntelPT &trace,
+                                                  size_t &raw_trace_size) {
   Expected<std::vector<uint8_t>> buffer =
-      trace.GetLiveThreadBuffer(thread.GetID());
+      trace.GetLiveThreadBuffer(thread_sp->GetID());
   if (!buffer)
     return buffer.takeError();
----------------
Now we don't need to return the raw_trace_size as an out parameter because we are creating the decoded thread deeper in the stack. 


================
Comment at: lldb/source/Plugins/Trace/intel-pt/IntelPTDecoder.cpp:247-248
+DecodedThreadSP LiveThreadDecoder::DoDecode() {
+  size_t raw_trace_size = 0;
+  return *(DecodeLiveThread(m_thread_sp, m_trace, raw_trace_size));
 }
----------------
we don't need raw_trace_size to be an out parameter anymore. We can directly use it deep inside our decoding logic


================
Comment at: lldb/source/Plugins/Trace/intel-pt/IntelPTDecoder.cpp:261
+                                                 const FileSpec &trace_file,
+                                                 size_t &raw_trace_size) {
+  ErrorOr<std::unique_ptr<MemoryBuffer>> trace_or_error =
----------------
remove raw_trace_size as an out parameter


================
Comment at: lldb/source/Plugins/Trace/intel-pt/IntelPTDecoder.cpp:274-275
+  raw_trace_size = trace_data.size();
+  return DecodeInMemoryTrace(thread_sp, process, trace_intel_pt, trace_data,
+                             raw_trace_size);
+}
----------------
the trace_size doesn't need to be passed 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122293