Details

Reviewers

None

Group Reviewers

Commits

rGc21bc6f3c242: ddb: Add CTF-based pretty printing

Summary

This patch adds basic CTF support and a CTF-powered pretty-printer to ddb.
It uses the features added in the first patch to load and use CTF data from ddb.

The db_ctf.* files expose a basic interface for fetching type data for ELF symbols, interacting with the CTF string table, and translating type identifiers to type data.
The db_pprint.c file uses the aforementioned interfaces to implement a pretty-printer for all kernel ELF symbols.
Additionally, the pretty printing command takes an optional 'depth' parameter, making it easy to print out specific parts of complex data types.

Diff Detail

Repository

rG FreeBSD src repository

Lint

Lint Skipped

Unit

Tests Skipped

Event Timeline

bnovkov created this revision.Dec 29 2022, 10:43 AM

Herald added a subscriber: imp. · View Herald TranscriptDec 29 2022, 10:43 AM

bnovkov requested review of this revision.Dec 29 2022, 10:43 AM

Set default depth before printing.

The ddb(4) manual page should be updated as well. I and others can help with formatting if needed, i.e., you can just document it here in the review somewhere.

sys/ddb/db_command.c
169	The MEMSAFE flag shouldn't be set on this command, for the same reason it isn't set on the "x" command: it allows arbitrary kernel memory to be dumped to the console.
sys/ddb/db_ctf.c
6	Isn't this new code? The copyright attribution seems wrong.
sys/ddb/db_ctf.h
2	Missing copyright.
sys/ddb/db_pprint.c
6	Same here.

Address @markj 's comments.

In D37899#862064, @markj wrote:

The ddb(4) manual page should be updated as well. I and others can help with formatting if needed, i.e., you can just document it here in the review somewhere.

Thank you! Do you think that this should be in a separate patch or should I squeeze it in here?

sys/ddb/db_command.c
169	Thank you for pointing this out, I was only aware of the write part of the flag.
sys/ddb/db_ctf.c
6	I completely forgot to check the copyright stuff before submitting the patch, should be fixed now.

zlei added a subscriber: zlei.Jan 30 2023, 2:24 AM

I've changed the way the db_ctf subsystem registers kernel CTF data - it will now use preload_search_by_type during ddb initialization check if the loader loaded the file containing raw kernel CTF data. I've also added an extra code to copy kernel CTF data info from ddb into linker_kernel_file upon KLD initialization.

The db_ctf subsystem is now also able to register CTF data from kernel modules. Actually loading and using kernel module CTF data is planned for another patch, but I lay down the foundations for this here.

I spent some time testing this. For the purpose of loading CTF, I think it's best to simply punt on the problem for now and load CTF info after mountroot, like subr_firmware does. I wrote a patch here to demonstrate what I mean: https://github.com/markjdb/freebsd/commit/3985ddda16c0d0a7eb076a55ed33349090ad472c

In particular, I think the kernel linker should be the source of truth whenever possible. kern_linker.c already has a number of interfaces for use by kernel debuggers; I think it's best if we add a new one which can be used to look up (cached) CTF info for a particular linker file.

Regarding the functionality:

I think it would be quite useful to be able to pretty print an arbitrary address. Most of the time I have some pointer printed by another DDB command, and I want to examine the memory pointed to by that. OpenBSD has implemented this as "show struct" - would it be difficult to do the same?
pprint would be much more convenient if it resolved symbol names.
pprint (specifically db_ctf_find_symbol()) only looks up symbols in the kernel, it doesn't look up anything in kernel modules. I think we want a kernel linker routine akin to linker_debug_lookup() which can return the CTF container corresponding to a given address (or symbol name).
The formatting of "pprint" is kind of wonky. For example: https://reviews.freebsd.org/P580 . Can we indent nested struct fields?
There is no documentation of the new pprint command in ddb.4. I'm happy to do this part ahead of committing the patch.

