This is an archive of the discontinued LLVM Phabricator instance.

[trace][intel pt] Support a new kernel section in LLDB’s trace bundle schema
ClosedPublic

Authored by persona0220 on Jul 29 2022, 4:06 PM.

Details

Summary

Add a new "kernel" section with following schema.

"kernel": {
  "loadAddress"?: decimal | hex string | string decimal 
  # This is optional. If it's not specified, use default address 0xffffffff81000000.
  "file": string
  # path to the kernel image
}

Here's more details of the diff:

  • If "kernel" section exist, it means current tracing mode is KernelMode.
  • If tracing mode is KernelMode, the "processes" section must be empty and the "kernel" and "cpus" section must be provided. This is tested with TestTraceLoad.
  • "kernel" section is parsed and turned into a new process with a single module which is the kernel image. The kernel process has N fake threads, one for each cpu.

Diff Detail

Event Timeline

persona0220 created this revision.Jul 29 2022, 4:06 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 29 2022, 4:06 PM
persona0220 requested review of this revision.Jul 29 2022, 4:06 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 29 2022, 4:06 PM
wallace retitled this revision from Support a new kernel section in LLDB’s trace bundle schema to [trace][intel pt] Support a new kernel section in LLDB’s trace bundle schema.Jul 30 2022, 4:07 PM
wallace requested changes to this revision.Aug 1 2022, 12:04 PM

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!

lldb/source/Plugins/Trace/intel-pt/TraceIntelPT.h
176

enum class

176

@jj10306 , TraceMode or TracingMode?

198
lldb/source/Plugins/Trace/intel-pt/TraceIntelPTBundleLoader.cpp
146–161

this part is the same as the one in ParseProcess. Create a function to dedup this code. Maybe a good name is CreateEmptyProcess

168–176

you can make this simpler

185

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

293
305
lldb/source/Plugins/Trace/intel-pt/TraceIntelPTBundleSaver.cpp
376

add this now. You have to update the corresponding tests to make sure that saving a loaded trace bundle produces the same information.

lldb/source/Plugins/Trace/intel-pt/TraceIntelPTConstants.h
25
lldb/test/API/commands/trace/intelpt-kernel-trace/trace.json
21–22

Make "processes" optional, so that it's not even necessary to specify this field if "kernel" is provided.

This revision now requires changes to proceed.Aug 1 2022, 12:04 PM
jj10306 requested changes to this revision.Aug 2 2022, 8:51 AM

Nice work!
qq: Do we plan to add this kernel tracing support for live tracing as well?

lldb/source/Plugins/Trace/intel-pt/TraceIntelPT.cpp
83–84

What's the trace_mode being used for? afaict it's unused in this method.

lldb/source/Plugins/Trace/intel-pt/TraceIntelPT.h
176

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.

190–200

Should these be for postmortem instead of live?

lldb/source/Plugins/Trace/intel-pt/TraceIntelPTConstants.h
25

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?
lldb/source/Plugins/Trace/intel-pt/TraceIntelPTJSONStructs.cpp
164–165
lldb/source/Plugins/Trace/intel-pt/TraceIntelPTJSONStructs.h
61

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.

qq: Do we plan to add this kernel tracing support for live tracing as well?

I think it won't be possible without major changes or at least a big discussion because LLDB is supposed to operated on processes it's debugging, so let's better leave it out of scope for now.

wallace added inline comments.Aug 2 2022, 12:23 PM
lldb/source/Plugins/Trace/intel-pt/TraceIntelPT.cpp
83–84

it'll be used later because this diff doesn't make decoding work yet

lldb/source/Plugins/Trace/intel-pt/TraceIntelPT.h
176

for now, live tracing will always be UserMode but postmortem could be any of those.

190–200

I think you are right :)

lldb/source/Plugins/Trace/intel-pt/TraceIntelPTConstants.h
25

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

lldb/source/Plugins/Trace/intel-pt/TraceIntelPTJSONStructs.h
61

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.

