ddb: Add basic CTF support [2/2]

[bnovkov]

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.

[bnovkov]

Set default depth before printing.

[markj]

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]

Address @markj 's comments.

[bnovkov]

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.

[bnovkov]

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]

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.

[bnovkov]

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]

sys/ddb/db_pprint.c

40

That was caused by clang-format, fixed.

[jrtc27]

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]

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.