From 5580e5e1dda4557bdbda14cdb4255a3e6facfe86 Mon Sep 17 00:00:00 2001 From: Aaron Hill Date: Fri, 24 Dec 2021 16:36:33 -0500 Subject: [PATCH] Ensure that `Fingerprint` caching respects hashing configuration Fixes #92266 In some `HashStable` impls, we use a cache to avoid re-computing the same `Fingerprint` from the same structure (e.g. an `AdtDef`). However, the `StableHashingContext` used can be configured to perform hashing in different ways (e.g. skipping `Span`s). This configuration information is not included in the cache key, which will cause an incorrect `Fingerprint` to be used if we hash the same structure with different `StableHashingContext` settings. To fix this, the configuration settings of `StableHashingContext` are split out into a separate `HashingControls` struct. This struct is used as part of the cache key, ensuring that our caches always produce the correct result for the given settings. With this in place, we now turn off `Span` hashing during the entire process of computing the hash included in legacy symbols. This current has no effect, but will matter when a future PR starts hashing more `Span`s that we currently skip. --- .../src/stable_hasher.rs | 19 +++++++++ compiler/rustc_middle/src/ty/adt.rs | 6 ++- compiler/rustc_middle/src/ty/impls_ty.rs | 5 ++- compiler/rustc_query_system/src/ich/hcx.rs | 40 ++++++++++-------- .../rustc_query_system/src/ich/impls_hir.rs | 8 ++-- compiler/rustc_query_system/src/ich/mod.rs | 3 +- compiler/rustc_span/src/hygiene.rs | 24 +++++++++++ compiler/rustc_span/src/lib.rs | 2 + compiler/rustc_symbol_mangling/src/legacy.rs | 42 +++++++++---------- 9 files changed, 101 insertions(+), 48 deletions(-) diff --git a/compiler/rustc_data_structures/src/stable_hasher.rs b/compiler/rustc_data_structures/src/stable_hasher.rs index 3da351789571..9c09a7f5f822 100644 --- a/compiler/rustc_data_structures/src/stable_hasher.rs +++ b/compiler/rustc_data_structures/src/stable_hasher.rs @@ -583,3 +583,22 @@ fn stable_hash_reduce( } } } + +#[derive(PartialEq, Eq, Clone, Copy, Hash, Debug)] +pub enum NodeIdHashingMode { + Ignore, + HashDefPath, +} + +/// Controls what data we do or not not hash. +/// Whenever a `HashStable` implementation caches its +/// result, it needs to include `HashingControls` as part +/// of the key, to ensure that is does not produce an incorrect +/// result (for example, using a `Fingerprint` produced while +/// hashing `Span`s when a `Fingeprint` without `Span`s is +/// being requested) +#[derive(Clone, Hash, Eq, PartialEq, Debug)] +pub struct HashingControls { + pub hash_spans: bool, + pub node_id_hashing_mode: NodeIdHashingMode, +} diff --git a/compiler/rustc_middle/src/ty/adt.rs b/compiler/rustc_middle/src/ty/adt.rs index 5cde54c9328d..6cec75d36e2c 100644 --- a/compiler/rustc_middle/src/ty/adt.rs +++ b/compiler/rustc_middle/src/ty/adt.rs @@ -4,6 +4,7 @@ use crate::ty::util::{Discr, IntTypeExt}; use rustc_data_structures::captures::Captures; use rustc_data_structures::fingerprint::Fingerprint; use rustc_data_structures::fx::FxHashMap; +use rustc_data_structures::stable_hasher::HashingControls; use rustc_data_structures::stable_hasher::{HashStable, StableHasher}; use rustc_errors::ErrorReported; use rustc_hir as hir; @@ -136,12 +137,13 @@ impl Hash for AdtDef { impl<'a> HashStable> for AdtDef { fn hash_stable(&self, hcx: &mut StableHashingContext<'a>, hasher: &mut StableHasher) { thread_local! { - static CACHE: RefCell> = Default::default(); + static CACHE: RefCell> = Default::default(); } let hash: Fingerprint = CACHE.with(|cache| { let addr = self as *const AdtDef as usize; - *cache.borrow_mut().entry(addr).or_insert_with(|| { + let hashing_controls = hcx.hashing_controls(); + *cache.borrow_mut().entry((addr, hashing_controls)).or_insert_with(|| { let ty::AdtDef { did, ref variants, ref flags, ref repr } = *self; let mut hasher = StableHasher::new(); diff --git a/compiler/rustc_middle/src/ty/impls_ty.rs b/compiler/rustc_middle/src/ty/impls_ty.rs index 9f47ed89f13f..00ce15bea3f2 100644 --- a/compiler/rustc_middle/src/ty/impls_ty.rs +++ b/compiler/rustc_middle/src/ty/impls_ty.rs @@ -6,6 +6,7 @@ use crate::mir; use crate::ty; use rustc_data_structures::fingerprint::Fingerprint; use rustc_data_structures::fx::FxHashMap; +use rustc_data_structures::stable_hasher::HashingControls; use rustc_data_structures::stable_hasher::{HashStable, StableHasher, ToStableHashKey}; use rustc_query_system::ich::StableHashingContext; use std::cell::RefCell; @@ -17,12 +18,12 @@ where { fn hash_stable(&self, hcx: &mut StableHashingContext<'a>, hasher: &mut StableHasher) { thread_local! { - static CACHE: RefCell> = + static CACHE: RefCell> = RefCell::new(Default::default()); } let hash = CACHE.with(|cache| { - let key = (self.as_ptr() as usize, self.len()); + let key = (self.as_ptr() as usize, self.len(), hcx.hashing_controls()); if let Some(&hash) = cache.borrow().get(&key) { return hash; } diff --git a/compiler/rustc_query_system/src/ich/hcx.rs b/compiler/rustc_query_system/src/ich/hcx.rs index 5f31fa04b8a6..9f9de707e7ab 100644 --- a/compiler/rustc_query_system/src/ich/hcx.rs +++ b/compiler/rustc_query_system/src/ich/hcx.rs @@ -3,6 +3,7 @@ use rustc_ast as ast; use rustc_data_structures::fx::FxHashSet; use rustc_data_structures::sorted_map::SortedMap; use rustc_data_structures::stable_hasher::{HashStable, StableHasher}; +use rustc_data_structures::stable_hasher::{HashingControls, NodeIdHashingMode}; use rustc_data_structures::sync::Lrc; use rustc_hir as hir; use rustc_hir::def_id::{DefId, LocalDefId}; @@ -27,19 +28,11 @@ pub struct StableHashingContext<'a> { definitions: &'a Definitions, cstore: &'a dyn CrateStore, pub(super) body_resolver: BodyResolver<'a>, - hash_spans: bool, - pub(super) node_id_hashing_mode: NodeIdHashingMode, - // Very often, we are hashing something that does not need the // `CachingSourceMapView`, so we initialize it lazily. raw_source_map: &'a SourceMap, caching_source_map: Option>, -} - -#[derive(PartialEq, Eq, Clone, Copy)] -pub enum NodeIdHashingMode { - Ignore, - HashDefPath, + pub(super) hashing_controls: HashingControls, } /// The `BodyResolver` allows mapping a `BodyId` to the corresponding `hir::Body`. @@ -72,8 +65,10 @@ impl<'a> StableHashingContext<'a> { cstore, caching_source_map: None, raw_source_map: sess.source_map(), - hash_spans: hash_spans_initial, - node_id_hashing_mode: NodeIdHashingMode::HashDefPath, + hashing_controls: HashingControls { + hash_spans: hash_spans_initial, + node_id_hashing_mode: NodeIdHashingMode::HashDefPath, + }, } } @@ -133,10 +128,10 @@ impl<'a> StableHashingContext<'a> { #[inline] pub fn while_hashing_spans(&mut self, hash_spans: bool, f: F) { - let prev_hash_spans = self.hash_spans; - self.hash_spans = hash_spans; + let prev_hash_spans = self.hashing_controls.hash_spans; + self.hashing_controls.hash_spans = hash_spans; f(self); - self.hash_spans = prev_hash_spans; + self.hashing_controls.hash_spans = prev_hash_spans; } #[inline] @@ -145,10 +140,10 @@ impl<'a> StableHashingContext<'a> { mode: NodeIdHashingMode, f: F, ) { - let prev = self.node_id_hashing_mode; - self.node_id_hashing_mode = mode; + let prev = self.hashing_controls.node_id_hashing_mode; + self.hashing_controls.node_id_hashing_mode = mode; f(self); - self.node_id_hashing_mode = prev; + self.hashing_controls.node_id_hashing_mode = prev; } #[inline] @@ -183,6 +178,10 @@ impl<'a> StableHashingContext<'a> { } IGNORED_ATTRIBUTES.with(|attrs| attrs.contains(&name)) } + + pub fn hashing_controls(&self) -> HashingControls { + self.hashing_controls.clone() + } } impl<'a> HashStable> for ast::NodeId { @@ -195,7 +194,7 @@ impl<'a> HashStable> for ast::NodeId { impl<'a> rustc_span::HashStableContext for StableHashingContext<'a> { #[inline] fn hash_spans(&self) -> bool { - self.hash_spans + self.hashing_controls.hash_spans } #[inline] @@ -215,6 +214,11 @@ impl<'a> rustc_span::HashStableContext for StableHashingContext<'a> { ) -> Option<(Lrc, usize, BytePos, usize, BytePos)> { self.source_map().span_data_to_lines_and_cols(span) } + + #[inline] + fn hashing_controls(&self) -> HashingControls { + self.hashing_controls.clone() + } } impl<'a> rustc_session::HashStableContext for StableHashingContext<'a> {} diff --git a/compiler/rustc_query_system/src/ich/impls_hir.rs b/compiler/rustc_query_system/src/ich/impls_hir.rs index 3117140a5b61..bf3cf6a48fd0 100644 --- a/compiler/rustc_query_system/src/ich/impls_hir.rs +++ b/compiler/rustc_query_system/src/ich/impls_hir.rs @@ -11,7 +11,7 @@ impl<'ctx> rustc_hir::HashStableContext for StableHashingContext<'ctx> { #[inline] fn hash_hir_id(&mut self, hir_id: hir::HirId, hasher: &mut StableHasher) { let hcx = self; - match hcx.node_id_hashing_mode { + match hcx.hashing_controls.node_id_hashing_mode { NodeIdHashingMode::Ignore => { // Don't do anything. } @@ -89,12 +89,12 @@ impl<'ctx> rustc_hir::HashStableContext for StableHashingContext<'ctx> { #[inline] fn hash_hir_item_like(&mut self, f: F) { - let prev_hash_node_ids = self.node_id_hashing_mode; - self.node_id_hashing_mode = NodeIdHashingMode::Ignore; + let prev_hash_node_ids = self.hashing_controls.node_id_hashing_mode; + self.hashing_controls.node_id_hashing_mode = NodeIdHashingMode::Ignore; f(self); - self.node_id_hashing_mode = prev_hash_node_ids; + self.hashing_controls.node_id_hashing_mode = prev_hash_node_ids; } #[inline] diff --git a/compiler/rustc_query_system/src/ich/mod.rs b/compiler/rustc_query_system/src/ich/mod.rs index 54416902e5fb..c42fcc9c82e1 100644 --- a/compiler/rustc_query_system/src/ich/mod.rs +++ b/compiler/rustc_query_system/src/ich/mod.rs @@ -1,6 +1,7 @@ //! ICH - Incremental Compilation Hash -pub use self::hcx::{NodeIdHashingMode, StableHashingContext}; +pub use self::hcx::StableHashingContext; +pub use rustc_data_structures::stable_hasher::NodeIdHashingMode; use rustc_span::symbol::{sym, Symbol}; mod hcx; diff --git a/compiler/rustc_span/src/hygiene.rs b/compiler/rustc_span/src/hygiene.rs index 315b706fbc44..6bb4f31cbffb 100644 --- a/compiler/rustc_span/src/hygiene.rs +++ b/compiler/rustc_span/src/hygiene.rs @@ -32,6 +32,7 @@ use crate::{HashStableContext, Span, DUMMY_SP}; use crate::def_id::{CrateNum, DefId, StableCrateId, CRATE_DEF_ID, LOCAL_CRATE}; use rustc_data_structures::fingerprint::Fingerprint; use rustc_data_structures::fx::{FxHashMap, FxHashSet}; +use rustc_data_structures::stable_hasher::HashingControls; use rustc_data_structures::stable_hasher::{HashStable, StableHasher}; use rustc_data_structures::sync::{Lock, Lrc}; use rustc_data_structures::unhash::UnhashMap; @@ -88,6 +89,27 @@ rustc_index::newtype_index! { } } +// Assert that the provided `HashStableContext` is configured with the 'default' +// `HashingControls`. We should always have bailed out before getting to here +// with a non-default mode. With this check in place, we can avoid the need +// to maintain separate versions of `ExpnData` hashes for each permutation +// of `HashingControls` settings. +fn assert_default_hashing_controls(ctx: &CTX, msg: &str) { + match ctx.hashing_controls() { + // Ideally, we would also check that `node_id_hashing_mode` was always + // `NodeIdHashingMode::HashDefPath`. However, we currently end up hashing + // `Span`s in this mode, and there's not an easy way to change that. + // All of the span-related data that we hash is pretty self-contained + // (in particular, we don't hash any `HirId`s), so this shouldn't result + // in any caching problems. + // FIXME: Enforce that we don't end up transitively hashing any `HirId`s, + // or ensure that this method is always invoked with the same + // `NodeIdHashingMode` + HashingControls { hash_spans: true, node_id_hashing_mode: _ } => {} + other => panic!("Attempted hashing of {msg} with non-default HashingControls: {:?}", other), + } +} + /// A unique hash value associated to an expansion. #[derive(Clone, Copy, PartialEq, Eq, Hash, Debug, Encodable, Decodable, HashStable_Generic)] pub struct ExpnHash(Fingerprint); @@ -1444,6 +1466,7 @@ fn update_disambiguator(expn_data: &mut ExpnData, mut ctx: impl HashStableContex "Already set disambiguator for ExpnData: {:?}", expn_data ); + assert_default_hashing_controls(&ctx, "ExpnData (disambiguator)"); let mut expn_hash = expn_data.hash_expn(&mut ctx); let disambiguator = HygieneData::with(|data| { @@ -1493,6 +1516,7 @@ impl HashStable for SyntaxContext { impl HashStable for ExpnId { fn hash_stable(&self, ctx: &mut CTX, hasher: &mut StableHasher) { + assert_default_hashing_controls(ctx, "ExpnId"); let hash = if *self == ExpnId::root() { // Avoid fetching TLS storage for a trivial often-used value. Fingerprint::ZERO diff --git a/compiler/rustc_span/src/lib.rs b/compiler/rustc_span/src/lib.rs index 3bbf2a0e4566..4d60ca481a05 100644 --- a/compiler/rustc_span/src/lib.rs +++ b/compiler/rustc_span/src/lib.rs @@ -42,6 +42,7 @@ pub mod hygiene; use hygiene::Transparency; pub use hygiene::{DesugaringKind, ExpnKind, MacroKind}; pub use hygiene::{ExpnData, ExpnHash, ExpnId, LocalExpnId, SyntaxContext}; +use rustc_data_structures::stable_hasher::HashingControls; pub mod def_id; use def_id::{CrateNum, DefId, DefPathHash, LocalDefId, LOCAL_CRATE}; pub mod lev_distance; @@ -2062,6 +2063,7 @@ pub trait HashStableContext { &mut self, span: &SpanData, ) -> Option<(Lrc, usize, BytePos, usize, BytePos)>; + fn hashing_controls(&self) -> HashingControls; } impl HashStable for Span diff --git a/compiler/rustc_symbol_mangling/src/legacy.rs b/compiler/rustc_symbol_mangling/src/legacy.rs index eebf618a5ded..6d02d04fe80e 100644 --- a/compiler/rustc_symbol_mangling/src/legacy.rs +++ b/compiler/rustc_symbol_mangling/src/legacy.rs @@ -113,29 +113,29 @@ fn get_symbol_hash<'tcx>( hcx.while_hashing_spans(false, |hcx| { hcx.with_node_id_hashing_mode(NodeIdHashingMode::HashDefPath, |hcx| { item_type.hash_stable(hcx, &mut hasher); + + // If this is a function, we hash the signature as well. + // This is not *strictly* needed, but it may help in some + // situations, see the `run-make/a-b-a-linker-guard` test. + if let ty::FnDef(..) = item_type.kind() { + item_type.fn_sig(tcx).hash_stable(hcx, &mut hasher); + } + + // also include any type parameters (for generic items) + substs.hash_stable(hcx, &mut hasher); + + if let Some(instantiating_crate) = instantiating_crate { + tcx.def_path_hash(instantiating_crate.as_def_id()) + .stable_crate_id() + .hash_stable(hcx, &mut hasher); + } + + // We want to avoid accidental collision between different types of instances. + // Especially, `VtableShim`s and `ReifyShim`s may overlap with their original + // instances without this. + discriminant(&instance.def).hash_stable(hcx, &mut hasher); }); }); - - // If this is a function, we hash the signature as well. - // This is not *strictly* needed, but it may help in some - // situations, see the `run-make/a-b-a-linker-guard` test. - if let ty::FnDef(..) = item_type.kind() { - item_type.fn_sig(tcx).hash_stable(&mut hcx, &mut hasher); - } - - // also include any type parameters (for generic items) - substs.hash_stable(&mut hcx, &mut hasher); - - if let Some(instantiating_crate) = instantiating_crate { - tcx.def_path_hash(instantiating_crate.as_def_id()) - .stable_crate_id() - .hash_stable(&mut hcx, &mut hasher); - } - - // We want to avoid accidental collision between different types of instances. - // Especially, `VtableShim`s and `ReifyShim`s may overlap with their original - // instances without this. - discriminant(&instance.def).hash_stable(&mut hcx, &mut hasher); }); // 64 bits should be enough to avoid collisions.