From 40db4de44ac4b2b42ee3a8b1fd406fb5ff141765 Mon Sep 17 00:00:00 2001 From: Chris Fallin Date: Wed, 24 Feb 2021 13:57:52 -0800 Subject: [PATCH] Fix incomplete trap metadata due to multiple traps at one address. If an instruction has more than one trap record associated with it (for example: a divide instruction that has participated in load-op fusion, so we have both a heap-out-of-bounds trap record due to its load and a divide-by-zero trap record due to its divide op), the current MachBuffer code would emit only one of the trap records to the sink. Separately, divide instructions probably shouldn't merge loads, because the two separate possible traps at one location might be confusing for some embedders (certainly in Lucet). Divide seems to be the only case in our current codegen where such merging might occur. This PR changes the lowering to always force the divisor into a register. Finally, while working out why trap records were not appearing, I had noticed that `isa::x64::emit_std_enc_mem()` was only emitting heap-OOB trap metadata for loads/stores when it had a srcloc. This PR ensures that the metadata is emitted even when the srcloc is empty. Note that none of the above presents a security or correctness problem; trap metadata only affects the status that we return to the embedder when a Wasm program terminates with a trap. --- cranelift/codegen/src/isa/x64/inst/emit.rs | 2 +- cranelift/codegen/src/isa/x64/lower.rs | 5 +- cranelift/codegen/src/machinst/buffer.rs | 118 ++++++++++++++++++--- 3 files changed, 108 insertions(+), 17 deletions(-) diff --git a/cranelift/codegen/src/isa/x64/inst/emit.rs b/cranelift/codegen/src/isa/x64/inst/emit.rs index 51775e0a23bb..f42953c2df36 100644 --- a/cranelift/codegen/src/isa/x64/inst/emit.rs +++ b/cranelift/codegen/src/isa/x64/inst/emit.rs @@ -232,7 +232,7 @@ fn emit_std_enc_mem( let srcloc = state.cur_srcloc(); let can_trap = mem_e.can_trap(); - if srcloc != SourceLoc::default() && can_trap { + if can_trap { sink.add_trap(srcloc, TrapCode::HeapOutOfBounds); } diff --git a/cranelift/codegen/src/isa/x64/lower.rs b/cranelift/codegen/src/isa/x64/lower.rs index c720b28a3230..d96718c0d76b 100644 --- a/cranelift/codegen/src/isa/x64/lower.rs +++ b/cranelift/codegen/src/isa/x64/lower.rs @@ -5113,7 +5113,10 @@ fn lower_insn_to_regs>( )); ctx.emit(Inst::checked_div_or_rem_seq(kind, size, divisor_copy, tmp)); } else { - let divisor = input_to_reg_mem(ctx, inputs[1]); + // We don't want more than one trap record for a single instruction, + // so let's not allow the "mem" case (load-op merging) here; force + // divisor into a register instead. + let divisor = RegMem::reg(put_input_in_reg(ctx, inputs[1])); // Fill in the high parts: if kind.is_signed() { diff --git a/cranelift/codegen/src/machinst/buffer.rs b/cranelift/codegen/src/machinst/buffer.rs index 7d75dd46d7b6..7a75c75cf1bb 100644 --- a/cranelift/codegen/src/machinst/buffer.rs +++ b/cranelift/codegen/src/machinst/buffer.rs @@ -1314,26 +1314,25 @@ impl MachBufferFinalized { let mut next_trap = 0; let mut next_call_site = 0; for (idx, byte) in self.data.iter().enumerate() { - if next_reloc < self.relocs.len() { + while next_reloc < self.relocs.len() + && self.relocs[next_reloc].offset == idx as CodeOffset + { let reloc = &self.relocs[next_reloc]; - if reloc.offset == idx as CodeOffset { - sink.reloc_external(reloc.srcloc, reloc.kind, &reloc.name, reloc.addend); - next_reloc += 1; - } + sink.reloc_external(reloc.srcloc, reloc.kind, &reloc.name, reloc.addend); + next_reloc += 1; } - if next_trap < self.traps.len() { + while next_trap < self.traps.len() && self.traps[next_trap].offset == idx as CodeOffset + { let trap = &self.traps[next_trap]; - if trap.offset == idx as CodeOffset { - sink.trap(trap.code, trap.srcloc); - next_trap += 1; - } + sink.trap(trap.code, trap.srcloc); + next_trap += 1; } - if next_call_site < self.call_sites.len() { + while next_call_site < self.call_sites.len() + && self.call_sites[next_call_site].ret_addr == idx as CodeOffset + { let call_site = &self.call_sites[next_call_site]; - if call_site.ret_addr == idx as CodeOffset { - sink.add_call_site(call_site.opcode, call_site.srcloc); - next_call_site += 1; - } + sink.add_call_site(call_site.opcode, call_site.srcloc); + next_call_site += 1; } sink.put1(*byte); } @@ -1468,11 +1467,14 @@ impl MachBranch { #[cfg(all(test, feature = "arm64"))] mod test { use super::*; + use crate::ir::{ConstantOffset, Function, JumpTable, Value}; use crate::isa::aarch64::inst::xreg; use crate::isa::aarch64::inst::{BranchTarget, CondBrKind, EmitInfo, Inst}; + use crate::isa::TargetIsa; use crate::machinst::MachInstEmit; use crate::settings; use std::default::Default; + use std::vec::Vec; fn label(n: u32) -> MachLabel { MachLabel::from_block(n) @@ -1819,4 +1821,90 @@ mod test { assert_eq!(&golden_data[..], &buf.data[..]); } + + #[test] + fn metadata_records() { + let mut buf = MachBuffer::::new(); + + buf.reserve_labels_for_blocks(1); + + buf.bind_label(label(0)); + buf.put1(1); + buf.add_trap(SourceLoc::default(), TrapCode::HeapOutOfBounds); + buf.put1(2); + buf.add_trap(SourceLoc::default(), TrapCode::IntegerOverflow); + buf.add_trap(SourceLoc::default(), TrapCode::IntegerDivisionByZero); + buf.add_call_site(SourceLoc::default(), Opcode::Call); + buf.add_reloc( + SourceLoc::default(), + Reloc::Abs4, + &ExternalName::user(0, 0), + 0, + ); + buf.put1(3); + buf.add_reloc( + SourceLoc::default(), + Reloc::Abs8, + &ExternalName::user(1, 1), + 1, + ); + buf.put1(4); + + let buf = buf.finish(); + + #[derive(Default)] + struct TestCodeSink { + offset: CodeOffset, + traps: Vec<(CodeOffset, TrapCode)>, + callsites: Vec<(CodeOffset, Opcode)>, + relocs: Vec<(CodeOffset, Reloc)>, + } + impl CodeSink for TestCodeSink { + fn offset(&self) -> CodeOffset { + self.offset + } + fn put1(&mut self, _: u8) { + self.offset += 1; + } + fn put2(&mut self, _: u16) { + self.offset += 2; + } + fn put4(&mut self, _: u32) { + self.offset += 4; + } + fn put8(&mut self, _: u64) { + self.offset += 8; + } + fn reloc_external(&mut self, _: SourceLoc, r: Reloc, _: &ExternalName, _: Addend) { + self.relocs.push((self.offset, r)); + } + fn reloc_constant(&mut self, _: Reloc, _: ConstantOffset) {} + fn reloc_jt(&mut self, _: Reloc, _: JumpTable) {} + fn trap(&mut self, t: TrapCode, _: SourceLoc) { + self.traps.push((self.offset, t)); + } + fn begin_jumptables(&mut self) {} + fn begin_rodata(&mut self) {} + fn end_codegen(&mut self) {} + fn add_stack_map(&mut self, _: &[Value], _: &Function, _: &dyn TargetIsa) {} + fn add_call_site(&mut self, op: Opcode, _: SourceLoc) { + self.callsites.push((self.offset, op)); + } + } + + let mut sink = TestCodeSink::default(); + buf.emit(&mut sink); + + assert_eq!(sink.offset, 4); + assert_eq!( + sink.traps, + vec![ + (1, TrapCode::HeapOutOfBounds), + (2, TrapCode::IntegerOverflow), + (2, TrapCode::IntegerDivisionByZero) + ] + ); + assert_eq!(sink.callsites, vec![(2, Opcode::Call),]); + assert_eq!(sink.relocs, vec![(2, Reloc::Abs4), (3, Reloc::Abs8)]); + } }