Refactor gimli implementation to avoid mk! macro (#379)

alexcrichton · web-flow · commit 47da4e109fae · 2020-10-29T10:22:56.000-05:00
This commit refactors a bit to avoid using a macro for constructing a
`Mapping` and instead adds a dedicated function. This is a bit easier to
navigate and will hopefully help deduplicate construction of a `Mapping`
as well.
diff --git a/src/symbolize/gimli.rs b/src/symbolize/gimli.rs
@@ -54,55 +54,60 @@ mod stash;
 
 const MAPPINGS_CACHE_SIZE: usize = 4;
 
-struct Context<'a> {
-    dwarf: addr2line::Context<EndianSlice<'a, Endian>>,
-    object: Object<'a>,
-}
-
 struct Mapping {
     // 'static lifetime is a lie to hack around lack of support for self-referential structs.
     cx: Context<'static>,
     _map: Mmap,
     _stash: Stash,
 }
 
-fn cx<'data>(stash: &'data Stash, object: Object<'data>) -> Option<Context<'data>> {
-    fn load_section<'data, S>(stash: &'data Stash, obj: &Object<'data>) -> S
+impl Mapping {
+    fn mk<F>(data: Mmap, mk: F) -> Option<Mapping>
     where
-        S: gimli::Section<gimli::EndianSlice<'data, Endian>>,
+        F: for<'a> Fn(&'a [u8], &'a Stash) -> Option<Context<'a>>,
     {
-        let data = obj.section(stash, S::section_name()).unwrap_or(&[]);
-        S::from(EndianSlice::new(data, Endian))
+        let stash = Stash::new();
+        let cx = mk(&data, &stash)?;
+        Some(Mapping {
+            // Convert to 'static lifetimes since the symbols should
+            // only borrow `map` and `stash` and we're preserving them below.
+            cx: unsafe { core::mem::transmute::<Context<'_>, Context<'static>>(cx) },
+            _map: data,
+            _stash: stash,
+        })
     }
+}
 
-    let dwarf = addr2line::Context::from_sections(
-        load_section(stash, &object),
-        load_section(stash, &object),
-        load_section(stash, &object),
-        load_section(stash, &object),
-        load_section(stash, &object),
-        load_section(stash, &object),
-        load_section(stash, &object),
-        load_section(stash, &object),
-        load_section(stash, &object),
-        gimli::EndianSlice::new(&[], Endian),
-    )
-    .ok()?;
-    Some(Context { dwarf, object })
+struct Context<'a> {
+    dwarf: addr2line::Context<EndianSlice<'a, Endian>>,
+    object: Object<'a>,
 }
 