persona0220 marked 15 inline comments as done.
  • Remove duplicate files under lldb/test/API/commands/trace/intelpt-kernel-trace and use relative path to ../intelpt-multi-core-trace/
  • Update TraceIntelBundleSaver to save "kernel" section
  • Add test to TestTraceSave
  • Add CreateEmptyProcess to dedup between ParseProccess and PareKernel.
persona0220 marked 7 inline comments as done.Aug 2 2022, 2:57 PM
persona0220 added inline comments.
lldb/source/Plugins/Trace/intel-pt/TraceIntelPTConstants.h
25
  1. It's the address of startup_64, the start address of loaded kernel image. This is a good documentation https://0xax.gitbooks.io/linux-insides/content/Theory/linux-theory-2.html
  2. It depends on architecture, and this is for x86. Thus, user can skip "loadAddress" section in "kernel" section in x86, and for other architecture, they have to specify the loadAddress.
  3. This is a good question! We don't have a logic to detect it yet. What I can come up roughly is analyzing the given elf file with load address and check whether it's valid?
25

As we discussed offline, kernel load address is always set to ffffffff81000000 for x86, so I'll keep default value and accept an optional load address.

persona0220 marked an inline comment as done.
wallace requested changes to this revision.Aug 2 2022, 4:36 PM

just some minor cosmetic changes :)

lldb/source/Plugins/Trace/intel-pt/TraceIntelPTBundleLoader.cpp
177–184

try to reduce duplication

lldb/source/Plugins/Trace/intel-pt/TraceIntelPTBundleSaver.cpp
345–350

convert these checks into asserts, as they should crash the program if they happen.

403–405

too much duplication. Better first calculate the processes and kernel sections, and then just perform one single invocation to JSONTraceBundleDescription()

lldb/source/Plugins/Trace/intel-pt/TraceIntelPTJSONStructs.cpp
149

revert your change that removes parsing processes here, that way you can dedup changes below

169–170

check for its existence because it's now an optional

178–181

remove this

lldb/source/Plugins/Trace/intel-pt/TraceIntelPTJSONStructs.h
58

also make this one Optional, following the changes in the schema

lldb/test/API/commands/trace/TestTraceLoad.py
273

also check the load address

This revision now requires changes to proceed.Aug 2 2022, 4:36 PM
wallace added inline comments.Aug 2 2022, 4:38 PM
lldb/source/Plugins/Trace/intel-pt/TraceIntelPTConstants.h
26

mention that github link here

persona0220 marked 9 inline comments as done.

Resolved comments

wallace requested changes to this revision.Aug 4 2022, 2:12 PM

almost there!

lldb/source/Plugins/Trace/intel-pt/TraceIntelPT.cpp
103–104

why did you remove this?

lldb/source/Plugins/Trace/intel-pt/TraceIntelPTBundleLoader.cpp
305

remove this line. We should allow the user to pass an empty array for "processes" or not passing it at all. This will make the json easier to work with

306

please break this into multiple lines of at most 80 chars each

lldb/source/Plugins/Trace/intel-pt/TraceIntelPTBundleSaver.cpp
376

that is enough

377

that is enough

lldb/test/API/commands/trace/TestTraceLoad.py
273
274

remove this one

lldb/test/API/commands/trace/intelpt-kernel-trace/trace.json
26

write another test with a custom load address

This revision now requires changes to proceed.Aug 4 2022, 2:12 PM
persona0220 marked 6 inline comments as done.
wallace added inline comments.Aug 4 2022, 2:53 PM
lldb/source/Plugins/Trace/intel-pt/TraceIntelPTBundleLoader.cpp
306

you didn't apply these suggestions:

  • add or not passed at all,
  • break into multiple lines
persona0220 marked an inline comment as done.Aug 4 2022, 3:03 PM
persona0220 added inline comments.
lldb/source/Plugins/Trace/intel-pt/TraceIntelPT.cpp
103–104

Because it seems that tids vector is not used anywhere

wallace accepted this revision.Aug 4 2022, 3:03 PM

nice job! Now you just need to connect it with the collector :)

rebasing to the upstream

This revision was not accepted when it landed; it landed in state Needs Review.Aug 4 2022, 5:15 PM
This revision was automatically updated to reflect the committed changes.