[Lldb-commits] [PATCH] D107669: [trace] [intel pt] Create a "process trace save" command

[Greg Clayton via Phabricator via lldb-commits]

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


================
Comment at: lldb/include/lldb/Target/Trace.h:75
+  virtual llvm::Error SaveToDisk(FileSpec directory,
+                                 lldb::ProcessSP &process_sp) = 0;
+
----------------
This class will already have a process in "Process *m_live_process" member variable, so no need to pass in the process. This will be NULL if we loaded a trace from disk already, but that should be ok because it was already loaded from something on disk.


================
Comment at: lldb/source/Commands/CommandObjectProcess.cpp:1687-1693
+            "Save the trace of the current process in the specified directory, "
+            "which will be created if needed."
+            "This will also create a file <directory>/trace.json with the main "
+            "properties of the trace session, along with others files which "
+            "contain the actual trace data. The trace.json file can be used "
+            "later as input for the \"trace load\" command to load the trace "
+            "in LLDB",
----------------



================
Comment at: lldb/source/Commands/CommandObjectProcess.cpp:1708
+    if (llvm::Error err =
+            trace_sp->SaveToDisk(m_options.m_directory, process_sp))
+      result.AppendError(toString(std::move(err)));
----------------
No need for the process as mentioned in above inline comment.


================
Comment at: lldb/source/Plugins/Trace/intel-pt/IntelPTDecoder.cpp:267
 }
+llvm::Expected<std::vector<uint8_t>> PostMortemThreadDecoder::GetRawTrace() {
+  FileSpec trace_file = m_trace_thread->GetTraceFile();
----------------
llvm::ArrayRef


================
Comment at: lldb/source/Plugins/Trace/intel-pt/IntelPTDecoder.cpp:297
+
+llvm::Expected<std::vector<uint8_t>> LiveThreadDecoder::GetRawTrace() {
+  return m_trace.GetLiveThreadBuffer(m_thread_sp->GetID());
----------------
llvm::ArrayRef


================
Comment at: lldb/source/Plugins/Trace/intel-pt/IntelPTDecoder.h:40
+  ///     raw trace from the thread buffer as bytes.
+  virtual llvm::Expected<std::vector<uint8_t>> GetRawTrace() = 0;
+
----------------
This data is quite large, can we just get a reference to the data without making a copy by using llvm::ArrayRef?


================
Comment at: lldb/source/Plugins/Trace/intel-pt/IntelPTDecoder.h:70
 
+  llvm::Expected<std::vector<uint8_t>> GetRawTrace() override;
+
----------------
llvm::ArrayRef?


================
Comment at: lldb/source/Plugins/Trace/intel-pt/IntelPTDecoder.h:88
 
+  llvm::Expected<std::vector<uint8_t>> GetRawTrace() override;
+
----------------
llvm::ArrayRef


================
Comment at: lldb/source/Plugins/Trace/intel-pt/TraceIntelPT.cpp:72
+llvm::Error TraceIntelPT::SaveToDisk(FileSpec directory,
+                                     lldb::ProcessSP &process_sp) {
+  RefreshLiveProcessState();
----------------
remove process_sp and use "m_live_process"


================
Comment at: lldb/source/Plugins/Trace/intel-pt/TraceIntelPT.cpp:78
+
+llvm::Expected<std::vector<uint8_t>>
+TraceIntelPT::GetThreadBuffer(Thread &thread) {
----------------
llvm::ArrayRef


================
Comment at: lldb/source/Plugins/Trace/intel-pt/TraceIntelPT.h:25
+  llvm::Error SaveToDisk(FileSpec directory,
+                         lldb::ProcessSP &process_sp) override;
+
----------------
remove process


================
Comment at: lldb/source/Plugins/Trace/intel-pt/TraceIntelPT.h:27
+
+  llvm::Expected<std::vector<uint8_t>> GetThreadBuffer(Thread &thread);
+
----------------
llvm::ArrayRef


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

https://reviews.llvm.org/D107669