Add option to use symbol table #61

philipc · 2017-10-21T07:45:53Z

Fixes #3

This gives results that match binutil addr2line for all addresses except for a couple of oddities (eg zero size symbol).

philipc · 2017-10-21T08:13:10Z

Failure is due to tarpaulin. I'm looking into it, but seems unrelated to this PR.

fitzgen

Some comments below, but mostly half-baked brainstorming. Nothing that should stop this from landing.

Thanks @philipc !

fitzgen · 2017-10-21T16:53:21Z

src/lib.rs

@@ -250,7 +262,7 @@ impl Mapping {
    pub fn locate(
        &mut self,
        addr: u64,
-    ) -> Result<Option<(path::PathBuf, Option<u64>, Option<Cow<str>>)>> {
+    ) -> Result<Option<(Option<path::PathBuf>, Option<u64>, Option<Cow<str>>)>> {


Does it make sense to still return an option of this tuple, now that each of its components is also an option? Should we return Result<(Option<PathBuf>, Option<u64>, Option<Cow<str>>)> instead? Should we give this return value a nominal, rather than structural, type? It's getting a bit hairy, and unclear (at least to me, at a glance) what it means when which thing is present or missing with these multiple layers of Option.

Yeah, you're probably right. I left the Option there so that the caller can quickly tell if there was any info at all. We still need to convert this tuple to a struct, so can investigate getting rid of the Option as part of that.

fwiw, the roadblock I hit when trying to work with gimli-addr2line is the lack of inlining info. Ultimately I think this needs to return an iterator, like I'm doing here: https://github.com/main--/unwind-rs/blob/988932544a01621109d476cb7a2381d71d886fb8/hardliner/src/lib.rs#L368

Yep the current plan in #11 is to return an iterator.

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

src/lib.rs

@@ -272,7 +284,7 @@ impl<'input> BufferMapping<'input> {
    pub fn locate(
        &mut self,
        addr: u64,
-    ) -> Result<Option<(path::PathBuf, Option<u64>, Option<Cow<'input, str>>)>> {
+    ) -> Result<Option<(Option<path::PathBuf>, Option<u64>, Option<Cow<'input, str>>)>> {


fitzgen · 2017-10-21T16:54:29Z

src/lib.rs

@@ -294,7 +306,7 @@ impl<'input> EndianDebugInfo<'input> {
    fn locate(
        &mut self,
        addr: u64,
-    ) -> Result<Option<(path::PathBuf, Option<u64>, Option<Cow<'input, str>>)>> {
+    ) -> Result<Option<(Option<path::PathBuf>, Option<u64>, Option<Cow<'input, str>>)>> {


fitzgen · 2017-10-21T16:54:48Z

src/lib.rs

        })
    }

    pub fn locate(
        &mut self,
        addr: u64,
-    ) -> Result<Option<(path::PathBuf, Option<u64>, Option<Cow<'input, str>>)>> {
+    ) -> Result<Option<(Option<path::PathBuf>, Option<u64>, Option<Cow<'input, str>>)>> {


fitzgen · 2017-10-21T17:05:44Z

src/lib.rs

+    name: &'input [u8],
+    file: Option<&'input [u8]>,
+    begin: u64,
+    end: u64,


Thinking out loud...

Demangling can be expensive -- we might want to add a refcell of the demangled symbol name here.

Ideally we would want this on some part of the common path for both debug info symbols and symbol table symbols.

But also probably not for very single symbol all the time... maybe an associative cache?

Maybe we just don't symbolicate addresses from the same function very often and this isn't worth it at all?

Or maybe demangling should be deferred to the caller? We could return the language attribute so the caller knows how to demangle it.

Or maybe demangling should be deferred to the caller?

This is what I decided to do:

https://github.com/main--/unwind-rs/blob/988932544a01621109d476cb7a2381d71d886fb8/hardliner/src/lib.rs#L246

(other than the lack of inlining info, I wrote my hardliner-addr2line implementation also because gimli-addr2line's API design feels a little bolted-on (pass options in, get data out is weird unless you're a command line tool that wants to do exactly this))

I agree about the API. I'm not sure that the memmap belongs in this crate.

fitzgen · 2017-10-21T17:08:06Z

src/lib.rs

+            }
+            SymbolKind::Section | SymbolKind::Common | SymbolKind::Tls => continue,
+        }
+        if symbol.is_undefined() || symbol.name().len() == 0 || symbol.size() == 0 {


Here and just above: symbol.name().is_empty() is slightly more idiomatic than symbol.name().len() == 0.

Ah yeah, I often forget that in rust.

fitzgen · 2017-10-21T17:12:00Z

src/lib.rs

+        if ord != cmp::Ordering::Equal {
+            return ord;
+        }
+        a.end.cmp(&b.end)


This can be rewritten as

a.begin.cmp(&b.begin).then_with(|| a.end.cmp(&b.end)

Oooh, I didn't know that trick!

fitzgen · 2017-10-21T17:12:49Z

src/lib.rs

+
+    let mut prev_end = 0;
+    for symbol in &mut symbols {
+        if symbol.begin < prev_end {


This fixup stuff deserves a comment.

I tried to write a comment to justify this, but it made no difference on any of my test binaries, so I've deleted it.

jonhoo · 2017-10-21T17:25:39Z

src/lib.rs

+        });
+    }
+
+    symbols.sort_by(|a, b| {


Would it perhaps be an idea to insert the symbols into a BTreeMap instead? I believe that should be faster.

I don't think we want to use BTreeMap. I think what we want is a different array layout (see https://arxiv.org/abs/1509.05053). I'm not aware of an implementation of this in rust though.

I guess the question here is what we're expecting n to be, and the expected number of searches. Figure 17 of that paper is likely the comparison we want. When n is large (as in > =2^18, or perhaps smaller depending on the efficiency of Rust's binary search implementation), or when the number of lookups is small, I believe a BTreeMap is the appropriate structure until someone does implement Eytzinger (the former is supported by the paper, the latter by the fact that the array also needs to be sorted in order to do binary searches). I suspect that we expect n to be fairly small, though I don't have a good sense of the number of symbols in a representative symbol file. It'd be useful if the paper above also included the cost of sorting, but a cursory reading suggests that they don't.

Without an implementation of Eytzinger (which seems like it might be interesting), I think BTreeMap is still the right way to go. For small n, latency will be pretty short anyway, and for large n, BTreeMap wins.

Reading a bit more closely, their B-tree isn't anywhere near the same as a Rust BTreeMap. It assume contiguous storage. Hard to say exactly how the two would compare, but Rust's would likely be slower. I guess then I'm fine with the current code, even though I feel like the sorting will actually introduce a non-trivial up-front cost that might not be right trade-off if we expect few lookups.

@philipc fyi: patmorin/arraylayout#3

Yes, I meant we want to use contiguous storage as currently, but with a better layout and search algorithm than binary search, so not rust's BTreeMap because it is not contiguous. Whether that sorting strategy should be B-tree or Eytzinger needs testing.

I guess then I'm fine with the current code, even though I feel like the sorting will actually introduce a non-trivial up-front cost that might not be right trade-off if we expect few lookups.

So I'm not familiar with the costs of BTreeMap construction, but I didn't expect it to be significantly better than a sort?

I should also point out that the object crate is already sorting Mach-O symbols internally to calculate symbol sizes (but it doesn't give us the sorted array), and there's a bunch of inefficiencies due to creating multiple arrays of these symbols (eg for ELF there's one in the goblin crate, one in the object crate and one here) so that needs optimizing too. Ideally goblin and object would both give iterators instead, but I don't know how to get that to work with the Mach-O symbol sizes.

So I'm not familiar with the costs of BTreeMap construction, but I didn't expect it to be significantly better than a sort?

I believe it should be faster, as you avoid walking all the elements once at the beginning to push them onto the list. The sorting itself is probably roughly the same, although that depends on the exact algorithm Rust uses internally (since it might shift things in the array). You might also want to use sort_unstable_by here.

Ah, I wasn't aware of the sorting in object. I don't know which of these approaches are best suited for adopting an already-sorted array, though it might very well be push + sort_by.

@philipc First implementation here: https://github.com/jonhoo/ordsearch

Nice. I'm going to leave optimisation of this for future work.

philipc · 2017-10-23T11:58:14Z

Tarpaulin failure should be fixed soon. Nightly failure is unrelated to this PR, I'll investigate separately.

sfackler · 2017-11-12T22:02:46Z

This appears to have been undone by #64 - was that intentional?

fitzgen · 2017-11-12T22:30:37Z

IIUC, the intention is to have object provide the symbol table interface. It would be nice if there was an EZ mode for users who just want file/function/line/column/etc and don't care about which source the information came from...

cc @philipc @main--

sfackler · 2017-11-12T22:37:18Z

I kind of imagine addr2line as the place for that EZ mode interface, right? "Give me as much information as possible about this address."

main-- · 2017-11-12T23:12:44Z

What you're describing is one possible usecase but that's not the right abstraction for everyone. As #64 introduced inlining support, addr2line now always gives you complete traces without any missing frames. We can't uphold this guarantee if we silently fall back to symtab however. Thus, symbol lookup would need to be its own separate interface.

The old API was basically a straight adaptation from the addr2line console application, but the new one aims to be more flexible. One part of that (not done yet) is that we want to make the object dependency optional so you can use your own parser (if necessary). But this means that you can only use addr2line in one of two ways:

Either you use it with object (the only option right now) and you construct the object on your side, then pass a reference to us. In that case you're already holding the object and you can just get the symbols from there.
Or you provide a custom implementation and we would literally just pass take symbols from you and pass them back out.

So the consensus in #63 was to keep symbol table handling in the object crate.

sfackler · 2017-11-12T23:46:05Z

Cool - it makes sense that you'd want a library to handle the DWARF-side of symbolication specifically. Might make sense to rename it at some point though, since addr2line (to me at least) implies a more general kind of thing like what I described earlier.

philipc · 2017-11-13T07:23:59Z

I'm fine with adding an optional EZ mode if it's useful. But as soon as you start needing to configure the behaviour then you're better off just using the flexible APIs. So the addr2line example wouldn't use any EZ mode we added.

Add option to use symbol table

566311f

fitzgen approved these changes Oct 21, 2017

View reviewed changes

jonhoo reviewed Oct 21, 2017

View reviewed changes

jonhoo mentioned this pull request Oct 22, 2017

Some pointers for which variants to use patmorin/arraylayout#3

Open

philipc added 2 commits October 23, 2017 13:44

Don't demangle symbol table names unless requested

739bdc4

Cleanup symbol table parsing

2ba36eb

philipc merged commit 785ec1c into gimli-rs:master Oct 23, 2017

philipc deleted the issue-3 branch October 23, 2017 11:58

philipc mentioned this pull request Oct 25, 2017

API flexibility #63

Closed

Add option to use symbol table #61

Add option to use symbol table #61

Conversation

philipc commented Oct 21, 2017

philipc commented Oct 21, 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

main-- Oct 24, 2017 • edited

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

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 Oct 22, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

philipc commented Oct 23, 2017

sfackler commented Nov 12, 2017

fitzgen commented Nov 12, 2017

sfackler commented Nov 12, 2017

main-- commented Nov 12, 2017

sfackler commented Nov 12, 2017

philipc commented Nov 13, 2017

main-- Oct 24, 2017 •

edited

jonhoo Oct 22, 2017 •

edited