[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.

Are the files in `lldb/test/API/commands/trace/intelpt-kernel-trace/cores/` actual kernel traces? If not, just use some trace files that are already present in the repo. You can use relative paths of the form ../../trace_sample/cores/... to refer to them.

Good start for this task, btw!



================
Comment at: lldb/source/Plugins/Trace/intel-pt/TraceIntelPT.h:176
 
+  enum TraceMode {
+    UserMode,
----------------
enum class


================
Comment at: lldb/source/Plugins/Trace/intel-pt/TraceIntelPT.h:176
 
+  enum TraceMode {
+    UserMode,
----------------
wallace wrote:
> enum class
@jj10306 , TraceMode or TracingMode?


================
Comment at: lldb/source/Plugins/Trace/intel-pt/TraceIntelPT.h:199
+  /// \param[in] trace_mode
+  ///     The tracing mode in the live session. One of TraceMode enum value.
+  ///
----------------



================
Comment at: lldb/source/Plugins/Trace/intel-pt/TraceIntelPTBundleLoader.cpp:133-148
+  TargetSP target_sp;
+  Status error = m_debugger.GetTargetList().CreateTarget(
+      m_debugger, /*user_exe_path*/ StringRef(), "",
+      eLoadDependentsNo,
+      /*platform_options*/ nullptr, target_sp);
+
+  if (!target_sp)
----------------
this part is the same as the one in ParseProcess. Create a function to dedup this code. Maybe a good name is CreateEmptyProcess


================
Comment at: lldb/source/Plugins/Trace/intel-pt/TraceIntelPTBundleLoader.cpp:155-163
+    char thread_name[20];
+    sprintf(thread_name, "kernel_cpu_%d", cpu.id);
+    lldb::tid_t tid = static_cast<lldb::tid_t>(cpu.id);
+
+    Optional<FileSpec> trace_file;
+    trace_file = FileSpec(cpu.ipt_trace);
+
----------------
you can make this simpler


================
Comment at: lldb/source/Plugins/Trace/intel-pt/TraceIntelPTBundleLoader.cpp:172
+  module_spec.GetFileSpec() = system_file_spec;
+  module_spec.GetPlatformFileSpec() = system_file_spec;
+
----------------
try without setting this one. Hopefully you only need to set one file spec. The PlatformFileSpec helps distinguish the binary path on the lldb's machine from the one were collection happened, but I don't think that feature will be useful for this kernel thing


================
Comment at: lldb/source/Plugins/Trace/intel-pt/TraceIntelPTBundleLoader.cpp:291
+    "loadAddress"?: integer | string decimal | hex string,
+        // Kernel's start address. 0xffffffff81000000 on default.
+    "file": string,
----------------



================
Comment at: lldb/source/Plugins/Trace/intel-pt/TraceIntelPTBundleLoader.cpp:302
 - "tscPerfZeroConversion" must be provided if "cpus" is provided.
+- If tracing mode is "kernel", then the "processes" section must be empty and the "kernel" and "cpus" section must be provided.
  })";
----------------



================
Comment at: lldb/source/Plugins/Trace/intel-pt/TraceIntelPTBundleSaver.cpp:362
 
+  //TODO: Add kernel section
+
----------------
add this now. You have to update the corresponding tests to make sure that saving a loaded trace bundle produces the same information.


================
Comment at: lldb/source/Plugins/Trace/intel-pt/TraceIntelPTConstants.h:25
 const bool kDefaultDisableCgroupFiltering = false;
+const uint64_t kDefaultKernelLoadAddress = 0xffffffff81000000;
 
----------------



================
Comment at: lldb/test/API/commands/trace/intelpt-kernel-trace/trace.json:20-21
+  },
+  "processes": [
+  ],
+  "tscPerfZeroConversion": {
----------------
Make "processes" optional, so that it's not even necessary to specify this field if "kernel" is provided. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130805