[Lldb-commits] [PATCH] D130805: [trace][intel pt] Support a new kernel section in LLDB’s trace bundle schema

[walter erquinigo via Phabricator via lldb-commits]

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

just some minor cosmetic changes :)



================
Comment at: lldb/source/Plugins/Trace/intel-pt/TraceIntelPTBundleLoader.cpp:177-184
+  if (bundle_description.kernel->load_address)
+    module_sp->SetLoadAddress(*parsed_process->target_sp,
+                              bundle_description.kernel->load_address->value,
+                              false, load_addr_changed);
+  else
+    module_sp->SetLoadAddress(*parsed_process->target_sp,
+                              kDefaultKernelLoadAddress, false,
----------------
try to reduce duplication


================
Comment at: lldb/source/Plugins/Trace/intel-pt/TraceIntelPTBundleSaver.cpp:345-350
+  if (processes.size() != 1)
+    return createStringError(inconvertibleErrorCode(),
+                             "User processes exist in kernel mode");
+  if (kernel_process->GetID() != kDefaultKernelProcessID)
+    return createStringError(inconvertibleErrorCode(),
+                             "Kernel process not exist");
----------------
convert these checks into asserts, as they should crash the program if they happen.


================
Comment at: lldb/source/Plugins/Trace/intel-pt/TraceIntelPTBundleSaver.cpp:379-410
+  if (trace_ipt.GetTraceMode() == TraceIntelPT::TraceMode::KernelMode) {
+    std::vector<JSONProcess> json_processes;
+    Expected<JSONKernel> json_kernel = BuildKernelSection(trace_ipt, directory);
+
+    if (!json_kernel)
+      return json_kernel.takeError();
+
----------------
too much duplication. Better first calculate the processes and kernel sections, and then just perform one single invocation to JSONTraceBundleDescription()


================
Comment at: lldb/source/Plugins/Trace/intel-pt/TraceIntelPTJSONStructs.cpp:148
+  if (!(o && o.map("type", bundle_description.type) &&
+        o.map("cpus", bundle_description.cpus) &&
+        o.map("tscPerfZeroConversion",
----------------
revert your change that removes parsing processes here, that way you can dedup changes below


================
Comment at: lldb/source/Plugins/Trace/intel-pt/TraceIntelPTJSONStructs.cpp:168-169
+  if (bundle_description.kernel) {
+    if (o.map("processes", bundle_description.processes) &&
+        !bundle_description.processes.empty()) {
+      path.report("\"processes\" must be empty when \"kernel\" is provided");
----------------
check for its existence because it's now an optional


================
Comment at: lldb/source/Plugins/Trace/intel-pt/TraceIntelPTJSONStructs.cpp:177-180
+  } else if (!o.map("processes", bundle_description.processes)) {
+    // Usermode tracing requires processes section.
+    return false;
+  }
----------------
remove this


================
Comment at: lldb/source/Plugins/Trace/intel-pt/TraceIntelPTJSONStructs.h:58
   pt_cpu cpu_info;
   std::vector<JSONProcess> processes;
   llvm::Optional<std::vector<JSONCpu>> cpus;
----------------
also make this one Optional, following the changes in the schema


================
Comment at: lldb/test/API/commands/trace/TestTraceLoad.py:273
+
+        self.expect("image list", substrs=["modules/m.out"])
+
----------------
also check the load address


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

https://reviews.llvm.org/D130805