Added experimental ELF/Mach-O/PE compatibility using gimli/object. #74

data-pup · 2018-07-02T20:44:49Z

This PR aims to introduce the ability to parse subroutines from an ELF/Mach-O binary, using the gimli/object crates.

Fixes #69.

data-pup · 2018-07-02T21:10:03Z

Travis failed on the nightly's wasm job, with this error:

error: Could not compile `miniz-sys`.
warning: build failed, waiting for other jobs to finish...
error: build failed

fitzgen

Thanks so much for diving into this @data-pup! 🎉

I think we can fix the wasm compilation issues with:

making a "dwarf" cargo feature for this DWARF/etc support
enabling the "dwarf" feature for the twiggy CLI, but disabling it for twiggy-wasm-api

A bunch of inline questions and nitpicks below, but overall this is looking super sharp.

Thanks again @data-pup !

fitzgen · 2018-07-03T17:39:08Z

parser/Cargo.toml

-twiggy-traits = { version = "=0.2.0", path = "../traits" }
+typed-arena = "1.3.0"
+twiggy-ir = { version = "0.2.0", path = "../ir" }
+twiggy-traits = { version = "0.2.0", path = "../traits" }


Was there a reason to remove the = bound on the version here? That helps us ensure that we are exactly matching twiggy-* crates together.

Oops! This was definitely accidental. I ran into some conflicts while rebasing this work before opening this PR, good eye! I'll fix that :)

fitzgen · 2018-07-03T17:39:45Z

parser/Cargo.toml

 parity-wasm = "0.28.0"
-twiggy-ir = { version = "=0.2.0", path = "../ir" }
-twiggy-traits = { version = "=0.2.0", path = "../traits" }
+typed-arena = "1.3.0"


Let's do typed-arena = "1.4" here

fitzgen · 2018-07-03T17:56:26Z

parser/object_parse/compilation_unit_parse/mod.rs

+        // Use the abbreviations to create an entries cursor, and move it to the root.
+        let abbrevs = self
+            .abbreviations(&debug_abbrev)
+            .expect("Could not find abbreviations");


We should propagate errors with ? here, and maybe use failure::ResultExt::context if we want to provide an extra context message.

fitzgen · 2018-07-03T17:57:30Z

parser/object_parse/compilation_unit_parse/mod.rs

+            .abbreviations(&debug_abbrev)
+            .expect("Could not find abbreviations");
+        let mut die_cursor = self.entries(&abbrevs);
+        assert!(die_cursor.next_dfs().unwrap().is_some());


I don't think we should assert! this -- if we get malformed input, that should result in an error returned and formatted to the user, not a panic.

fitzgen · 2018-07-03T18:09:03Z

ir/ir.rs

+    Scope(Scope),
+
+    /// Subroutine item.
+    Subroutine(Subroutine),


How is this intended to be different from Code? I would have expected parsing a DW_TAG_subroutine to create a Code IR item kind.

fitzgen · 2018-07-03T18:19:48Z

parser/object_parse/die_parse/item_kind.rs

+        gimli::DW_TAG_member => None,
+        // Variant entries. (Section 5.7.10)
+        gimli::DW_TAG_variant => None,
+        gimli::DW_TAG_variant_part => None,


I don't think we should do an exclusive match here, because we explicitly don't care about most of these things. It would be easier to do:

match die.tag() { // Match things we care about, and do what we will do with them. gimli::DW_TAG_something_we_care_about => do_stuff(die), // And then just skip everything else. _ => return None, }

fitzgen · 2018-07-03T18:26:35Z

parser/object_parse/die_parse/mod.rs

+/// This struct represents the extra items required by the Parse trait's
+/// `parse_items` method. This is constructed by the compilation unit's
+/// own implementation of `parse_items`.
+pub struct DIEItemsExtra<'unit, R>


Nitpick: Rust style is to only capitalize the first letter of an acronym inside an identifier: DieItemsExtra

fitzgen · 2018-07-03T18:29:12Z

parser/object_parse/die_parse/mod.rs

+                    .entity_size(addr_size, dwarf_version, rnglists)?
+                    .map(|size| ir::Item::new(id, name, size as u32, kind))
+            }
+            _ => None,


If we early return Ok(()) here, then item doesn't need to be an Option and we can avoid the if let Some below. This would be a nice little code clean up.

fitzgen · 2018-07-03T18:30:37Z

parser/object_parse/mod.rs

+
+        // Load the sections of the file containing debugging information.
+        let debug_abbrev: gimli::DebugAbbrev<_> = load_section(&arena, self, endian);
+        let _debug_aranges: gimli::DebugAranges<_> = load_section(&arena, self, endian);


