[Lldb-commits] [PATCH] D126015: [trace][intelpt] Support system-wide tracing [12] - Support multi-core trace load and save

[Jakob Johnson via Phabricator via lldb-commits]

jj10306 added a comment.

feeback-v3 - completed review



================
Comment at: lldb/docs/use/intel_pt.rst:168
 ::
   {
+    "type": "intel-pt",
----------------
Consider adding a section on the perfTscConversion parameters while we're editing this file as I don't currently see that in this file. Also, it doesn't appear the "cores" section is here either


================
Comment at: lldb/source/Target/Trace.cpp:30-32
 struct JSONSimpleTraceSession {
-  JSONSimplePluginSettings trace;
+  std::string type;
 };
----------------
do we need this struct or can we just use a string directly since the struct only has a single member?


================
Comment at: lldb/source/Target/Trace.cpp:134
+    return None;
+  std::unordered_map<std::string, size_t> &single_core_data = it->second;
+  auto single_thread_data_it = single_core_data.find(kind.str());
----------------



================
Comment at: lldb/source/Target/Trace.cpp:135
+  std::unordered_map<std::string, size_t> &single_core_data = it->second;
+  auto single_thread_data_it = single_core_data.find(kind.str());
+  if (single_thread_data_it == single_core_data.end())
----------------
why is this named thread_data, isn't this the size of the core's buffer?


================
Comment at: lldb/source/Target/Trace.cpp:325
+
+  std::unordered_map<std::string, FileSpec> &data_kind_to_file_spec_map =
+      it->second;
----------------



================
Comment at: lldb/unittests/Process/Linux/PerfTests.cpp:9
 
-#ifdef __x86_64__
-
----------------
isn't this needed since `readTsc()` uses intel specific assembly instructions?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126015