Skip to content

Commit

Permalink
Auto merge of #92278 - Aaron1011:fix-fingerprint-caching, r=michaelwo…
Browse files Browse the repository at this point in the history
…erister

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.
  • Loading branch information
bors committed Jan 10, 2022
2 parents 092e1c9 + 4ca275a commit d63a8d9
Show file tree
Hide file tree
Showing 9 changed files with 118 additions and 46 deletions.
19 changes: 19 additions & 0 deletions compiler/rustc_data_structures/src/stable_hasher.rs
Original file line number Diff line number Diff line change
Expand Up @@ -583,3 +583,22 @@ fn stable_hash_reduce<HCX, I, C, F>(
}
}
}

#[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,
}
6 changes: 4 additions & 2 deletions compiler/rustc_middle/src/ty/adt.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -136,12 +137,13 @@ impl Hash for AdtDef {
impl<'a> HashStable<StableHashingContext<'a>> for AdtDef {
fn hash_stable(&self, hcx: &mut StableHashingContext<'a>, hasher: &mut StableHasher) {
thread_local! {
static CACHE: RefCell<FxHashMap<usize, Fingerprint>> = Default::default();
static CACHE: RefCell<FxHashMap<(usize, HashingControls), Fingerprint>> = 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();
Expand Down
5 changes: 3 additions & 2 deletions compiler/rustc_middle/src/ty/impls_ty.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -17,12 +18,12 @@ where
{
fn hash_stable(&self, hcx: &mut StableHashingContext<'a>, hasher: &mut StableHasher) {
thread_local! {
static CACHE: RefCell<FxHashMap<(usize, usize), Fingerprint>> =
static CACHE: RefCell<FxHashMap<(usize, usize, HashingControls), Fingerprint>> =
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;
}
Expand Down
50 changes: 32 additions & 18 deletions compiler/rustc_query_system/src/ich/hcx.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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};
Expand All @@ -26,20 +27,15 @@ fn compute_ignored_attr_names() -> FxHashSet<Symbol> {
pub struct StableHashingContext<'a> {
definitions: &'a Definitions,
cstore: &'a dyn CrateStore,
// 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>,
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<CachingSourceMapView<'a>>,
}

#[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`.
Expand Down Expand Up @@ -70,10 +66,13 @@ impl<'a> StableHashingContext<'a> {
body_resolver: BodyResolver::Forbidden,
definitions,
cstore,
incremental_ignore_spans: sess.opts.debugging_opts.incremental_ignore_spans,
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,
},
}
}

Expand Down Expand Up @@ -133,10 +132,10 @@ impl<'a> StableHashingContext<'a> {

#[inline]
pub fn while_hashing_spans<F: FnOnce(&mut Self)>(&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]
Expand All @@ -145,10 +144,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]
Expand Down Expand Up @@ -183,6 +182,11 @@ impl<'a> StableHashingContext<'a> {
}
IGNORED_ATTRIBUTES.with(|attrs| attrs.contains(&name))
}

#[inline]
pub fn hashing_controls(&self) -> HashingControls {
self.hashing_controls.clone()
}
}

impl<'a> HashStable<StableHashingContext<'a>> for ast::NodeId {
Expand All @@ -195,7 +199,12 @@ impl<'a> HashStable<StableHashingContext<'a>> 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]
fn debug_opts_incremental_ignore_spans(&self) -> bool {
self.incremental_ignore_spans
}

#[inline]
Expand All @@ -215,6 +224,11 @@ impl<'a> rustc_span::HashStableContext for StableHashingContext<'a> {
) -> Option<(Lrc<SourceFile>, 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> {}
8 changes: 4 additions & 4 deletions compiler/rustc_query_system/src/ich/impls_hir.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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.
}
Expand Down Expand Up @@ -89,12 +89,12 @@ impl<'ctx> rustc_hir::HashStableContext for StableHashingContext<'ctx> {

#[inline]
fn hash_hir_item_like<F: FnOnce(&mut Self)>(&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]
Expand Down
3 changes: 2 additions & 1 deletion compiler/rustc_query_system/src/ich/mod.rs
Original file line number Diff line number Diff line change
@@ -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;
Expand Down
30 changes: 30 additions & 0 deletions compiler/rustc_span/src/hygiene.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -88,6 +89,33 @@ 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: HashStableContext>(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`
//
// 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),
}
}

/// A unique hash value associated to an expansion.
#[derive(Clone, Copy, PartialEq, Eq, Hash, Debug, Encodable, Decodable, HashStable_Generic)]
pub struct ExpnHash(Fingerprint);
Expand Down Expand Up @@ -1444,6 +1472,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| {
Expand Down Expand Up @@ -1493,6 +1522,7 @@ impl<CTX: HashStableContext> HashStable<CTX> for SyntaxContext {

impl<CTX: HashStableContext> HashStable<CTX> 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
Expand Down
5 changes: 5 additions & 0 deletions compiler/rustc_span/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -2057,11 +2058,15 @@ 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,
span: &SpanData,
) -> Option<(Lrc<SourceFile>, usize, BytePos, usize, BytePos)>;
fn hashing_controls(&self) -> HashingControls;
}

impl<CTX> HashStable<CTX> for Span
Expand Down
38 changes: 19 additions & 19 deletions compiler/rustc_symbol_mangling/src/legacy.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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(&mut 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(&mut 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(&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(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);
// 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);
});
});
});

// 64 bits should be enough to avoid collisions.
Expand Down

0 comments on commit d63a8d9

Please sign in to comment.