Initial .eh_frame_hdr implementation #250

main-- · 2017-10-04T10:42:56Z

No description provided.

coveralls · 2017-10-04T11:06:47Z

Coverage remained the same at 0.0% when pulling 045b44f on main--:eh_frame_hdr into 6a3f391 on gimli-rs:master.

coveralls · 2017-10-04T11:06:47Z

Coverage remained the same at 0.0% when pulling 045b44f on main--:eh_frame_hdr into 6a3f391 on gimli-rs:master.

fitzgen

This is a great start!

The big thing we need before merging this is tests. We strive for branch coverage in our unit tests:

https://github.com/gimli-rs/gimli/blob/master/CONTRIBUTING.md#coverage

The ideal we aim to reach is having our unit tests exercise every branch in gimli. We allow an exception for branches which propagate errors inside a try!(..) invocation, but we do want to exercise the original error paths.

Pull requests adding new code should ensure that this ideal is met.

We use the test-assembler crate to create test cases for binary data. A little more details and links to examples here.

Here are some scenarios where I think we need test coverage:

The successful parsing case where we are given good .eh_frame_hdr data
When fde_count_enc is omit but table_enc is not omit, and vice versa. ParsedEhFrameHdr::table should do the right thing in these cases.
When each of fde_count_enc, table_enc, and eh_frame_ptr_enc are indirectly encoded.
When the version is not 1
When the binary search table has LEB encoding
When EhHdrTable::lookup is given an address that it does not have an entry for
When EhHdrTable::lookup is given an address it does have an entry for

Thanks a ton @main-- !

fitzgen · 2017-10-04T16:43:26Z

src/cfi.rs

+/// `EhFrameHdr` contains the information about the `.eh_frame` section.
+///
+/// A pointer to the start of the `.eh_frame` data, and optionally, a binary
+/// search table of pointers to the `.eh_frame` records are found in this section.


Nitpick:

to the .eh_frame records that are found in this section.

fitzgen · 2017-10-04T16:45:31Z

src/cfi.rs

@@ -89,6 +90,149 @@ impl<R: Reader> From<R> for DebugFrame<R> {
    }
 }

+/// `EhFrameHdr` contains the information about the `.eh_frame` section.


Nitpick: this introduction sentence should mention that this is the type for the .eh_frame_hdr section.

Oh, also please add this type to the list of sections / API entry points in the top level crate docs in src/lib.rs! Thanks!

fitzgen · 2017-10-04T16:53:43Z

src/cfi.rs

