[Lldb-commits] [PATCH] D130805: [trace][intel pt] Support a new kernel section in LLDB’s trace bundle schema

[Jakob Johnson via Phabricator via lldb-commits]

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