[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/source/Plugins/Trace/intel-pt/TraceIntelPTSessionFileSaver.cpp:19
+#include "lldb/lldb-types.h"
+#include "llvm/Analysis/ModuleSummaryAnalysis.h"
+#include "llvm/Support/Error.h"
----------------
this doesn't seem relevant


================
Comment at: lldb/source/Plugins/Trace/intel-pt/TraceIntelPTSessionFileSaver.cpp:22
+#include "llvm/Support/JSON.h"
+#include <fstream>
+#include <sstream>
----------------
leave a black line before this


================
Comment at: lldb/source/Plugins/Trace/intel-pt/TraceIntelPTSessionFileSaver.cpp:31
+
+llvm::Error TraceIntelPTSessionFileSaver::SaveToDisk(
+    lldb::ProcessSP &process_sp, TraceIntelPT &trace_ipt, FileSpec directory) {
----------------
Better rename this class to TraceIntelPTSessionSaver, because this is not just working with a file, it's instead dealing with a directory of many files


================
Comment at: lldb/source/Plugins/Trace/intel-pt/TraceIntelPTSessionFileSaver.cpp:56
+
+void TraceIntelPTSessionFileSaver::WriteIntelPTSchemaToFile(
+    const JSONTraceIntelPTSchema &json_trace_intel_pt_schema,
----------------
To make these useful functions more generic so that they can be reused by other Trace technologies, you can create a file TraceSessionSaver.h/cpp in Plugins/Trace/common.
Then you can move this function to that file an call it WriteTraceSession(llvm::Value session_json, FileSpec directory)


================
Comment at: lldb/source/Plugins/Trace/intel-pt/TraceIntelPTSessionFileSaver.cpp:87
+llvm::Expected<JSONTraceSessionBase>
+TraceIntelPTSessionFileSaver::BuildProcessesSection(lldb::ProcessSP &process_sp,
+                                                    TraceIntelPT &trace_ipt,
----------------
you could move BuildProcessesSection, BuildThreadsSection and BuildModulesSection to TraceSessionSaver.h/cpp if you provide a callback that receives a thread_id and returns a raw trace. Then almost all the code would be reusable by other Trace plug-ins


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

https://reviews.llvm.org/D107669