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 line to address functinality #133

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

FlyingCanoe
Copy link

This PR add lines to address functionality to the library and to the repl example, but not to the GUI example.

Originally, I planned to write the line to address layers from scratch. But, as I struggle to do just that, I decided to read through, the addr2line crate source code to see how they deal with location in dwarf. While I was reading through, the source, I realize that the majority of the code needed for the functionality was already in addr2line, so is was simpler to implement the functionality in addr2line.

I asked if line2addr functionality was considered within the scope of addr2line and the answer was no.

This PR switch to a vendored version addr2line (the first three commit) and then implement the line to address functionality. The idea being that at a later time, the code of addr2line will be interpreted within headcrab symbolization layer. The symbolization layer is already kind of hackish and this PR certainly did not help with that.

If you ignore the switch to the vendored version of addr2line, this PR has approximately 300 additions and 100 deletion.

this commit update the gimli and addr2line dependancy

This requires changing a CompilationUnitHeader to a UnitHeader, since the former was removed in version 23.0.
this commit update the gimli and addr2line dependency

The load method was changed in version 24.0

Before, there was one load method which could be used to load the main object file as well as supplementary object file.
In the 24.0 version, there are two methods, load and load_sup. One for the main object file and the other for supplementary objects files.

since head crab does not currently support supplementary object files, this commit delete the stub loader which was use.
@nbaksalyar
Copy link
Member

Hey, thanks for this - looks great! I'm going to review this before the end of the day.

@FlyingCanoe
Copy link
Author

Also, before I forget, if you prefer, I could import the git history of addr2line. My PR did not import the history since I feel a 400 commit long PR may be awkward to navigate.

Also you may want to disable addr2line CI for the time being

Copy link
Member

@nbaksalyar nbaksalyar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The changes in the core headcrab libs look good. 👍 I haven't seen all changes in addr2line though - it's a bit harder to see what's been modified.

What I think we should do is to create a fork of addr2line under the organisation name and move it out of this source tree. It would make it easier to maintain it separately, plus we'll have CI and other nice things. You can then include it as a dependency like e.g. addr2line = { git = "https://github.com/headcrab-rs/addr2line"}.

What do you think?

Comment on lines 203 to 206
/// Find a address for each substatement in the `Location`.
/// This function return a hashmap where the key are the column number of the statement,
/// and where the value is a address of a instruction from that statement.
/// If the `Location` specify a column number, the hashmap will only contain one value.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nitpicking some typos (similar thing about the other docs for find_location_addr):

Suggested change
/// Find a address for each substatement in the `Location`.
/// This function return a hashmap where the key are the column number of the statement,
/// and where the value is a address of a instruction from that statement.
/// If the `Location` specify a column number, the hashmap will only contain one value.
/// Find an address for each substatement in the `Location`.
/// This function returns a hashmap where a key is a column number of each statement
/// and a value is an address of an instruction from that statement.
/// If the `Location` specifies a column number, the hashmap will only contain one value.

@nbaksalyar
Copy link
Member

Anyway, I just forked addr2line so please feel free to submit your PR there :) https://github.com/headcrab-rs/addr2line

@FlyingCanoe
Copy link
Author

What I think we should do is to create a fork of addr2line under the organisation name and move it out of this source tree. It would make it easier to maintain it separately, plus we'll have CI and other nice things.

It would be easier to maintain. That being said, I think it would be better to integrate addr2line with the symbolization layer as the interaction between the two are currently a bit messy. For example, the gimli::Dwarf in headcrab::ParsedDwarf currently resides within addr2line::Context so every time ParsedDwarf need access to gimli::Dwarf it must fist retrieve it. Or headcrab::Frame which contains an addr2line::Frame.
That being said, all those improvements would involve pretty heavy refactoring and I don't think we should prioritize this right now.
Also, it may make more sense to move functionality from headcrab to addr2line and have the fork become headcrab symbolization layer.
And to be honest I dont have experience working with large codebase, so I dont rely know the trade off between a monorepo and multirepo.
Also, this commit break some addr2line test (their more detail in the commit message) so the CI is also broken :(

this commit update the gimli and addr2line dependency
This commit add the pluming require to expose the line to address functionality from the fork of addr2line through headcrabe symbolization layer.
This commit breakup the set_breakpoint into multiple function.

First, by separating the logic to get the address from the one which put the breakpoint.
And second, by putting the logic for parsing the address and the symbols into their own function.

Those change will help to add line to addr functionality in a later commit.
this commit add the ability to set a  breakpoints by specifying the file name, the line number and (optional) the column number of the source code.

While it work, it is not very polish.

The error handling is sloppy.
It does no path canonization.
I have not update the UI the advertise the capability.
There are no path auto-completion.

In other word, it still is a prototype, but it work.
@FlyingCanoe
Copy link
Author

note, the version a just push switch to my fork of addr2line not the headcrabe fork of addr2line.

so before it is merge, the PR I send to the headcrabe should be merge and the URL in this commit should be change to point to the headcrabe fork of addr2line

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