Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add UnitRef::shared_attrs #756

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

jyn514
Copy link

@jyn514 jyn514 commented Jan 21, 2025

Often, consumers of a DWARF file will want to know if a given function has an attribute, either directly or indirectly. Currently, finding that information is tedious and prone to error. Add a helper function in gimli to make this easier.

This does not attempt to have the maximum possible performance; consumers who care about that can still implement the logic themselves. In particular, the caches are recalculated each time shared_attrs is called, introducing an allocation.

The logic works roughly as follows (inspired by addr2line::Function::parse_children):

  • Create a cache of all DWARF units known to the UnitRef
  • Given an entry offset, iterate over all its attributes and pass them to a callback chosen by the caller.
  • For each indirect attribute, such as DW_AT_abstract_origin, follow the pointer to the abstract source and do the same thing again there.

I have tested this downstream (see rust-lang/rust#134831), but I have not yet added unit tests. Let me know how you would like this to be tested.

Often, consumers of a DWARF file will want to know if a given function has an attribute, either directly or indirectly. Currently, finding that information is tedious and prone to error. Add a helper function in gimli to make this easier.

This does not attempt to have the maximum possible performance; consumers who care about that can still implement the logic themselves. In particular, the caches are recalculated each time `shared_attrs` is called, introducing an allocation.

The logic works roughly as follows (inspired by [addr2line::Function::parse_children](https://github.com/gimli-rs/addr2line/blob/28ba2d45f2d22134915f55b46ddd6a039559366b/src/function.rs#L278)):

- Create a cache of all DWARF units known to the `UnitRef`
- Given an entry offset, iterate over all its attributes and pass them to a callback chosen by the caller.
- For each indirect attribute, such as `DW_AT_abstract_origin`, follow the pointer to the abstract source and do the same thing again there.

I have tested this downstream (see [rust-lang/rust#134831](rust-lang/rust#134831)), but I have not yet added unit tests. Let me know how you would like this to be tested.
jyn514 added a commit to jyn514/backtrace-rs that referenced this pull request Jan 21, 2025
This implements the runtime side of rust-lang/compiler-team#818.

- Extract a helper out of `find_frames` for iterating over a `LookupContinuation`
- Make the type signature for `search_object_map` consistent across all platforms. Backtraces are inherently platform specific, but let's not make it any harder on ourselves than we have to.
- Add a new `pub fn short_backtraces() -> enum { ThisFrameOnly, Start, End }` API
- Use the new [`gimli::UnitRef::shared_attrs`](gimli-rs/gimli#756) API to determine whether a given frame has a short backtrace. This, for now, requires a git dependency on gimli.
jyn514 added a commit to jyn514/backtrace-rs that referenced this pull request Jan 21, 2025
This implements the runtime side of rust-lang/compiler-team#818.

- Extract a helper out of `find_frames` for iterating over a `LookupContinuation`
- Make the type signature for `search_object_map` consistent across all platforms. Backtraces are inherently platform specific, but let's not make it any harder on ourselves than we have to.
- Add a new `pub fn short_backtraces() -> enum { ThisFrameOnly, Start, End }` API
- Use the new [`gimli::UnitRef::shared_attrs`](gimli-rs/gimli#756) API to determine whether a given frame has a short backtrace. This, for now, requires a git dependency on gimli.

Note that this currently does not work on MacOS. I do not have a Mac to test this; someone lent me theirs and I tracked it down to `cx.find_dwarf_and_unit` returning `None`, but I have not had the time or resources to narrow it down further than that.
jyn514 added a commit to jyn514/backtrace-rs that referenced this pull request Jan 21, 2025
This implements the runtime side of rust-lang/compiler-team#818.

- Extract a helper out of `find_frames` for iterating over a `LookupContinuation`
- Make the type signature for `search_object_map` consistent across all platforms. Backtraces are inherently platform specific, but let's not make it any harder on ourselves than we have to.
- Add a new `pub fn short_backtraces() -> enum { ThisFrameOnly, Start, End }` API
- Use the new [`gimli::UnitRef::shared_attrs`](gimli-rs/gimli#756) API to determine whether a given frame has a short backtrace. This, for now, requires a git dependency on gimli.

Note that this currently does not work on MacOS. I do not have a Mac to test this; someone lent me theirs and I tracked it down to `cx.find_dwarf_and_unit` returning `None`, but I have not had the time or resources to narrow it down further than that.
@philipc
Copy link
Collaborator

philipc commented Jan 22, 2025

I agree that it would be nice to make this easier, but I don't think this is the API we should be providing. I would like to see at least the following:

  • any cache that is created should allow reuse between calls
  • if this is something that addr2line also does, then the API should be suitable for use by addr2line as well, without loss of performance

Also note that UnitRef is a thin wrapper around Unit, so we shouldn't be adding large methods like this directly on UnitRef; they should be simple calls to something else.

@philipc
Copy link
Collaborator

philipc commented Jan 22, 2025

It appears you want to use this in backtrace which is already using addr2line, so a simpler alternative for now may to be add a method that does this to addr2line, so that it can use the cache that already exists there.

@jyn514
Copy link
Author

jyn514 commented Jan 23, 2025

It appears you want to use this in backtrace which is already using addr2line, so a simpler alternative for now may to be add a method that does this to addr2line, so that it can use the cache that already exists there.

hmm, I don't mind adding it there, but I worry that it's slightly out of scope for addr2line? If you're happy to take the PR I'm happy to make it though, I'll start work on that.

@philipc
Copy link
Collaborator

philipc commented Jan 23, 2025

I worry that it's slightly out of scope for addr2line?

Yes it is slightly, but so is find_dwarf_and_unit.

@jyn514
Copy link
Author

jyn514 commented Jan 25, 2025

a simpler alternative for now may to be add a method that does this to addr2line, so that it can use the cache that already exists there.

this turned out to a be a little more complicated than expected, so I'm going to try and make this new shared_attrs API more performant instead.

I can make the cache reusable, that's not too much trouble (although it makes the API slightly more complicated). addr2line is also doing a little dance where it uses unit.entries_raw instead of the simpler entries API. do you know why it does that? EntriesRaw has this comment:

EntriesRaw lacks some features of EntriesCursor, such as the ability to skip to the next sibling DIE. However, this also allows it to optimize better, since it does not need to perform the extra bookkeeping required to support these features, and thus it is suitable for cases where performance is important.

but addr2line::Function::parse_children is still skipping to the next sibling DIE, so I'm not sure it's saving time in practice? has this been benchmarked?

@philipc
Copy link
Collaborator

philipc commented Jan 26, 2025

but addr2line::Function::parse_children is still skipping to the next sibling DIE, so I'm not sure it's saving time in practice? has this been benchmarked?

The comment means that EntriesRaw does not have the equivalent of EntriesCursor::next_sibling, so you have to implement it yourself, and it is possible to do that in a way that is faster than EntriesCursor could do it. The use of EntriesRaw was benchmarked.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants