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
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