sys/ddb/db_ctf.h
5	Do you want to include your name here?
sys/ddb/db_main.c
50 ↗	(On Diff #123221)	Why did you move this include? It's a prerequisite for other DDB headers.
sys/ddb/db_pprint.c
29	No need for cdefs.h or types.h here.
40	ddb.h should come before other ddb includes.

Address @markj 's comments.

List of changes:

Applied @markj 's mountroot loading patch
Output is now properly indented
pprint now takes either a symbol name or a struct name + address
All pprint commands now scan both the kernel and all loaded kernel modules for CTF info or symbol info
Added relevant docs in ddb.4

bnovkov marked 7 inline comments as done.Sep 20 2023, 6:36 PM

bnovkov added inline comments.

sys/ddb/db_pprint.c
40	That was caused by clang-format, fixed.

This revision was not accepted when it landed; it landed in state Needs Review.Mar 22 2024, 3:57 AM

Closed by commit rGc21bc6f3c242: ddb: Add CTF-based pretty printing (authored by bnovkov). · Explain Why

This revision was automatically updated to reflect the committed changes.

bnovkov marked an inline comment as done.

bnovkov added a commit: rGc21bc6f3c242: ddb: Add CTF-based pretty printing.

Herald added a subscriber: olce. · View Herald TranscriptMar 22 2024, 3:57 AM

jrtc27 added a subscriber: jrtc27.Mar 22 2024, 8:05 AM

jrtc27 added inline comments.

sys/ddb/db_ctf.h
3	These should all be BSD-2-Clause not the deprecated and misunderstood -FreeBSD variant.
sys/ddb/db_pprint.c
226–235	is surely what you want (well, with 32-bit fixed)? Otherwise this won't print the value if it's not one of the labeled enum values (e.g. if you do `myval = FOO \| BAR`, which is totally legal). Plus printing the parens around the value for the missing name case is strange.
272	Doesn't this make T and T * print identically in the depth not exceeded case?
279	Throwing away the qualifiers seems unhelpful. Though is this ever a case that's really hit, or is it only when the CTF is corrupted? (Perhaps anonymous structs hit this?)
sys/kern/kern_linker.c
1780	?
sys/kern/link_elf.c
153	Why the blank line?

bnovkov added inline comments.Mar 28 2024, 7:11 PM

sys/ddb/db_ctf.h
3	Oh, I forgot to change it before commiting, thanks for catching this!
sys/ddb/db_pprint.c
226–235	I agree, this makes more sense.
272	Sorry, I'm drawing a blank at the moment, could you provide an example?
279	Agreed, it makes more sense to them keep around. I've not encountered this case while testing personally, but the ones you listed seem plausible.
sys/kern/link_elf.c
153	This got in here by mistake, will revert. Same goes for the deleted blank line.

D59473 ixv: Add Hyper-V virtual function support

·Reviewers: Intel Networking, whu, kgalazka, Restricted Owners Package

Mon, Sep 7, 4:29 AM

Author: kbowling

ddb: Add basic CTF support [2/2]
ClosedPublic
Actions

Details

Diff Detail

Event Timeline

Revision Contents
Changeset List

Diff 127582

share/man/man4/ddb.4

sys/conf/files

sys/ddb/db_command.c

sys/ddb/db_ctf.h

sys/ddb/db_ctf.c

sys/ddb/db_pprint.c

sys/ddb/ddb.h

sys/kern/kern_ctf.c

sys/kern/kern_linker.c

sys/kern/link_elf.c

sys/kern/link_elf_obj.c

sys/kern/linker_if.m

sys/sys/linker.h

ddb: Add basic CTF support [2/2]ClosedPublicActions

Details

Diff Detail

Event Timeline

Revision ContentsChangeset List

Diff 127582

share/man/man4/ddb.4

sys/conf/files

sys/ddb/db_command.c

sys/ddb/db_ctf.h

sys/ddb/db_ctf.c

sys/ddb/db_pprint.c

sys/ddb/ddb.h

sys/kern/kern_ctf.c

sys/kern/kern_linker.c

sys/kern/link_elf.c

sys/kern/link_elf_obj.c

sys/kern/linker_if.m

sys/sys/linker.h

ddb: Add basic CTF support [2/2]
ClosedPublic
Actions

Revision Contents
Changeset List