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

[walter erquinigo via Phabricator via lldb-commits]

wallace requested changes to this revision.
wallace added a comment.
This revision now requires changes to proceed.

much closer! I'm glad you are starting to understand the patterns we use for this kind of code



================
Comment at: lldb/source/Plugins/Trace/intel-pt/DecodedThread.cpp:13
 #include <memory>
+#include <unordered_map>
+#include <utility>
----------------
delete this


================
Comment at: lldb/source/Plugins/Trace/intel-pt/DecodedThread.cpp:14
+#include <unordered_map>
+#include <utility>
 
----------------
you don't need to import it. Maybe your c++ vscode extension has been autoimporting this one, in which case it's better to disable that feature.


================
Comment at: lldb/source/Plugins/Trace/intel-pt/DecodedThread.cpp:134-137
+      m_errors.insert(std::pair<uint64_t, std::unique_ptr<ErrorInfoBase>>(
+          m_instructions.size(), 
+          std::move(info)
+      ));
----------------
emplace will prevent unnecessary copies and also doesn't need you to pass a pair


================
Comment at: lldb/source/Plugins/Trace/intel-pt/DecodedThread.cpp:142-144
+  return m_raw_trace_size +
+         IntelPTInstruction::GetNonErrorMemoryUsage() * m_instructions.size() +
+         sizeof(DecodedThread);
----------------
in order to have correct formatting all along, you need to use git clang-format: https://llvm.org/docs/Contributing.html#format-patches

follow that guide. Whenever you are going to submit a patch, first run git clang-format and it will format your code correctly, so that you never again have to lose time doing that. It can even format comments


================
Comment at: lldb/source/Plugins/Trace/intel-pt/DecodedThread.cpp:143-144
+  return m_raw_trace_size +
+         IntelPTInstruction::GetNonErrorMemoryUsage() * m_instructions.size() +
+         sizeof(DecodedThread);
 }
----------------
here you also need to ask for the size of the DenseMap


================
Comment at: lldb/source/Plugins/Trace/intel-pt/DecodedThread.h:42-45
+  // /// \param[in] string_error
+  // ///     StringError to copy from
+  // IntelPTError(StringError string_error);
+
----------------
delete commented code


================
Comment at: lldb/source/Plugins/Trace/intel-pt/DecodedThread.h:76
   /// libipt errors should use the underlying \a IntelPTError class.
-  IntelPTInstruction(llvm::Error err);
+  IntelPTInstruction(llvm::Error &err);
 
----------------
Pass the error by const reference, because we don't modify it


================
Comment at: lldb/source/Plugins/Trace/intel-pt/DecodedThread.h:141
 
-  /// Get the instructions from the decoded trace. Some of them might indicate
-  /// errors (i.e. gaps) in the trace.
+  /// Get the instructions from the decoded trace.
   ///
----------------



================
Comment at: lldb/source/Plugins/Trace/intel-pt/DecodedThread.h:150
+  /// \return
+  ///   The error or \a llvm::Error::success if the given index 
+  ///   points to a valid instruction.
----------------



================
Comment at: lldb/source/Plugins/Trace/intel-pt/DecodedThread.h:155
+  /// Append a successfully decoded instruction.
+  void AppendInstruction(IntelPTInstruction ins);
+
----------------
remove this one. We need the version that accepts a parameter pack, as we discussed offline


================
Comment at: lldb/source/Plugins/Trace/intel-pt/DecodedThread.h:157-158
+
+  /// Append a decoding error (i.e. an instruction that failed to be decoded).
+  void AppendError(llvm::Error err);
+
----------------
same here


================
Comment at: lldb/source/Plugins/Trace/intel-pt/IntelPTDecoder.cpp:111-112
+  DecodedThreadSP decoded_thread =
+      std::make_shared<DecodedThread>(DecodedThread(
+          thread_sp, std::vector<IntelPTInstruction>(), raw_trace_size));
 
