search

headcrab-rs / headcrab

A modern Rust debugging library 🦀
https://headcrab.rs
Other
888 stars 32 forks source link

add line to address functinality #133

Open FlyingCanoe opened 3 years ago

FlyingCanoe commented 3 years ago

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.

nbaksalyar commented 3 years ago

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

FlyingCanoe commented 3 years ago

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

nbaksalyar commented 3 years ago

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

FlyingCanoe commented 3 years ago

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 :(

FlyingCanoe commented 3 years ago

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