Page MenuHomeFreeBSD
Differential D37899

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

Authored by bnovkov on Dec 29 2022, 10:43 AM.
Tags
None
Referenced Files
Unknown Object (File)
Thu, Jun 4, 4:49 PM
Unknown Object (File)
Thu, Jun 4, 12:32 AM
Unknown Object (File)
Wed, May 27, 9:48 PM
Unknown Object (File)
Wed, May 27, 11:25 AM
Unknown Object (File)
Wed, May 27, 10:38 AM
Unknown Object (File)
Wed, May 27, 8:48 AM
Unknown Object (File)
Wed, May 27, 6:33 AM
Unknown Object (File)
Tue, May 26, 10:30 AM
View All Files
Subscribers
imp
jrtc27
markj
olce
zlei

Details

Reviewers
None
Group Reviewers
Contributor Reviews (src)
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
bnovkov updated this revision to Diff 114631.Dec 29 2022, 10:50 AM

Set default depth before printing.

markj added a subscriber: markj.Jan 4 2023, 3:27 AM

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.

bnovkov updated this revision to Diff 114781.Jan 5 2023, 7:40 PM

Address @markj 's comments.

bnovkov added a comment.Jan 5 2023, 7:45 PM
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
bnovkov updated this revision to Diff 123221.Jun 14 2023, 3:40 PM

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.

markj added a comment.Jun 19 2023, 10:11 PM

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
4

Do you want to include your name here?

sys/ddb/db_main.c
50

Why did you move this include? It's a prerequisite for other DDB headers.

sys/ddb/db_pprint.c
28

No need for cdefs.h or types.h here.

39

ddb.h should come before other ddb includes.

bnovkov updated this revision to Diff 127582.Sep 20 2023, 6:34 PM

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
39

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
1784 ↗(On Diff #136061)

?

sys/kern/link_elf.c
152

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
152

This got in here by mistake, will revert. Same goes for the deleted blank line.

Revision Contents
Changeset List

PathSize
sys/
conf/
2 lines
ddb/
1 line
59 lines
375 lines
6 lines
391 lines
1 line
kern/
14 lines

Diff 123221

View Options

sys/conf/files

Loading...
View Options

sys/ddb/db_command.c

Loading...
View Options

sys/ddb/db_ctf.h

Loading...
View Options

sys/ddb/db_ctf.c

Loading...
View Options

sys/ddb/db_main.c

Loading...
View Options

sys/ddb/db_pprint.c

Loading...
View Options

sys/ddb/ddb.h

Loading...
View Options

sys/kern/link_elf.c

Loading...