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.

Sign up for GitHub

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

Jump to bottom

Implemented a solution for #67 #102

Merged
merged 3 commits into from
Sep 30, 2018
Merged

Implemented a solution for #67 #102

merged 3 commits into from
Sep 30, 2018

Conversation

ibabushkin
Copy link
Contributor

I think the provided iterator implementation and boilerplate does what is needed to lazily parse relocations.

m4b
m4b reviewed Sep 29, 2018
View reviewed changes
Copy link
Owner

@m4b m4b left a comment

Choose a reason for hiding this comment

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

@ibabushkin This is really great, thanks for the PR! This looks good to me, modulo the doc comments; in the meantime, can you rebase on latest master, so the build/CI is fixed?

Also, I'll check this is backwards compatible with something like bingrep, but I think it's ok regardless, since we need a lazy iterator on relocs; this is great work!

Thanks for your contribution!

src/elf/reloc.rs
@@ -391,4 +380,111 @@ if_alloc! {
}
}
}

#[derive(Default)]
pub struct RelocSection<'a> {
Copy link
Owner

Choose a reason for hiding this comment

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

Doc comments please :)

src/elf/reloc.rs

impl<'a> RelocSection<'a> {
#[cfg(feature = "endian_fd")]
pub fn parse(bytes: &'a [u8], offset: usize, filesz: usize, is_rela: bool, ctx: Ctx) -> ::error::Result<RelocSection<'a>> {
Copy link
Owner

Choose a reason for hiding this comment

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

Doc comment

src/elf/reloc.rs
if index >= self.count {
None
} else {
Some(self.bytes.pread_with(index * Reloc::size_with(&self.ctx), self.ctx).unwrap())
Copy link
Owner

Choose a reason for hiding this comment

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

Because constructor checks the size, we're safe to unwrap here and in the iterator, correct?

Copy link
Contributor Author

@ibabushkin ibabushkin Sep 30, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Yep, pretty much. The Symtab iterator is constructed in the same way.

@m4b m4b mentioned this pull request Sep 29, 2018
Closed
@ibabushkin
Copy link
Contributor Author

Alright, I rebased on the new master and added the doc comments you asked for :)

@m4b
Copy link
Owner

m4b commented Sep 30, 2018

@ibabushkin Thanks so much! So the PR is techncially backwards incompatible; i used as_slice a couple of times, but I don't think its that big of a deal.

However, this one is strange:

error[E0631]: type mismatch in closure arguments                                                                                                      
   --> src/format_elf.rs:336:59                                                                                                                       
    |                                                                                                                                                 
336 |         let num_shdr_relocs = self.elf.shdr_relocs.iter().fold(0, &|acc, &(_, ref v): &(usize, Vec<_>)| acc + v.len());                         
    |                                                           ^^^^     -------------------------------------------------- found signature of `for<'r> fn(_, &'r (usize, std::vec::Vec<_>)) -> _`
    |                                                           |                                                                                     
    |                                                           expected signature of `fn({integer}, &(usize, goblin::elf::reloc::RelocSection<'_>)) -> _`
    |                                                                                                                                                 
    = note: required because of the requirements on the impl of `std::ops::FnMut<({integer}, &(usize, goblin::elf::reloc::RelocSection<'_>))>` for `&[closure@src/format_elf.rs:336:68: 336:118]`

I probably just didn't read it carefully; @philipc are you ok with these changes? Is it going to be a pain for object?

@m4b
Copy link
Owner

m4b commented Sep 30, 2018

To be clear I don't care much about moving from Vec to iterator, as we need lazy iteration, and this is great!

Just not missing anything from the api it is presenting :)

m4b
m4b approved these changes Sep 30, 2018
View reviewed changes
Copy link
Owner

@m4b m4b left a comment

Choose a reason for hiding this comment

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

Awesome, LGTM. This appears to have no effect on object, so merging!

@m4b m4b merged commit 9693b81 into m4b:master Sep 30, 2018
@m4b
Copy link
Owner

m4b commented Sep 30, 2018

@ibabushkin Thanks for the PR, decided to just merge :) We need lazy relocation iterators

@ibabushkin
Copy link
Contributor Author

Great! Glad to help.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@m4b m4b m4b approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@ibabushkin @m4b