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

More CFI stuff #241

Merged
merged 7 commits into from
Aug 23, 2017
Merged

More CFI stuff #241

merged 7 commits into from
Aug 23, 2017

Conversation

fitzgen
Copy link
Member

@fitzgen fitzgen commented Aug 21, 2017

See each commit for details.

Holding off on any version bumps until I've verified down that all the APIs I need really are exposed now.

It had dang well better be signal safe to reset an initialized context ;)
Previously, we only enabled recovery of the unwinding context when operations
would succeed. This commit enables recovery when operations fail as well.

Things get a little funky because of the borrow checker, unfortunately.
Copy link
Collaborator

@philipc philipc left a comment

Choose a reason for hiding this comment

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

Approved once the build is working, looks like object crate version problems.

@@ -639,11 +641,15 @@ struct CfiEntryCommon<R: Reader> {
/// end-of-entries sentinel, return `Ok(None)`. Otherwise, return
/// `Ok(Some(tuple))`, where `tuple.0` is the start of the next entry and
/// `tuple.1` is the parsed CFI entry data.
fn parse_cfi_entry_common<Section, R>(input: &mut R) -> Result<Option<CfiEntryCommon<R>>>
fn parse_cfi_entry_common<Section, R>(
section: Section,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nit: maybe passing &R for the section would be better.

Copy link
Member Author

Choose a reason for hiding this comment

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

By passing Section, we know its pointing to the start of the whole section, whereas with an &R, it could be at some offset within the section.

@@ -178,7 +192,7 @@ fn main() {
let file = match object::File::parse(unsafe { file.as_slice() }) {
Ok(file) => file,
Err(err) => {
println!("Failed to parse file '{}': {}", file_path, err);
println!("Failed to parse file '{}': {}", file_path, Error::from(err));
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why is this needed?

None => return Ok(()),
Some(gimli::CieOrFde::Cie(cie)) => {
println!();
// TODO: CIE offset
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is something different from the offset on the next line?

Copy link
Member Author

Choose a reason for hiding this comment

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

... no :-P

I just forgot to remove the TODO

@@ -8,6 +8,9 @@ extern crate memmap;
extern crate object;

use fallible_iterator::FallibleIterator;
use gimli::UnwindSection;
use object::Object;
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is deprecated, and object::Error doesn't exist. Seems like you are using an old object version? Cargo.toml specifies 0.4.0 though.

@fitzgen
Copy link
Member Author

fitzgen commented Aug 22, 2017

I think I must have either been working with an old Cargo.lock and a bug in cargo, or I must have forgotten to pull down new changes or something...

This does most of the information that the canonical dwarfdump does. Still
remaining are things like symbolicating the start addresses in address ranges,
turning register numbers into the target architecture's register names, and
printing augmentation data. None of this is impossible to implement for any
reason right now, I just haven't done it.

Fixes half of gimli-rs#116
@fitzgen
Copy link
Member Author

fitzgen commented Aug 23, 2017

Going to just go ahead and merge this. OSX backlogs on travis CI are super high again, but 2/3 of them succeeded, so I'm pretty confident the last one will as well.

@fitzgen fitzgen merged commit de7eeff into gimli-rs:master Aug 23, 2017
@fitzgen fitzgen deleted the more-cfi-stuff branch August 23, 2017 02:19
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.08%) to 94.039% when pulling 3a01eda on fitzgen:more-cfi-stuff into 9b134a0 on gimli-rs:master.

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.

None yet

3 participants