If we aren't using the section, then I don't think it makes sense to load it. We can just drop this line.

fitzgen · 2018-07-03T18:34:18Z

parser/object_parse/mod.rs

+        };
+
+        // Load the sections of the file containing debugging information.
+        let debug_abbrev: gimli::DebugAbbrev<_> = load_section(&arena, self, endian);


Idle thought: it may eventually make sense to make all the sections be Extra parameters for this parse implementation, and then that way we wouldn't need to duplicate the loads of each section (which potentially means decompressing sections multiple times).

Definitely not something we need to worry about yet, however!

I tried taking a swing at that, and ended up running into some errors regarding unconstrained type parameters. I'm sure this is doable, it's just a little outside of my previous Rust knowledge. Would happily create an issue and follow up on that once the rest of the things here are taken care of, if you think that makes sense :)

Follow up issue is fine :)

data-pup · 2018-07-03T19:07:23Z

Thanks for the feedback! Figured I'd answer some of the questions above in a single comment.

Re: ir::ItemKind variants, the Type kind is definitely superfluous. That's a holdover from some earlier iterations of this work, I can take that out. Same goes for Scope, I can remove that as well. Your point about the Subroutine variant is also a good one, I can adjust the code so that this creates a Code item.

Thanks for the other points, I think those all make a lot of sense! I'll make some edits to take care of those issues, and push a new commit once that's ready. Thanks for the feeback 😄

data-pup · 2018-07-06T00:36:59Z

Pushed some commits to address the various notes above. The point about features is still a little new to me however. I can read up on those tomorrow and see if I can take care of that detail. Would that be a feature in the parser/Cargo.toml file specifically?

Update: Took a swing at creating a "dwarf" feature, but it seemed to stall the build 😢 If you have any advice on what I may be doing wrong here, let me know! Haven't used Cargo's features functionality before.

data-pup · 2018-07-06T14:41:08Z

parser/object_parse/die_parse/mod.rs

+                    ir::Item::new(id, name, size as u32, kind)
+                } else {
+                    return Ok(());
+                }


I added early returns here, I think it helped clean things up a little. It's worth noting that the entity_size method here returns an Option because (if I understand correctly) not all DW_AT_subprogram DIEs are describing a definition. These DIEs don't include the same location attributes, in which case this method returns None.

fitzgen · 2018-07-06T17:13:46Z

parser/Cargo.toml

+
+[features]
+default = ["dwarf"]
+dwarf = ["fallible-iterator", "gimli", "object", "typed-arena"]


Yep, this looks correct. Not sure why builds are hanging now.

It seems like maybe this was just a Travis CI infra issue -- I've restarted the job to see.

fitzgen · 2018-07-06T17:17:11Z

parser/object_parse/compilation_unit_parse/mod.rs

@@ -1,10 +1,12 @@
+#[cfg(feature = "dwarf")]


It could be easier to define a macro like:

macro_rules! if_dwarf { ( $( $x:item )* ) => { $( #[cfg(feature = "dwarf")] $x )* } } if_dwarf! { // ... definitions and things for when dwarf is enabled go in here ... }

and then you could use it throughout the rest of the crate rather than adding the #[cfg(...)]s manually on every single definition.

data-pup · 2018-07-10T18:12:03Z

I think I ended up finding a cleaner way around this. Because nothing in parser/object_parse would ever be used unless we've enabled the 'dwarf' feature, we can use the cfg attribute on that mod statement specifically. This keeps the various #[cfg(feature = "dwarf")] statements localized to parser.rs, and eliminates the need to have these scattered about in the object file parsing code.

Edit: One minor detail I did notice, is that because this work introduces a dependency on gimli to the traits crate, it'd be nice to only include that dependency if we're using the dwarf feature. Not totally sure how to do that atm, but I can cycle back on that once this does build successfully :)

fitzgen · 2018-07-10T19:10:32Z

Edit: One minor detail I did notice, is that because this work introduces a dependency on gimli to the traits crate, it'd be nice to only include that dependency if we're using the dwarf feature. Not totally sure how to do that atm, but I can cycle back on that once this does build successfully :)

The way to do this is to make gimli and object be optional dependencies, and have the dwarf feature enable them:

[features]
dwarf = ["gimli", "object"]

fitzgen

Looking great! Just one tiny nitpick left before we merge :)

fitzgen · 2018-07-10T19:44:13Z

traits/Cargo.toml

@@ -13,6 +13,7 @@ path = "./traits.rs"

 [dependencies]
 failure = "0.1.1"
+gimli = "0.16.0"


This dependency should be optional, and only enabled when the parser has the dwarf feature enabled.

fitzgen · 2018-07-10T19:44:51Z

traits/traits.rs

