Clean up addr2line so it is a real library #1

jonhoo · 2016-12-04T02:50:05Z

Sorry for not breaking this down into smaller commits, but there wasn't a good way of doing that without the intermittent commits being unintuitive, broken, or both. This moves all the logic into the library, provides a documented API (with #[deny(missing_docs)]), and extends the binary such that its interface matches that of addr2line. It builds cleanly on both stable and nightly.

Next steps would probably be to add support for extracting containing function, not just file + line. I think this shouldn't be too painful, but you never know. I would also like to try and find a good way to test this, but I'm not entirely sure how to go about that. Suggestions welcome!

The darkest corners of this PR is the nested use of owning_ref::OwningHandle. This is necessary so that we can construct a struct which contains a memmap, an object::File that borrows the map, and gimli objects that borrow the object::File. Luckily, it all works out somewhat nicely. The biggest bummer at the moment is that errors in the construction of the object::File and gimli objects cause panics, which is due to the lack of being able to forward errors through owning_ref::OwningHandle. I've filed a bug about this, so hopefully it will be resolved in not too long.

I've left the Unit stuff mostly intact, including the comment saying

TODO: most of this should be moved to the main library.

coveralls · 2016-12-04T02:55:42Z

Coverage decreased (-1.7%) to 0.99% when pulling cdc9973 on jonhoo:master into 0331845 on gimli-rs:master.

coveralls · 2016-12-04T02:55:42Z

Coverage decreased (-1.7%) to 0.99% when pulling cdc9973 on jonhoo:master into 0331845 on gimli-rs:master.

fitzgen · 2016-12-04T04:55:45Z

Thanks for the pull request! Digging in now...

Sorry for not breaking this down into smaller commits, but there wasn't a good way of doing that without the intermittent commits being unintuitive, broken, or both.

No worries.

This moves all the logic into the library, provides a documented API (with #[deny(missing_docs)]), and extends the binary such that its interface matches that of addr2line. It builds cleanly on both stable and nightly.

Thank you!!

I would also like to try and find a good way to test this, but I'm not entirely sure how to go about that. Suggestions welcome!

It should be easy enough to write integration tests by using nm on our addr2line itself to find addresses and function names, and then pass those addresses into our addr2line and make sure that we get something.

We could also try comparing our results with the canonical addr2line's results.

As far as unit tests go, its more about semi-tediously writing test data and breaking down each function as far as possible.

These things can, of course, be done in follow up PRs if you are so inclined :)

fitzgen

LGTM! Just a couple nitpicks that I'd like to see fixed before we merge this in :)

Thanks again for doing this!

fitzgen · 2016-12-04T05:00:45Z

Cargo.toml

 gimli = "0.9.0"
 memmap = "0.5.0"
 object = "0.1.0"
+owning_ref = { git = "https://github.com/Kimundi/owning-ref-rs" } # need OwningHandle


This is just a placeholder until the next release is cut? We won't be able to publish on crates.io with a git dependency :-/

fitzgen · 2016-12-04T05:16:12Z

Cargo.toml


 [dependencies]
 fallible-iterator = "0.1.3"
-getopts = "0.2.14"
+clap = "2.*"


I'd prefer specifying a proper version here. Cargo will automatically treat 2.3.6 as ^2.3.6, which is anything in the range [2.3.6, 3.0.0).

Is there a particular reason you want 2.3.6, or should we start at the current latest (2.19.1)?

2.3.6 was just an example. The latest version is perfect, thanks!

fitzgen · 2016-12-04T05:16:49Z

src/bin/addr2line.rs

+            .short("e")
+            .long("exe")
+            .default_value("a.out")
+            .help("Specify the name of the executable for which addresses should be translated.")


s/name/path/ ?

I took the help text verbatim from the addr2line man page for consistency, but I'm happy to change it if you prefer path.

Ah ok, disregard then.

jonhoo · 2016-12-04T05:33:21Z

It should be easy enough to write integration tests by using nm on our addr2line itself to find addresses and function names, and then pass those addresses into our addr2line and make sure that we get something.

Do we have a way of referring to the path to build artifacts from inside an integration test?
Also, how were you thinking of using nm here? It wouldn't actually give us the mapping from source line to address (though we could cook something up by using placeholder functions).

We could also try comparing our results with the canonical addr2line's results.

This was more in line with what I was thinking, though the questions above still apply with regards to getting access to our own build artifact in the tests. Also, can we rely on the test running (Travis) to have addr2line?

As far as unit tests go, its more about semi-tediously writing test data and breaking down each function as far as possible.

