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

Implement addr2line #47

Merged
merged 1 commit into from
Aug 18, 2016
Merged

Implement addr2line #47

merged 1 commit into from
Aug 18, 2016

Conversation

khuey
Copy link
Contributor

@khuey khuey commented Aug 16, 2016

This definitely isn't ready for merging yet, but it might be ready to look at.

Notable todos:

  • Should we factor out object loading support for the examples/ somewhere?
  • Should parsing DW_AT_stmt_list be handled inside gimli somewhere? (probably)
  • Get the directory name instead of just printing the index

@fitzgen
Copy link
Member

fitzgen commented Aug 16, 2016

Should we factor out object loading support for the examples/ somewhere?

That would be great. I don't know if we can add modules to examples/ or not, but I feel like the obj loading doesn't belong in the library proper (especially as it is currently implemented).

Maybe we should factor it out into a new crate and do it Right (unless some other crate abstracting obj files already exists that seems good enough).

Should parsing DW_AT_stmt_list be handled inside gimli somewhere? (probably)

Yes, it is on my TODO list to make a method on UnitHeader that takes the DebugLine and returns the LineNumberProgramHeader (probably as an Option because it might not exist and I'm also unsure whether partial units have them). First, I need to write more unit tests for line number program execution, however. Feel free to make that method yourself, if you want, as you'll probably be able to beat me to the punch.

@@ -15,6 +15,7 @@ version = "0.6.0"
[dependencies]
byteorder = "0.5.3"
leb128 = "0.2.1"
getopts = "0.2"
Copy link
Member

Choose a reason for hiding this comment

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

This should be under [dev-dependencies], which are only included for tests and examples.

http://doc.crates.io/specifying-dependencies.html#development-dependencies

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@khuey khuey force-pushed the addr2line branch 4 times, most recently from b257f2a to a65de63 Compare August 17, 2016 03:24
@khuey
Copy link
Contributor Author

khuey commented Aug 17, 2016

This is ready for another pass, @fitzgen.

}
}

return u64::from_str_radix(string, 16).expect("Failed to parse address");
Copy link
Member

Choose a reason for hiding this comment

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

Nit: rust style is to leave out return and the semicolon when in tail position.

It's not just functions either. Rust's block "statements" will evaluate to their last expression, if you drop the semicolon, eg:

let foo = {
    bar();
    1
};
assert_eq(foo, 1);

Same for ifs, etc.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I know this, but it's a hard impulse to break ;) Thanks for the reminder.

@fitzgen
Copy link
Member

fitzgen commented Aug 17, 2016

Ok, I left a bunch of comments, but they are all very nit picky. Consider it an r=me with comments. I'll merge as soon you update.

Thanks!

@khuey
Copy link
Contributor Author

khuey commented Aug 18, 2016

If github isn't sending you emails when I overwrite commits, this is ready to merge @fitzgen.

@fitzgen
Copy link
Member

fitzgen commented Aug 18, 2016

Huh, I'm not getting those emails.

@fitzgen fitzgen merged commit 25ce778 into gimli-rs:master Aug 18, 2016
@fitzgen
Copy link
Member

fitzgen commented Aug 18, 2016

Thanks again!

@khuey
Copy link
Contributor Author

khuey commented Aug 18, 2016

np, I'm learning a lot from your reviews :)

@khuey khuey deleted the addr2line branch August 18, 2016 14:55
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