Pluggable Symbol finder API, with Python support #241

brenns10 · 2023-01-20T00:51:18Z

This implements a pluggable Symbol Finder API, to complement the Object and Type finding APIs. There is also a Python binding.

I think it's a bit easier to review this by reading each commit rather than the full diff, but I don't know how easy Github makes that.

I'll highlight a few architectural points here that may merit discussion or debate.

The symbol finder API has a "result builder", because the API allows requesting exactly one, or multiple symbols. I think this is pretty important and wanted to avoid the additional allocation for an array of size one. The result builder helps smooth over the API and simplifies error handling.
Symbols now contain ownership info to determine how Drgn should treat the string (should it be copied? freed? or left alone). This wasn't necessary when passing symbols from drgn->python, because the names had the lifetime of the Program (LIFETIME_STATIC in this new system). Strings passed from python->drgn are subject to garbage collection, so they need to be copied and drgn will manage their lifetime.
~~The API exposes drgn_module as a struct forward declaration, it's unused at this time, except by the ELF symbol finder.~~

There are added tests which do demonstrate use from Python. You can also see the current kallsyms_finder branch for an example of using the C API.

(PR description is re-edited now that I've moved this out of draft state.)

brenns10 · 2023-08-22T18:03:48Z

@osandov I think the Symbol finder API is now ready to review! I figured I'd continue using this old PR for the branch, but I did update the description so it's all relevant.

brenns10 · 2023-09-18T16:15:53Z

@osandov I know you're busy, but I wanted to check if you had any initial feedback I can start working on here? Or perhaps I can break it down further into smaller chunks?

osandov

Sorry for the delay, this is looking pretty good. The biggest architectural comment I have is regarding the strdup in Symbol_wrap; the rest are more about the implementation or style.

libdrgn/drgn.h.in

libdrgn/program.c

libdrgn/python/symbol.c

libdrgn/python/program.c

libdrgn/python/symbol.c

libdrgn/python/program.c

_drgn.pyi

osandov · 2023-10-16T23:59:43Z

Fyi, I merged the first commit in this PR ("Correct types/docs for object+type finder API").

_drgn.pyi

libdrgn/python/program.c

brenns10 · 2023-10-20T23:35:47Z

Ok, I went ahead and rebased this branch on top of the latest main. That did involve some changes regarding the prog->dbinfo change you mentioned, and now the symbol finder API looks more or less exactly like the type/object finders.

I also tried to do a better review of my commits prior to sending them, and caught a few issues. I marked them so you could see what they were.

osandov

I'm finally getting back to this. I only re-reviewed patch 1 so far, but I just wanted to submit these before I head out for the day. Feel free to wait for me to finish looking at the rest. Thanks for your continued work on this!

libdrgn/symbol.c

libdrgn/symbol.h

libdrgn/program.c

libdrgn/program.h

libdrgn/symbol.c

libdrgn/program.h

libdrgn/stack_trace.c

libdrgn/symbol.h

libdrgn/drgn.h

libdrgn/python/program.c

libdrgn/drgn.h

brenns10 · 2024-02-29T19:00:40Z

Alright, I believe I've addressed everything from your review on Github as well as our discussion. The drgn_module is now removed from the API as discussed, which I recall was the only major change we decided on in the discussion. That does reduce the extent of these changes nicely, since I don't need to touch all the callers of the symbol finding APIs.

Normally I like to keep my branches rebased on top of the latest main, but it seems easiest for me to leave the base of the branch where it is, so you can more easily compare the revisions:

https://github.com/osandov/drgn/compare/43cf71669fcbc5f66c38f5904326de53c3dd346e..711399a7a159d0e9a772d16562c814382a86fc2f

I did do a quick compile & test for each commit in the branch to make sure all of my changes were "fixed up" into the correct commits. There are also two new commits on the branch as well: one for the __attribute__((__packed__)) change, and one to move the ELF symbol finders into debug_info.c.

osandov

A handful of mostly aesthetic things that I missed the first time around and a bug or two, but this looks great overall, thank you!

libdrgn/drgn.h

libdrgn/symbol.h

_drgn.pyi

libdrgn/python/program.c

osandov

Just a couple of comments that didn't get updated, and the commit message for "libdrgn: Use Symbol Finder API in find_symbol_by_address_internal()" is also stale: "the lookup API does allow us to keep the already-determined module" is no longer true.

Other than those tiny complaints, I'm happy with this! There's a bunch of stuff currently in main that I want to get in a release, but I'd like to give this new API a chance to sit for a bit before putting it in a release. So, my plan is to cut a new release early next week and then merge this immediately afterwards.

libdrgn/drgn.h

By using __attribute__((__packed__)), we shrink each enum from the default integer size of four bytes, down to the minimum size of one. This reduces the size of drgn_symbol from 32 bytes down to 26, with 6 bytes of padding. It doesn't have a practical benefit yet, but adding fields to struct drgn_symbol in the future may not increase the size. Signed-off-by: Stephen Brennan <stephen.s.brennan@oracle.com>

Symbol lookup is not yet modular, like type or object lookup. However, making it modular would enable easier development and prototyping of alternative Symbol providers, such as Linux kernel module symbol tables, vmlinux kallsyms tables, and BPF function symbols. To begin with, create a modular Symbol API within libdrgn, and refactor the ELF symbol search to use it. For now, we leave drgn_program_find_symbol_by_address_internal() alone. Its conversion will require some surgery, since the new API can return errors, whereas this function cannot. Signed-off-by: Stephen Brennan <stephen.s.brennan@oracle.com>

The following commit will modify it to use drgn_program_symbols_search(), a static function declared below. Move it underneath in preparation. No changes to the function. Signed-off-by: Stephen Brennan <stephen.s.brennan@oracle.com>

The drgn_program_find_symbol_by_address_internal() function is used when libdrgn itself may want to lookup a symbol: in particular, when formatting stack traces or objects. It does less work by possibly already having a Dwfl_Module looked up, and by avoiding memory allocation of a symbol, and it's more convenient because it doesn't return any errors, including on lookup failure. Unfortunately, the new symbol finder API breaks all of these properties: the returned symbol is now allocated via malloc() which needs cleanup on error, and errors can be returned by any finder via the lookup API. What's more, the finder API doesn't allow specifying an already-known module. Thankfully, error handling can be improved using the cleanup API, and looking up a module for an address is usually a reasonably cheap binary tree operation. Switch the internal method over to the new finder API. The major difference now is simply that lookup failures don't result in an error: they simply result in a NULL symbol. Signed-off-by: Stephen Brennan <stephen.s.brennan@oracle.com>

Now that the symbol finder API is created, we can move the ELF symbol implementation into the debug_info.c file, where it more logically belongs. The only change to these functions in the move is to declare elf_symbols_search as static. Signed-off-by: Stephen Brennan <stephen.s.brennan@oracle.com>

Previously, Symbol objects could not be constructed in Python. However, in order to allow Python Symbol finders, this needs to be changed. Unfortunately, Symbol name lifetimes are tricky to manage. We introduce a lifetime enumeration to handle this. The lifetime may be "static", i.e. longer than the life of the program; "external", i.e. longer than the life of the symbol, but no guarantees beyond that; or "owned", i.e. owned by the Symbol itself. Symbol objects constructed in Python are "external". The Symbol struct owns the pointer to the drgn_symbol, and it holds a reference to the Python object keeping the name valid (either the program, or a PyUnicode object). The added complexity is justified by the fact that most symbols are from the ELF file, and thus share a lifetime with the Program. It would be a waste to constantly strdup() these strings, just to support a small number of Symbols created by Python code. Signed-off-by: Stephen Brennan <stephen.s.brennan@oracle.com>

Expose the Symbol finder API so that Python code can be used to lookup additional symbols by name or address. Signed-off-by: Stephen Brennan <stephen.s.brennan@oracle.com>

Specify a "fake" symbol finder and then test that its results are plumbed through the API successfully. While this is a contrived test, it helps build confidence in the plumbing of the API. Signed-off-by: Stephen Brennan <stephen.s.brennan@oracle.com>

brenns10 · 2024-03-02T00:52:02Z

Thanks for the last catches, clearly I need a better find/replace workflow 😅
I updated the commit message as you mentioned, and also went ahead and reset the entire branch's author date, so that it doesn't have a crazy mixture of author dates in the log (I hate that!)

The release plan makes total sense, I wouldn't plop a this into a brand new release immediately if I were you. Looking forward to 0.0.26!

Thanks again for the detailed review, it's a much better product as a result!

brenns10 marked this pull request as draft January 20, 2023 00:51

brenns10 mentioned this pull request Mar 3, 2023

Implement kallsyms for vmlinux only #177

Closed

brenns10 force-pushed the symbol_finder branch from 205e08b to 4c65518 Compare March 3, 2023 23:42

brenns10 force-pushed the symbol_finder branch 2 times, most recently from 9cdb211 to 5171962 Compare May 11, 2023 22:28

brenns10 force-pushed the symbol_finder branch from 5171962 to b5e5fb8 Compare May 31, 2023 22:07

brenns10 force-pushed the symbol_finder branch 2 times, most recently from 45c555d to 9dc4c77 Compare June 29, 2023 17:00

brenns10 force-pushed the symbol_finder branch from 9dc4c77 to 0f6ef14 Compare August 22, 2023 16:18

brenns10 changed the title ~~Pluggable Symbol finder API, with Python support, + vmcoreinfo goodies~~ Pluggable Symbol finder API, with Python support Aug 22, 2023

brenns10 force-pushed the symbol_finder branch from 159620e to 50fd02b Compare August 22, 2023 17:59

brenns10 marked this pull request as ready for review August 22, 2023 18:02

This was referenced Aug 21, 2023

DWARFless Debugging #176

Open

[Draft] Kallsyms Symbol Finder #351

Closed

brenns10 force-pushed the symbol_finder branch from 50fd02b to a1dcb87 Compare September 12, 2023 01:15

osandov requested changes Oct 2, 2023

View reviewed changes

brenns10 commented Oct 2, 2023

View reviewed changes

libdrgn/python/program.c Outdated Show resolved Hide resolved

osandov reviewed Oct 9, 2023

View reviewed changes

_drgn.pyi Outdated Show resolved Hide resolved

_drgn.pyi Outdated Show resolved Hide resolved

brenns10 commented Oct 20, 2023

View reviewed changes

_drgn.pyi Outdated Show resolved Hide resolved

brenns10 commented Oct 20, 2023

View reviewed changes

libdrgn/python/program.c Outdated Show resolved Hide resolved

brenns10 force-pushed the symbol_finder branch from a1dcb87 to ed23e92 Compare October 20, 2023 23:34

brenns10 force-pushed the symbol_finder branch from ed23e92 to 14b65d3 Compare October 21, 2023 06:34

brenns10 force-pushed the symbol_finder branch from 14b65d3 to 43cf716 Compare December 8, 2023 23:52

osandov reviewed Feb 28, 2024

View reviewed changes

brenns10 commented Feb 29, 2024

View reviewed changes

libdrgn/drgn.h Outdated Show resolved Hide resolved

libdrgn/drgn.h Outdated Show resolved Hide resolved

brenns10 force-pushed the symbol_finder branch 2 times, most recently from 8fefbaf to 711399a Compare February 29, 2024 18:51

brenns10 requested a review from osandov February 29, 2024 19:00

osandov requested changes Mar 1, 2024

View reviewed changes

osandov reviewed Mar 1, 2024

View reviewed changes

libdrgn/python/program.c Outdated Show resolved Hide resolved

brenns10 force-pushed the symbol_finder branch from 711399a to eae4b39 Compare March 1, 2024 23:55

brenns10 requested a review from osandov March 1, 2024 23:58

osandov reviewed Mar 2, 2024

View reviewed changes

libdrgn/drgn.h Outdated Show resolved Hide resolved

libdrgn/drgn.h Outdated Show resolved Hide resolved

brenns10 added 8 commits March 1, 2024 16:46

libdrgn: move find_symbol_by_address_internal

70cd097

The following commit will modify it to use drgn_program_symbols_search(), a static function declared below. Move it underneath in preparation. No changes to the function. Signed-off-by: Stephen Brennan <stephen.s.brennan@oracle.com>

python: Add Program.add_symbol_finder()

50a2377

Expose the Symbol finder API so that Python code can be used to lookup additional symbols by name or address. Signed-off-by: Stephen Brennan <stephen.s.brennan@oracle.com>

Add test for Symbol Finder API

902e30f

Specify a "fake" symbol finder and then test that its results are plumbed through the API successfully. While this is a contrived test, it helps build confidence in the plumbing of the API. Signed-off-by: Stephen Brennan <stephen.s.brennan@oracle.com>

brenns10 force-pushed the symbol_finder branch from eae4b39 to 902e30f Compare March 2, 2024 00:48

osandov approved these changes Mar 4, 2024

View reviewed changes

osandov merged commit f96a3f5 into osandov:main Mar 11, 2024
38 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Pluggable Symbol finder API, with Python support #241

Pluggable Symbol finder API, with Python support #241

brenns10 commented Jan 20, 2023 •

edited

Loading

brenns10 commented Aug 22, 2023

brenns10 commented Sep 18, 2023

osandov left a comment

osandov commented Oct 16, 2023

brenns10 commented Oct 20, 2023

osandov left a comment

brenns10 commented Feb 29, 2024

osandov left a comment

osandov left a comment

brenns10 commented Mar 2, 2024

Pluggable Symbol finder API, with Python support #241

Pluggable Symbol finder API, with Python support #241

Conversation

brenns10 commented Jan 20, 2023 • edited Loading

brenns10 commented Aug 22, 2023

brenns10 commented Sep 18, 2023

osandov left a comment

Choose a reason for hiding this comment

osandov commented Oct 16, 2023

brenns10 commented Oct 20, 2023

osandov left a comment

Choose a reason for hiding this comment

brenns10 commented Feb 29, 2024

osandov left a comment

Choose a reason for hiding this comment

osandov left a comment

Choose a reason for hiding this comment

brenns10 commented Mar 2, 2024

brenns10 commented Jan 20, 2023 •

edited

Loading