These things can, of course, be done in follow up PRs if you are so inclined :)

I think I'd prefer for them to be follow-up PRs :)

jonhoo · 2016-12-04T05:34:50Z

Just pushed the Cargo.toml version change. Do you also want the commits squashed?

coveralls · 2016-12-04T05:39:20Z

Coverage decreased (-1.7%) to 0.99% when pulling 59bec17 on jonhoo:master into 0331845 on gimli-rs:master.

coveralls · 2016-12-04T05:39:20Z

Coverage decreased (-1.7%) to 0.99% when pulling 59bec17 on jonhoo:master into 0331845 on gimli-rs:master.

fitzgen · 2016-12-04T05:41:29Z

Do we have a way of referring to the path to build artifacts from inside an integration test?

From http://doc.crates.io/environment-variables.html:

CARGO_TARGET_DIR - Location of where to place all generated artifacts, relative to the current working directory.

So we could do something like this:

use std::env;

let mut addr2line = PathBuf::new(env::var("CARGO_TARGET_DIR").unwrap());
addr2line.push("addr2line");

Also, how were you thinking of using nm here? It wouldn't actually give us the mapping from source line to address (though we could cook something up by using placeholder functions).

Right -- we would just want to make sure that we didn't get ?:?, but I'm not sure how reliable that would actually be... Maybe if we filtered for only known functions within gimli or addr2line...

Also, can we rely on the test running (Travis) to have addr2line?

I think so. If not, we can install it with apt from whatever package it might be in from inside .travis.yml.

I think I'd prefer for them to be follow-up PRs :)

Of course :)

Do you also want the commits squashed?

Nah, its fine.

Thanks again! Very appreciated!

jonhoo · 2016-12-04T06:11:47Z

Unfortunately CARGO_TARGET_DIR is a configuration option, not something that's set by cargo. We may be able to use OUT_DIR, but I don't know that there's any guarantee that the binary has been built by the time the tests run.. :/

jonhoo · 2016-12-04T06:17:09Z

One alternative is to just stuff some C code in there and use native code compilation.

philipc · 2016-12-04T06:45:16Z

What would the C code do that we can't do in rust?

You could look at how others have solved it, eg https://github.com/BurntSushi/ripgrep/blob/master/tests/workdir.rs, doesn't look like there is a simple solution though.

jonhoo · 2016-12-04T07:31:01Z

It's just easier to compile reliably from running commands is all. See #4. std::env::current_exe is a neat trick that might be applicable, though I think getting up-and-running on analyzing a trivial C program first is a good first step.

Clean up addr2line so it is a real library

cdc9973

jonhoo mentioned this pull request Dec 4, 2016

addr2line is slower than the binutils version gimli-rs/gimli#53

Closed

fitzgen requested changes Dec 4, 2016

View reviewed changes

Specify current version of clap at time of writing

59bec17

fitzgen approved these changes Dec 4, 2016

View reviewed changes

fitzgen merged commit 90038ab into gimli-rs:master Dec 4, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Clean up addr2line so it is a real library #1

Clean up addr2line so it is a real library #1

jonhoo commented Dec 4, 2016

coveralls commented Dec 4, 2016

coveralls commented Dec 4, 2016

fitzgen commented Dec 4, 2016

fitzgen left a comment

fitzgen Dec 4, 2016

jonhoo Dec 4, 2016

fitzgen Dec 4, 2016

jonhoo Dec 4, 2016

fitzgen Dec 4, 2016

fitzgen Dec 4, 2016

jonhoo Dec 4, 2016

fitzgen Dec 4, 2016

jonhoo commented Dec 4, 2016

jonhoo commented Dec 4, 2016

coveralls commented Dec 4, 2016

coveralls commented Dec 4, 2016

fitzgen commented Dec 4, 2016

jonhoo commented Dec 4, 2016

jonhoo commented Dec 4, 2016

philipc commented Dec 4, 2016

jonhoo commented Dec 4, 2016

Clean up addr2line so it is a real library #1

Clean up addr2line so it is a real library #1

Conversation

jonhoo commented Dec 4, 2016

coveralls commented Dec 4, 2016

coveralls commented Dec 4, 2016

fitzgen commented Dec 4, 2016

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

jonhoo commented Dec 4, 2016

jonhoo commented Dec 4, 2016

coveralls commented Dec 4, 2016

coveralls commented Dec 4, 2016

fitzgen commented Dec 4, 2016

jonhoo commented Dec 4, 2016

jonhoo commented Dec 4, 2016

philipc commented Dec 4, 2016

jonhoo commented Dec 4, 2016