From 58322d0021ee0f4d4a5bc23f8bec4cb4723de732 Mon Sep 17 00:00:00 2001 From: Bobby Holley Date: Thu, 5 Oct 2017 14:51:33 -0700 Subject: [PATCH] Add canary and journaling. MozReview-Commit-ID: 582ZiTmcvgs --- components/hashglobe/src/diagnostic.rs | 81 +++++++++++++++++++++++--- components/hashglobe/src/hash_map.rs | 11 +++- components/hashglobe/src/table.rs | 8 ++- 3 files changed, 90 insertions(+), 10 deletions(-) diff --git a/components/hashglobe/src/diagnostic.rs b/components/hashglobe/src/diagnostic.rs index 498ede0ca72..0a5a6c4d077 100644 --- a/components/hashglobe/src/diagnostic.rs +++ b/components/hashglobe/src/diagnostic.rs @@ -1,15 +1,30 @@ use hash_map::HashMap; use std::borrow::Borrow; use std::hash::{BuildHasher, Hash}; +use table::SafeHash; use FailedAllocationError; +#[cfg(target_pointer_width = "32")] +const CANARY: usize = 0x42cafe99; +#[cfg(target_pointer_width = "64")] +const CANARY: usize = 0x42cafe9942cafe99; + +#[derive(Clone, Debug)] +enum JournalEntry { + Insert(SafeHash), + GetOrInsertWith(SafeHash), + Remove(SafeHash), + DidClear(usize), +} + #[derive(Clone, Debug)] pub struct DiagnosticHashMap where K: Eq + Hash, S: BuildHasher { - map: HashMap, + map: HashMap, + journal: Vec, readonly: bool, } @@ -18,7 +33,7 @@ impl DiagnosticHashMap S: BuildHasher { #[inline(always)] - pub fn inner(&self) -> &HashMap { + pub fn inner(&self) -> &HashMap { &self.map } @@ -32,12 +47,27 @@ impl DiagnosticHashMap pub fn end_mutation(&mut self) { assert!(!self.readonly); self.readonly = true; + + let mut position = 0; + let mut bad_canary: Option<(usize, *const usize)> = None; + for (_,v) in self.map.iter() { + let canary_ref = &v.0; + if *canary_ref == CANARY { + position += 1; + continue; + } + bad_canary = Some((*canary_ref, canary_ref)); + } + if let Some(c) = bad_canary { + self.report_corruption(c.0, c.1, position); + } } #[inline(always)] pub fn with_hasher(hash_builder: S) -> Self { Self { - map: HashMap::::with_hasher(hash_builder), + map: HashMap::::with_hasher(hash_builder), + journal: Vec::new(), readonly: true, } } @@ -65,7 +95,7 @@ impl DiagnosticHashMap where K: Borrow, Q: Hash + Eq { - self.map.get(k) + self.map.get(k).map(|v| &v.1) } #[inline(always)] @@ -75,14 +105,17 @@ impl DiagnosticHashMap default: F ) -> Result<&mut V, FailedAllocationError> { assert!(!self.readonly); + self.journal.push(JournalEntry::GetOrInsertWith(self.map.make_hash(&key))); let entry = self.map.try_entry(key)?; - Ok(entry.or_insert_with(default)) + Ok(&mut entry.or_insert_with(|| (CANARY, default())).1) } #[inline(always)] pub fn try_insert(&mut self, k: K, v: V) -> Result, FailedAllocationError> { assert!(!self.readonly); - self.map.try_insert(k, v) + self.journal.push(JournalEntry::Insert(self.map.make_hash(&k))); + let old = self.map.try_insert(k, (CANARY, v))?; + Ok(old.map(|x| x.1)) } #[inline(always)] @@ -91,7 +124,8 @@ impl DiagnosticHashMap Q: Hash + Eq { assert!(!self.readonly); - self.map.remove(k) + self.journal.push(JournalEntry::Remove(self.map.make_hash(k))); + self.map.remove(k).map(|x| x.1) } #[inline(always)] @@ -99,9 +133,36 @@ impl DiagnosticHashMap // We handle scoped mutations for the caller here, since callsites that // invoke clear() don't benefit from the coalescing we do around insertion. self.begin_mutation(); + self.journal.clear(); + self.journal.push(JournalEntry::DidClear(self.map.raw_capacity())); self.map.clear(); self.end_mutation(); } + + #[inline(never)] + fn report_corruption( + &mut self, + canary: usize, + canary_addr: *const usize, + position: usize + ) { + unsafe { + Gecko_AddBufferToCrashReport( + self.journal.as_ptr() as *const _, + self.journal.len() * ::std::mem::size_of::(), + ); + } + panic!( + "HashMap Corruption (sz={}, cap={}, pairsz={}, cnry={:#x}, pos={}, base_addr={:?}, cnry_addr={:?})", + self.map.len(), + self.map.raw_capacity(), + ::std::mem::size_of::<(K, (usize, V))>(), + canary, + position, + self.map.raw_buffer(), + canary_addr, + ); + } } impl PartialEq for DiagnosticHashMap @@ -128,6 +189,7 @@ impl Default for DiagnosticHashMap fn default() -> Self { Self { map: HashMap::default(), + journal: Vec::new(), readonly: true, } } @@ -141,3 +203,8 @@ impl Drop for DiagnosticHashMap debug_assert!(self.readonly, "Dropped while mutating"); } } + +extern "C" { + pub fn Gecko_AddBufferToCrashReport(addr: *const ::std::os::raw::c_void, + bytes: usize); +} diff --git a/components/hashglobe/src/hash_map.rs b/components/hashglobe/src/hash_map.rs index 5b3f7b7c02b..4fc1a12093a 100644 --- a/components/hashglobe/src/hash_map.rs +++ b/components/hashglobe/src/hash_map.rs @@ -534,7 +534,7 @@ impl HashMap where K: Eq + Hash, S: BuildHasher { - fn make_hash(&self, x: &X) -> SafeHash + pub fn make_hash(&self, x: &X) -> SafeHash where X: Hash { table::make_hash(&self.hash_builder, x) @@ -683,10 +683,17 @@ impl HashMap /// Returns the hash map's raw capacity. #[inline] - fn raw_capacity(&self) -> usize { + pub fn raw_capacity(&self) -> usize { self.table.capacity() } + /// Returns a raw pointer to the table's buffer. + #[inline] + pub fn raw_buffer(&self) -> *const u8 { + assert!(self.len() != 0); + self.table.raw_buffer() + } + /// Reserves capacity for at least `additional` more elements to be inserted /// in the `HashMap`. The collection may reserve more space to avoid /// frequent reallocations. diff --git a/components/hashglobe/src/table.rs b/components/hashglobe/src/table.rs index 71212ccf6ab..8fc306a39c3 100644 --- a/components/hashglobe/src/table.rs +++ b/components/hashglobe/src/table.rs @@ -182,7 +182,7 @@ pub struct GapThenFull { /// A hash that is not zero, since we use a hash of zero to represent empty /// buckets. -#[derive(PartialEq, Copy, Clone)] +#[derive(PartialEq, Copy, Clone, Debug)] pub struct SafeHash { hash: HashUint, } @@ -816,6 +816,12 @@ impl RawTable { } } + /// Returns a raw pointer to the table's buffer. + #[inline] + pub fn raw_buffer(&self) -> *const u8 { + self.hashes.ptr() as *const u8 + } + /// Creates a new raw table from a given capacity. All buckets are /// initially empty. pub fn new(capacity: usize) -> Result, FailedAllocationError> {