+
+/// `ParsedEhFrameHdr` contains the parsed information fron the `.eh_frame` section.
+#[derive(Clone, Debug)]
+pub struct ParsedEhFrameHdr<R: Reader> {


I don't love this name, but I also don't have any great suggestions :-p

fitzgen · 2017-10-04T16:54:22Z

src/cfi.rs

+    pub fn parse(&self, bases: &BaseAddresses, addr_size: u8) -> Result<ParsedEhFrameHdr<R>> {
+        let mut reader = self.0.clone();
+        let version = reader.read_u8()?;
+        assert_eq!(version, 1); // FIXME return error


Let's fix this before merging :)

fitzgen · 2017-10-04T17:04:28Z

src/cfi.rs

+        let fde_count = parse_encoded_pointer(fde_count_enc, bases, addr_size, &self.0, &mut reader)?;
+        let fde_count = match fde_count {
+            Pointer::Direct(c) => c,
+            Pointer::Indirect(_) => unreachable!(),


Is this really unreachable? I would expect there to be some early return if fde_count_enc.is_indirect() in order to make this actually unreachable, and not just a bad .eh_frame_hdr.

I think this probably should just return an error.

I mean, this isn't even a pointer, it's just a count that uses the pointer-encoding mechanism. I can't imagine any sane (or insane) reason why a compiler would emit an indirect pointer here and supporting this case just makes everything so very awkward (since we definitely can't just deref) - so why bother when this won't ever happen?

I think this probably should just return an error.

Agreed, and we should have test coverage.

fitzgen · 2017-10-04T17:12:01Z

src/cfi.rs

+    pub fn lookup(&self, address: u64, bases: &BaseAddresses) -> Result<Pointer> {
+        let size = match self.hdr.table_enc.format() {
+            constants::DW_EH_PE_uleb128 | constants::DW_EH_PE_sleb128
+                => return Err(Error::VariableLengthSearchTable),


I don't see anything in https://refspecs.linuxfoundation.org/LSB_1.3.0/gLSB/gLSB/ehframehdr.html that suggests that using LEBs is not valid here, although it does break binary search, as you point out. How about a comment to the effect that it doesn't make sense because of the binary search stuff, but that it isn't strictly prohibited.

Yeah, another tricky problem. Just like with fde_count, the compiler could in theory do something completely nonsensical here for no reason at all.

Again, I decided not to bother as that's what other implementations do (llvm, libunwind (lol)).

So I would say it's de facto prohibited.

Makes sense. We should leave a comment to this effect here and ... queue broken record ... have test coverage for this case.

libunwind (lol)

Wow...

fitzgen · 2017-10-04T17:23:00Z

src/cfi.rs

+                Pointer::Indirect(_) => return Err(Error::UnsupportedPointerEncoding),
+            };
+
+            match pivot.cmp(&address) {


I guess the reason that we can't read the table into a slice and use slice::binary_search_by is because we would need to repeat that code for all the different sizes?

I'd prefer to use the off-the-shelf binary search implementation if possible.

Perhaps some macro_rules to hide the code repetitition?

I actually originally intended to use std's binary search here. I ended up doing it this way instead when I didn't find a safe way (as in: not transmute) to convert from &[u8] to &[u32] and friends.

Yeah, I guess that transmute involves making sure that alignment is correct and all that too or else we'll perform unaligned loads and go into UB.

Ok, let's keep this binary search implementation.

fitzgen · 2017-10-04T17:36:02Z

Oh, and if you have any questions about test-assembler or anything like that, don't hesitate to ask me :)

coveralls · 2017-10-05T11:56:16Z

Coverage remained the same at 0.0% when pulling d79ba6d on main--:eh_frame_hdr into 6a3f391 on gimli-rs:master.

coveralls · 2017-10-05T11:56:16Z

Coverage remained the same at 0.0% when pulling d79ba6d on main--:eh_frame_hdr into 6a3f391 on gimli-rs:master.

philipc · 2017-10-10T03:05:20Z

Is this ready for another round of review?

main-- · 2017-10-10T04:06:36Z

Yes

fitzgen

👍 thanks a ton @main-- !

fitzgen · 2017-10-10T17:49:30Z

(For the record, it's always a safe-bet to drop a comment when a PR is ready to be looked at again -- half the time I don't get the "more commits pushed" emails...)

Initial .eh_frame_hdr implementation

045b44f

fitzgen requested changes Oct 4, 2017

View reviewed changes

main-- added 5 commits October 4, 2017 22:25

Fix docs

2fbe4d1

Fix errors

d5eba03

Handle empty tables, document behavior

0dfc604

Fix edge cases

caa8bda

Add tests

d79ba6d

main-- force-pushed the eh_frame_hdr branch from 241a852 to d79ba6d Compare October 5, 2017 11:34

fitzgen approved these changes Oct 10, 2017

View reviewed changes

fitzgen merged commit 0d69019 into gimli-rs:master Oct 10, 2017

main-- deleted the eh_frame_hdr branch October 10, 2017 18:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Initial .eh_frame_hdr implementation #250

Initial .eh_frame_hdr implementation #250

main-- commented Oct 4, 2017

coveralls commented Oct 4, 2017

coveralls commented Oct 4, 2017

fitzgen left a comment

fitzgen Oct 4, 2017

fitzgen Oct 4, 2017

fitzgen Oct 4, 2017

fitzgen Oct 4, 2017

main-- Oct 4, 2017

fitzgen Oct 4, 2017

fitzgen Oct 4, 2017

main-- Oct 4, 2017

fitzgen Oct 4, 2017

fitzgen Oct 4, 2017

main-- Oct 4, 2017

fitzgen Oct 4, 2017

fitzgen Oct 4, 2017

fitzgen Oct 4, 2017

main-- Oct 4, 2017

fitzgen Oct 4, 2017

fitzgen commented Oct 4, 2017

coveralls commented Oct 5, 2017

coveralls commented Oct 5, 2017

philipc commented Oct 10, 2017

main-- commented Oct 10, 2017

fitzgen left a comment

fitzgen commented Oct 10, 2017

Initial .eh_frame_hdr implementation #250

Initial .eh_frame_hdr implementation #250

Conversation

main-- commented Oct 4, 2017

coveralls commented Oct 4, 2017

coveralls commented Oct 4, 2017

fitzgen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fitzgen commented Oct 4, 2017

coveralls commented Oct 5, 2017

coveralls commented Oct 5, 2017

philipc commented Oct 10, 2017

main-- commented Oct 10, 2017

fitzgen left a comment

Choose a reason for hiding this comment

fitzgen commented Oct 10, 2017