[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 added inline comments.


================
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));
----------------
jj10306 wrote:
> What's the `trace_mode` being used for? afaict it's unused in this method.
it'll be used later because this diff doesn't make decoding work yet


================
Comment at: lldb/source/Plugins/Trace/intel-pt/TraceIntelPT.h:176
 
+  enum TraceMode {
+    UserMode,
----------------
jj10306 wrote:
> 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.
for now, live tracing will always be UserMode but postmortem could be any of those.


================
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.
   ///
----------------
jj10306 wrote:
> Should these be for postmortem instead of live?
I think you are right :)


================
Comment at: lldb/source/Plugins/Trace/intel-pt/TraceIntelPTConstants.h:25
 const bool kDefaultDisableCgroupFiltering = false;
+const uint64_t kDefaultKernelLoadAddress = 0xffffffff81000000;
 
----------------
jj10306 wrote:
> 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?
Just like the comment I left below, I think it's better not to have this default value here because it's not guaranteed to work on every system.

What we can implement in the collector is the following:

read /proc/kcore and look for the line with the entry `SYMBOL(_stext)=ffffffff81000000`

then in the documentation we can mention that this a way to get this load address, and we can implement that in the collector


================
Comment at: lldb/source/Plugins/Trace/intel-pt/TraceIntelPTJSONStructs.h:61
   llvm::Optional<LinuxPerfZeroTscConversion> tsc_perf_zero_conversion;
+  llvm::Optional<JSONKernel> kernel;
 
----------------
jj10306 wrote:
> 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.
I think JSONKernel here is fine just to allow more easily other fields to be added to this struct, like kernel version or any other information that could be useful in the future.
Regarding the default, after giving a second thought, I think you are right. Let's better not have a default but instead show in the documentation ways to figure out this value.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130805