From e416ba4e711c5ec086f3ef17b25b79c8a2fdae43 Mon Sep 17 00:00:00 2001 From: Mohanson Date: Wed, 20 Mar 2024 09:45:15 +0800 Subject: [PATCH] fix: When track pages receive more writes, treat them as dirty pages (#423) * chore: Use debug assertions to document get_page_indices input requirements * fix: When track pages receive more writes, treat them as dirty pages Credit goes to @mohanson for providing the tests Co-authored-by: Xuejie Xiao --- src/memory/mod.rs | 1 + src/snapshot2.rs | 9 ++++++--- tests/test_resume2.rs | 39 +++++++++++++++++++++++++++++++++++++-- 3 files changed, 44 insertions(+), 5 deletions(-) diff --git a/src/memory/mod.rs b/src/memory/mod.rs index 2108df19..aed0a131 100644 --- a/src/memory/mod.rs +++ b/src/memory/mod.rs @@ -102,6 +102,7 @@ pub fn fill_page_data( // `size` should be none zero u64 pub fn get_page_indices(addr: u64, size: u64) -> Result<(u64, u64), Error> { + debug_assert!(size > 0); let (addr_end, overflowed) = addr.overflowing_add(size); if overflowed { return Err(Error::MemOutOfBound); diff --git a/src/snapshot2.rs b/src/snapshot2.rs index 9999956c..1566fbc0 100644 --- a/src/snapshot2.rs +++ b/src/snapshot2.rs @@ -156,9 +156,6 @@ impl> Snapshot2Context { pub fn make_snapshot(&self, machine: &mut M) -> Result, Error> { let mut dirty_pages: Vec<(u64, u8, Vec)> = vec![]; for i in 0..machine.memory().memory_pages() as u64 { - if self.pages.contains_key(&i) { - continue; - } let flag = machine.memory_mut().fetch_flag(i)?; if flag & FLAG_DIRTY == 0 { continue; @@ -178,6 +175,12 @@ impl> Snapshot2Context { let mut pages: Vec = self.pages.keys().copied().collect(); pages.sort_unstable(); for page in pages { + // Some pages might be marked as cached pages from data source, but receives + // memory writes later(and marked as dirty). We are safely skipping those pages + // here as they will be gathered as dirty pages. + if machine.memory_mut().fetch_flag(page)? & FLAG_DIRTY != 0 { + continue; + } let address = page * PAGE_SIZE; let (id, offset, flag) = &self.pages[&page]; let mut appended_to_last = false; diff --git a/tests/test_resume2.rs b/tests/test_resume2.rs index 30f35b8a..f7caa952 100644 --- a/tests/test_resume2.rs +++ b/tests/test_resume2.rs @@ -11,7 +11,7 @@ use ckb_vm::machine::{ use ckb_vm::memory::{sparse::SparseMemory, wxorx::WXorXMemory}; use ckb_vm::registers::{A0, A1, A7}; use ckb_vm::snapshot2::{DataSource, Snapshot2, Snapshot2Context}; -use ckb_vm::{DefaultMachineBuilder, Error, Register, Syscalls, ISA_A, ISA_IMC}; +use ckb_vm::{DefaultMachineBuilder, Error, Memory, Register, Syscalls, ISA_A, ISA_IMC}; use std::collections::HashMap; use std::fs::File; use std::io::Read; @@ -531,7 +531,7 @@ impl Machine { #[cfg(not(feature = "enable-chaos-mode-by-default"))] fn full_memory(&mut self) -> Result { - use ckb_vm::{Memory, RISCV_MAX_MEMORY}; + use ckb_vm::RISCV_MAX_MEMORY; use Machine::*; match self { Asm(inner, _) => inner @@ -659,3 +659,38 @@ pub fn test_store_bytes_twice() { assert_eq!(mem1, mem2); } + +#[cfg(not(feature = "enable-chaos-mode-by-default"))] +#[test] +pub fn test_mixing_snapshot2_writes_with_machine_raw_writes() { + let data_source = load_program("tests/programs/sc_after_snapshot"); + + let mut machine = MachineTy::Asm.build(data_source.clone(), VERSION2); + machine.set_max_cycles(u64::MAX); + machine.load_program(&vec!["main".into()]).unwrap(); + + match machine { + Machine::Asm(ref mut inner, ref ctx) => { + ctx.lock() + .unwrap() + .store_bytes(&mut inner.machine, 0, &DATA_ID, 0, 29186) + .unwrap(); + inner + .machine + .memory_mut() + .store_bytes(0, &vec![0x42; 29186]) + .unwrap(); + } + _ => unimplemented!(), + } + + let mem1 = machine.full_memory().unwrap(); + + let snapshot = machine.snapshot().unwrap(); + let mut machine2 = MachineTy::Asm.build(data_source.clone(), VERSION2); + machine2.resume(snapshot).unwrap(); + machine2.set_max_cycles(u64::MAX); + let mem2 = machine2.full_memory().unwrap(); + + assert_eq!(mem1, mem2); +}