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

almost there!



================
Comment at: lldb/source/Plugins/Trace/intel-pt/TraceIntelPT.cpp:104-108
     std::vector<tid_t> tids;
     for (const JSONProcess &process : bundle_description.processes)
       for (const JSONThread &thread : process.threads)
         tids.push_back(thread.tid);
 
----------------
why did you remove this?


================
Comment at: lldb/source/Plugins/Trace/intel-pt/TraceIntelPTBundleLoader.cpp:305
 - "tscPerfZeroConversion" must be provided if "cpus" is provided.
+- "processes" is provided if and only if "kernel" is not provided.
+- If "kernel" is provided, then the "processes" section must be empty and the "cpus" section must be provided. This configuration indicates that the kernel was traced and user processes weren't. Besides that, the kernel is treated as a single process with one thread per CPU core. This doesn't handle actual kernel threads, but instead treats all the instructions executed by the kernel on each core as an individual thread.
----------------
remove this line. We should allow the user to pass an empty array for "processes" or not passing it at all. This will make the json easier to work with


================
Comment at: lldb/source/Plugins/Trace/intel-pt/TraceIntelPTBundleLoader.cpp:306
+- "processes" is provided if and only if "kernel" is not provided.
+- If "kernel" is provided, then the "processes" section must be empty and the "cpus" section must be provided. This configuration indicates that the kernel was traced and user processes weren't. Besides that, the kernel is treated as a single process with one thread per CPU core. This doesn't handle actual kernel threads, but instead treats all the instructions executed by the kernel on each core as an individual thread.
  })";
----------------
please break this into multiple lines of at most 80 chars each


================
Comment at: lldb/source/Plugins/Trace/intel-pt/TraceIntelPTBundleSaver.cpp:376
 
-  JSONTraceBundleDescription json_intel_pt_bundle_desc{"intel-pt", *cpu_info, *json_processes,
-                                         *json_cpus,
-                                         trace_ipt.GetPerfZeroTscConversion()};
+  Optional<std::vector<JSONProcess>> json_processes = llvm::None;
+  Optional<JSONKernel> json_kernel = llvm::None;
----------------
that is enough


================
Comment at: lldb/source/Plugins/Trace/intel-pt/TraceIntelPTBundleSaver.cpp:377
+  Optional<std::vector<JSONProcess>> json_processes = llvm::None;
+  Optional<JSONKernel> json_kernel = llvm::None;
+
----------------
that is enough


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



================
Comment at: lldb/test/API/commands/trace/TestTraceLoad.py:274
+        self.expect("image list", substrs=["modules/m.out"])
+        self.expect("image list", substrs=["0xffffffff81000000"])
+
----------------
remove this one


================
Comment at: lldb/test/API/commands/trace/intelpt-kernel-trace/trace.json:26
+  "kernel": {
+    "file": "../intelpt-multi-core-trace/modules/m.out"
+  },
----------------
write another test with a custom load address


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

https://reviews.llvm.org/D130805