----------------
the magic of make_shared is that it uses parameter packs, so that it only constructs the object right where it'll store it, thus preventing unnecessary copies


================
Comment at: lldb/source/Plugins/Trace/intel-pt/IntelPTDecoder.cpp:184
+static Expected<DecodedThreadSP>
+DecodeInMemoryTrace(const ThreadSP &thread_sp, Process &process,
+                    TraceIntelPT &trace_intel_pt,
----------------
don't pass the process, because we can get it from the thread_sp object


================
Comment at: lldb/source/Plugins/Trace/intel-pt/IntelPTDecoder.cpp:207
 
   int errcode = pt_image_set_callback(image, ReadProcessMemory, &process);
   assert(errcode == 0);
----------------
if the compiler complains mentioning that the Process pointer you are passing is const, you can do an unsafe cast that removes the const qualifier and write a comment here. I hope you don't need it anyway


================
Comment at: lldb/source/Plugins/Trace/intel-pt/IntelPTDecoder.cpp:235
   if (!buffer)
-    return buffer.takeError();
-  raw_trace_size = buffer->size();
-  if (Expected<pt_cpu> cpu_info = trace.GetCPUInfo())
-    return DecodeInMemoryTrace(*thread.GetProcess(), trace,
-                               MutableArrayRef<uint8_t>(*buffer));
+    return DecodedThread(m_thread_sp, buffer.takeError()).shared_from_this();
+
----------------
use make_shared instead of shared_from_this. That will be much more performant


================
Comment at: lldb/source/Plugins/Trace/intel-pt/IntelPTDecoder.cpp:239
+    if (Expected<DecodedThreadSP> dtsp =
+            DecodeInMemoryTrace(m_thread_sp, *m_thread_sp->GetProcess(),
+                                m_trace, MutableArrayRef<uint8_t>(*buffer)))
----------------
don't pass the process, it's not necessary anymore


================
Comment at: lldb/source/Plugins/Trace/intel-pt/IntelPTDecoder.cpp:243-247
+      return DecodedThread(m_thread_sp, cpu_info.takeError())
+          .shared_from_this();
   else
-    return cpu_info.takeError();
+    return DecodedThread(m_thread_sp, cpu_info.takeError())
+        .shared_from_this();
----------------
same here


================
Comment at: lldb/source/Plugins/Trace/intel-pt/IntelPTDecoder.cpp:256
 
-DecodedThreadSP PostMortemThreadDecoder::DoDecode() {
-  size_t raw_trace_size = 0;
-  if (Expected<std::vector<IntelPTInstruction>> instructions =
-          DecodeTraceFile(*m_trace_thread->GetProcess(), m_trace,
-                          m_trace_thread->GetTraceFile(), raw_trace_size))
-    return std::make_shared<DecodedThread>(m_trace_thread->shared_from_this(),
-                                           std::move(*instructions),
-                                           raw_trace_size);
-  else
-    return std::make_shared<DecodedThread>(m_trace_thread->shared_from_this(),
-                                           instructions.takeError());
-}
+static Expected<DecodedThreadSP> DecodeTraceFile(const ThreadSP &thread_sp,
+                                                 Process &process,
----------------
just return a direct DecodedThreadSP that holds any errors you might find here, same like LiveThreadDecoder::DoDecode()


================
Comment at: lldb/source/Plugins/Trace/intel-pt/IntelPTDecoder.cpp:275-280
+  if (Expected<DecodedThreadSP> dtsp = DecodeTraceFile(m_trace_thread, *m_trace_thread->GetProcess(),
+                          m_trace, m_trace_thread->GetTraceFile()))
+    return *dtsp;
+  // else
+  //   return DecodedThread(m_trace_thread, cpu_info.takeError())
+  //       .shared_from_this();
----------------
this will become simpler once DecodeTraceFile returns directly a DecodedThreadSP


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122293