jj10306 requested changes to this revision.
jj10306 added a comment.
Nice work!
qq: Do we plan to add this kernel tracing support for live tracing as well?
================
Comment at: lldb/source/Plugins/Trace/intel-pt/TraceIntelPT.cpp:83
JSONTraceBundleDescription &bundle_description, ArrayRef<ProcessSP>
traced_processes,
- ArrayRef<ThreadPostMortemTraceSP> traced_threads) {
+ ArrayRef<ThreadPostMortemTraceSP> traced_threads, TraceMode trace_mode) {
TraceIntelPTSP trace_sp(new TraceIntelPT(bundle_description,
traced_processes));
----------------
What's the `trace_mode` being used for? afaict it's unused in this method.
================
Comment at: lldb/source/Plugins/Trace/intel-pt/TraceIntelPT.h:176
+ enum TraceMode {
+ UserMode,
----------------
wallace wrote:
> wallace wrote:
> > enum class
> @jj10306 , TraceMode or TracingMode?
I don't have a strong preference on the naming, but I was wondering how/if this
enum is being used currently?
I understand this would be necessary when configuring a live tracing session bc
this enum would control the configuration of the perf_event, but afaict this
diff is only adding support for the kernel in the post mortem case so I don't
quite understand how it's being used.
================
Comment at: lldb/source/Plugins/Trace/intel-pt/TraceIntelPT.h:191-199
/// \param[in] traced_processes
/// The processes traced in the live session.
///
/// \param[in] trace_threads
/// The threads traced in the live session. They must belong to the
/// processes mentioned above.
///
----------------
Should these be for postmortem instead of live?
================
Comment at: lldb/source/Plugins/Trace/intel-pt/TraceIntelPTConstants.h:25
const bool kDefaultDisableCgroupFiltering = false;
+const uint64_t kDefaultKernelLoadAddress = 0xffffffff81000000;
----------------
wallace wrote:
>
a couple questsions here:
1. where is this value coming from? Would be useful to provide a link to
documentation
2. does this depend on the system?
3. I know you and @wallace were discussing the implications of ASLR offline,
but curious what will happen if this load address is not correct due to ASLR or
any other reason. Do we have a way to detect this?
================
Comment at:
lldb/source/Plugins/Trace/intel-pt/TraceIntelPTJSONStructs.cpp:161-162
return false;
+ // When kernel section is present, this is kernel mode tracing. Thus, throw
an
+ // error if there is any user process or cpus section is not present.
+ if (bundle_description.kernel){
----------------
================
Comment at: lldb/source/Plugins/Trace/intel-pt/TraceIntelPTJSONStructs.h:61
llvm::Optional<LinuxPerfZeroTscConversion> tsc_perf_zero_conversion;
+ llvm::Optional<JSONKernel> kernel;
----------------
nit: any reason not to use reuse `JSONModule` here? The only difference I see
is that the load address is optional.
Not sure if there's a strong reason against this, but imo it should be the
bundle creator's responsibility to provide a load address of the kernel and not
LLDB's responsibility to use a default kernel load address.
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
