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