-
Notifications
You must be signed in to change notification settings - Fork 4
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
Conversation
src/lib.rs
Outdated
assert_ne!(o.addr(), 0); | ||
|
||
let obj_name = o.name().clone().into_string().unwrap(); | ||
println!("obj name: '{}'", obj_name); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Stray debugging output?
There was a problem hiding this comment.
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>, |
There was a problem hiding this comment.
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. |
There was a problem hiding this comment.
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 { |
There was a problem hiding this comment.
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("|"))); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Slightly random blank line?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 { |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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)."?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right.
There was a problem hiding this comment.
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.
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). |
Not saying it's a bad idea, but why would that be useful? |
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). |
I'd rather add |
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. |
That's all of the comments done, I think. |
Ah, we should have a |
OK, prepare for travis testing commits. |
That's the upstream bug I mentioned. |
Aha! |
It hurt my head, but I think I found the root of the test failure. bindgen erroneously emits Reported to upstream. |
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: |
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. |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 ...".
Give this a shot |
Added the rustfmt config and ran rustfmt. |
I think we're ready to squash? |
Splat. |
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