-macro_rules! mk {
-    (Mapping { $map:expr, $inner:expr, $stash:expr }) => {{
-        fn assert_lifetimes<'a>(_: &'a Mmap, _: &Context<'a>, _: &'a Stash) {}
-        assert_lifetimes(&$map, &$inner, &$stash);
-        Mapping {
-            // Convert to 'static lifetimes since the symbols should
-            // only borrow `map` and `stash` and we're preserving them below.
-            cx: unsafe { core::mem::transmute::<Context<'_>, Context<'static>>($inner) },
-            _map: $map,
-            _stash: $stash,
+impl<'data> Context<'data> {
+    fn new(stash: &'data Stash, object: Object<'data>) -> Option<Context<'data>> {
+        fn load_section<'data, S>(stash: &'data Stash, obj: &Object<'data>) -> S
+        where
+            S: gimli::Section<gimli::EndianSlice<'data, Endian>>,
+        {
+            let data = obj.section(stash, S::section_name()).unwrap_or(&[]);
+            S::from(EndianSlice::new(data, Endian))
         }
-    }};
+
+        let dwarf = addr2line::Context::from_sections(
+            load_section(stash, &object),
+            load_section(stash, &object),
+            load_section(stash, &object),
+            load_section(stash, &object),
+            load_section(stash, &object),
+            load_section(stash, &object),
+            load_section(stash, &object),
+            load_section(stash, &object),
+            load_section(stash, &object),
+            gimli::EndianSlice::new(&[], Endian),
+        )
+        .ok()?;
+        Some(Context { dwarf, object })
+    }
 }
 
 fn mmap(path: &Path) -> Option<Mmap> {
diff --git a/src/symbolize/gimli/coff.rs b/src/symbolize/gimli/coff.rs
@@ -1,4 +1,4 @@
-use super::{Context, Mapping, Mmap, Path, Stash, Vec};
+use super::{Context, Mapping, Path, Stash, Vec};
 use core::convert::TryFrom;
 use object::pe::{ImageDosHeader, ImageSymbol};
 use object::read::pe::{ImageNtHeaders, ImageOptionalHeader, SectionTable};
@@ -13,9 +13,7 @@ type Pe = object::pe::ImageNtHeaders64;
 impl Mapping {
     pub fn new(path: &Path) -> Option<Mapping> {
         let map = super::mmap(path)?;
-        let stash = Stash::new();
-        let cx = super::cx(&stash, Object::parse(&map)?)?;
-        Some(mk!(Mapping { map, cx, stash }))
+        Mapping::mk(map, |data, stash| Context::new(stash, Object::parse(data)?))
     }
 }
 
diff --git a/src/symbolize/gimli/elf.rs b/src/symbolize/gimli/elf.rs
@@ -1,4 +1,4 @@
-use super::{Context, Mapping, Mmap, Path, Stash, Vec};
+use super::{Context, Mapping, Path, Stash, Vec};
 use core::convert::TryFrom;
 use object::elf::{ELFCOMPRESS_ZLIB, SHF_COMPRESSED};
 use object::read::elf::{CompressionHeader, FileHeader, SectionHeader, SectionTable, Sym};
@@ -13,9 +13,7 @@ type Elf = object::elf::FileHeader64<NativeEndian>;
 impl Mapping {
     pub fn new(path: &Path) -> Option<Mapping> {
         let map = super::mmap(path)?;
-        let stash = Stash::new();
-        let cx = super::cx(&stash, Object::parse(&map)?)?;
-        Some(mk!(Mapping { map, cx, stash }))
+        Mapping::mk(map, |data, stash| Context::new(stash, Object::parse(data)?))
     }
 }
 
diff --git a/src/symbolize/gimli/macho.rs b/src/symbolize/gimli/macho.rs
@@ -1,4 +1,4 @@
-use super::{Box, Context, Mapping, Mmap, Path, Stash, Vec};
+use super::{Box, Context, Mapping, Path, Stash, Vec};
 use core::convert::TryInto;
 use object::macho;
 use object::read::macho::{MachHeader, Nlist, Section, Segment as _};
@@ -30,8 +30,25 @@ impl Mapping {
         // contains and try to find a macho file which has a matching UUID as
         // the one of our own file. If we find a match that's the dwarf file we
         // want to return.
-        let parent = path.parent()?;
-        for entry in parent.read_dir().ok()? {
+        if let Some(parent) = path.parent() {
+            if let Some(mapping) = Mapping::load_dsym(parent, uuid) {
+                return Some(mapping);
+            }
+        }
+
+        // Looks like nothing matched our UUID, so let's at least return our own
+        // file. This should have the symbol table for at least some
+        // symbolication purposes.
+        Mapping::mk(map, |data, stash| {
+            let (macho, data) = find_header(Bytes(data))?;
+            let endian = macho.endian().ok()?;
+            let obj = Object::parse(macho, endian, data)?;
+            Context::new(stash, obj)
+        })
+    }
+
+    fn load_dsym(dir: &Path, uuid: [u8; 16]) -> Option<Mapping> {
+        for entry in dir.read_dir().ok()? {
             let entry = entry.ok()?;
             let filename = match entry.file_name().into_string() {
                 Ok(name) => name,
@@ -41,38 +58,36 @@ impl Mapping {
                 continue;
             }
             let candidates = entry.path().join("Contents/Resources/DWARF");
-            if let Some(mapping) = load_dsym(&candidates, uuid) {
+            if let Some(mapping) = Mapping::try_dsym_candidate(&candidates, uuid) {
                 return Some(mapping);
             }
         }
+        None
+    }
 
-        // Looks like nothing matched our UUID, so let's at least return our own
-        // file. This should have the symbol table for at least some
-        // symbolication purposes.
-        let stash = Stash::new();
-        let inner = super::cx(&stash, Object::parse(macho, endian, data)?)?;
-        return Some(mk!(Mapping { map, inner, stash }));
-
-        fn load_dsym(dir: &Path, uuid: [u8; 16]) -> Option<Mapping> {
-            for entry in dir.read_dir().ok()? {
-                let entry = entry.ok()?;
-                let map = super::mmap(&entry.path())?;
-                let (macho, data) = find_header(Bytes(&map))?;
+    fn try_dsym_candidate(dir: &Path, uuid: [u8; 16]) -> Option<Mapping> {
+        // Look for files in the `DWARF` directory which have a matching uuid to
+        // the original object file. If we find one then we found the debug
+        // information.
+        for entry in dir.read_dir().ok()? {
+            let entry = entry.ok()?;
+            let map = super::mmap(&entry.path())?;
+            let candidate = Mapping::mk(map, |data, stash| {
+                let (macho, data) = find_header(Bytes(data))?;
                 let endian = macho.endian().ok()?;
                 let entry_uuid = macho.uuid(endian, data).ok()??;
                 if entry_uuid != uuid {
-                    continue;
-                }
-                let stash = Stash::new();
-                if let Some(cx) =
-                    Object::parse(macho, endian, data).and_then(|o| super::cx(&stash, o))
-                {
-                    return Some(mk!(Mapping { map, cx, stash }));
+                    return None;
                 }
+                let obj = Object::parse(macho, endian, data)?;
+                Context::new(stash, obj)
+            });
+            if let Some(candidate) = candidate {
+                return Some(candidate);
             }
-
-            None
         }
+
+        None
     }
 }
 
@@ -269,25 +284,30 @@ fn object_mapping(path: &[u8]) -> Option<Mapping> {
     let map;
 
     // `N_OSO` symbol names can be either `/path/to/object.o` or `/path/to/archive.a(object.o)`.
-    let data = if let Some((archive_path, member_name)) = split_archive_path(path) {
+    let member_name = if let Some((archive_path, member_name)) = split_archive_path(path) {
         map = super::mmap(Path::new(OsStr::from_bytes(archive_path)))?;
-        let archive = object::read::archive::ArchiveFile::parse(&map).ok()?;
-        let member = archive
-            .members()
-            .filter_map(Result::ok)
-            .find(|m| m.name() == member_name)?;
-        Bytes(member.data())
+        Some(member_name)
     } else {
         map = super::mmap(Path::new(OsStr::from_bytes(path)))?;
-        Bytes(&map)
+        None
     };
-
-    let (macho, data) = find_header(data)?;
-    let endian = macho.endian().ok()?;
-    let object = Object::parse(macho, endian, data)?;
-    let stash = Stash::new();
-    let inner = super::cx(&stash, object)?;
-    Some(mk!(Mapping { map, inner, stash }))
+    Mapping::mk(map, |data, stash| {
+        let data = match member_name {
+            Some(member_name) => {
+                let archive = object::read::archive::ArchiveFile::parse(data).ok()?;
+                let member = archive
+                    .members()
+                    .filter_map(Result::ok)
+                    .find(|m| m.name() == member_name)?;
+                Bytes(member.data())
+            }
+            None => Bytes(data),
+        };
+        let (macho, data) = find_header(data)?;
+        let endian = macho.endian().ok()?;
+        let obj = Object::parse(macho, endian, data)?;
+        Context::new(stash, obj)
+    })
 }
 
 fn split_archive_path(path: &[u8]) -> Option<(&[u8], &[u8])> {