@@ -57,6 +58,15 @@ enum ErrorInner {

    #[fail(display = "Regex error: {}", _0)]
    Regex(#[cause] regex::Error),
+
+    #[fail(display = "Gimli error: {}", _0)]


And this variant should be cfged for when gimli is enabled.

fitzgen · 2018-07-10T22:24:05Z

traits/traits.rs

@@ -99,6 +110,14 @@ impl From<regex::Error> for Error {
    }
 }

+impl From<gimli::Error> for Error {


(This is going to need the cfg too -- sorry I forgot to point it out earlier!)

No problem! Pardon, I should've seen that too. Fixing that now!

fitzgen · 2018-07-10T23:24:28Z

🎉 🎈 🎉

data-pup requested a review from fitzgen July 2, 2018 20:46

fitzgen reviewed Jul 3, 2018

View reviewed changes

data-pup force-pushed the binary-compat branch from 28bdfcc to 803ad83 Compare July 6, 2018 01:03

data-pup commented Jul 6, 2018

View reviewed changes

fitzgen approved these changes Jul 6, 2018

View reviewed changes

data-pup force-pushed the binary-compat branch 2 times, most recently from d98c96d to 8c9216f Compare July 6, 2018 17:57

data-pup added 14 commits July 10, 2018 13:30

Added experimental ELF/Mach-O/PE compatibility using gimli/object.

bf43b00

twiggy-* crates must be the exact correct version.

611539a

Upgrading the typed-arena dependency to 1.4.0.

fb81b50

Removed the type entry variant of ir::ItemKind.

dae9034

Removed the scope variant of ir::ItemKind.

c5dbb8c

Removed potential panics, added error handling.

7767042

Replaced calls to unimplemented macro.

935a1a2

Cleaned up exclusive matched for various DWARF tags.

eefa7a6

Renamed the DieItemsExtra class.

c333615

Added early returns to the DIE parse_items implementation.

bf3ef65

Removed unused section in the object parse section.

12764f8

Added a dwarf feature to twiggy-parser.

06cf987

Added cfg flags.

1970669

Cleaned up the Subroutine variant of the ItemKind enum.

822161b

data-pup force-pushed the binary-compat branch from 8c9216f to 00ab11a Compare July 10, 2018 18:07

Only use the object_parse module if the dwarf feature is enabled.

82ecd65

data-pup force-pushed the binary-compat branch from 00ab11a to 82ecd65 Compare July 10, 2018 18:10

fitzgen approved these changes Jul 10, 2018

View reviewed changes

Added dwarf feature to traits crate.

d0e395f

fitzgen reviewed Jul 10, 2018

View reviewed changes

Added cfg flag for From trait.

84f78f2

fitzgen merged commit 19076f9 into rustwasm:master Jul 10, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added experimental ELF/Mach-O/PE compatibility using gimli/object. #74

Added experimental ELF/Mach-O/PE compatibility using gimli/object. #74

data-pup commented Jul 2, 2018

data-pup commented Jul 2, 2018

fitzgen left a comment

fitzgen Jul 3, 2018

data-pup Jul 3, 2018

fitzgen Jul 3, 2018

fitzgen Jul 3, 2018

fitzgen Jul 3, 2018

fitzgen Jul 3, 2018

fitzgen Jul 3, 2018

fitzgen Jul 3, 2018

fitzgen Jul 3, 2018

fitzgen Jul 3, 2018

fitzgen Jul 3, 2018

data-pup Jul 6, 2018

fitzgen Jul 6, 2018

data-pup commented Jul 3, 2018

data-pup commented Jul 6, 2018 •

edited

Loading

data-pup Jul 6, 2018

fitzgen Jul 6, 2018

fitzgen Jul 6, 2018

fitzgen Jul 6, 2018

data-pup commented Jul 10, 2018 •

edited

Loading

fitzgen commented Jul 10, 2018

fitzgen left a comment

fitzgen Jul 10, 2018

fitzgen Jul 10, 2018

fitzgen Jul 10, 2018

data-pup Jul 10, 2018

fitzgen commented Jul 10, 2018

Added experimental ELF/Mach-O/PE compatibility using gimli/object. #74

Added experimental ELF/Mach-O/PE compatibility using gimli/object. #74

Conversation

data-pup commented Jul 2, 2018

data-pup commented Jul 2, 2018

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

data-pup commented Jul 3, 2018

data-pup commented Jul 6, 2018 • edited Loading

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

data-pup commented Jul 10, 2018 • edited Loading

fitzgen commented Jul 10, 2018

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

fitzgen commented Jul 10, 2018

data-pup commented Jul 6, 2018 •

edited

Loading

data-pup commented Jul 10, 2018 •

edited

Loading