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

[walter erquinigo via Phabricator via lldb-commits]

wallace added inline comments.


================
Comment at: lldb/include/lldb/Target/Trace.h:75
+  virtual llvm::Error SaveToDisk(FileSpec directory,
+                                 lldb::ProcessSP &process_sp) = 0;
+
----------------
clayborg wrote:
> 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.
Fair enough. Then better rename this method to SaveLiveProcessTraceToDisk or SaveLiveTraceToDisk, and make a comment mentioning that this will return an error if this is not tracing a live process.


================
Comment at: lldb/source/Plugins/Trace/intel-pt/IntelPTDecoder.h:36
 
+  /// Get thr raw trace of the thread.
+  ///
----------------
thr -> the


================
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;
+
----------------
clayborg wrote:
> This data is quite large, can we just get a reference to the data without making a copy by using llvm::ArrayRef?
Sadly this can't work. LiveThreadDecoder::GetRawTrace() gets the raw trace from lldb-server and puts it in a vector without any object owning it. So the actual vector will need to be returned because no one wants to own it. It's not that bad though, because Expected just moves the inner data without creating copies.


================
Comment at: lldb/test/API/commands/trace/TestTraceSave.py:38-43
+        self.expect("process trace save -d " +
+            os.path.join(self.getSourceDir(), "intelpt-trace", "trace_copy_dir"))
+        # Load the trace just saved
+        self.expect("trace load -v " +
+            os.path.join(self.getSourceDir(), "intelpt-trace", "trace_copy_dir", "trace.json"),
+            substrs=["intel-pt"])
----------------
use self.getBuildDir() instead of self.getSourceDir() for the files were you will store data


================
Comment at: lldb/test/API/commands/trace/TestTraceSave.py:69-70
+
+        #remove <trace_copy_dir>
+        shutil.rmtree(os.path.join(self.getSourceDir(), "intelpt-trace", "trace_copy_dir"))
----------------
you don't need to do this. Every time the test runs all the previous data will be wiped out


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

https://reviews.llvm.org/D107669