From 5580e5e1dda4557bdbda14cdb4255a3e6facfe86 Mon Sep 17 00:00:00 2001 From: Aaron Hill Date: Fri, 24 Dec 2021 16:36:33 -0500 Subject: 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. --- compiler/rustc_query_system/src/ich/hcx.rs | 40 +++++++++++++----------- compiler/rustc_query_system/src/ich/impls_hir.rs | 8 ++--- compiler/rustc_query_system/src/ich/mod.rs | 3 +- 3 files changed, 28 insertions(+), 23 deletions(-) (limited to 'compiler/rustc_query_system/src') diff --git a/compiler/rustc_query_system/src/ich/hcx.rs b/compiler/rustc_query_system/src/ich/hcx.rs index 5f31fa04b8a..9f9de707e7a 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 3117140a5b6..bf3cf6a48fd 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 54416902e5f..c42fcc9c82e 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; -- cgit 1.4.1-3-g733a5 From 560c90f5df4ecfbb75e20ae19bf10028756a3613 Mon Sep 17 00:00:00 2001 From: Aaron Hill Date: Sun, 26 Dec 2021 17:45:21 -0500 Subject: Adjust assert_default_hashing_controls --- compiler/rustc_query_system/src/ich/hcx.rs | 7 +++++++ compiler/rustc_span/src/hygiene.rs | 18 ++++++++++++------ compiler/rustc_span/src/lib.rs | 3 +++ 3 files changed, 22 insertions(+), 6 deletions(-) (limited to 'compiler/rustc_query_system/src') diff --git a/compiler/rustc_query_system/src/ich/hcx.rs b/compiler/rustc_query_system/src/ich/hcx.rs index 9f9de707e7a..6838a32c7b3 100644 --- a/compiler/rustc_query_system/src/ich/hcx.rs +++ b/compiler/rustc_query_system/src/ich/hcx.rs @@ -27,6 +27,7 @@ fn compute_ignored_attr_names() -> FxHashSet { pub struct StableHashingContext<'a> { definitions: &'a Definitions, cstore: &'a dyn CrateStore, + sess: &'a Session, pub(super) body_resolver: BodyResolver<'a>, // Very often, we are hashing something that does not need the // `CachingSourceMapView`, so we initialize it lazily. @@ -63,6 +64,7 @@ impl<'a> StableHashingContext<'a> { body_resolver: BodyResolver::Forbidden, definitions, cstore, + sess, caching_source_map: None, raw_source_map: sess.source_map(), hashing_controls: HashingControls { @@ -197,6 +199,11 @@ impl<'a> rustc_span::HashStableContext for StableHashingContext<'a> { self.hashing_controls.hash_spans } + #[inline] + fn debug_opts_incremental_ignore_spans(&self) -> bool { + self.sess.opts.debugging_opts.incremental_ignore_spans + } + #[inline] fn def_path_hash(&self, def_id: DefId) -> DefPathHash { self.def_path_hash(def_id) diff --git a/compiler/rustc_span/src/hygiene.rs b/compiler/rustc_span/src/hygiene.rs index 6bb4f31cbff..7b70c20d307 100644 --- a/compiler/rustc_span/src/hygiene.rs +++ b/compiler/rustc_span/src/hygiene.rs @@ -89,11 +89,11 @@ 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. +/// 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 @@ -105,7 +105,13 @@ fn assert_default_hashing_controls(ctx: &CTX, msg: &str) // 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: _ } => {} + // + // Note that we require that `hash_spans` be set according to the global + // `-Z incremental-ignore-spans` option. Normally, this option is disabled, + // which will cause us to require that this method always be called with `Span` hashing + // enabled. + HashingControls { hash_spans, node_id_hashing_mode: _ } + if hash_spans == !ctx.debug_opts_incremental_ignore_spans() => {} other => panic!("Attempted hashing of {msg} with non-default HashingControls: {:?}", other), } } diff --git a/compiler/rustc_span/src/lib.rs b/compiler/rustc_span/src/lib.rs index 4d60ca481a0..9602bc5d0b7 100644 --- a/compiler/rustc_span/src/lib.rs +++ b/compiler/rustc_span/src/lib.rs @@ -2058,6 +2058,9 @@ impl InnerSpan { pub trait HashStableContext { fn def_path_hash(&self, def_id: DefId) -> DefPathHash; fn hash_spans(&self) -> bool; + /// Accesses `sess.opts.debugging_opts.incremental_ignore_spans` since + /// we don't have easy access to a `Session` + fn debug_opts_incremental_ignore_spans(&self) -> bool; fn def_span(&self, def_id: LocalDefId) -> Span; fn span_data_to_lines_and_cols( &mut self, -- cgit 1.4.1-3-g733a5 From 4ca275add0b443a3fe9f3a0fc4470bb9d9628cfd Mon Sep 17 00:00:00 2001 From: Aaron Hill Date: Wed, 5 Jan 2022 10:30:49 -0500 Subject: Address review comments --- compiler/rustc_query_system/src/ich/hcx.rs | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) (limited to 'compiler/rustc_query_system/src') diff --git a/compiler/rustc_query_system/src/ich/hcx.rs b/compiler/rustc_query_system/src/ich/hcx.rs index 6838a32c7b3..76e21be17bc 100644 --- a/compiler/rustc_query_system/src/ich/hcx.rs +++ b/compiler/rustc_query_system/src/ich/hcx.rs @@ -27,7 +27,9 @@ fn compute_ignored_attr_names() -> FxHashSet { pub struct StableHashingContext<'a> { definitions: &'a Definitions, cstore: &'a dyn CrateStore, - sess: &'a Session, + // The value of `-Z incremental-ignore-spans`. + // This field should only be used by `debug_opts_incremental_ignore_span` + incremental_ignore_spans: bool, pub(super) body_resolver: BodyResolver<'a>, // Very often, we are hashing something that does not need the // `CachingSourceMapView`, so we initialize it lazily. @@ -64,7 +66,7 @@ impl<'a> StableHashingContext<'a> { body_resolver: BodyResolver::Forbidden, definitions, cstore, - sess, + incremental_ignore_spans: sess.opts.debugging_opts.incremental_ignore_spans, caching_source_map: None, raw_source_map: sess.source_map(), hashing_controls: HashingControls { @@ -181,6 +183,7 @@ impl<'a> StableHashingContext<'a> { IGNORED_ATTRIBUTES.with(|attrs| attrs.contains(&name)) } + #[inline] pub fn hashing_controls(&self) -> HashingControls { self.hashing_controls.clone() } @@ -201,7 +204,7 @@ impl<'a> rustc_span::HashStableContext for StableHashingContext<'a> { #[inline] fn debug_opts_incremental_ignore_spans(&self) -> bool { - self.sess.opts.debugging_opts.incremental_ignore_spans + self.incremental_ignore_spans } #[inline] -- cgit 1.4.1-3-g733a5