Avoid transmutes with Context (#429)

This commit re-consolidates transmutes around `Context` to just one
location to ensure that this is in as few places as possible. This
reorganizes the location of the `map_sup` variable and places it inside
of `Stash` instead which is a sort of auxiliary bag for extra
information that can be referenced anyway. Afterwards with some
refactoring an a new `Mapping::mk` function it's back to one transmute,
yay!

Author: alexcrichton

src/symbolize/gimli.rs

@@ -58,24 +58,45 @@ struct Mapping {
     // 'static lifetime is a lie to hack around lack of support for self-referential structs.
     cx: Context<'static>,
     _map: Mmap,
-    _map_sup: Option<Mmap>,
     _stash: Stash,
 }
 
-impl Mapping {
+enum Either<A, B> {
     #[allow(dead_code)]
+    A(A),
+    B(B),
+}
+
+impl Mapping {
+    /// Creates a `Mapping` by ensuring that the `data` specified is used to
+    /// create a `Context` and it can only borrow from that or the `Stash` of
+    /// decompressed sections or auxiliary data.
     fn mk<F>(data: Mmap, mk: F) -> Option<Mapping>
     where
-        F: for<'a> Fn(&'a [u8], &'a Stash) -> Option<Context<'a>>,
+        F: for<'a> FnOnce(&'a [u8], &'a Stash) -> Option<Context<'a>>,
+    {
+        Mapping::mk_or_other(data, move |data, stash| {
+            let cx = mk(data, stash)?;
+            Some(Either::B(cx))
+        })
+    }
+
+    /// Creates a `Mapping` from `data`, or if the closure decides to, returns a
+    /// different mapping.
+    fn mk_or_other<F>(data: Mmap, mk: F) -> Option<Mapping>
+    where
+        F: for<'a> FnOnce(&'a [u8], &'a Stash) -> Option<Either<Mapping, Context<'a>>>,
     {
         let stash = Stash::new();
-        let cx = mk(&data, &stash)?;
+        let cx = match mk(&data, &stash)? {
+            Either::A(mapping) => return Some(mapping),
+            Either::B(cx) => cx,
+        };
         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,
-            _map_sup: None,
             _stash: stash,
         })
     }

src/symbolize/gimli/elf.rs

@@ -2,6 +2,7 @@ use super::mystd::ffi::{OsStr, OsString};
 use super::mystd::fs;
 use super::mystd::os::unix::ffi::{OsStrExt, OsStringExt};
 use super::mystd::path::{Path, PathBuf};
+use super::Either;
 use super::{Context, Mapping, Stash, Vec};
 use core::convert::{TryFrom, TryInto};
 use core::str;
@@ -18,75 +19,50 @@ type Elf = object::elf::FileHeader64<NativeEndian>;
 impl Mapping {
     pub fn new(path: &Path) -> Option<Mapping> {
         let map = super::mmap(path)?;
-        let object = Object::parse(&map)?;
+        Mapping::mk_or_other(map, |map, stash| {
+            let object = Object::parse(&map)?;
 
-        // Try to locate an external debug file using the build ID.
-        if let Some(path_debug) = object.build_id().and_then(locate_build_id) {
-            if let Some(mapping) = Mapping::new_debug(path_debug, None) {
-                return Some(mapping);
+            // Try to locate an external debug file using the build ID.
+            if let Some(path_debug) = object.build_id().and_then(locate_build_id) {
+                if let Some(mapping) = Mapping::new_debug(path_debug, None) {
+                    return Some(Either::A(mapping));
+                }
             }
-        }
 
-        // Try to locate an external debug file using the GNU debug link section.
-        if let Some((path_debug, crc)) = object.gnu_debuglink_path(path) {
-            if let Some(mapping) = Mapping::new_debug(path_debug, Some(crc)) {
-                return Some(mapping);
+            // Try to locate an external debug file using the GNU debug link section.
+            if let Some((path_debug, crc)) = object.gnu_debuglink_path(path) {
+                if let Some(mapping) = Mapping::new_debug(path_debug, Some(crc)) {
+                    return Some(Either::A(mapping));
+                }
             }
-        }
 
-        let stash = Stash::new();
-        let cx = Context::new(&stash, object, None)?;
-        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: map,
-            _map_sup: None,
-            _stash: stash,
+            Context::new(stash, object, None).map(Either::B)
         })
     }
 
     /// Load debuginfo from an external debug file.
     fn new_debug(path: PathBuf, crc: Option<u32>) -> Option<Mapping> {
         let map = super::mmap(&path)?;
-        let object = Object::parse(&map)?;
+        Mapping::mk(map, |map, stash| {
+            let object = Object::parse(&map)?;
 
-        if let Some(_crc) = crc {
-            // TODO: check crc
-        }
+            if let Some(_crc) = crc {
+                // TODO: check crc
+            }
 
-        // Try to locate a supplementary object file.
-        if let Some((path_sup, build_id_sup)) = object.gnu_debugaltlink_path(&path) {
-            if let Some(map_sup) = super::mmap(&path_sup) {
-                if let Some(sup) = Object::parse(&map_sup) {
-                    if sup.build_id() == Some(build_id_sup) {
-                        let stash = Stash::new();
-                        let cx = Context::new(&stash, object, Some(sup))?;
-                        return Some(Mapping {
-                            // Convert to 'static lifetimes since the symbols should
-                            // only borrow `map`, `map_sup`, and `stash` and we're
-                            // preserving them below.
-                            cx: unsafe {
-                                core::mem::transmute::<Context<'_>, Context<'static>>(cx)
-                            },
-                            _map: map,
-                            _map_sup: Some(map_sup),
-                            _stash: stash,
-                        });
+            // Try to locate a supplementary object file.
+            if let Some((path_sup, build_id_sup)) = object.gnu_debugaltlink_path(&path) {
+                if let Some(map_sup) = super::mmap(&path_sup) {
+                    let map_sup = stash.set_mmap_aux(map_sup);
+                    if let Some(sup) = Object::parse(map_sup) {
+                        if sup.build_id() == Some(build_id_sup) {
+                            return Context::new(stash, object, Some(sup));
+                        }
                     }
                 }
             }
-        }
 
-        let stash = Stash::new();
-        let cx = Context::new(&stash, object, None)?;
-        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: map,
-            _map_sup: None,
-            _stash: stash,
+            Context::new(stash, object, None)
         })
     }
 }

src/symbolize/gimli/stash.rs

@@ -1,19 +1,22 @@
 // only used on Linux right now, so allow dead code elsewhere
 #![cfg_attr(not(target_os = "linux"), allow(dead_code))]
 
+use super::Mmap;
 use alloc::vec;
 use alloc::vec::Vec;
 use core::cell::UnsafeCell;
 
 /// A simple arena allocator for byte buffers.
 pub struct Stash {
     buffers: UnsafeCell<Vec<Vec<u8>>>,
+    mmap_aux: UnsafeCell<Option<Mmap>>,
 }
 
 impl Stash {
     pub fn new() -> Stash {
         Stash {
             buffers: UnsafeCell::new(Vec::new()),
+            mmap_aux: UnsafeCell::new(None),
         }
     }
 
@@ -29,4 +32,21 @@ impl Stash {
         // to the data inside any buffer will live as long as `self` does.
         &mut buffers[i]
     }
+
+    /// Stores a `Mmap` for the lifetime of this `Stash`, returning a pointer
+    /// which is scoped to just this lifetime.
+    pub fn set_mmap_aux(&self, map: Mmap) -> &[u8] {
+        // SAFETY: this is the only location for a mutable pointer to
+        // `mmap_aux`, and this structure isn't threadsafe to shared across
+        // threads either. This also is careful to store at most one `mmap_aux`
+        // since overwriting a previous one would invalidate the previous
+        // pointer. Given that though we can safely return a pointer to our
+        // interior-owned contents.
+        unsafe {
+            let mmap_aux = &mut *self.mmap_aux.get();
+            assert!(mmap_aux.is_none());
+            *mmap_aux = Some(map);
+            mmap_aux.as_ref().unwrap()
+        }
+    }
 }