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

A small Rust wrapper around dl_iterate_phdr(3). #1

Merged
merged 1 commit into from
Oct 3, 2018
Merged

Conversation

vext01
Copy link
Member

@vext01 vext01 commented Sep 17, 2018

Let's start getting some Yorick stuff out of private repos.

Starting with the easy stuff, this is a little library to make it painless to inspect the program headers from Rust. I wrote it some time ago, and spent some time today tidying it up for code review.

In Yorick, we use this to know which object (compilation unit) a virtual address from a PT trace comes from.

One auto-generated test block us from merging this. I raised an issue for this a while back, but upstream have not yet had a chance to look: rust-lang/rust-bindgen#1370

We can still do the code review.

Cheers

src/lib.rs Outdated
assert_ne!(o.addr(), 0);

let obj_name = o.name().clone().into_string().unwrap();
println!("obj name: '{}'", obj_name);
Copy link
Member

Choose a reason for hiding this comment

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

Stray debugging output?

Copy link
Member Author

Choose a reason for hiding this comment

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

Oops, yes.

src/lib.rs Outdated
/// The name of the object.
name: CString,
/// Vector of program headers.
//phdrs: Vec<*const p_ffi::Elf_Phdr>,
Copy link
Member

Choose a reason for hiding this comment

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

Not sure I understand this line.

src/lib.rs Outdated
}
}

// The same as what `#[derive(Debug)]` would have done, but formats the fields using hex.
Copy link
Member

Choose a reason for hiding this comment

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

I think you can delete this comment.

src/lib.rs Outdated

impl ProgramHeader {
/// Returns the segment type (as one of the `PT_*` constants).
pub fn typ(&self) -> Elf_Word {
Copy link
Member

Choose a reason for hiding this comment

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

It's probably more conventional to call this type_.

flag_strs.push("PF_MASKPROC");
}
to_write.push_str(&format!("flags=<{}>, ", flag_strs.join("|")));

Copy link
Member

Choose a reason for hiding this comment

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

Slightly random blank 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.

The last 15 lines all deal with one field. It separates this from the following fields, which have one LoC per field.

Copy link
Member

Choose a reason for hiding this comment

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

OK.

src/lib.rs Outdated
type Item = ProgramHeader;

fn next(&mut self) -> Option<Self::Item> {
match self.num {
Copy link
Member

Choose a reason for hiding this comment

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

I think if self.num == 0 { ... } else { ... } would be clearer.

src/lib.rs Outdated
}

/// Returns a `Vec` of objects loaded into the current address space.
/// The reason this interface returns a `Vec` and not some kind of iterator, is that the C function
Copy link
Member

Choose a reason for hiding this comment

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

Something like "This returns a Vec because we have to read all the values from dl_iterate_phdr calls in one go (i.e. we have to ensure that calls to dl_iterate_phdr are not interspersed)."?

Copy link
Member Author

Choose a reason for hiding this comment

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

The first sentence is fine, but the second doesn't make sense. C calls our callback once per header, and we have no way to interrupt this to offer the user an intermediate result. Makes sense?

Copy link
Member

Choose a reason for hiding this comment

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

Because it's a callback?

Copy link
Member Author

Choose a reason for hiding this comment

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

Right.

Copy link
Member

Choose a reason for hiding this comment

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

OK, then I think I would simply delete the "the reason" sentence. It's not very important detail and (if I'm being brutal) it's a bit waffly at the moment. It's probably best that we only tell the user things they really need to know, and I don't think an API is a great place for this.

@ltratt
Copy link
Member

ltratt commented Sep 17, 2018

It might be nice to compile the example program at the same point as everything else? You might be able to use a workspace (such as https://github.com/softdevteam/grmtools/blob/master/Cargo.toml).

@vext01
Copy link
Member Author

vext01 commented Sep 17, 2018

Not saying it's a bad idea, but why would that be useful?

@ltratt
Copy link
Member

ltratt commented Sep 17, 2018

The example program might catch some compile-time bugs that aren't caught elsewhere? OK, in this case, probably not; but in things like grmtools, it can be useful (e.g. certain macro use stuff).

@vext01
Copy link
Member Author

vext01 commented Sep 17, 2018

I'd rather add cargo build --examples to the travis config (when that comes).

@ltratt
Copy link
Member

ltratt commented Sep 17, 2018

Yes, but then only Travis will catch things. Anyway, in this case, it's not a big deal either way (the example is tiny), but it might be worth thinking about for other repos in the future.

@vext01
Copy link
Member Author

vext01 commented Sep 17, 2018

That's all of the comments done, I think.

@ltratt
Copy link
Member

ltratt commented Sep 17, 2018

Ah, we should have a .travis.yml in this PR, and switch testing on. Why not do a commit for that, then we can think about merging?

@vext01
Copy link
Member Author

vext01 commented Sep 17, 2018

OK, prepare for travis testing commits.

@vext01
Copy link
Member Author

vext01 commented Sep 17, 2018

test p_ffi::bindgen_test_layout_La_x86_64_retval ...FAILED

That's the upstream bug I mentioned.

@ltratt
Copy link
Member

ltratt commented Sep 17, 2018

Aha!

@vext01
Copy link
Member Author

vext01 commented Sep 18, 2018

It hurt my head, but I think I found the root of the test failure.

bindgen erroneously emits f64s for long doubles and it causes a test failure.

Reported to upstream.

@vext01
Copy link
Member Author

vext01 commented Sep 19, 2018

Just to keep this PR in the loop. A fix for the failing test was in the works, but has become blocked on an LLVM alignment issue:
rust-lang/rust#54341

@vext01
Copy link
Member Author

vext01 commented Oct 3, 2018

OK, this fixes the auto-generated test. Ready for re-review.

.travis.yml Outdated
- export PATH="$PATH:$HOME/.cargo/bin"

script:
# By installing rustfmt, auto-generated code becomes more readable.
Copy link
Member

Choose a reason for hiding this comment

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

We install rustfmt but don't seem to use it?

Copy link
Member

Choose a reason for hiding this comment

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

Edd confirms that without the use of rustfmt, auto-generated code contains lines so long that they cause Travis problems.

Copy link
Member

Choose a reason for hiding this comment

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

A possible comment might be something like "We need to use rustfmt on auto-generated code: without it, code is put on a single line, which then causes Travis to crash because ...".

@vext01
Copy link
Member Author

vext01 commented Oct 3, 2018

Give this a shot

@vext01
Copy link
Member Author

vext01 commented Oct 3, 2018

Added the rustfmt config and ran rustfmt.

@ltratt
Copy link
Member

ltratt commented Oct 3, 2018

I think we're ready to squash?

@vext01
Copy link
Member Author

vext01 commented Oct 3, 2018

Splat.

@ltratt ltratt merged commit 5e2fa32 into master Oct 3, 2018
@ltratt ltratt deleted the initial branch October 3, 2018 16:25
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