Browse Source

Ensure the sequence number doesn't leak out of Layout (#6061)

Previously it could affect the PartialEq and Hash impls. Ignoring the
sequence number in PartialEq and Hash allows us to not renumber all
blocks in the incremental cache.
pull/6072/head
bjorn3 2 years ago
committed by GitHub
parent
commit
49bab6db7f
No known key found for this signature in database GPG Key ID: 4AEE18F83AFDEB23
  1. 18
      cranelift/codegen/src/incremental_cache.rs
  2. 22
      cranelift/codegen/src/ir/layout.rs
  3. 4
      fuzz/fuzz_targets/cranelift-icache.rs

18
cranelift/codegen/src/incremental_cache.rs

@ -44,7 +44,7 @@ impl Context {
let cache_key_hash = { let cache_key_hash = {
let _tt = timing::try_incremental_cache(); let _tt = timing::try_incremental_cache();
let cache_key_hash = compute_cache_key(isa, &mut self.func); let cache_key_hash = compute_cache_key(isa, &self.func);
if let Some(blob) = cache_store.get(&cache_key_hash.0) { if let Some(blob) = cache_store.get(&cache_key_hash.0) {
match try_finish_recompile(&self.func, &blob) { match try_finish_recompile(&self.func, &blob) {
@ -162,19 +162,7 @@ impl<'a> CacheKey<'a> {
/// Creates a new cache store key for a function. /// Creates a new cache store key for a function.
/// ///
/// This is a bit expensive to compute, so it should be cached and reused as much as possible. /// This is a bit expensive to compute, so it should be cached and reused as much as possible.
fn new(isa: &dyn TargetIsa, f: &'a mut Function) -> Self { fn new(isa: &dyn TargetIsa, f: &'a Function) -> Self {
// Make sure the blocks and instructions are sequenced the same way as we might
// have serialized them earlier. This is the symmetric of what's done in
// `try_load`.
let mut block = f.stencil.layout.entry_block().expect("Missing entry block");
loop {
f.stencil.layout.full_block_renumber(block);
if let Some(next_block) = f.stencil.layout.next_block(block) {
block = next_block;
} else {
break;
}
}
CacheKey { CacheKey {
stencil: &f.stencil, stencil: &f.stencil,
parameters: CompileParameters::from_isa(isa), parameters: CompileParameters::from_isa(isa),
@ -185,7 +173,7 @@ impl<'a> CacheKey<'a> {
/// Compute a cache key, and hash it on your behalf. /// Compute a cache key, and hash it on your behalf.
/// ///
/// Since computing the `CacheKey` is a bit expensive, it should be done as least as possible. /// Since computing the `CacheKey` is a bit expensive, it should be done as least as possible.
pub fn compute_cache_key(isa: &dyn TargetIsa, func: &mut Function) -> CacheKeyHash { pub fn compute_cache_key(isa: &dyn TargetIsa, func: &Function) -> CacheKeyHash {
use core::hash::{Hash as _, Hasher}; use core::hash::{Hash as _, Hasher};
use sha2::Digest as _; use sha2::Digest as _;

22
cranelift/codegen/src/ir/layout.rs

@ -209,7 +209,7 @@ impl Layout {
/// ///
/// This doesn't affect the position of anything, but it gives more room in the internal /// This doesn't affect the position of anything, but it gives more room in the internal
/// sequence numbers for inserting instructions later. /// sequence numbers for inserting instructions later.
pub(crate) fn full_block_renumber(&mut self, block: Block) { fn full_block_renumber(&mut self, block: Block) {
let _tt = timing::layout_renumber(); let _tt = timing::layout_renumber();
// Avoid 0 as this is reserved for the program point indicating the block itself // Avoid 0 as this is reserved for the program point indicating the block itself
let mut seq = MAJOR_STRIDE; let mut seq = MAJOR_STRIDE;
@ -599,7 +599,7 @@ impl Layout {
} }
} }
#[derive(Clone, Debug, Default, PartialEq, Hash)] #[derive(Clone, Debug, Default)]
struct InstNode { struct InstNode {
/// The Block containing this instruction, or `None` if the instruction is not yet inserted. /// The Block containing this instruction, or `None` if the instruction is not yet inserted.
block: PackedOption<Block>, block: PackedOption<Block>,
@ -608,6 +608,24 @@ struct InstNode {
seq: SequenceNumber, seq: SequenceNumber,
} }
impl PartialEq for InstNode {
fn eq(&self, other: &Self) -> bool {
// Ignore the sequence number as it is an optimization used by pp_cmp and may be different
// even for equivalent layouts.
self.block == other.block && self.prev == other.prev && self.next == other.next
}
}
impl core::hash::Hash for InstNode {
fn hash<H: core::hash::Hasher>(&self, state: &mut H) {
// Ignore the sequence number as it is an optimization used by pp_cmp and may be different
// even for equivalent layouts.
self.block.hash(state);
self.prev.hash(state);
self.next.hash(state);
}
}
/// Iterate over instructions in a block in layout order. See `Layout::block_insts()`. /// Iterate over instructions in a block in layout order. See `Layout::block_insts()`.
pub struct Insts<'f> { pub struct Insts<'f> {
layout: &'f Layout, layout: &'f Layout,

4
fuzz/fuzz_targets/cranelift-icache.rs

@ -103,7 +103,7 @@ impl<'a> Arbitrary<'a> for FunctionWithIsa {
fuzz_target!(|func: FunctionWithIsa| { fuzz_target!(|func: FunctionWithIsa| {
let FunctionWithIsa { mut func, isa } = func; let FunctionWithIsa { mut func, isa } = func;
let cache_key_hash = icache::compute_cache_key(&*isa, &mut func); let cache_key_hash = icache::compute_cache_key(&*isa, &func);
let mut context = Context::for_function(func.clone()); let mut context = Context::for_function(func.clone());
let prev_stencil = match context.compile_stencil(&*isa) { let prev_stencil = match context.compile_stencil(&*isa) {
@ -187,7 +187,7 @@ fuzz_target!(|func: FunctionWithIsa| {
false false
}; };
let new_cache_key_hash = icache::compute_cache_key(&*isa, &mut func); let new_cache_key_hash = icache::compute_cache_key(&*isa, &func);
if expect_cache_hit { if expect_cache_hit {
assert!(cache_key_hash == new_cache_key_hash); assert!(cache_key_hash == new_cache_key_hash);

Loading…
Cancel
Save