From 673b448cfe855dc612e64d8ee3a2daad279ea9a8 Mon Sep 17 00:00:00 2001 From: Chris Fallin Date: Mon, 6 Feb 2023 17:21:09 -0800 Subject: [PATCH] Cranelift DFG: make inst clone deep-clone varargs lists. (#5727) When investigating #5716, I found that rematerialization of a `call`, in addition to blowing up for other reasons, caused aliasing of the varargs list (the `EntityList` in the `ListPool`), such that editing the args of the second copy of the call instruction inadvertently updated the first as well. This PR modifies `DataFlowGraph::clone_inst` so that it always clones the varargs list if present. This shouldn't have any functional impact on Cranelift today, because we don't rematerialize any instructions with varargs; but it's important to get it right to avoid a bug later! --- cranelift/codegen/meta/src/gen_inst.rs | 77 ++++++++++++++++++++++++ cranelift/codegen/src/ir/dfg.rs | 25 ++++++++ cranelift/codegen/src/ir/instructions.rs | 9 +++ 3 files changed, 111 insertions(+) diff --git a/cranelift/codegen/meta/src/gen_inst.rs b/cranelift/codegen/meta/src/gen_inst.rs index 25b413c7d2..db43caef62 100644 --- a/cranelift/codegen/meta/src/gen_inst.rs +++ b/cranelift/codegen/meta/src/gen_inst.rs @@ -383,6 +383,83 @@ fn gen_instruction_data_impl(formats: &[&InstructionFormat], fmt: &mut Formatter }); fmt.line("}"); }); + fmt.line("}"); + + fmt.empty_line(); + + fmt.doc_comment(r#" + Deep-clone an `InstructionData`, including any referenced lists. + + This operation requires a reference to a `ValueListPool` to + clone the `ValueLists`. + "#); + fmt.line("pub fn deep_clone(&self, pool: &mut ir::ValueListPool) -> Self {"); + fmt.indent(|fmt| { + fmt.line("match *self {"); + fmt.indent(|fmt| { + for format in formats { + let name = format!("Self::{}", format.name); + let mut members = vec!["opcode"]; + + if format.has_value_list { + members.push("ref args"); + } else if format.num_value_operands == 1 { + members.push("arg"); + } else if format.num_value_operands > 0 { + members.push("args"); + } + + match format.num_block_operands { + 0 => {} + 1 => { + members.push("destination"); + } + _ => { + members.push("blocks"); + } + }; + + for field in &format.imm_fields { + members.push(field.member); + } + let members = members.join(", "); + + fmtln!(fmt, "{}{{{}}} => {{", name, members ); // beware the moustaches + fmt.indent(|fmt| { + fmtln!(fmt, "Self::{} {{", format.name); + fmt.indent(|fmt| { + fmtln!(fmt, "opcode,"); + + if format.has_value_list { + fmtln!(fmt, "args: args.deep_clone(pool),"); + } else if format.num_value_operands == 1 { + fmtln!(fmt, "arg,"); + } else if format.num_value_operands > 0 { + fmtln!(fmt, "args,"); + } + + match format.num_block_operands { + 0 => {} + 1 => { + fmtln!(fmt, "destination: destination.deep_clone(pool),"); + } + 2 => { + fmtln!(fmt, "blocks: [blocks[0].deep_clone(pool), blocks[1].deep_clone(pool)],"); + } + _ => panic!("Too many block targets in instruction"), + } + + for field in &format.imm_fields { + fmtln!(fmt, "{},", field.member); + } + }); + fmtln!(fmt, "}"); + }); + fmtln!(fmt, "}"); + } + }); + fmt.line("}"); + }); fmt.line("}"); }); fmt.line("}"); diff --git a/cranelift/codegen/src/ir/dfg.rs b/cranelift/codegen/src/ir/dfg.rs index d180695219..7b3246187b 100644 --- a/cranelift/codegen/src/ir/dfg.rs +++ b/cranelift/codegen/src/ir/dfg.rs @@ -907,6 +907,10 @@ impl DataFlowGraph { pub fn clone_inst(&mut self, inst: Inst) -> Inst { // First, add a clone of the InstructionData. let inst_data = self.insts[inst].clone(); + // If the `inst_data` has a reference to a ValueList, clone it + // as well, because we can't share these (otherwise mutating + // one would affect the other). + let inst_data = inst_data.deep_clone(&mut self.value_lists); let new_inst = self.make_inst(inst_data); // Get the controlling type variable. let ctrl_typevar = self.ctrl_typevar(inst); @@ -1615,4 +1619,25 @@ mod tests { assert_eq!(pos.func.dfg.resolve_aliases(c2), c2); assert_eq!(pos.func.dfg.resolve_aliases(c), c2); } + + #[test] + fn cloning() { + use crate::ir::InstBuilder; + + let mut func = Function::new(); + let mut sig = Signature::new(crate::isa::CallConv::SystemV); + sig.params.push(ir::AbiParam::new(types::I32)); + let sig = func.import_signature(sig); + let block0 = func.dfg.make_block(); + let mut pos = FuncCursor::new(&mut func); + pos.insert_block(block0); + let v1 = pos.ins().iconst(types::I32, 0); + let v2 = pos.ins().iconst(types::I32, 1); + let call_inst = pos.ins().call_indirect(sig, v1, &[v1]); + let func = pos.func; + + let call_inst_dup = func.dfg.clone_inst(call_inst); + func.dfg.inst_args_mut(call_inst)[0] = v2; + assert_eq!(v1, func.dfg.inst_args(call_inst_dup)[0]); + } } diff --git a/cranelift/codegen/src/ir/instructions.rs b/cranelift/codegen/src/ir/instructions.rs index 2771915a54..9513949a6e 100644 --- a/cranelift/codegen/src/ir/instructions.rs +++ b/cranelift/codegen/src/ir/instructions.rs @@ -123,6 +123,15 @@ impl BlockCall { pub fn display<'a>(&self, pool: &'a ValueListPool) -> DisplayBlockCall<'a> { DisplayBlockCall { block: *self, pool } } + + /// Deep-clone the underlying list in the same pool. The returned + /// list will have identical contents but changes to this list + /// will not change its contents or vice-versa. + pub fn deep_clone(&self, pool: &mut ValueListPool) -> Self { + Self { + values: self.values.deep_clone(pool), + } + } } /// Wrapper for the context needed to display a [BlockCall] value.