From 17b92136e180cbb894aed9bdafb21f05c3e469a3 Mon Sep 17 00:00:00 2001 From: Rune Tynan Date: Thu, 22 May 2025 11:59:14 -0700 Subject: [PATCH 01/28] Implement file cloning on Windows --- .../miri/src/shims/windows/foreign_items.rs | 22 ++++++ src/tools/miri/src/shims/windows/handle.rs | 74 +++++++++++++++++++ src/tools/miri/tests/pass/shims/fs.rs | 2 +- 3 files changed, 97 insertions(+), 1 deletion(-) diff --git a/src/tools/miri/src/shims/windows/foreign_items.rs b/src/tools/miri/src/shims/windows/foreign_items.rs index d822dd07fcd7b..98099e07b2eac 100644 --- a/src/tools/miri/src/shims/windows/foreign_items.rs +++ b/src/tools/miri/src/shims/windows/foreign_items.rs @@ -572,6 +572,14 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { let ret = this.WaitForSingleObject(handle, timeout)?; this.write_scalar(ret, dest)?; } + "GetCurrentProcess" => { + let [] = this.check_shim(abi, sys_conv, link_name, args)?; + + this.write_scalar( + Handle::Pseudo(PseudoHandle::CurrentProcess).to_scalar(this), + dest, + )?; + } "GetCurrentThread" => { let [] = this.check_shim(abi, sys_conv, link_name, args)?; @@ -693,6 +701,20 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { let res = this.GetStdHandle(which)?; this.write_scalar(res, dest)?; } + "DuplicateHandle" => { + let [src_proc, src_handle, target_proc, target_handle, access, inherit, options] = + this.check_shim(abi, sys_conv, link_name, args)?; + let res = this.DuplicateHandle( + src_proc, + src_handle, + target_proc, + target_handle, + access, + inherit, + options, + )?; + this.write_scalar(res, dest)?; + } "CloseHandle" => { let [handle] = this.check_shim(abi, sys_conv, link_name, args)?; diff --git a/src/tools/miri/src/shims/windows/handle.rs b/src/tools/miri/src/shims/windows/handle.rs index 5c04271fac590..1e30bf25ed92e 100644 --- a/src/tools/miri/src/shims/windows/handle.rs +++ b/src/tools/miri/src/shims/windows/handle.rs @@ -9,6 +9,7 @@ use crate::*; #[derive(Clone, Copy, Debug, PartialEq, Eq, Hash)] pub enum PseudoHandle { CurrentThread, + CurrentProcess, } /// Miri representation of a Windows `HANDLE` @@ -23,16 +24,19 @@ pub enum Handle { impl PseudoHandle { const CURRENT_THREAD_VALUE: u32 = 0; + const CURRENT_PROCESS_VALUE: u32 = 1; fn value(self) -> u32 { match self { Self::CurrentThread => Self::CURRENT_THREAD_VALUE, + Self::CurrentProcess => Self::CURRENT_PROCESS_VALUE, } } fn from_value(value: u32) -> Option { match value { Self::CURRENT_THREAD_VALUE => Some(Self::CurrentThread), + Self::CURRENT_PROCESS_VALUE => Some(Self::CurrentProcess), _ => None, } } @@ -244,6 +248,76 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { interp_ok(handle.to_scalar(this)) } + fn DuplicateHandle( + &mut self, + src_proc: &OpTy<'tcx>, // HANDLE + src_handle: &OpTy<'tcx>, // HANDLE + target_proc: &OpTy<'tcx>, // HANDLE + target_handle: &OpTy<'tcx>, // LPHANDLE + desired_access: &OpTy<'tcx>, // DWORD + inherit: &OpTy<'tcx>, // BOOL + options: &OpTy<'tcx>, // DWORD + ) -> InterpResult<'tcx, Scalar> { + // ^ Returns BOOL (i32 on Windows) + let this = self.eval_context_mut(); + + let src_proc = this.read_handle(src_proc, "DuplicateHandle")?; + let src_handle = this.read_handle(src_handle, "DuplicateHandle")?; + let target_proc = this.read_handle(target_proc, "DuplicateHandle")?; + let target_handle_ptr = this.read_pointer(target_handle)?; + // Since we only support DUPLICATE_SAME_ACCESS, this value is ignored, but should be valid + let _ = this.read_scalar(desired_access)?.to_u32()?; + // We don't support the CreateProcess API, so inheritable or not means nothing. + // If we ever add CreateProcess support, this will need to be implemented. + let _ = this.read_scalar(inherit)?; + let options = this.read_scalar(options)?; + + if src_proc != Handle::Pseudo(PseudoHandle::CurrentProcess) { + throw_unsup_format!( + "`DuplicateHandle` `hSourceProcessHandle` parameter is not the current process, which is unsupported" + ); + } + + if target_proc != Handle::Pseudo(PseudoHandle::CurrentProcess) { + throw_unsup_format!( + "`DuplicateHandle` `hSourceProcessHandle` parameter is not the current process, which is unsupported" + ); + } + + if this.ptr_is_null(target_handle_ptr)? { + throw_unsup_format!( + "`DuplicateHandle` `lpTargetHandle` parameter is null, which is unsupported" + ); + } + + if options != this.eval_windows("c", "DUPLICATE_SAME_ACCESS") { + throw_unsup_format!( + "`DuplicateHandle` `dwOptions` parameter is not `DUPLICATE_SAME_ACCESS`, which is unsupported" + ); + } + + let new_handle = match src_handle { + Handle::File(old_fd_num) => { + let Some(fd) = this.machine.fds.get(old_fd_num) else { + this.invalid_handle("DuplicateHandle")? + }; + Handle::File(this.machine.fds.insert(fd)) + } + Handle::Thread(_) => { + throw_unsup_format!( + "`DuplicateHandle` called on a thread handle, which is unsupported" + ); + } + Handle::Pseudo(pseudo) => Handle::Pseudo(pseudo), + Handle::Null | Handle::Invalid => this.invalid_handle("DuplicateHandle")?, + }; + + let target_place = this.deref_pointer_as(target_handle, this.machine.layouts.usize)?; + this.write_scalar(new_handle.to_scalar(this), &target_place)?; + + interp_ok(this.eval_windows("c", "TRUE")) + } + fn CloseHandle(&mut self, handle_op: &OpTy<'tcx>) -> InterpResult<'tcx, Scalar> { let this = self.eval_context_mut(); diff --git a/src/tools/miri/tests/pass/shims/fs.rs b/src/tools/miri/tests/pass/shims/fs.rs index 315637ff7ec73..87df43ca7e57f 100644 --- a/src/tools/miri/tests/pass/shims/fs.rs +++ b/src/tools/miri/tests/pass/shims/fs.rs @@ -23,9 +23,9 @@ fn main() { test_seek(); test_errors(); test_from_raw_os_error(); + test_file_clone(); // Windows file handling is very incomplete. if cfg!(not(windows)) { - test_file_clone(); test_file_set_len(); test_file_sync(); test_rename(); From 5f3242c0848bc7e7b02a6fb56cf8ac2f6633c335 Mon Sep 17 00:00:00 2001 From: The Miri Cronjob Bot Date: Fri, 23 May 2025 04:53:31 +0000 Subject: [PATCH 02/28] Preparing for merge from rustc --- src/tools/miri/rust-version | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/tools/miri/rust-version b/src/tools/miri/rust-version index 4698969530292..88a53545d51aa 100644 --- a/src/tools/miri/rust-version +++ b/src/tools/miri/rust-version @@ -1 +1 @@ -2b96ddca1272960623e41829439df8dae82d20af +e7f4317ea0e891296163414c6f681ccec976abc3 From 5d5fb18e54fe875d247f41c81a3932273adeabc0 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Fri, 23 May 2025 08:57:11 +0200 Subject: [PATCH 03/28] ./miri bench: sort results alphabetically --- src/tools/miri/miri-script/src/commands.rs | 26 +++++++++++++--------- 1 file changed, 15 insertions(+), 11 deletions(-) diff --git a/src/tools/miri/miri-script/src/commands.rs b/src/tools/miri/miri-script/src/commands.rs index 3b7b159aeab76..1781ca119eeec 100644 --- a/src/tools/miri/miri-script/src/commands.rs +++ b/src/tools/miri/miri-script/src/commands.rs @@ -1,4 +1,4 @@ -use std::collections::HashMap; +use std::collections::BTreeMap; use std::ffi::{OsStr, OsString}; use std::fmt::Write as _; use std::fs::{self, File}; @@ -489,7 +489,9 @@ impl Command { sh.read_dir(benches_dir)? .into_iter() .filter(|path| path.is_dir()) - .map(|path| path.into_os_string().into_string().unwrap()) + // Only keep the basename: that matches the usage with a manual bench list, + // and it ensure the path concatenations below work as intended. + .map(|path| path.file_name().unwrap().to_owned().into_string().unwrap()) .collect() } else { benches.into_iter().collect() @@ -530,14 +532,16 @@ impl Command { stddev: f64, } - let gather_results = || -> Result> { + let gather_results = || -> Result> { let baseline_temp_dir = results_json_dir.unwrap(); - let mut results = HashMap::new(); + let mut results = BTreeMap::new(); for bench in &benches { - let result = File::open(path!(baseline_temp_dir / format!("{bench}.bench.json")))?; - let mut result: serde_json::Value = - serde_json::from_reader(BufReader::new(result))?; - let result: BenchResult = serde_json::from_value(result["results"][0].take())?; + let result = File::open(path!(baseline_temp_dir / format!("{bench}.bench.json"))) + .context("failed to read hyperfine JSON")?; + let mut result: serde_json::Value = serde_json::from_reader(BufReader::new(result)) + .context("failed to parse hyperfine JSON")?; + let result: BenchResult = serde_json::from_value(result["results"][0].take()) + .context("failed to interpret hyperfine JSON")?; results.insert(bench as &str, result); } Ok(results) @@ -549,15 +553,15 @@ impl Command { serde_json::to_writer_pretty(BufWriter::new(baseline), &results)?; } else if let Some(baseline_file) = load_baseline { let new_results = gather_results()?; - let baseline_results: HashMap = { + let baseline_results: BTreeMap = { let f = File::open(baseline_file)?; serde_json::from_reader(BufReader::new(f))? }; println!( "Comparison with baseline (relative speed, lower is better for the new results):" ); - for (bench, new_result) in new_results.iter() { - let Some(baseline_result) = baseline_results.get(*bench) else { continue }; + for (bench, new_result) in new_results { + let Some(baseline_result) = baseline_results.get(bench) else { continue }; // Compare results (inspired by hyperfine) let ratio = new_result.mean / baseline_result.mean; From 24d950398cf0181c052d969f9496f967fb186800 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Fri, 23 May 2025 11:38:42 +0200 Subject: [PATCH 04/28] many-seeds tests have become a lot faster thanks to multithreading --- src/tools/miri/ci/ci.sh | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-) diff --git a/src/tools/miri/ci/ci.sh b/src/tools/miri/ci/ci.sh index 9ae15739dcbe1..ad5524f175010 100755 --- a/src/tools/miri/ci/ci.sh +++ b/src/tools/miri/ci/ci.sh @@ -142,12 +142,10 @@ case $HOST_TARGET in # Host GC_STRESS=1 MIR_OPT=1 MANY_SEEDS=64 TEST_BENCH=1 CARGO_MIRI_ENV=1 run_tests # Extra tier 1 - # With reduced many-seed count to avoid spending too much time on that. - # (All OSes and ABIs are run with 64 seeds at least once though via the macOS runner.) - MANY_SEEDS=16 TEST_TARGET=i686-unknown-linux-gnu run_tests - MANY_SEEDS=16 TEST_TARGET=aarch64-unknown-linux-gnu run_tests - MANY_SEEDS=16 TEST_TARGET=x86_64-apple-darwin run_tests - MANY_SEEDS=16 TEST_TARGET=x86_64-pc-windows-gnu run_tests + MANY_SEEDS=64 TEST_TARGET=i686-unknown-linux-gnu run_tests + MANY_SEEDS=64 TEST_TARGET=aarch64-unknown-linux-gnu run_tests + MANY_SEEDS=64 TEST_TARGET=x86_64-apple-darwin run_tests + MANY_SEEDS=64 TEST_TARGET=x86_64-pc-windows-gnu run_tests ;; aarch64-apple-darwin) # Host @@ -178,7 +176,7 @@ case $HOST_TARGET in # Host # Without GC_STRESS and with reduced many-seeds count as this is the slowest runner. # (The macOS runner checks windows-msvc with full many-seeds count.) - MIR_OPT=1 MANY_SEEDS=16 TEST_BENCH=1 run_tests + MIR_OPT=1 MANY_SEEDS=64 TEST_BENCH=1 run_tests # Extra tier 1 # We really want to ensure a Linux target works on a Windows host, # and a 64bit target works on a 32bit host. From 20236c6094884ea97d9f14dcbe862a41ff4fab6b Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Fri, 23 May 2025 11:41:28 +0200 Subject: [PATCH 05/28] test some aarch64 windows targets --- src/tools/miri/ci/ci.sh | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/tools/miri/ci/ci.sh b/src/tools/miri/ci/ci.sh index ad5524f175010..8941af681a478 100755 --- a/src/tools/miri/ci/ci.sh +++ b/src/tools/miri/ci/ci.sh @@ -146,6 +146,8 @@ case $HOST_TARGET in MANY_SEEDS=64 TEST_TARGET=aarch64-unknown-linux-gnu run_tests MANY_SEEDS=64 TEST_TARGET=x86_64-apple-darwin run_tests MANY_SEEDS=64 TEST_TARGET=x86_64-pc-windows-gnu run_tests + # Extra tier 1 candidate + MANY_SEEDS=64 TEST_TARGET=aarch64-pc-windows-msvc run_tests ;; aarch64-apple-darwin) # Host @@ -154,7 +156,8 @@ case $HOST_TARGET in MANY_SEEDS=64 TEST_TARGET=i686-pc-windows-gnu run_tests MANY_SEEDS=64 TEST_TARGET=x86_64-pc-windows-msvc CARGO_MIRI_ENV=1 run_tests # Extra tier 2 - MANY_SEEDS=16 TEST_TARGET=arm-unknown-linux-gnueabi run_tests + MANY_SEEDS=16 TEST_TARGET=arm-unknown-linux-gnueabi run_tests # 32bit ARM + MANY_SEEDS=16 TEST_TARGET=aarch64-pc-windows-gnullvm run_tests # gnullvm ABI MANY_SEEDS=16 TEST_TARGET=s390x-unknown-linux-gnu run_tests # big-endian architecture of choice # Not officially supported tier 2 MANY_SEEDS=16 TEST_TARGET=mips-unknown-linux-gnu run_tests # a 32bit big-endian target, and also a target without 64bit atomics From 4bbcb47239d65893ca6ff6aa8da6266ee5396719 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Sat, 24 May 2025 20:19:30 +0200 Subject: [PATCH 06/28] fix zulip topic URL --- src/tools/miri/tests/many-seeds/reentrant-lock.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/tools/miri/tests/many-seeds/reentrant-lock.rs b/src/tools/miri/tests/many-seeds/reentrant-lock.rs index 8a363179a9cc7..4c2dc463f4858 100644 --- a/src/tools/miri/tests/many-seeds/reentrant-lock.rs +++ b/src/tools/miri/tests/many-seeds/reentrant-lock.rs @@ -1,6 +1,6 @@ #![feature(reentrant_lock)] //! This is a regression test for -//! . +//! . use std::cell::Cell; use std::sync::ReentrantLock; From 5c65c35d2626db646faa510481edd4ac59f1bafe Mon Sep 17 00:00:00 2001 From: The Miri Cronjob Bot Date: Sun, 25 May 2025 04:53:55 +0000 Subject: [PATCH 07/28] Preparing for merge from rustc --- src/tools/miri/rust-version | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/tools/miri/rust-version b/src/tools/miri/rust-version index 88a53545d51aa..306fd0d27e8ff 100644 --- a/src/tools/miri/rust-version +++ b/src/tools/miri/rust-version @@ -1 +1 @@ -e7f4317ea0e891296163414c6f681ccec976abc3 +3d86494a0d0131c32eb15e3a4b685707b9ff000d From f25ec486d7d6cad76ca38be4ac8f17452f795fcb Mon Sep 17 00:00:00 2001 From: The Miri Cronjob Bot Date: Sun, 25 May 2025 05:02:30 +0000 Subject: [PATCH 08/28] fmt --- src/tools/miri/src/bin/miri.rs | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/tools/miri/src/bin/miri.rs b/src/tools/miri/src/bin/miri.rs index 7098ef5130dce..0121472d330f5 100644 --- a/src/tools/miri/src/bin/miri.rs +++ b/src/tools/miri/src/bin/miri.rs @@ -281,7 +281,9 @@ impl rustc_driver::Callbacks for MiriBeRustCompilerCalls { } let codegen_fn_attrs = tcx.codegen_fn_attrs(local_def_id); if codegen_fn_attrs.contains_extern_indicator() - || codegen_fn_attrs.flags.contains(CodegenFnAttrFlags::USED_COMPILER) + || codegen_fn_attrs + .flags + .contains(CodegenFnAttrFlags::USED_COMPILER) || codegen_fn_attrs.flags.contains(CodegenFnAttrFlags::USED_LINKER) { Some(( From aa94ac1252a6479e3c4c1570f23e546157eea7c3 Mon Sep 17 00:00:00 2001 From: The Miri Cronjob Bot Date: Tue, 27 May 2025 04:54:50 +0000 Subject: [PATCH 09/28] Preparing for merge from rustc --- src/tools/miri/rust-version | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/tools/miri/rust-version b/src/tools/miri/rust-version index 306fd0d27e8ff..9c82eb416d182 100644 --- a/src/tools/miri/rust-version +++ b/src/tools/miri/rust-version @@ -1 +1 @@ -3d86494a0d0131c32eb15e3a4b685707b9ff000d +d76fe154029e03aeb64af721beafdcef856d576a From bf0aab9b2e5d8ebd33517668e121c8caf554acb0 Mon Sep 17 00:00:00 2001 From: The Miri Cronjob Bot Date: Tue, 27 May 2025 05:03:22 +0000 Subject: [PATCH 10/28] fmt --- src/tools/miri/src/concurrency/thread.rs | 6 +++++- src/tools/miri/src/machine.rs | 4 +++- 2 files changed, 8 insertions(+), 2 deletions(-) diff --git a/src/tools/miri/src/concurrency/thread.rs b/src/tools/miri/src/concurrency/thread.rs index 15f15572c93d2..5014bbeedb0b0 100644 --- a/src/tools/miri/src/concurrency/thread.rs +++ b/src/tools/miri/src/concurrency/thread.rs @@ -902,7 +902,11 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { let mut alloc = alloc.inner().adjust_from_tcx( &this.tcx, |bytes, align| { - interp_ok(MiriAllocBytes::from_bytes(std::borrow::Cow::Borrowed(bytes), align, ())) + interp_ok(MiriAllocBytes::from_bytes( + std::borrow::Cow::Borrowed(bytes), + align, + (), + )) }, |ptr| this.global_root_pointer(ptr), )?; diff --git a/src/tools/miri/src/machine.rs b/src/tools/miri/src/machine.rs index 5d5c19a24fa3d..f1d3e949eaed8 100644 --- a/src/tools/miri/src/machine.rs +++ b/src/tools/miri/src/machine.rs @@ -1805,7 +1805,9 @@ impl<'tcx> Machine<'tcx> for MiriMachine<'tcx> { } /// Placeholder! - fn get_default_alloc_params(&self) -> ::AllocParams { () } + fn get_default_alloc_params(&self) -> ::AllocParams { + () + } } /// Trait for callbacks handling asynchronous machine operations. From 86d03991678f391eba7b6ac489a44da178c8f8d4 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Tue, 27 May 2025 08:18:25 +0200 Subject: [PATCH 11/28] pacify clippy --- src/tools/miri/src/machine.rs | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/src/tools/miri/src/machine.rs b/src/tools/miri/src/machine.rs index f1d3e949eaed8..0c5127d802f84 100644 --- a/src/tools/miri/src/machine.rs +++ b/src/tools/miri/src/machine.rs @@ -1805,9 +1805,7 @@ impl<'tcx> Machine<'tcx> for MiriMachine<'tcx> { } /// Placeholder! - fn get_default_alloc_params(&self) -> ::AllocParams { - () - } + fn get_default_alloc_params(&self) -> ::AllocParams {} } /// Trait for callbacks handling asynchronous machine operations. From 9aad0094952475087d8827bae1badb11409605a5 Mon Sep 17 00:00:00 2001 From: tiif Date: Fri, 21 Feb 2025 00:02:39 +0800 Subject: [PATCH 12/28] Support F_GETFL and F_SETFL for fcntl --- src/tools/miri/src/shims/files.rs | 14 +++ src/tools/miri/src/shims/unix/fd.rs | 21 ++++ .../miri/src/shims/unix/unnamed_socket.rs | 96 +++++++++++++++++-- .../libc/fcntl_fsetfl_while_blocking.rs | 20 ++++ .../libc/fcntl_fsetfl_while_blocking.stderr | 19 ++++ .../miri/tests/pass-dep/libc/libc-pipe.rs | 67 +++++++++++++ .../tests/pass-dep/libc/libc-socketpair.rs | 33 +++++++ 7 files changed, 262 insertions(+), 8 deletions(-) create mode 100644 src/tools/miri/tests/fail-dep/libc/fcntl_fsetfl_while_blocking.rs create mode 100644 src/tools/miri/tests/fail-dep/libc/fcntl_fsetfl_while_blocking.stderr diff --git a/src/tools/miri/src/shims/files.rs b/src/tools/miri/src/shims/files.rs index 42603e784bbd7..5347b0b09c020 100644 --- a/src/tools/miri/src/shims/files.rs +++ b/src/tools/miri/src/shims/files.rs @@ -196,6 +196,20 @@ pub trait FileDescription: std::fmt::Debug + FileDescriptionExt { fn as_unix<'tcx>(&self, _ecx: &MiriInterpCx<'tcx>) -> &dyn UnixFileDescription { panic!("Not a unix file descriptor: {}", self.name()); } + + /// Implementation of fcntl(F_GETFL) for this FD. + fn get_flags<'tcx>(&self, _ecx: &mut MiriInterpCx<'tcx>) -> InterpResult<'tcx, Scalar> { + throw_unsup_format!("fcntl: {} is not supported for F_GETFL", self.name()); + } + + /// Implementation of fcntl(F_SETFL) for this FD. + fn set_flags<'tcx>( + &self, + _flag: i32, + _ecx: &mut MiriInterpCx<'tcx>, + ) -> InterpResult<'tcx, Scalar> { + throw_unsup_format!("fcntl: {} is not supported for F_SETFL", self.name()); + } } impl FileDescription for io::Stdin { diff --git a/src/tools/miri/src/shims/unix/fd.rs b/src/tools/miri/src/shims/unix/fd.rs index 156814a26fa7a..71102d9f2f33f 100644 --- a/src/tools/miri/src/shims/unix/fd.rs +++ b/src/tools/miri/src/shims/unix/fd.rs @@ -141,6 +141,8 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { let f_getfd = this.eval_libc_i32("F_GETFD"); let f_dupfd = this.eval_libc_i32("F_DUPFD"); let f_dupfd_cloexec = this.eval_libc_i32("F_DUPFD_CLOEXEC"); + let f_getfl = this.eval_libc_i32("F_GETFL"); + let f_setfl = this.eval_libc_i32("F_SETFL"); // We only support getting the flags for a descriptor. match cmd { @@ -175,6 +177,25 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { this.set_last_error_and_return_i32(LibcError("EBADF")) } } + cmd if cmd == f_getfl => { + // Check if this is a valid open file descriptor. + let Some(fd) = this.machine.fds.get(fd_num) else { + return this.set_last_error_and_return_i32(LibcError("EBADF")); + }; + + fd.get_flags(this) + } + cmd if cmd == f_setfl => { + // Check if this is a valid open file descriptor. + let Some(fd) = this.machine.fds.get(fd_num) else { + return this.set_last_error_and_return_i32(LibcError("EBADF")); + }; + + let [flag] = check_min_vararg_count("fcntl(fd, F_SETFL, ...)", varargs)?; + let flag = this.read_scalar(flag)?.to_i32()?; + + fd.set_flags(flag, this) + } cmd if this.tcx.sess.target.os == "macos" && cmd == this.eval_libc_i32("F_FULLFSYNC") => { diff --git a/src/tools/miri/src/shims/unix/unnamed_socket.rs b/src/tools/miri/src/shims/unix/unnamed_socket.rs index 135d8f6bee7e1..817ddd7954df4 100644 --- a/src/tools/miri/src/shims/unix/unnamed_socket.rs +++ b/src/tools/miri/src/shims/unix/unnamed_socket.rs @@ -20,6 +20,16 @@ use crate::*; /// be configured in the real system. const MAX_SOCKETPAIR_BUFFER_CAPACITY: usize = 212992; +#[derive(Debug, PartialEq)] +enum AnonSocketType { + // Either end of the socketpair fd. + Socketpair, + // Read end of the pipe. + PipeRead, + // Write end of the pipe. + PipeWrite, +} + /// One end of a pair of connected unnamed sockets. #[derive(Debug)] struct AnonSocket { @@ -40,7 +50,10 @@ struct AnonSocket { /// A list of thread ids blocked because the buffer was full. /// Once another thread reads some bytes, these threads will be unblocked. blocked_write_tid: RefCell>, - is_nonblock: bool, + /// Whether this fd is non-blocking or not. + is_nonblock: Cell, + // Differentiate between different AnonSocket fd types. + fd_type: AnonSocketType, } #[derive(Debug)] @@ -63,7 +76,10 @@ impl AnonSocket { impl FileDescription for AnonSocket { fn name(&self) -> &'static str { - "socketpair" + match self.fd_type { + AnonSocketType::Socketpair => "socketpair", + AnonSocketType::PipeRead | AnonSocketType::PipeWrite => "pipe", + } } fn close<'tcx>( @@ -110,6 +126,66 @@ impl FileDescription for AnonSocket { fn as_unix<'tcx>(&self, _ecx: &MiriInterpCx<'tcx>) -> &dyn UnixFileDescription { self } + + fn get_flags<'tcx>(&self, ecx: &mut MiriInterpCx<'tcx>) -> InterpResult<'tcx, Scalar> { + let mut flags = 0; + + // Get flag for file access mode. + // The flag for both socketpair and pipe will remain the same even when the peer + // fd is closed, so we need to look at the original type of this socket, not at whether + // the peer socket still exists. + match self.fd_type { + AnonSocketType::Socketpair => { + flags |= ecx.eval_libc_i32("O_RDWR"); + } + AnonSocketType::PipeRead => { + flags |= ecx.eval_libc_i32("O_RDONLY"); + } + AnonSocketType::PipeWrite => { + flags |= ecx.eval_libc_i32("O_WRONLY"); + } + } + + // Get flag for blocking status. + if self.is_nonblock.get() { + flags |= ecx.eval_libc_i32("O_NONBLOCK"); + } + + interp_ok(Scalar::from_i32(flags)) + } + + fn set_flags<'tcx>( + &self, + mut flag: i32, + ecx: &mut MiriInterpCx<'tcx>, + ) -> InterpResult<'tcx, Scalar> { + // FIXME: File creation flags should be ignored. + + let o_nonblock = ecx.eval_libc_i32("O_NONBLOCK"); + let o_rdonly = ecx.eval_libc_i32("O_RDONLY"); + let o_wronly = ecx.eval_libc_i32("O_WRONLY"); + let o_rdwr = ecx.eval_libc_i32("O_RDWR"); + + // O_NONBLOCK flag can be set / unset by user. + if flag & o_nonblock == o_nonblock { + self.is_nonblock.set(true); + flag &= !o_nonblock; + } else { + self.is_nonblock.set(false); + } + + // Ignore all file access mode flags. + flag &= !(o_rdonly | o_wronly | o_rdwr); + + // Throw error if there is any unsupported flag. + if flag != 0 { + throw_unsup_format!( + "fcntl: only O_NONBLOCK is supported for F_SETFL on socketpairs and pipes" + ) + } + + interp_ok(Scalar::from_i32(0)) + } } /// Write to AnonSocket based on the space available and return the written byte size. @@ -141,7 +217,7 @@ fn anonsocket_write<'tcx>( // Let's see if we can write. let available_space = MAX_SOCKETPAIR_BUFFER_CAPACITY.strict_sub(writebuf.borrow().buf.len()); if available_space == 0 { - if self_ref.is_nonblock { + if self_ref.is_nonblock.get() { // Non-blocking socketpair with a full buffer. return finish.call(ecx, Err(ErrorKind::WouldBlock.into())); } else { @@ -223,7 +299,7 @@ fn anonsocket_read<'tcx>( // Socketpair with no peer and empty buffer. // 0 bytes successfully read indicates end-of-file. return finish.call(ecx, Ok(0)); - } else if self_ref.is_nonblock { + } else if self_ref.is_nonblock.get() { // Non-blocking socketpair with writer and empty buffer. // https://linux.die.net/man/2/read // EAGAIN or EWOULDBLOCK can be returned for socket, @@ -407,7 +483,8 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { peer_lost_data: Cell::new(false), blocked_read_tid: RefCell::new(Vec::new()), blocked_write_tid: RefCell::new(Vec::new()), - is_nonblock: is_sock_nonblock, + is_nonblock: Cell::new(is_sock_nonblock), + fd_type: AnonSocketType::Socketpair, }); let fd1 = fds.new_ref(AnonSocket { readbuf: Some(RefCell::new(Buffer::new())), @@ -415,7 +492,8 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { peer_lost_data: Cell::new(false), blocked_read_tid: RefCell::new(Vec::new()), blocked_write_tid: RefCell::new(Vec::new()), - is_nonblock: is_sock_nonblock, + is_nonblock: Cell::new(is_sock_nonblock), + fd_type: AnonSocketType::Socketpair, }); // Make the file descriptions point to each other. @@ -475,7 +553,8 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { peer_lost_data: Cell::new(false), blocked_read_tid: RefCell::new(Vec::new()), blocked_write_tid: RefCell::new(Vec::new()), - is_nonblock, + is_nonblock: Cell::new(is_nonblock), + fd_type: AnonSocketType::PipeRead, }); let fd1 = fds.new_ref(AnonSocket { readbuf: None, @@ -483,7 +562,8 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { peer_lost_data: Cell::new(false), blocked_read_tid: RefCell::new(Vec::new()), blocked_write_tid: RefCell::new(Vec::new()), - is_nonblock, + is_nonblock: Cell::new(is_nonblock), + fd_type: AnonSocketType::PipeWrite, }); // Make the file descriptions point to each other. diff --git a/src/tools/miri/tests/fail-dep/libc/fcntl_fsetfl_while_blocking.rs b/src/tools/miri/tests/fail-dep/libc/fcntl_fsetfl_while_blocking.rs new file mode 100644 index 0000000000000..eef32136a0a48 --- /dev/null +++ b/src/tools/miri/tests/fail-dep/libc/fcntl_fsetfl_while_blocking.rs @@ -0,0 +1,20 @@ +//@ignore-target: windows # Sockets/pipes are not implemented yet +//~^ ERROR: deadlock: the evaluated program deadlocked +//@compile-flags: -Zmiri-deterministic-concurrency +use std::thread; + +/// If an O_NONBLOCK flag is set while the fd is blocking, that fd will not be woken up. +fn main() { + let mut fds = [-1, -1]; + let res = unsafe { libc::pipe(fds.as_mut_ptr()) }; + assert_eq!(res, 0); + let mut buf: [u8; 5] = [0; 5]; + let _thread1 = thread::spawn(move || { + // Add O_NONBLOCK flag while pipe is still block on read. + let res = unsafe { libc::fcntl(fds[0], libc::F_SETFL, libc::O_NONBLOCK) }; + assert_eq!(res, 0); + }); + // Main thread will block on read. + let _res = unsafe { libc::read(fds[0], buf.as_mut_ptr().cast(), buf.len() as libc::size_t) }; + //~^ ERROR: deadlock: the evaluated program deadlocked +} diff --git a/src/tools/miri/tests/fail-dep/libc/fcntl_fsetfl_while_blocking.stderr b/src/tools/miri/tests/fail-dep/libc/fcntl_fsetfl_while_blocking.stderr new file mode 100644 index 0000000000000..9ca5598abae64 --- /dev/null +++ b/src/tools/miri/tests/fail-dep/libc/fcntl_fsetfl_while_blocking.stderr @@ -0,0 +1,19 @@ +error: deadlock: the evaluated program deadlocked + | + = note: the evaluated program deadlocked + = note: (no span available) + = note: BACKTRACE on thread `unnamed-ID`: + +error: deadlock: the evaluated program deadlocked + --> tests/fail-dep/libc/fcntl_fsetfl_while_blocking.rs:LL:CC + | +LL | let _res = unsafe { libc::read(fds[0], buf.as_mut_ptr().cast(), buf.len() as libc::size_t) }; + | ^ the evaluated program deadlocked + | + = note: BACKTRACE: + = note: inside `main` at tests/fail-dep/libc/fcntl_fsetfl_while_blocking.rs:LL:CC + +note: some details are omitted, run with `MIRIFLAGS=-Zmiri-backtrace=full` for a verbose backtrace + +error: aborting due to 2 previous errors + diff --git a/src/tools/miri/tests/pass-dep/libc/libc-pipe.rs b/src/tools/miri/tests/pass-dep/libc/libc-pipe.rs index 05f6c870c3d77..bc755af864c5e 100644 --- a/src/tools/miri/tests/pass-dep/libc/libc-pipe.rs +++ b/src/tools/miri/tests/pass-dep/libc/libc-pipe.rs @@ -15,6 +15,8 @@ fn main() { ))] // `pipe2` only exists in some specific os. test_pipe2(); + test_pipe_setfl_getfl(); + test_pipe_fcntl_threaded(); } fn test_pipe() { @@ -127,3 +129,68 @@ fn test_pipe2() { let res = unsafe { libc::pipe2(fds.as_mut_ptr(), libc::O_NONBLOCK) }; assert_eq!(res, 0); } + +/// Basic test for pipe fcntl's F_SETFL and F_GETFL flag. +fn test_pipe_setfl_getfl() { + // Initialise pipe fds. + let mut fds = [-1, -1]; + let res = unsafe { libc::pipe(fds.as_mut_ptr()) }; + assert_eq!(res, 0); + + // Both sides should either have O_RONLY or O_WRONLY. + let res = unsafe { libc::fcntl(fds[0], libc::F_GETFL) }; + assert_eq!(res, libc::O_RDONLY); + let res = unsafe { libc::fcntl(fds[1], libc::F_GETFL) }; + assert_eq!(res, libc::O_WRONLY); + + // Add the O_NONBLOCK flag with F_SETFL. + let res = unsafe { libc::fcntl(fds[0], libc::F_SETFL, libc::O_NONBLOCK) }; + assert_eq!(res, 0); + + // Test if the O_NONBLOCK flag is successfully added. + let res = unsafe { libc::fcntl(fds[0], libc::F_GETFL) }; + assert_eq!(res, libc::O_RDONLY | libc::O_NONBLOCK); + + // The other side remains unchanged. + let res = unsafe { libc::fcntl(fds[1], libc::F_GETFL) }; + assert_eq!(res, libc::O_WRONLY); + + // Test if O_NONBLOCK flag can be unset. + let res = unsafe { libc::fcntl(fds[0], libc::F_SETFL, 0) }; + assert_eq!(res, 0); + let res = unsafe { libc::fcntl(fds[0], libc::F_GETFL) }; + assert_eq!(res, libc::O_RDONLY); +} + +/// Test the behaviour of F_SETFL/F_GETFL when a fd is blocking. +/// The expected execution is: +/// 1. Main thread blocks on fds[0] `read`. +/// 2. Thread 1 sets O_NONBLOCK flag on fds[0], +/// checks the value of F_GETFL, +/// then writes to fds[1] to unblock main thread's `read`. +fn test_pipe_fcntl_threaded() { + let mut fds = [-1, -1]; + let res = unsafe { libc::pipe(fds.as_mut_ptr()) }; + assert_eq!(res, 0); + let mut buf: [u8; 5] = [0; 5]; + let thread1 = thread::spawn(move || { + // Add O_NONBLOCK flag while pipe is still blocked on read. + let res = unsafe { libc::fcntl(fds[0], libc::F_SETFL, libc::O_NONBLOCK) }; + assert_eq!(res, 0); + + // Check the new flag value while the main thread is still blocked on fds[0]. + let res = unsafe { libc::fcntl(fds[0], libc::F_GETFL) }; + assert_eq!(res, libc::O_NONBLOCK); + + // The write below will unblock the `read` in main thread: even though + // the socket is now "non-blocking", the shim needs to deal correctly + // with threads that were blocked before the socket was made non-blocking. + let data = "abcde".as_bytes().as_ptr(); + let res = unsafe { libc::write(fds[1], data as *const libc::c_void, 5) }; + assert_eq!(res, 5); + }); + // The `read` below will block. + let res = unsafe { libc::read(fds[0], buf.as_mut_ptr().cast(), buf.len() as libc::size_t) }; + thread1.join().unwrap(); + assert_eq!(res, 5); +} diff --git a/src/tools/miri/tests/pass-dep/libc/libc-socketpair.rs b/src/tools/miri/tests/pass-dep/libc/libc-socketpair.rs index 9e48410f7045e..c36f6b112244e 100644 --- a/src/tools/miri/tests/pass-dep/libc/libc-socketpair.rs +++ b/src/tools/miri/tests/pass-dep/libc/libc-socketpair.rs @@ -12,6 +12,7 @@ fn main() { test_race(); test_blocking_read(); test_blocking_write(); + test_socketpair_setfl_getfl(); } fn test_socketpair() { @@ -182,3 +183,35 @@ fn test_blocking_write() { thread1.join().unwrap(); thread2.join().unwrap(); } + +/// Basic test for socketpair fcntl's F_SETFL and F_GETFL flag. +fn test_socketpair_setfl_getfl() { + // Initialise socketpair fds. + let mut fds = [-1, -1]; + let res = unsafe { libc::socketpair(libc::AF_UNIX, libc::SOCK_STREAM, 0, fds.as_mut_ptr()) }; + assert_eq!(res, 0); + + // Test if both sides have O_RDWR. + let res = unsafe { libc::fcntl(fds[0], libc::F_GETFL) }; + assert_eq!(res, libc::O_RDWR); + let res = unsafe { libc::fcntl(fds[1], libc::F_GETFL) }; + assert_eq!(res, libc::O_RDWR); + + // Add the O_NONBLOCK flag with F_SETFL. + let res = unsafe { libc::fcntl(fds[0], libc::F_SETFL, libc::O_NONBLOCK) }; + assert_eq!(res, 0); + + // Test if the O_NONBLOCK flag is successfully added. + let res = unsafe { libc::fcntl(fds[0], libc::F_GETFL) }; + assert_eq!(res, libc::O_RDWR | libc::O_NONBLOCK); + + // The other side remains unchanged. + let res = unsafe { libc::fcntl(fds[1], libc::F_GETFL) }; + assert_eq!(res, libc::O_RDWR); + + // Test if O_NONBLOCK flag can be unset. + let res = unsafe { libc::fcntl(fds[0], libc::F_SETFL, 0) }; + assert_eq!(res, 0); + let res = unsafe { libc::fcntl(fds[0], libc::F_GETFL) }; + assert_eq!(res, libc::O_RDWR); +} From d7c62a037c4fd4ed087247c5b1c8780d4847c3ca Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Tue, 27 May 2025 12:29:41 +0200 Subject: [PATCH 13/28] attempt to fix squash on Windows --- src/tools/miri/miri-script/src/commands.rs | 23 +++++++++++++++++++++- 1 file changed, 22 insertions(+), 1 deletion(-) diff --git a/src/tools/miri/miri-script/src/commands.rs b/src/tools/miri/miri-script/src/commands.rs index 1781ca119eeec..86362145d4759 100644 --- a/src/tools/miri/miri-script/src/commands.rs +++ b/src/tools/miri/miri-script/src/commands.rs @@ -404,7 +404,28 @@ impl Command { // We want to forward the host stdin so apparently we cannot use `cmd!`. let mut cmd = process::Command::new("git"); cmd.arg("rebase").arg(&base).arg("--interactive"); - cmd.env("GIT_SEQUENCE_EDITOR", env::current_exe()?); + let current_exe = { + if cfg!(windows) { + // Apparently git-for-Windows gets confused by backslashes if we just use + // `current_exe()` here. So replace them by forward slashes if this is not a "magic" + // path starting with "\\". This is clearly a git bug but we work around it here. + // Also see . + let bin = env::current_exe()?; + match bin.into_os_string().into_string() { + Err(not_utf8) => not_utf8.into(), // :shrug: + Ok(str) => { + if str.starts_with(r"\\") { + str.into() // don't touch these magic paths, they must use backslashes + } else { + str.replace('\\', "/").into() + } + } + } + } else { + env::current_exe()? + } + }; + cmd.env("GIT_SEQUENCE_EDITOR", current_exe); cmd.env("MIRI_SCRIPT_IS_GIT_SEQUENCE_EDITOR", "1"); cmd.current_dir(sh.current_dir()); let result = cmd.status()?; From 9d9206c398c22c5408a737365fb4eda017d7e215 Mon Sep 17 00:00:00 2001 From: The Miri Cronjob Bot Date: Wed, 28 May 2025 04:53:47 +0000 Subject: [PATCH 14/28] Preparing for merge from rustc --- src/tools/miri/rust-version | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/tools/miri/rust-version b/src/tools/miri/rust-version index 9c82eb416d182..a319ca41cf9dc 100644 --- a/src/tools/miri/rust-version +++ b/src/tools/miri/rust-version @@ -1 +1 @@ -d76fe154029e03aeb64af721beafdcef856d576a +04a67d5a0587ed98632f82c404ae20f9f0a51a1d From 6b8a92201e02b2ddafb4f7915e4146241758ffa8 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Wed, 28 May 2025 18:18:39 +0200 Subject: [PATCH 15/28] fix comment in before_stack_pop --- src/tools/miri/src/machine.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/tools/miri/src/machine.rs b/src/tools/miri/src/machine.rs index 0c5127d802f84..c99423ae4502e 100644 --- a/src/tools/miri/src/machine.rs +++ b/src/tools/miri/src/machine.rs @@ -1637,7 +1637,7 @@ impl<'tcx> Machine<'tcx> for MiriMachine<'tcx> { fn before_stack_pop(ecx: &mut InterpCx<'tcx, Self>) -> InterpResult<'tcx> { let frame = ecx.frame(); // We want this *before* the return value copy, because the return place itself is protected - // until we do `end_call` here. + // until we do `on_stack_pop` here, and we need to un-protect it to copy the return value. if ecx.machine.borrow_tracker.is_some() { ecx.on_stack_pop(frame)?; } From c9addbe0e5ed754ed356fa1422db6fdfcc87a6f6 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Thu, 29 May 2025 11:51:21 +0200 Subject: [PATCH 16/28] Preparing for merge from rustc --- src/tools/miri/rust-version | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/tools/miri/rust-version b/src/tools/miri/rust-version index a319ca41cf9dc..6b5ec06c13617 100644 --- a/src/tools/miri/rust-version +++ b/src/tools/miri/rust-version @@ -1 +1 @@ -04a67d5a0587ed98632f82c404ae20f9f0a51a1d +38081f22c2d7380f272aa1d7fa9b935637701c2d From 268440a09c3d25505442e3a6292825f9526c9520 Mon Sep 17 00:00:00 2001 From: Nia Espera Date: Tue, 27 May 2025 16:18:17 +0200 Subject: [PATCH 17/28] add separate allocator for MiriMachine Update src/alloc/isolated_alloc.rs Co-authored-by: Ralf Jung allow multiple seeds use bitsets fix xcompile listened to reason and made my life so much easier fmt Update src/machine.rs Co-authored-by: Ralf Jung fixups avoid some clones Update src/alloc/isolated_alloc.rs Co-authored-by: Ralf Jung Update src/alloc/isolated_alloc.rs Co-authored-by: Ralf Jung address review Update src/alloc/isolated_alloc.rs Co-authored-by: Ralf Jung fixup comment Update src/alloc/isolated_alloc.rs Co-authored-by: Ralf Jung Update src/alloc/isolated_alloc.rs Co-authored-by: Ralf Jung address review pt 2 nit rem fn Update src/alloc/isolated_alloc.rs Co-authored-by: Ralf Jung Update src/alloc/isolated_alloc.rs Co-authored-by: Ralf Jung address review unneeded unsafe --- src/tools/miri/no_alloc | 46 +++ src/tools/miri/src/{ => alloc}/alloc_bytes.rs | 64 ++- src/tools/miri/src/alloc/isolated_alloc.rs | 382 ++++++++++++++++++ src/tools/miri/src/alloc/mod.rs | 5 + src/tools/miri/src/alloc_addresses/mod.rs | 13 +- src/tools/miri/src/concurrency/thread.rs | 3 +- src/tools/miri/src/lib.rs | 5 +- src/tools/miri/src/machine.rs | 23 +- 8 files changed, 519 insertions(+), 22 deletions(-) create mode 100644 src/tools/miri/no_alloc rename src/tools/miri/src/{ => alloc}/alloc_bytes.rs (66%) create mode 100644 src/tools/miri/src/alloc/isolated_alloc.rs create mode 100644 src/tools/miri/src/alloc/mod.rs diff --git a/src/tools/miri/no_alloc b/src/tools/miri/no_alloc new file mode 100644 index 0000000000000..89e333c15dd3a --- /dev/null +++ b/src/tools/miri/no_alloc @@ -0,0 +1,46 @@ +{ + "backtraces": { + "mean": 2.3829520464, + "stddev": 0.07651981051526706 + }, + "big-allocs": { + "mean": 0.1673647059473684, + "stddev": 0.013478818300072831 + }, + "mse": { + "mean": 0.8133679916000001, + "stddev": 0.050075600632164104 + }, + "range-iteration": { + "mean": 4.243566763599999, + "stddev": 0.03380701243732224 + }, + "serde1": { + "mean": 2.589135003, + "stddev": 0.0700829518878718 + }, + "serde2": { + "mean": 5.955532229, + "stddev": 0.20599769835375004 + }, + "slice-chunked": { + "mean": 0.4702839168333333, + "stddev": 0.017522346103318015 + }, + "slice-get-unchecked": { + "mean": 0.8169163450000001, + "stddev": 0.013873697449548328 + }, + "string-replace": { + "mean": 0.5258388794, + "stddev": 0.032950972318742694 + }, + "unicode": { + "mean": 3.4562345727999997, + "stddev": 0.11276913990962134 + }, + "zip-equal": { + "mean": 3.0626452212, + "stddev": 0.0554291549675083 + } +} \ No newline at end of file diff --git a/src/tools/miri/src/alloc_bytes.rs b/src/tools/miri/src/alloc/alloc_bytes.rs similarity index 66% rename from src/tools/miri/src/alloc_bytes.rs rename to src/tools/miri/src/alloc/alloc_bytes.rs index 2bac2659ec09b..2a253952b27af 100644 --- a/src/tools/miri/src/alloc_bytes.rs +++ b/src/tools/miri/src/alloc/alloc_bytes.rs @@ -1,12 +1,23 @@ use std::alloc::Layout; use std::borrow::Cow; use std::{alloc, slice}; +#[cfg(target_os = "linux")] +use std::{cell::RefCell, rc::Rc}; use rustc_abi::{Align, Size}; use rustc_middle::mir::interpret::AllocBytes; +#[cfg(target_os = "linux")] +use crate::alloc::isolated_alloc::IsolatedAlloc; use crate::helpers::ToU64 as _; +#[derive(Clone, Debug)] +pub enum MiriAllocParams { + Global, + #[cfg(target_os = "linux")] + Isolated(Rc>), +} + /// Allocation bytes that explicitly handle the layout of the data they're storing. /// This is necessary to interface with native code that accesses the program store in Miri. #[derive(Debug)] @@ -18,13 +29,16 @@ pub struct MiriAllocBytes { /// * If `self.layout.size() == 0`, then `self.ptr` was allocated with the equivalent layout with size 1. /// * Otherwise, `self.ptr` points to memory allocated with `self.layout`. ptr: *mut u8, + /// Whether this instance of `MiriAllocBytes` had its allocation created by calling `alloc::alloc()` + /// (`Global`) or the discrete allocator (`Isolated`) + params: MiriAllocParams, } impl Clone for MiriAllocBytes { fn clone(&self) -> Self { let bytes: Cow<'_, [u8]> = Cow::Borrowed(self); let align = Align::from_bytes(self.layout.align().to_u64()).unwrap(); - MiriAllocBytes::from_bytes(bytes, align, ()) + MiriAllocBytes::from_bytes(bytes, align, self.params.clone()) } } @@ -37,8 +51,16 @@ impl Drop for MiriAllocBytes { } else { self.layout }; + // SAFETY: Invariant, `self.ptr` points to memory allocated with `self.layout`. - unsafe { alloc::dealloc(self.ptr, alloc_layout) } + unsafe { + match self.params.clone() { + MiriAllocParams::Global => alloc::dealloc(self.ptr, alloc_layout), + #[cfg(target_os = "linux")] + MiriAllocParams::Isolated(alloc) => + alloc.borrow_mut().dealloc(self.ptr, alloc_layout), + } + } } } @@ -67,7 +89,8 @@ impl MiriAllocBytes { fn alloc_with( size: u64, align: u64, - alloc_fn: impl FnOnce(Layout) -> *mut u8, + params: MiriAllocParams, + alloc_fn: impl FnOnce(Layout, &MiriAllocParams) -> *mut u8, ) -> Result { let size = usize::try_from(size).map_err(|_| ())?; let align = usize::try_from(align).map_err(|_| ())?; @@ -75,27 +98,36 @@ impl MiriAllocBytes { // When size is 0 we allocate 1 byte anyway, to ensure each allocation has a unique address. let alloc_layout = if size == 0 { Layout::from_size_align(1, align).unwrap() } else { layout }; - let ptr = alloc_fn(alloc_layout); + let ptr = alloc_fn(alloc_layout, ¶ms); if ptr.is_null() { Err(()) } else { // SAFETY: All `MiriAllocBytes` invariants are fulfilled. - Ok(Self { ptr, layout }) + Ok(Self { ptr, layout, params }) } } } impl AllocBytes for MiriAllocBytes { - /// Placeholder! - type AllocParams = (); + type AllocParams = MiriAllocParams; - fn from_bytes<'a>(slice: impl Into>, align: Align, _params: ()) -> Self { + fn from_bytes<'a>( + slice: impl Into>, + align: Align, + params: MiriAllocParams, + ) -> Self { let slice = slice.into(); let size = slice.len(); let align = align.bytes(); // SAFETY: `alloc_fn` will only be used with `size != 0`. - let alloc_fn = |layout| unsafe { alloc::alloc(layout) }; - let alloc_bytes = MiriAllocBytes::alloc_with(size.to_u64(), align, alloc_fn) + let alloc_fn = |layout, params: &MiriAllocParams| unsafe { + match params { + MiriAllocParams::Global => alloc::alloc(layout), + #[cfg(target_os = "linux")] + MiriAllocParams::Isolated(alloc) => alloc.borrow_mut().alloc(layout), + } + }; + let alloc_bytes = MiriAllocBytes::alloc_with(size.to_u64(), align, params, alloc_fn) .unwrap_or_else(|()| { panic!("Miri ran out of memory: cannot create allocation of {size} bytes") }); @@ -105,12 +137,18 @@ impl AllocBytes for MiriAllocBytes { alloc_bytes } - fn zeroed(size: Size, align: Align, _params: ()) -> Option { + fn zeroed(size: Size, align: Align, params: MiriAllocParams) -> Option { let size = size.bytes(); let align = align.bytes(); // SAFETY: `alloc_fn` will only be used with `size != 0`. - let alloc_fn = |layout| unsafe { alloc::alloc_zeroed(layout) }; - MiriAllocBytes::alloc_with(size, align, alloc_fn).ok() + let alloc_fn = |layout, params: &MiriAllocParams| unsafe { + match params { + MiriAllocParams::Global => alloc::alloc_zeroed(layout), + #[cfg(target_os = "linux")] + MiriAllocParams::Isolated(alloc) => alloc.borrow_mut().alloc_zeroed(layout), + } + }; + MiriAllocBytes::alloc_with(size, align, params, alloc_fn).ok() } fn as_mut_ptr(&mut self) -> *mut u8 { diff --git a/src/tools/miri/src/alloc/isolated_alloc.rs b/src/tools/miri/src/alloc/isolated_alloc.rs new file mode 100644 index 0000000000000..56c5f4a016c6f --- /dev/null +++ b/src/tools/miri/src/alloc/isolated_alloc.rs @@ -0,0 +1,382 @@ +use std::alloc::{self, Layout}; + +use rustc_index::bit_set::DenseBitSet; + +/// How many bytes of memory each bit in the bitset represents. +const COMPRESSION_FACTOR: usize = 4; + +/// A dedicated allocator for interpreter memory contents, ensuring they are stored on dedicated +/// pages (not mixed with Miri's own memory). This is very useful for native-lib mode. +#[derive(Debug)] +pub struct IsolatedAlloc { + /// Pointers to page-aligned memory that has been claimed by the allocator. + /// Every pointer here must point to a page-sized allocation claimed via + /// the global allocator. + page_ptrs: Vec<*mut u8>, + /// Pointers to multiple-page-sized allocations. These must also be page-aligned, + /// with their size stored as the second element of the vector. + huge_ptrs: Vec<(*mut u8, usize)>, + /// Metadata about which bytes have been allocated on each page. The length + /// of this vector must be the same as that of `page_ptrs`, and the domain + /// size of the bitset must be exactly `page_size / COMPRESSION_FACTOR`. + /// + /// Conceptually, each bit of the bitset represents the allocation status of + /// one n-byte chunk on the corresponding element of `page_ptrs`. Thus, + /// indexing into it should be done with a value one-nth of the corresponding + /// offset on the matching `page_ptrs` element (n = `COMPRESSION_FACTOR`). + page_infos: Vec>, + /// The host (not emulated) page size. + page_size: usize, +} + +impl IsolatedAlloc { + /// Creates an empty allocator. + pub fn new() -> Self { + Self { + page_ptrs: Vec::new(), + huge_ptrs: Vec::new(), + page_infos: Vec::new(), + // SAFETY: `sysconf(_SC_PAGESIZE)` is always safe to call at runtime + // See https://www.man7.org/linux/man-pages/man3/sysconf.3.html + page_size: unsafe { libc::sysconf(libc::_SC_PAGESIZE).try_into().unwrap() }, + } + } + + /// Expands the available memory pool by adding one page. + fn add_page(&mut self) -> (*mut u8, &mut DenseBitSet) { + let page_layout = Layout::from_size_align(self.page_size, self.page_size).unwrap(); + // SAFETY: The system page size, which is the layout size, cannot be 0 + let page_ptr = unsafe { alloc::alloc(page_layout) }; + // `page_infos` has to have one bit for each `COMPRESSION_FACTOR`-sized chunk of bytes in the page. + assert!(self.page_size % COMPRESSION_FACTOR == 0); + self.page_infos.push(DenseBitSet::new_empty(self.page_size / COMPRESSION_FACTOR)); + self.page_ptrs.push(page_ptr); + (page_ptr, self.page_infos.last_mut().unwrap()) + } + + /// For simplicity, we serve small allocations in multiples of COMPRESSION_FACTOR + /// bytes with at least that alignment. + #[inline] + fn normalized_layout(layout: Layout) -> (usize, usize) { + let align = + if layout.align() < COMPRESSION_FACTOR { COMPRESSION_FACTOR } else { layout.align() }; + let size = layout.size().next_multiple_of(COMPRESSION_FACTOR); + (size, align) + } + + /// If the allocation is greater than a page, then round to the nearest page #. + /// Since we pass this into the global allocator, it's more useful to return + /// a `Layout` instead of a pair of usizes. + #[inline] + fn huge_normalized_layout(layout: Layout, page_size: usize) -> Layout { + // Allocate in page-sized chunks + let size = layout.size().next_multiple_of(page_size); + // And make sure the align is at least one page + let align = std::cmp::max(layout.align(), page_size); + Layout::from_size_align(size, align).unwrap() + } + + /// Determined whether a given (size, align) should be sent to `alloc_huge` / + /// `dealloc_huge`. + #[inline] + fn is_huge_alloc(size: usize, align: usize, page_size: usize) -> bool { + align >= page_size || size >= page_size + } + + /// Allocates memory as described in `Layout`. This memory should be deallocated + /// by calling `dealloc` on this same allocator. + /// + /// SAFETY: See `alloc::alloc()` + pub unsafe fn alloc(&mut self, layout: Layout) -> *mut u8 { + // SAFETY: Upheld by caller + unsafe { self.allocate(layout, false) } + } + + /// Same as `alloc`, but zeroes out the memory. + /// + /// SAFETY: See `alloc::alloc_zeroed()` + pub unsafe fn alloc_zeroed(&mut self, layout: Layout) -> *mut u8 { + // SAFETY: Upheld by caller + unsafe { self.allocate(layout, true) } + } + + /// Abstracts over the logic of `alloc_zeroed` vs `alloc`, as determined by + /// the `zeroed` argument. + /// + /// SAFETY: See `alloc::alloc()`, with the added restriction that `page_size` + /// corresponds to the host pagesize. + unsafe fn allocate(&mut self, layout: Layout, zeroed: bool) -> *mut u8 { + let (size, align) = IsolatedAlloc::normalized_layout(layout); + if IsolatedAlloc::is_huge_alloc(size, align, self.page_size) { + // SAFETY: Validity of `layout` upheld by caller; we checked that + // the size and alignment are appropriate for being a huge alloc + unsafe { self.alloc_huge(layout, zeroed) } + } else { + for (&mut page, pinfo) in std::iter::zip(&mut self.page_ptrs, &mut self.page_infos) { + // SAFETY: The value in `self.page_size` is used to allocate + // `page`, with page alignment + if let Some(ptr) = + unsafe { Self::alloc_from_page(self.page_size, layout, page, pinfo, zeroed) } + { + return ptr; + } + } + + // We get here only if there's no space in our existing pages + let page_size = self.page_size; + // Add another page and allocate from it; this cannot fail since the + // new page is empty and we already asserted it fits into a page + let (page, pinfo) = self.add_page(); + + // SAFETY: See comment on `alloc_from_page` above + unsafe { Self::alloc_from_page(page_size, layout, page, pinfo, zeroed).unwrap() } + } + } + + /// Used internally by `allocate` to abstract over some logic. + /// + /// SAFETY: `page` must be a page-aligned pointer to an allocated page, + /// where the allocation is (at least) `page_size` bytes. + unsafe fn alloc_from_page( + page_size: usize, + layout: Layout, + page: *mut u8, + pinfo: &mut DenseBitSet, + zeroed: bool, + ) -> Option<*mut u8> { + let (size, align) = IsolatedAlloc::normalized_layout(layout); + + // Check every alignment-sized block and see if there exists a `size` + // chunk of empty space i.e. forall idx . !pinfo.contains(idx / n) + for idx in (0..page_size).step_by(align) { + let idx_pinfo = idx / COMPRESSION_FACTOR; + let size_pinfo = size / COMPRESSION_FACTOR; + // DenseBitSet::contains() panics if the index is out of bounds + if pinfo.domain_size() < idx_pinfo + size_pinfo { + break; + } + // FIXME: is there a more efficient way to check whether the entire range is unset + // in the bitset? + let range_avail = !(idx_pinfo..idx_pinfo + size_pinfo).any(|idx| pinfo.contains(idx)); + if range_avail { + pinfo.insert_range(idx_pinfo..idx_pinfo + size_pinfo); + // SAFETY: We checked the available bytes after `idx` in the call + // to `domain_size` above and asserted there are at least `idx + + // layout.size()` bytes available and unallocated after it. + // `page` must point to the start of the page, so adding `idx` + // is safe per the above. + unsafe { + let ptr = page.add(idx); + if zeroed { + // Only write the bytes we were specifically asked to + // zero out, even if we allocated more + ptr.write_bytes(0, layout.size()); + } + return Some(ptr); + } + } + } + None + } + + /// Allocates in multiples of one page on the host system. + /// + /// SAFETY: Same as `alloc()`. + unsafe fn alloc_huge(&mut self, layout: Layout, zeroed: bool) -> *mut u8 { + let layout = IsolatedAlloc::huge_normalized_layout(layout, self.page_size); + // SAFETY: Upheld by caller + let ret = + unsafe { if zeroed { alloc::alloc_zeroed(layout) } else { alloc::alloc(layout) } }; + self.huge_ptrs.push((ret, layout.size())); + ret + } + + /// Deallocates a pointer from this allocator. + /// + /// SAFETY: This pointer must have been allocated by calling `alloc()` (or + /// `alloc_zeroed()`) with the same layout as the one passed on this same + /// `IsolatedAlloc`. + pub unsafe fn dealloc(&mut self, ptr: *mut u8, layout: Layout) { + let (size, align) = IsolatedAlloc::normalized_layout(layout); + + if IsolatedAlloc::is_huge_alloc(size, align, self.page_size) { + // SAFETY: Partly upheld by caller, and we checked that the size + // and align, meaning this must have been allocated via `alloc_huge` + unsafe { + self.dealloc_huge(ptr, layout); + } + } else { + // Offset of the pointer in the current page + let ptr_idx = ptr.addr() % self.page_size; + // And then the page's base address + let page_addr = ptr.addr() - ptr_idx; + + // Find the page this allocation belongs to. + // This could be made faster if the list was sorted -- the allocator isn't fully optimized at the moment. + let pinfo = std::iter::zip(&mut self.page_ptrs, &mut self.page_infos) + .enumerate() + .find(|(_, (page, _))| page.addr() == page_addr); + let Some((idx_of_pinfo, (_, pinfo))) = pinfo else { + panic!( + "Freeing in an unallocated page: {ptr:?}\nHolding pages {:?}", + self.page_ptrs + ) + }; + // Mark this range as available in the page. + let ptr_idx_pinfo = ptr_idx / COMPRESSION_FACTOR; + let size_pinfo = size / COMPRESSION_FACTOR; + for idx in ptr_idx_pinfo..ptr_idx_pinfo + size_pinfo { + pinfo.remove(idx); + } + + // This may have been the last allocation on this page. If so, free the entire page. + // FIXME: this can lead to threshold effects, we should probably add some form + // of hysteresis. + if pinfo.is_empty() { + let page_layout = Layout::from_size_align(self.page_size, self.page_size).unwrap(); + self.page_infos.remove(idx_of_pinfo); + // SAFETY: We checked that there are no outstanding allocations + // from us pointing to this page, and we know it was allocated + // with this layout + unsafe { + alloc::dealloc(self.page_ptrs.remove(idx_of_pinfo), page_layout); + } + } + } + } + + /// SAFETY: Same as `dealloc()` with the added requirement that `layout` + /// must ask for a size larger than the host pagesize. + unsafe fn dealloc_huge(&mut self, ptr: *mut u8, layout: Layout) { + let layout = IsolatedAlloc::huge_normalized_layout(layout, self.page_size); + // Find the pointer matching in address with the one we got + let idx = self + .huge_ptrs + .iter() + .position(|pg| ptr.addr() == pg.0.addr()) + .expect("Freeing unallocated pages"); + // And kick it from the list + self.huge_ptrs.remove(idx); + // SAFETY: Caller ensures validity of the layout + unsafe { + alloc::dealloc(ptr, layout); + } + } +} + +#[cfg(test)] +mod tests { + use super::*; + + /// Helper function to assert that all bytes from `ptr` to `ptr.add(layout.size())` + /// are zeroes. + /// + /// SAFETY: `ptr` must have been allocated with `layout`. + unsafe fn assert_zeroes(ptr: *mut u8, layout: Layout) { + // SAFETY: Caller ensures this is valid + unsafe { + for ofs in 0..layout.size() { + assert_eq!(0, ptr.add(ofs).read()); + } + } + } + + /// Check that small (sub-pagesize) allocations are properly zeroed out. + #[test] + fn small_zeroes() { + let mut alloc = IsolatedAlloc::new(); + // 256 should be less than the pagesize on *any* system + let layout = Layout::from_size_align(256, 32).unwrap(); + // SAFETY: layout size is the constant above, not 0 + let ptr = unsafe { alloc.alloc_zeroed(layout) }; + // SAFETY: `ptr` was just allocated with `layout` + unsafe { + assert_zeroes(ptr, layout); + alloc.dealloc(ptr, layout); + } + } + + /// Check that huge (> 1 page) allocations are properly zeroed out also. + #[test] + fn huge_zeroes() { + let mut alloc = IsolatedAlloc::new(); + // 16k is about as big as pages get e.g. on macos aarch64 + let layout = Layout::from_size_align(16 * 1024, 128).unwrap(); + // SAFETY: layout size is the constant above, not 0 + let ptr = unsafe { alloc.alloc_zeroed(layout) }; + // SAFETY: `ptr` was just allocated with `layout` + unsafe { + assert_zeroes(ptr, layout); + alloc.dealloc(ptr, layout); + } + } + + /// Check that repeatedly reallocating the same memory will still zero out + /// everything properly + #[test] + fn repeated_allocs() { + let mut alloc = IsolatedAlloc::new(); + // Try both sub-pagesize allocs and those larger than / equal to a page + for sz in (1..=(16 * 1024)).step_by(128) { + let layout = Layout::from_size_align(sz, 1).unwrap(); + // SAFETY: all sizes in the range above are nonzero as we start from 1 + let ptr = unsafe { alloc.alloc_zeroed(layout) }; + // SAFETY: `ptr` was just allocated with `layout`, which was used + // to bound the access size + unsafe { + assert_zeroes(ptr, layout); + ptr.write_bytes(255, sz); + alloc.dealloc(ptr, layout); + } + } + } + + /// Checks that allocations of different sizes do not overlap, then for memory + /// leaks that might have occurred. + #[test] + fn check_leaks_and_overlaps() { + let mut alloc = IsolatedAlloc::new(); + + // Some random sizes and aligns + let mut sizes = vec![32; 10]; + sizes.append(&mut vec![15; 4]); + sizes.append(&mut vec![256; 12]); + // Give it some multi-page ones too + sizes.append(&mut vec![32 * 1024; 4]); + + // Matching aligns for the sizes + let mut aligns = vec![16; 12]; + aligns.append(&mut vec![256; 2]); + aligns.append(&mut vec![64; 12]); + aligns.append(&mut vec![4096; 4]); + + // Make sure we didn't mess up in the test itself! + assert_eq!(sizes.len(), aligns.len()); + + // Aggregate the sizes and aligns into a vec of layouts, then allocate them + let layouts: Vec<_> = std::iter::zip(sizes, aligns) + .map(|(sz, al)| Layout::from_size_align(sz, al).unwrap()) + .collect(); + // SAFETY: all sizes specified in `sizes` are nonzero + let ptrs: Vec<_> = + layouts.iter().map(|layout| unsafe { alloc.alloc_zeroed(*layout) }).collect(); + + for (&ptr, &layout) in std::iter::zip(&ptrs, &layouts) { + // We requested zeroed allocations, so check that that's true + // Then write to the end of the current size, so if the allocs + // overlap (or the zeroing is wrong) then `assert_zeroes` will panic. + // Also check that the alignment we asked for was respected + assert_eq!(ptr.addr().strict_rem(layout.align()), 0); + // SAFETY: each `ptr` was allocated with its corresponding `layout`, + // which is used to bound the access size + unsafe { + assert_zeroes(ptr, layout); + ptr.write_bytes(255, layout.size()); + alloc.dealloc(ptr, layout); + } + } + + // And then verify that no memory was leaked after all that + assert!(alloc.page_ptrs.is_empty() && alloc.huge_ptrs.is_empty()); + } +} diff --git a/src/tools/miri/src/alloc/mod.rs b/src/tools/miri/src/alloc/mod.rs new file mode 100644 index 0000000000000..3be885920d226 --- /dev/null +++ b/src/tools/miri/src/alloc/mod.rs @@ -0,0 +1,5 @@ +mod alloc_bytes; +#[cfg(target_os = "linux")] +pub mod isolated_alloc; + +pub use self::alloc_bytes::{MiriAllocBytes, MiriAllocParams}; diff --git a/src/tools/miri/src/alloc_addresses/mod.rs b/src/tools/miri/src/alloc_addresses/mod.rs index d2977a55e465f..12a320b967678 100644 --- a/src/tools/miri/src/alloc_addresses/mod.rs +++ b/src/tools/miri/src/alloc_addresses/mod.rs @@ -135,11 +135,12 @@ trait EvalContextExtPriv<'tcx>: crate::MiriInterpCxExt<'tcx> { if this.machine.native_lib.is_some() { // In native lib mode, we use the "real" address of the bytes for this allocation. // This ensures the interpreted program and native code have the same view of memory. + let params = this.machine.get_default_alloc_params(); let base_ptr = match info.kind { AllocKind::LiveData => { if memory_kind == MiriMemoryKind::Global.into() { // For new global allocations, we always pre-allocate the memory to be able use the machine address directly. - let prepared_bytes = MiriAllocBytes::zeroed(info.size, info.align, ()) + let prepared_bytes = MiriAllocBytes::zeroed(info.size, info.align, params) .unwrap_or_else(|| { panic!("Miri ran out of memory: cannot create allocation of {size:?} bytes", size = info.size) }); @@ -158,8 +159,11 @@ trait EvalContextExtPriv<'tcx>: crate::MiriInterpCxExt<'tcx> { } AllocKind::Function | AllocKind::VTable => { // Allocate some dummy memory to get a unique address for this function/vtable. - let alloc_bytes = - MiriAllocBytes::from_bytes(&[0u8; 1], Align::from_bytes(1).unwrap(), ()); + let alloc_bytes = MiriAllocBytes::from_bytes( + &[0u8; 1], + Align::from_bytes(1).unwrap(), + params, + ); let ptr = alloc_bytes.as_ptr(); // Leak the underlying memory to ensure it remains unique. std::mem::forget(alloc_bytes); @@ -429,7 +433,8 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { prepared_alloc_bytes.copy_from_slice(bytes); interp_ok(prepared_alloc_bytes) } else { - interp_ok(MiriAllocBytes::from_bytes(std::borrow::Cow::Borrowed(bytes), align, ())) + let params = this.machine.get_default_alloc_params(); + interp_ok(MiriAllocBytes::from_bytes(std::borrow::Cow::Borrowed(bytes), align, params)) } } diff --git a/src/tools/miri/src/concurrency/thread.rs b/src/tools/miri/src/concurrency/thread.rs index 5014bbeedb0b0..ba1436b77b8cd 100644 --- a/src/tools/miri/src/concurrency/thread.rs +++ b/src/tools/miri/src/concurrency/thread.rs @@ -897,6 +897,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { if tcx.is_foreign_item(def_id) { throw_unsup_format!("foreign thread-local statics are not supported"); } + let params = this.machine.get_default_alloc_params(); let alloc = this.ctfe_query(|tcx| tcx.eval_static_initializer(def_id))?; // We make a full copy of this allocation. let mut alloc = alloc.inner().adjust_from_tcx( @@ -905,7 +906,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { interp_ok(MiriAllocBytes::from_bytes( std::borrow::Cow::Borrowed(bytes), align, - (), + params, )) }, |ptr| this.global_root_pointer(ptr), diff --git a/src/tools/miri/src/lib.rs b/src/tools/miri/src/lib.rs index 9d663ca9edf1f..8802216448f6e 100644 --- a/src/tools/miri/src/lib.rs +++ b/src/tools/miri/src/lib.rs @@ -11,6 +11,7 @@ #![feature(nonzero_ops)] #![feature(strict_overflow_ops)] #![feature(pointer_is_aligned_to)] +#![feature(ptr_metadata)] #![feature(unqualified_local_imports)] #![feature(derive_coerce_pointee)] #![feature(arbitrary_self_types)] @@ -69,8 +70,8 @@ extern crate rustc_target; #[allow(unused_extern_crates)] extern crate rustc_driver; +mod alloc; mod alloc_addresses; -mod alloc_bytes; mod borrow_tracker; mod clock; mod concurrency; @@ -105,8 +106,8 @@ pub type OpTy<'tcx> = interpret::OpTy<'tcx, machine::Provenance>; pub type PlaceTy<'tcx> = interpret::PlaceTy<'tcx, machine::Provenance>; pub type MPlaceTy<'tcx> = interpret::MPlaceTy<'tcx, machine::Provenance>; +pub use crate::alloc::MiriAllocBytes; pub use crate::alloc_addresses::{EvalContextExt as _, ProvenanceMode}; -pub use crate::alloc_bytes::MiriAllocBytes; pub use crate::borrow_tracker::stacked_borrows::{ EvalContextExt as _, Item, Permission, Stack, Stacks, }; diff --git a/src/tools/miri/src/machine.rs b/src/tools/miri/src/machine.rs index d88fcf1a41f04..15b3653d7aef9 100644 --- a/src/tools/miri/src/machine.rs +++ b/src/tools/miri/src/machine.rs @@ -532,6 +532,10 @@ pub struct MiriMachine<'tcx> { /// Needs to be queried by ptr_to_int, hence needs interior mutability. pub(crate) rng: RefCell, + /// The allocator used for the machine's `AllocBytes` in native-libs mode. + #[cfg(target_os = "linux")] + pub(crate) allocator: Option>>, + /// The allocation IDs to report when they are being allocated /// (helps for debugging memory leaks and use after free bugs). tracked_alloc_ids: FxHashSet, @@ -715,6 +719,10 @@ impl<'tcx> MiriMachine<'tcx> { local_crates, extern_statics: FxHashMap::default(), rng: RefCell::new(rng), + #[cfg(target_os = "linux")] + allocator: if config.native_lib.is_some() { + Some(Rc::new(RefCell::new(crate::alloc::isolated_alloc::IsolatedAlloc::new()))) + } else { None }, tracked_alloc_ids: config.tracked_alloc_ids.clone(), track_alloc_accesses: config.track_alloc_accesses, check_alignment: config.check_alignment, @@ -917,6 +925,8 @@ impl VisitProvenance for MiriMachine<'_> { backtrace_style: _, local_crates: _, rng: _, + #[cfg(target_os = "linux")] + allocator: _, tracked_alloc_ids: _, track_alloc_accesses: _, check_alignment: _, @@ -1804,8 +1814,17 @@ impl<'tcx> Machine<'tcx> for MiriMachine<'tcx> { Cow::Borrowed(ecx.machine.union_data_ranges.entry(ty).or_insert_with(compute_range)) } - /// Placeholder! - fn get_default_alloc_params(&self) -> ::AllocParams {} + fn get_default_alloc_params(&self) -> ::AllocParams { + use crate::alloc::MiriAllocParams; + + #[cfg(target_os = "linux")] + match &self.allocator { + Some(alloc) => MiriAllocParams::Isolated(alloc.clone()), + None => MiriAllocParams::Global, + } + #[cfg(not(target_os = "linux"))] + MiriAllocParams::Global + } } /// Trait for callbacks handling asynchronous machine operations. From 47e24f0fe361cd02888b7d78208d597c21202297 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Thu, 29 May 2025 19:41:28 +0200 Subject: [PATCH 18/28] some refactoring of the allocator --- src/tools/miri/src/alloc/isolated_alloc.rs | 141 +++++++++++---------- 1 file changed, 74 insertions(+), 67 deletions(-) diff --git a/src/tools/miri/src/alloc/isolated_alloc.rs b/src/tools/miri/src/alloc/isolated_alloc.rs index 56c5f4a016c6f..7b74d17137341 100644 --- a/src/tools/miri/src/alloc/isolated_alloc.rs +++ b/src/tools/miri/src/alloc/isolated_alloc.rs @@ -6,16 +6,13 @@ use rustc_index::bit_set::DenseBitSet; const COMPRESSION_FACTOR: usize = 4; /// A dedicated allocator for interpreter memory contents, ensuring they are stored on dedicated -/// pages (not mixed with Miri's own memory). This is very useful for native-lib mode. +/// pages (not mixed with Miri's own memory). This is used in native-lib mode. #[derive(Debug)] pub struct IsolatedAlloc { /// Pointers to page-aligned memory that has been claimed by the allocator. /// Every pointer here must point to a page-sized allocation claimed via - /// the global allocator. + /// the global allocator. These pointers are used for "small" allocations. page_ptrs: Vec<*mut u8>, - /// Pointers to multiple-page-sized allocations. These must also be page-aligned, - /// with their size stored as the second element of the vector. - huge_ptrs: Vec<(*mut u8, usize)>, /// Metadata about which bytes have been allocated on each page. The length /// of this vector must be the same as that of `page_ptrs`, and the domain /// size of the bitset must be exactly `page_size / COMPRESSION_FACTOR`. @@ -25,6 +22,9 @@ pub struct IsolatedAlloc { /// indexing into it should be done with a value one-nth of the corresponding /// offset on the matching `page_ptrs` element (n = `COMPRESSION_FACTOR`). page_infos: Vec>, + /// Pointers to multiple-page-sized allocations. These must also be page-aligned, + /// with their size stored as the second element of the vector. + huge_ptrs: Vec<(*mut u8, usize)>, /// The host (not emulated) page size. page_size: usize, } @@ -42,31 +42,23 @@ impl IsolatedAlloc { } } - /// Expands the available memory pool by adding one page. - fn add_page(&mut self) -> (*mut u8, &mut DenseBitSet) { - let page_layout = Layout::from_size_align(self.page_size, self.page_size).unwrap(); - // SAFETY: The system page size, which is the layout size, cannot be 0 - let page_ptr = unsafe { alloc::alloc(page_layout) }; - // `page_infos` has to have one bit for each `COMPRESSION_FACTOR`-sized chunk of bytes in the page. - assert!(self.page_size % COMPRESSION_FACTOR == 0); - self.page_infos.push(DenseBitSet::new_empty(self.page_size / COMPRESSION_FACTOR)); - self.page_ptrs.push(page_ptr); - (page_ptr, self.page_infos.last_mut().unwrap()) - } - /// For simplicity, we serve small allocations in multiples of COMPRESSION_FACTOR /// bytes with at least that alignment. #[inline] - fn normalized_layout(layout: Layout) -> (usize, usize) { + fn normalized_layout(layout: Layout) -> Layout { let align = if layout.align() < COMPRESSION_FACTOR { COMPRESSION_FACTOR } else { layout.align() }; let size = layout.size().next_multiple_of(COMPRESSION_FACTOR); - (size, align) + Layout::from_size_align(size, align).unwrap() + } + + /// Returns the layout used to allocate the pages that hold small allocations. + #[inline] + fn page_layout(&self) -> Layout { + Layout::from_size_align(self.page_size, self.page_size).unwrap() } /// If the allocation is greater than a page, then round to the nearest page #. - /// Since we pass this into the global allocator, it's more useful to return - /// a `Layout` instead of a pair of usizes. #[inline] fn huge_normalized_layout(layout: Layout, page_size: usize) -> Layout { // Allocate in page-sized chunks @@ -76,11 +68,11 @@ impl IsolatedAlloc { Layout::from_size_align(size, align).unwrap() } - /// Determined whether a given (size, align) should be sent to `alloc_huge` / - /// `dealloc_huge`. + /// Determined whether a given normalized (size, align) should be sent to + /// `alloc_huge` / `dealloc_huge`. #[inline] - fn is_huge_alloc(size: usize, align: usize, page_size: usize) -> bool { - align >= page_size || size >= page_size + fn is_huge_alloc(&self, layout: &Layout) -> bool { + layout.align() > self.page_size / 2 || layout.size() >= self.page_size / 2 } /// Allocates memory as described in `Layout`. This memory should be deallocated @@ -106,8 +98,8 @@ impl IsolatedAlloc { /// SAFETY: See `alloc::alloc()`, with the added restriction that `page_size` /// corresponds to the host pagesize. unsafe fn allocate(&mut self, layout: Layout, zeroed: bool) -> *mut u8 { - let (size, align) = IsolatedAlloc::normalized_layout(layout); - if IsolatedAlloc::is_huge_alloc(size, align, self.page_size) { + let layout = IsolatedAlloc::normalized_layout(layout); + if self.is_huge_alloc(&layout) { // SAFETY: Validity of `layout` upheld by caller; we checked that // the size and alignment are appropriate for being a huge alloc unsafe { self.alloc_huge(layout, zeroed) } @@ -116,7 +108,7 @@ impl IsolatedAlloc { // SAFETY: The value in `self.page_size` is used to allocate // `page`, with page alignment if let Some(ptr) = - unsafe { Self::alloc_from_page(self.page_size, layout, page, pinfo, zeroed) } + unsafe { Self::alloc_small(self.page_size, layout, page, pinfo, zeroed) } { return ptr; } @@ -129,7 +121,7 @@ impl IsolatedAlloc { let (page, pinfo) = self.add_page(); // SAFETY: See comment on `alloc_from_page` above - unsafe { Self::alloc_from_page(page_size, layout, page, pinfo, zeroed).unwrap() } + unsafe { Self::alloc_small(page_size, layout, page, pinfo, zeroed).unwrap() } } } @@ -137,36 +129,34 @@ impl IsolatedAlloc { /// /// SAFETY: `page` must be a page-aligned pointer to an allocated page, /// where the allocation is (at least) `page_size` bytes. - unsafe fn alloc_from_page( + unsafe fn alloc_small( page_size: usize, layout: Layout, page: *mut u8, pinfo: &mut DenseBitSet, zeroed: bool, ) -> Option<*mut u8> { - let (size, align) = IsolatedAlloc::normalized_layout(layout); - // Check every alignment-sized block and see if there exists a `size` // chunk of empty space i.e. forall idx . !pinfo.contains(idx / n) - for idx in (0..page_size).step_by(align) { - let idx_pinfo = idx / COMPRESSION_FACTOR; - let size_pinfo = size / COMPRESSION_FACTOR; + for offset in (0..page_size).step_by(layout.align()) { + let offset_pinfo = offset / COMPRESSION_FACTOR; + let size_pinfo = layout.size() / COMPRESSION_FACTOR; // DenseBitSet::contains() panics if the index is out of bounds - if pinfo.domain_size() < idx_pinfo + size_pinfo { + if pinfo.domain_size() < offset_pinfo + size_pinfo { break; } // FIXME: is there a more efficient way to check whether the entire range is unset // in the bitset? - let range_avail = !(idx_pinfo..idx_pinfo + size_pinfo).any(|idx| pinfo.contains(idx)); + let range_avail = !(offset_pinfo..offset_pinfo + size_pinfo).any(|i| pinfo.contains(i)); if range_avail { - pinfo.insert_range(idx_pinfo..idx_pinfo + size_pinfo); + pinfo.insert_range(offset_pinfo..offset_pinfo + size_pinfo); // SAFETY: We checked the available bytes after `idx` in the call // to `domain_size` above and asserted there are at least `idx + // layout.size()` bytes available and unallocated after it. // `page` must point to the start of the page, so adding `idx` // is safe per the above. unsafe { - let ptr = page.add(idx); + let ptr = page.add(offset); if zeroed { // Only write the bytes we were specifically asked to // zero out, even if we allocated more @@ -179,6 +169,17 @@ impl IsolatedAlloc { None } + /// Expands the available memory pool by adding one page. + fn add_page(&mut self) -> (*mut u8, &mut DenseBitSet) { + // SAFETY: The system page size, which is the layout size, cannot be 0 + let page_ptr = unsafe { alloc::alloc(self.page_layout()) }; + // `page_infos` has to have one bit for each `COMPRESSION_FACTOR`-sized chunk of bytes in the page. + assert!(self.page_size % COMPRESSION_FACTOR == 0); + self.page_infos.push(DenseBitSet::new_empty(self.page_size / COMPRESSION_FACTOR)); + self.page_ptrs.push(page_ptr); + (page_ptr, self.page_infos.last_mut().unwrap()) + } + /// Allocates in multiples of one page on the host system. /// /// SAFETY: Same as `alloc()`. @@ -197,54 +198,60 @@ impl IsolatedAlloc { /// `alloc_zeroed()`) with the same layout as the one passed on this same /// `IsolatedAlloc`. pub unsafe fn dealloc(&mut self, ptr: *mut u8, layout: Layout) { - let (size, align) = IsolatedAlloc::normalized_layout(layout); + let layout = IsolatedAlloc::normalized_layout(layout); - if IsolatedAlloc::is_huge_alloc(size, align, self.page_size) { + if self.is_huge_alloc(&layout) { // SAFETY: Partly upheld by caller, and we checked that the size // and align, meaning this must have been allocated via `alloc_huge` unsafe { self.dealloc_huge(ptr, layout); } } else { - // Offset of the pointer in the current page - let ptr_idx = ptr.addr() % self.page_size; - // And then the page's base address - let page_addr = ptr.addr() - ptr_idx; - - // Find the page this allocation belongs to. - // This could be made faster if the list was sorted -- the allocator isn't fully optimized at the moment. - let pinfo = std::iter::zip(&mut self.page_ptrs, &mut self.page_infos) - .enumerate() - .find(|(_, (page, _))| page.addr() == page_addr); - let Some((idx_of_pinfo, (_, pinfo))) = pinfo else { - panic!( - "Freeing in an unallocated page: {ptr:?}\nHolding pages {:?}", - self.page_ptrs - ) - }; - // Mark this range as available in the page. - let ptr_idx_pinfo = ptr_idx / COMPRESSION_FACTOR; - let size_pinfo = size / COMPRESSION_FACTOR; - for idx in ptr_idx_pinfo..ptr_idx_pinfo + size_pinfo { - pinfo.remove(idx); - } + // SAFETY: It's not a huge allocation, therefore it is a small one. + let idx = unsafe { self.dealloc_small(ptr, layout) }; // This may have been the last allocation on this page. If so, free the entire page. // FIXME: this can lead to threshold effects, we should probably add some form // of hysteresis. - if pinfo.is_empty() { - let page_layout = Layout::from_size_align(self.page_size, self.page_size).unwrap(); - self.page_infos.remove(idx_of_pinfo); + if self.page_infos[idx].is_empty() { + self.page_infos.remove(idx); + let page_ptr = self.page_ptrs.remove(idx); // SAFETY: We checked that there are no outstanding allocations // from us pointing to this page, and we know it was allocated // with this layout unsafe { - alloc::dealloc(self.page_ptrs.remove(idx_of_pinfo), page_layout); + alloc::dealloc(page_ptr, self.page_layout()); } } } } + /// Returns the index of the page that this was deallocated from + /// + /// SAFETY: the pointer must have been allocated with `alloc_small`. + unsafe fn dealloc_small(&mut self, ptr: *mut u8, layout: Layout) -> usize { + // Offset of the pointer in the current page + let offset = ptr.addr() % self.page_size; + // And then the page's base address + let page_addr = ptr.addr() - offset; + + // Find the page this allocation belongs to. + // This could be made faster if the list was sorted -- the allocator isn't fully optimized at the moment. + let pinfo = std::iter::zip(&mut self.page_ptrs, &mut self.page_infos) + .enumerate() + .find(|(_, (page, _))| page.addr() == page_addr); + let Some((idx_of_pinfo, (_, pinfo))) = pinfo else { + panic!("Freeing in an unallocated page: {ptr:?}\nHolding pages {:?}", self.page_ptrs) + }; + // Mark this range as available in the page. + let ptr_idx_pinfo = offset / COMPRESSION_FACTOR; + let size_pinfo = layout.size() / COMPRESSION_FACTOR; + for idx in ptr_idx_pinfo..ptr_idx_pinfo + size_pinfo { + pinfo.remove(idx); + } + idx_of_pinfo + } + /// SAFETY: Same as `dealloc()` with the added requirement that `layout` /// must ask for a size larger than the host pagesize. unsafe fn dealloc_huge(&mut self, ptr: *mut u8, layout: Layout) { From ecc006f835067e8aa2d1693c11d8596a893afb6e Mon Sep 17 00:00:00 2001 From: tiif Date: Fri, 30 May 2025 13:56:08 +0800 Subject: [PATCH 19/28] Change diagnostic wording --- src/tools/miri/src/diagnostics.rs | 3 +-- .../tests/fail-dep/libc/unsupported_incomplete_function.stderr | 3 +-- src/tools/miri/tests/fail/alloc/no_global_allocator.stderr | 3 +-- src/tools/miri/tests/fail/unsupported_foreign_function.stderr | 3 +-- src/tools/miri/tests/native-lib/fail/function_not_in_so.stderr | 3 +-- src/tools/miri/tests/native-lib/fail/private_function.stderr | 3 +-- 6 files changed, 6 insertions(+), 12 deletions(-) diff --git a/src/tools/miri/src/diagnostics.rs b/src/tools/miri/src/diagnostics.rs index 10570a37e5d86..1728a9cfd6de6 100644 --- a/src/tools/miri/src/diagnostics.rs +++ b/src/tools/miri/src/diagnostics.rs @@ -255,8 +255,7 @@ pub fn report_error<'tcx>( ], UnsupportedForeignItem(_) => { vec![ - note!("if this is a basic API commonly used on this target, please report an issue with Miri"), - note!("however, note that Miri does not aim to support every FFI function out there; for instance, we will not support APIs for things such as GUIs, scripting languages, or databases"), + note!("this means the program tried to do something Miri does not support; it does not indicate a bug in the program"), ] } StackedBorrowsUb { help, history, .. } => { diff --git a/src/tools/miri/tests/fail-dep/libc/unsupported_incomplete_function.stderr b/src/tools/miri/tests/fail-dep/libc/unsupported_incomplete_function.stderr index a92a97cef3b34..52a93ab263d3f 100644 --- a/src/tools/miri/tests/fail-dep/libc/unsupported_incomplete_function.stderr +++ b/src/tools/miri/tests/fail-dep/libc/unsupported_incomplete_function.stderr @@ -4,8 +4,7 @@ error: unsupported operation: can't call foreign function `signal` on $OS LL | libc::signal(libc::SIGPIPE, libc::SIG_IGN); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ can't call foreign function `signal` on $OS | - = help: if this is a basic API commonly used on this target, please report an issue with Miri - = help: however, note that Miri does not aim to support every FFI function out there; for instance, we will not support APIs for things such as GUIs, scripting languages, or databases + = help: this means the program tried to do something Miri does not support; it does not indicate a bug in the program = note: BACKTRACE: = note: inside `main` at tests/fail-dep/libc/unsupported_incomplete_function.rs:LL:CC diff --git a/src/tools/miri/tests/fail/alloc/no_global_allocator.stderr b/src/tools/miri/tests/fail/alloc/no_global_allocator.stderr index 541af64b894da..e80a364671411 100644 --- a/src/tools/miri/tests/fail/alloc/no_global_allocator.stderr +++ b/src/tools/miri/tests/fail/alloc/no_global_allocator.stderr @@ -4,8 +4,7 @@ error: unsupported operation: can't call foreign function `__rust_alloc` on $OS LL | __rust_alloc(1, 1); | ^^^^^^^^^^^^^^^^^^ can't call foreign function `__rust_alloc` on $OS | - = help: if this is a basic API commonly used on this target, please report an issue with Miri - = help: however, note that Miri does not aim to support every FFI function out there; for instance, we will not support APIs for things such as GUIs, scripting languages, or databases + = help: this means the program tried to do something Miri does not support; it does not indicate a bug in the program = note: BACKTRACE: = note: inside `miri_start` at tests/fail/alloc/no_global_allocator.rs:LL:CC diff --git a/src/tools/miri/tests/fail/unsupported_foreign_function.stderr b/src/tools/miri/tests/fail/unsupported_foreign_function.stderr index 4fe45b0868a93..bbfc5c312568f 100644 --- a/src/tools/miri/tests/fail/unsupported_foreign_function.stderr +++ b/src/tools/miri/tests/fail/unsupported_foreign_function.stderr @@ -4,8 +4,7 @@ error: unsupported operation: can't call foreign function `foo` on $OS LL | foo(); | ^^^^^ can't call foreign function `foo` on $OS | - = help: if this is a basic API commonly used on this target, please report an issue with Miri - = help: however, note that Miri does not aim to support every FFI function out there; for instance, we will not support APIs for things such as GUIs, scripting languages, or databases + = help: this means the program tried to do something Miri does not support; it does not indicate a bug in the program = note: BACKTRACE: = note: inside `main` at tests/fail/unsupported_foreign_function.rs:LL:CC diff --git a/src/tools/miri/tests/native-lib/fail/function_not_in_so.stderr b/src/tools/miri/tests/native-lib/fail/function_not_in_so.stderr index bf1cfd573b8a5..b663fd41457d6 100644 --- a/src/tools/miri/tests/native-lib/fail/function_not_in_so.stderr +++ b/src/tools/miri/tests/native-lib/fail/function_not_in_so.stderr @@ -4,8 +4,7 @@ error: unsupported operation: can't call foreign function `foo` on $OS LL | foo(); | ^^^^^ can't call foreign function `foo` on $OS | - = help: if this is a basic API commonly used on this target, please report an issue with Miri - = help: however, note that Miri does not aim to support every FFI function out there; for instance, we will not support APIs for things such as GUIs, scripting languages, or databases + = help: this means the program tried to do something Miri does not support; it does not indicate a bug in the program = note: BACKTRACE: = note: inside `main` at tests/native-lib/fail/function_not_in_so.rs:LL:CC diff --git a/src/tools/miri/tests/native-lib/fail/private_function.stderr b/src/tools/miri/tests/native-lib/fail/private_function.stderr index 2cfc062212b6d..0368124001558 100644 --- a/src/tools/miri/tests/native-lib/fail/private_function.stderr +++ b/src/tools/miri/tests/native-lib/fail/private_function.stderr @@ -4,8 +4,7 @@ error: unsupported operation: can't call foreign function `not_exported` on $OS LL | not_exported(); | ^^^^^^^^^^^^^^ can't call foreign function `not_exported` on $OS | - = help: if this is a basic API commonly used on this target, please report an issue with Miri - = help: however, note that Miri does not aim to support every FFI function out there; for instance, we will not support APIs for things such as GUIs, scripting languages, or databases + = help: this means the program tried to do something Miri does not support; it does not indicate a bug in the program = note: BACKTRACE: = note: inside `main` at tests/native-lib/fail/private_function.rs:LL:CC From 089acfaeb484d45a3c27fba758ca11672300c327 Mon Sep 17 00:00:00 2001 From: Xinglu Chen Date: Fri, 18 Apr 2025 08:47:18 +0200 Subject: [PATCH 20/28] Track permissions on the byte-level Co-authored-by: Ralf Jung Co-authored-by: Johannes Hostert --- .../src/borrow_tracker/tree_borrows/mod.rs | 228 +++++++++++++----- .../src/borrow_tracker/tree_borrows/perms.rs | 21 +- .../src/borrow_tracker/tree_borrows/tree.rs | 69 +++--- src/tools/miri/src/range_map.rs | 22 +- .../fail/tree_borrows/cell-inside-struct.rs | 33 +++ .../tree_borrows/cell-inside-struct.stderr | 26 ++ .../pass/both_borrows/basic_aliasing_model.rs | 20 +- .../tree_borrows/cell-alternate-writes.stderr | 4 +- .../cell-lazy-write-to-surrounding.rs | 22 ++ 9 files changed, 329 insertions(+), 116 deletions(-) create mode 100644 src/tools/miri/tests/fail/tree_borrows/cell-inside-struct.rs create mode 100644 src/tools/miri/tests/fail/tree_borrows/cell-inside-struct.stderr create mode 100644 src/tools/miri/tests/pass/tree_borrows/cell-lazy-write-to-surrounding.rs diff --git a/src/tools/miri/src/borrow_tracker/tree_borrows/mod.rs b/src/tools/miri/src/borrow_tracker/tree_borrows/mod.rs index f3e32e75f2f2c..e1e35b43af7d1 100644 --- a/src/tools/miri/src/borrow_tracker/tree_borrows/mod.rs +++ b/src/tools/miri/src/borrow_tracker/tree_borrows/mod.rs @@ -3,6 +3,8 @@ use rustc_middle::mir::{Mutability, RetagKind}; use rustc_middle::ty::layout::HasTypingEnv; use rustc_middle::ty::{self, Ty}; +use self::foreign_access_skipping::IdempotentForeignAccess; +use self::tree::LocationState; use crate::borrow_tracker::{GlobalState, GlobalStateInner, ProtectorKind}; use crate::concurrency::data_race::NaReadType; use crate::*; @@ -113,16 +115,19 @@ impl<'tcx> Tree { /// Policy for a new borrow. #[derive(Debug, Clone, Copy)] -struct NewPermission { - /// Which permission should the pointer start with. - initial_state: Permission, +pub struct NewPermission { + /// Permission for the frozen part of the range. + freeze_perm: Permission, + /// Whether a read access should be performed on the frozen part on a retag. + freeze_access: bool, + /// Permission for the non-frozen part of the range. + nonfreeze_perm: Permission, + /// Whether a read access should be performed on the non-frozen + /// part on a retag. + nonfreeze_access: bool, /// Whether this pointer is part of the arguments of a function call. /// `protector` is `Some(_)` for all pointers marked `noalias`. protector: Option, - /// Whether a read should be performed on a retag. This should be `false` - /// for `Cell` because this could cause data races when using thread-safe - /// data types like `Mutex`. - initial_read: bool, } impl<'tcx> NewPermission { @@ -133,27 +138,42 @@ impl<'tcx> NewPermission { kind: RetagKind, cx: &crate::MiriInterpCx<'tcx>, ) -> Option { - let ty_is_freeze = pointee.is_freeze(*cx.tcx, cx.typing_env()); let ty_is_unpin = pointee.is_unpin(*cx.tcx, cx.typing_env()); let is_protected = kind == RetagKind::FnEntry; - // As demonstrated by `tests/fail/tree_borrows/reservedim_spurious_write.rs`, - // interior mutability and protectors interact poorly. - // To eliminate the case of Protected Reserved IM we override interior mutability - // in the case of a protected reference: protected references are always considered - // "freeze" in their reservation phase. - let (initial_state, initial_read) = match mutability { + let protector = is_protected.then_some(ProtectorKind::StrongProtector); + + Some(match mutability { Mutability::Mut if ty_is_unpin => - (Permission::new_reserved(ty_is_freeze, is_protected), true), - Mutability::Not if ty_is_freeze => (Permission::new_frozen(), true), - Mutability::Not if !ty_is_freeze => (Permission::new_cell(), false), - // Raw pointers never enter this function so they are not handled. - // However raw pointers are not the only pointers that take the parent - // tag, this also happens for `!Unpin` `&mut`s, which are excluded above. + NewPermission { + freeze_perm: Permission::new_reserved( + /* ty_is_freeze */ true, + is_protected, + ), + freeze_access: true, + nonfreeze_perm: Permission::new_reserved( + /* ty_is_freeze */ false, + is_protected, + ), + // If we have a mutable reference, then the non-frozen part will + // have state `ReservedIM` or `Reserved`, which can have an initial read access + // performed on it because you cannot have multiple mutable borrows. + nonfreeze_access: true, + protector, + }, + Mutability::Not => + NewPermission { + freeze_perm: Permission::new_frozen(), + freeze_access: true, + nonfreeze_perm: Permission::new_cell(), + // If it is a shared reference, then the non-frozen + // part will have state `Cell`, which should not have an initial access, + // as this can cause data races when using thread-safe data types like + // `Mutex`. + nonfreeze_access: false, + protector, + }, _ => return None, - }; - - let protector = is_protected.then_some(ProtectorKind::StrongProtector); - Some(Self { initial_state, protector, initial_read }) + }) } /// Compute permission for `Box`-like type (`Box` always, and also `Unique` if enabled). @@ -168,13 +188,17 @@ impl<'tcx> NewPermission { pointee.is_unpin(*cx.tcx, cx.typing_env()).then_some(()).map(|()| { // Regular `Unpin` box, give it `noalias` but only a weak protector // because it is valid to deallocate it within the function. - let ty_is_freeze = pointee.is_freeze(*cx.tcx, cx.typing_env()); - let protected = kind == RetagKind::FnEntry; - let initial_state = Permission::new_reserved(ty_is_freeze, protected); - Self { - initial_state, - protector: protected.then_some(ProtectorKind::WeakProtector), - initial_read: true, + let is_protected = kind == RetagKind::FnEntry; + let protector = is_protected.then_some(ProtectorKind::WeakProtector); + NewPermission { + freeze_perm: Permission::new_reserved(/* ty_is_freeze */ true, is_protected), + freeze_access: true, + nonfreeze_perm: Permission::new_reserved( + /* ty_is_freeze */ false, + is_protected, + ), + nonfreeze_access: true, + protector, } }) } @@ -194,8 +218,6 @@ trait EvalContextPrivExt<'tcx>: crate::MiriInterpCxExt<'tcx> { new_tag: BorTag, ) -> InterpResult<'tcx, Option> { let this = self.eval_context_mut(); - // Make sure the new permission makes sense as the initial permission of a fresh tag. - assert!(new_perm.initial_state.is_initial()); // Ensure we bail out if the pointer goes out-of-bounds (see miri#1050). this.check_ptr_access(place.ptr(), ptr_size, CheckInAllocMsg::Dereferenceable)?; @@ -206,7 +228,13 @@ trait EvalContextPrivExt<'tcx>: crate::MiriInterpCxExt<'tcx> { let global = this.machine.borrow_tracker.as_ref().unwrap().borrow(); let ty = place.layout.ty; if global.tracked_pointer_tags.contains(&new_tag) { - let kind_str = format!("initial state {} (pointee type {ty})", new_perm.initial_state); + let ty_is_freeze = ty.is_freeze(*this.tcx, this.typing_env()); + let kind_str = + if ty_is_freeze { + format!("initial state {} (pointee type {ty})", new_perm.freeze_perm) + } else { + format!("initial state {}/{} outside/inside UnsafeCell (pointee type {ty})", new_perm.freeze_perm, new_perm.nonfreeze_perm) + }; this.emit_diagnostic(NonHaltingDiagnostic::CreatedPointerTag( new_tag.inner(), Some(kind_str), @@ -285,43 +313,103 @@ trait EvalContextPrivExt<'tcx>: crate::MiriInterpCxExt<'tcx> { let span = this.machine.current_span(); let alloc_extra = this.get_alloc_extra(alloc_id)?; - let range = alloc_range(base_offset, ptr_size); let mut tree_borrows = alloc_extra.borrow_tracker_tb().borrow_mut(); - // All reborrows incur a (possibly zero-sized) read access to the parent - if new_perm.initial_read { - tree_borrows.perform_access( - orig_tag, - Some((range, AccessKind::Read, diagnostics::AccessCause::Reborrow)), - this.machine.borrow_tracker.as_ref().unwrap(), - alloc_id, - this.machine.current_span(), - )?; - } + // Store initial permissions and their corresponding range. + let mut perms_map: RangeMap = RangeMap::new( + ptr_size, + LocationState::new_init(Permission::new_disabled(), IdempotentForeignAccess::None), // this will be overwritten + ); + // Keep track of whether the node has any part that allows for interior mutability. + // FIXME: This misses `PhantomData>` which could be considered a marker + // for requesting interior mutability. + let mut has_unsafe_cell = false; + + // When adding a new node, the SIFA of its parents needs to be updated, potentially across + // the entire memory range. For the parts that are being accessed below, the access itself + // trivially takes care of that. However, we have to do some more work to also deal with + // the parts that are not being accessed. Specifically what we do is that we + // call `update_last_accessed_after_retag` on the SIFA of the permission set for the part of + // memory outside `perm_map` -- so that part is definitely taken care of. The remaining concern + // is the part of memory that is in the range of `perms_map`, but not accessed below. + // There we have two cases: + // * If we do have an `UnsafeCell` (`has_unsafe_cell` becomes true), then the non-accessed part + // uses `nonfreeze_perm`, so the `nonfreeze_perm` initialized parts are also fine. We enforce + // the `freeze_perm` parts to be accessed, and thus everything is taken care of. + // * If there is no `UnsafeCell`, then `freeze_perm` is used everywhere (both inside and outside the initial range), + // and we update everything to have the `freeze_perm`'s SIFA, so there are no issues. (And this assert below is not + // actually needed in this case). + assert!(new_perm.freeze_access); + + let protected = new_perm.protector.is_some(); + this.visit_freeze_sensitive(place, ptr_size, |range, frozen| { + has_unsafe_cell = has_unsafe_cell || !frozen; + + // We are only ever `Frozen` inside the frozen bits. + let (perm, access) = if frozen { + (new_perm.freeze_perm, new_perm.freeze_access) + } else { + (new_perm.nonfreeze_perm, new_perm.nonfreeze_access) + }; + + // Store initial permissions. + for (_loc_range, loc) in perms_map.iter_mut(range.start, range.size) { + let sifa = perm.strongest_idempotent_foreign_access(protected); + // NOTE: Currently, `access` is false if and only if `perm` is Cell, so this `if` + // doesn't not change whether any code is UB or not. We could just always use + // `new_init` and everything would stay the same. But that seems conceptually + // odd, so we keep the initial "accessed" bit of the `LocationState` in sync with whether + // a read access is performed below. + if access { + *loc = LocationState::new_init(perm, sifa); + } else { + *loc = LocationState::new_uninit(perm, sifa); + } + } + + // Some reborrows incur a read access to the parent. + if access { + // Adjust range to be relative to allocation start (rather than to `place`). + let mut range_in_alloc = range; + range_in_alloc.start += base_offset; + + tree_borrows.perform_access( + orig_tag, + Some((range_in_alloc, AccessKind::Read, diagnostics::AccessCause::Reborrow)), + this.machine.borrow_tracker.as_ref().unwrap(), + alloc_id, + this.machine.current_span(), + )?; + + // Also inform the data race model (but only if any bytes are actually affected). + if range.size.bytes() > 0 { + if let Some(data_race) = alloc_extra.data_race.as_vclocks_ref() { + data_race.read( + alloc_id, + range_in_alloc, + NaReadType::Retag, + Some(place.layout.ty), + &this.machine, + )? + } + } + } + interp_ok(()) + })?; + // Record the parent-child pair in the tree. tree_borrows.new_child( + base_offset, orig_tag, new_tag, - new_perm.initial_state, - range, + perms_map, + // Allow lazily writing to surrounding data if we found an `UnsafeCell`. + if has_unsafe_cell { new_perm.nonfreeze_perm } else { new_perm.freeze_perm }, + protected, span, - new_perm.protector.is_some(), )?; drop(tree_borrows); - // Also inform the data race model (but only if any bytes are actually affected). - if range.size.bytes() > 0 && new_perm.initial_read { - if let Some(data_race) = alloc_extra.data_race.as_vclocks_ref() { - data_race.read( - alloc_id, - range, - NaReadType::Retag, - Some(place.layout.ty), - &this.machine, - )?; - } - } - interp_ok(Some(Provenance::Concrete { alloc_id, tag: new_tag })) } @@ -508,15 +596,21 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { fn tb_protect_place(&mut self, place: &MPlaceTy<'tcx>) -> InterpResult<'tcx, MPlaceTy<'tcx>> { let this = self.eval_context_mut(); - // Note: if we were to inline `new_reserved` below we would find out that - // `ty_is_freeze` is eventually unused because it appears in a `ty_is_freeze || true`. - // We are nevertheless including it here for clarity. - let ty_is_freeze = place.layout.ty.is_freeze(*this.tcx, this.typing_env()); // Retag it. With protection! That is the entire point. let new_perm = NewPermission { - initial_state: Permission::new_reserved(ty_is_freeze, /* protected */ true), + // Note: If we are creating a protected Reserved, which can + // never be ReservedIM, the value of the `ty_is_freeze` + // argument doesn't matter + // (`ty_is_freeze || true` in `new_reserved` will always be `true`). + freeze_perm: Permission::new_reserved( + /* ty_is_freeze */ true, /* protected */ true, + ), + freeze_access: true, + nonfreeze_perm: Permission::new_reserved( + /* ty_is_freeze */ false, /* protected */ true, + ), + nonfreeze_access: true, protector: Some(ProtectorKind::StrongProtector), - initial_read: true, }; this.tb_retag_place(place, new_perm) } diff --git a/src/tools/miri/src/borrow_tracker/tree_borrows/perms.rs b/src/tools/miri/src/borrow_tracker/tree_borrows/perms.rs index 087f6fc3f24b0..38863ca0734a1 100644 --- a/src/tools/miri/src/borrow_tracker/tree_borrows/perms.rs +++ b/src/tools/miri/src/borrow_tracker/tree_borrows/perms.rs @@ -94,6 +94,7 @@ impl PermissionPriv { } /// Reject `ReservedIM` that cannot exist in the presence of a protector. + #[cfg(test)] fn compatible_with_protector(&self) -> bool { // FIXME(TB-Cell): It is unclear what to do here. // `Cell` will occur with a protector but won't provide the guarantees @@ -253,10 +254,6 @@ impl Permission { pub fn is_disabled(&self) -> bool { self.inner == Disabled } - /// Check if `self` is the post-child-write state of a pointer (is `Active`). - pub fn is_active(&self) -> bool { - self.inner == Active - } /// Check if `self` is the never-allow-writes-again state of a pointer (is `Frozen`). pub fn is_frozen(&self) -> bool { self.inner == Frozen @@ -289,6 +286,11 @@ impl Permission { /// is a protector is relevant because being protected takes priority over being /// interior mutable) pub fn new_reserved(ty_is_freeze: bool, protected: bool) -> Self { + // As demonstrated by `tests/fail/tree_borrows/reservedim_spurious_write.rs`, + // interior mutability and protectors interact poorly. + // To eliminate the case of Protected Reserved IM we override interior mutability + // in the case of a protected reference: protected references are always considered + // "freeze" in their reservation phase. if ty_is_freeze || protected { Self::new_reserved_frz() } else { Self::new_reserved_im() } } @@ -309,6 +311,7 @@ impl Permission { } /// Reject `ReservedIM` that cannot exist in the presence of a protector. + #[cfg(test)] pub fn compatible_with_protector(&self) -> bool { self.inner.compatible_with_protector() } @@ -393,11 +396,6 @@ impl PermTransition { self.from <= self.to } - pub fn from(from: Permission, to: Permission) -> Option { - let t = Self { from: from.inner, to: to.inner }; - t.is_possible().then_some(t) - } - pub fn is_noop(self) -> bool { self.from == self.to } @@ -407,11 +405,6 @@ impl PermTransition { (starting_point.inner == self.from).then_some(Permission { inner: self.to }) } - /// Extract starting point of a transition - pub fn started(self) -> Permission { - Permission { inner: self.from } - } - /// Determines if this transition would disable the permission. pub fn produces_disabled(self) -> bool { self.to == Disabled diff --git a/src/tools/miri/src/borrow_tracker/tree_borrows/tree.rs b/src/tools/miri/src/borrow_tracker/tree_borrows/tree.rs index 47ccaadbb9e36..0a35a70c2e367 100644 --- a/src/tools/miri/src/borrow_tracker/tree_borrows/tree.rs +++ b/src/tools/miri/src/borrow_tracker/tree_borrows/tree.rs @@ -10,6 +10,7 @@ //! and the relative position of the access; //! - idempotency properties asserted in `perms.rs` (for optimizations) +use std::ops::Range; use std::{fmt, mem}; use rustc_abi::Size; @@ -58,7 +59,7 @@ impl LocationState { /// to any foreign access yet. /// The permission is not allowed to be `Active`. /// `sifa` is the (strongest) idempotent foreign access, see `foreign_access_skipping.rs` - fn new_uninit(permission: Permission, sifa: IdempotentForeignAccess) -> Self { + pub fn new_uninit(permission: Permission, sifa: IdempotentForeignAccess) -> Self { assert!(permission.is_initial() || permission.is_disabled()); Self { permission, initialized: false, idempotent_foreign_access: sifa } } @@ -66,7 +67,7 @@ impl LocationState { /// Constructs a new initial state. It has not yet been subjected /// to any foreign access. However, it is already marked as having been accessed. /// `sifa` is the (strongest) idempotent foreign access, see `foreign_access_skipping.rs` - fn new_init(permission: Permission, sifa: IdempotentForeignAccess) -> Self { + pub fn new_init(permission: Permission, sifa: IdempotentForeignAccess) -> Self { Self { permission, initialized: true, idempotent_foreign_access: sifa } } @@ -612,20 +613,32 @@ impl Tree { } impl<'tcx> Tree { - /// Insert a new tag in the tree - pub fn new_child( + /// Insert a new tag in the tree. + /// + /// `initial_perms` defines the initial permissions for the part of memory + /// that is already considered "initialized" immediately. The ranges in this + /// map are relative to `base_offset`. + /// `default_perm` defines the initial permission for the rest of the allocation. + /// + /// For all non-accessed locations in the RangeMap (those that haven't had an + /// implicit read), their SIFA must be weaker than or as weak as the SIFA of + /// `default_perm`. + pub(super) fn new_child( &mut self, + base_offset: Size, parent_tag: BorTag, new_tag: BorTag, - default_initial_perm: Permission, - reborrow_range: AllocRange, + initial_perms: RangeMap, + default_perm: Permission, + protected: bool, span: Span, - prot: bool, ) -> InterpResult<'tcx> { - assert!(!self.tag_mapping.contains_key(&new_tag)); let idx = self.tag_mapping.insert(new_tag); let parent_idx = self.tag_mapping.get(&parent_tag).unwrap(); - let strongest_idempotent = default_initial_perm.strongest_idempotent_foreign_access(prot); + assert!(default_perm.is_initial()); + + let default_strongest_idempotent = + default_perm.strongest_idempotent_foreign_access(protected); // Create the node self.nodes.insert( idx, @@ -633,25 +646,36 @@ impl<'tcx> Tree { tag: new_tag, parent: Some(parent_idx), children: SmallVec::default(), - default_initial_perm, - default_initial_idempotent_foreign_access: strongest_idempotent, - debug_info: NodeDebugInfo::new(new_tag, default_initial_perm, span), + default_initial_perm: default_perm, + default_initial_idempotent_foreign_access: default_strongest_idempotent, + debug_info: NodeDebugInfo::new(new_tag, default_perm, span), }, ); // Register new_tag as a child of parent_tag self.nodes.get_mut(parent_idx).unwrap().children.push(idx); - // Initialize perms - let perm = LocationState::new_init(default_initial_perm, strongest_idempotent); - for (_perms_range, perms) in self.rperms.iter_mut(reborrow_range.start, reborrow_range.size) + + for (Range { start, end }, &perm) in + initial_perms.iter(Size::from_bytes(0), initial_perms.size()) { - perms.insert(idx, perm); + assert!(perm.is_initial()); + for (_perms_range, perms) in self + .rperms + .iter_mut(Size::from_bytes(start) + base_offset, Size::from_bytes(end - start)) + { + assert!( + default_strongest_idempotent + >= perm.permission.strongest_idempotent_foreign_access(protected) + ); + perms.insert(idx, perm); + } } // Inserting the new perms might have broken the SIFA invariant (see `foreign_access_skipping.rs`). // We now weaken the recorded SIFA for our parents, until the invariant is restored. // We could weaken them all to `LocalAccess`, but it is more efficient to compute the SIFA // for the new permission statically, and use that. - self.update_last_accessed_after_retag(parent_idx, strongest_idempotent); + // See the comment in `tb_reborrow` for why it is correct to use the SIFA of `default_uninit_perm`. + self.update_last_accessed_after_retag(parent_idx, default_strongest_idempotent); interp_ok(()) } @@ -1073,15 +1097,4 @@ impl AccessRelatedness { pub fn is_foreign(self) -> bool { matches!(self, AccessRelatedness::AncestorAccess | AccessRelatedness::CousinAccess) } - - /// Given the AccessRelatedness for the parent node, compute the AccessRelatedness - /// for the child node. This function assumes that we propagate away from the initial - /// access. - pub fn for_child(self) -> Self { - use AccessRelatedness::*; - match self { - AncestorAccess | This => AncestorAccess, - StrictChildAccess | CousinAccess => CousinAccess, - } - } } diff --git a/src/tools/miri/src/range_map.rs b/src/tools/miri/src/range_map.rs index 2c2484cd0bccf..29a5a8537a41d 100644 --- a/src/tools/miri/src/range_map.rs +++ b/src/tools/miri/src/range_map.rs @@ -31,6 +31,11 @@ impl RangeMap { RangeMap { v } } + pub fn size(&self) -> Size { + let size = self.v.last().map(|x| x.range.end).unwrap_or(0); + Size::from_bytes(size) + } + /// Finds the index containing the given offset. fn find_offset(&self, offset: u64) -> usize { self.v @@ -71,10 +76,7 @@ impl RangeMap { }; // The first offset that is not included any more. let end = offset + len; - assert!( - end <= self.v.last().unwrap().range.end, - "iterating beyond the bounds of this RangeMap" - ); + assert!(end <= self.size().bytes(), "iterating beyond the bounds of this RangeMap"); slice .iter() .take_while(move |elem| elem.range.start < end) @@ -327,4 +329,16 @@ mod tests { let map = RangeMap::::new(Size::from_bytes(20), -1); let _ = map.iter(Size::from_bytes(11), Size::from_bytes(11)); } + + #[test] + fn empty_map_iter() { + let map = RangeMap::::new(Size::from_bytes(0), -1); + let _ = map.iter(Size::from_bytes(0), Size::from_bytes(0)); + } + + #[test] + fn empty_map_iter_mut() { + let mut map = RangeMap::::new(Size::from_bytes(0), -1); + let _ = map.iter_mut(Size::from_bytes(0), Size::from_bytes(0)); + } } diff --git a/src/tools/miri/tests/fail/tree_borrows/cell-inside-struct.rs b/src/tools/miri/tests/fail/tree_borrows/cell-inside-struct.rs new file mode 100644 index 0000000000000..ff7978776822b --- /dev/null +++ b/src/tools/miri/tests/fail/tree_borrows/cell-inside-struct.rs @@ -0,0 +1,33 @@ +//! A version of `cell_inside_struct` that dumps the tree so that we can see what is happening. +//@compile-flags: -Zmiri-tree-borrows +#[path = "../../utils/mod.rs"] +#[macro_use] +mod utils; + +use std::cell::Cell; + +struct Foo { + field1: u32, + field2: Cell, +} + +pub fn main() { + let root = Foo { field1: 42, field2: Cell::new(88) }; + unsafe { + let a = &root; + + name!(a as *const Foo, "a"); + + let a: *const Foo = a as *const Foo; + let a: *mut Foo = a as *mut Foo; + + let alloc_id = alloc_id!(a); + print_state!(alloc_id); + + // Writing to `field2`, which is interior mutable, should be allowed. + (*a).field2.set(10); + + // Writing to `field1`, which is frozen, should not be allowed. + (*a).field1 = 88; //~ ERROR: /write access through .* is forbidden/ + } +} diff --git a/src/tools/miri/tests/fail/tree_borrows/cell-inside-struct.stderr b/src/tools/miri/tests/fail/tree_borrows/cell-inside-struct.stderr new file mode 100644 index 0000000000000..717f141945228 --- /dev/null +++ b/src/tools/miri/tests/fail/tree_borrows/cell-inside-struct.stderr @@ -0,0 +1,26 @@ +────────────────────────────────────────────────── +Warning: this tree is indicative only. Some tags may have been hidden. +0.. 4.. 8 +| Act | Act | └─┬── +| Frz |?Cel | └──── +────────────────────────────────────────────────── +error: Undefined Behavior: write access through (a) at ALLOC[0x0] is forbidden + --> tests/fail/tree_borrows/cell-inside-struct.rs:LL:CC + | +LL | (*a).field1 = 88; + | ^^^^^^^^^^^^^^^^ write access through (a) at ALLOC[0x0] is forbidden + | + = help: this indicates a potential bug in the program: it performed an invalid operation, but the Tree Borrows rules it violated are still experimental + = help: the accessed tag (a) has state Frozen which forbids this child write access +help: the accessed tag was created here, in the initial state Cell + --> tests/fail/tree_borrows/cell-inside-struct.rs:LL:CC + | +LL | let a = &root; + | ^^^^^ + = note: BACKTRACE (of the first span): + = note: inside `main` at tests/fail/tree_borrows/cell-inside-struct.rs:LL:CC + +note: some details are omitted, run with `MIRIFLAGS=-Zmiri-backtrace=full` for a verbose backtrace + +error: aborting due to 1 previous error + diff --git a/src/tools/miri/tests/pass/both_borrows/basic_aliasing_model.rs b/src/tools/miri/tests/pass/both_borrows/basic_aliasing_model.rs index c2b6a7e68be50..c06dba6252eb9 100644 --- a/src/tools/miri/tests/pass/both_borrows/basic_aliasing_model.rs +++ b/src/tools/miri/tests/pass/both_borrows/basic_aliasing_model.rs @@ -1,6 +1,7 @@ //@revisions: stack tree //@[tree]compile-flags: -Zmiri-tree-borrows #![feature(allocator_api)] +use std::cell::Cell; use std::ptr; // Test various aliasing-model-related things. @@ -23,6 +24,7 @@ fn main() { not_unpin_not_protected(); write_does_not_invalidate_all_aliases(); box_into_raw_allows_interior_mutable_alias(); + cell_inside_struct() } // Make sure that reading from an `&mut` does, like reborrowing to `&`, @@ -260,7 +262,7 @@ fn write_does_not_invalidate_all_aliases() { fn box_into_raw_allows_interior_mutable_alias() { unsafe { - let b = Box::new(std::cell::Cell::new(42)); + let b = Box::new(Cell::new(42)); let raw = Box::into_raw(b); let c = &*raw; let d = raw.cast::(); // bypassing `Cell` -- only okay in Miri tests @@ -270,3 +272,19 @@ fn box_into_raw_allows_interior_mutable_alias() { drop(Box::from_raw(raw)); } } + +fn cell_inside_struct() { + struct Foo { + field1: u32, + field2: Cell, + } + + let mut root = Foo { field1: 42, field2: Cell::new(88) }; + let a = &mut root; + + // Writing to `field2`, which is interior mutable, should be allowed. + (*a).field2.set(10); + + // Writing to `field1`, which is reserved, should also be allowed. + (*a).field1 = 88; +} diff --git a/src/tools/miri/tests/pass/tree_borrows/cell-alternate-writes.stderr b/src/tools/miri/tests/pass/tree_borrows/cell-alternate-writes.stderr index 75a30c9a08375..e09aed2cf5d01 100644 --- a/src/tools/miri/tests/pass/tree_borrows/cell-alternate-writes.stderr +++ b/src/tools/miri/tests/pass/tree_borrows/cell-alternate-writes.stderr @@ -3,8 +3,8 @@ Warning: this tree is indicative only. Some tags may have been hidden. 0.. 1 | Act | └─┬── | ReIM| └─┬── -| Cel | ├──── -| Cel | └──── +|?Cel | ├──── +|?Cel | └──── ────────────────────────────────────────────────── ────────────────────────────────────────────────── Warning: this tree is indicative only. Some tags may have been hidden. diff --git a/src/tools/miri/tests/pass/tree_borrows/cell-lazy-write-to-surrounding.rs b/src/tools/miri/tests/pass/tree_borrows/cell-lazy-write-to-surrounding.rs new file mode 100644 index 0000000000000..abe08f2cd2261 --- /dev/null +++ b/src/tools/miri/tests/pass/tree_borrows/cell-lazy-write-to-surrounding.rs @@ -0,0 +1,22 @@ +//@compile-flags: -Zmiri-tree-borrows + +use std::cell::Cell; + +fn foo(x: &Cell) { + unsafe { + let ptr = x as *const Cell as *mut Cell as *mut i32; + ptr.offset(1).write(0); + } +} + +fn main() { + let arr = [Cell::new(1), Cell::new(1)]; + foo(&arr[0]); + + let pair = (Cell::new(1), 1); + // TODO: Ideally, this would result in UB since the second element + // in `pair` is Frozen. We would need some way to express a + // "shared reference with permission to access surrounding + // interior mutable data". + foo(&pair.0); +} From 2107793ebef99dc75d48965db59306f0d8a48775 Mon Sep 17 00:00:00 2001 From: Xinglu Chen Date: Fri, 30 May 2025 14:01:19 +0200 Subject: [PATCH 21/28] Use "accessed" instead of "initialized" in `LocationState` --- .../tree_borrows/diagnostics.rs | 2 +- .../src/borrow_tracker/tree_borrows/mod.rs | 10 +-- .../src/borrow_tracker/tree_borrows/tree.rs | 79 ++++++++++--------- .../borrow_tracker/tree_borrows/tree/tests.rs | 42 +++++----- 4 files changed, 68 insertions(+), 65 deletions(-) diff --git a/src/tools/miri/src/borrow_tracker/tree_borrows/diagnostics.rs b/src/tools/miri/src/borrow_tracker/tree_borrows/diagnostics.rs index b2fd9b2bf054a..d430e9e679b20 100644 --- a/src/tools/miri/src/borrow_tracker/tree_borrows/diagnostics.rs +++ b/src/tools/miri/src/borrow_tracker/tree_borrows/diagnostics.rs @@ -504,7 +504,7 @@ impl DisplayFmt { if let Some(perm) = perm { format!( "{ac}{st}", - ac = if perm.is_initialized() { self.accessed.yes } else { self.accessed.no }, + ac = if perm.is_accessed() { self.accessed.yes } else { self.accessed.no }, st = perm.permission().short_name(), ) } else { diff --git a/src/tools/miri/src/borrow_tracker/tree_borrows/mod.rs b/src/tools/miri/src/borrow_tracker/tree_borrows/mod.rs index e1e35b43af7d1..411ae89da9060 100644 --- a/src/tools/miri/src/borrow_tracker/tree_borrows/mod.rs +++ b/src/tools/miri/src/borrow_tracker/tree_borrows/mod.rs @@ -97,7 +97,7 @@ impl<'tcx> Tree { /// A tag just lost its protector. /// /// This emits a special kind of access that is only applied - /// to initialized locations, as a protection against other + /// to accessed locations, as a protection against other /// tags not having been made aware of the existence of this /// protector. pub fn release_protector( @@ -318,7 +318,7 @@ trait EvalContextPrivExt<'tcx>: crate::MiriInterpCxExt<'tcx> { // Store initial permissions and their corresponding range. let mut perms_map: RangeMap = RangeMap::new( ptr_size, - LocationState::new_init(Permission::new_disabled(), IdempotentForeignAccess::None), // this will be overwritten + LocationState::new_accessed(Permission::new_disabled(), IdempotentForeignAccess::None), // this will be overwritten ); // Keep track of whether the node has any part that allows for interior mutability. // FIXME: This misses `PhantomData>` which could be considered a marker @@ -357,13 +357,13 @@ trait EvalContextPrivExt<'tcx>: crate::MiriInterpCxExt<'tcx> { let sifa = perm.strongest_idempotent_foreign_access(protected); // NOTE: Currently, `access` is false if and only if `perm` is Cell, so this `if` // doesn't not change whether any code is UB or not. We could just always use - // `new_init` and everything would stay the same. But that seems conceptually + // `new_accessed` and everything would stay the same. But that seems conceptually // odd, so we keep the initial "accessed" bit of the `LocationState` in sync with whether // a read access is performed below. if access { - *loc = LocationState::new_init(perm, sifa); + *loc = LocationState::new_accessed(perm, sifa); } else { - *loc = LocationState::new_uninit(perm, sifa); + *loc = LocationState::new_non_accessed(perm, sifa); } } diff --git a/src/tools/miri/src/borrow_tracker/tree_borrows/tree.rs b/src/tools/miri/src/borrow_tracker/tree_borrows/tree.rs index 0a35a70c2e367..48e4a19e263fb 100644 --- a/src/tools/miri/src/borrow_tracker/tree_borrows/tree.rs +++ b/src/tools/miri/src/borrow_tracker/tree_borrows/tree.rs @@ -33,18 +33,18 @@ mod tests; /// Data for a single *location*. #[derive(Debug, Clone, Copy, PartialEq, Eq, Hash)] pub(super) struct LocationState { - /// A location is initialized when it is child-accessed for the first time (and the initial + /// A location is "accessed" when it is child-accessed for the first time (and the initial /// retag initializes the location for the range covered by the type), and it then stays - /// initialized forever. - /// For initialized locations, "permission" is the current permission. However, for - /// uninitialized locations, we still need to track the "future initial permission": this will + /// accessed forever. + /// For accessed locations, "permission" is the current permission. However, for + /// non-accessed locations, we still need to track the "future initial permission": this will /// start out to be `default_initial_perm`, but foreign accesses need to be taken into account. /// Crucially however, while transitions to `Disabled` would usually be UB if this location is - /// protected, that is *not* the case for uninitialized locations. Instead we just have a latent + /// protected, that is *not* the case for non-accessed locations. Instead we just have a latent /// "future initial permission" of `Disabled`, causing UB only if an access is ever actually /// performed. - /// Note that the tree root is also always initialized, as if the allocation was a write access. - initialized: bool, + /// Note that the tree root is also always accessed, as if the allocation was a write access. + accessed: bool, /// This pointer's current permission / future initial permission. permission: Permission, /// See `foreign_access_skipping.rs`. @@ -59,30 +59,30 @@ impl LocationState { /// to any foreign access yet. /// The permission is not allowed to be `Active`. /// `sifa` is the (strongest) idempotent foreign access, see `foreign_access_skipping.rs` - pub fn new_uninit(permission: Permission, sifa: IdempotentForeignAccess) -> Self { + pub fn new_non_accessed(permission: Permission, sifa: IdempotentForeignAccess) -> Self { assert!(permission.is_initial() || permission.is_disabled()); - Self { permission, initialized: false, idempotent_foreign_access: sifa } + Self { permission, accessed: false, idempotent_foreign_access: sifa } } /// Constructs a new initial state. It has not yet been subjected /// to any foreign access. However, it is already marked as having been accessed. /// `sifa` is the (strongest) idempotent foreign access, see `foreign_access_skipping.rs` - pub fn new_init(permission: Permission, sifa: IdempotentForeignAccess) -> Self { - Self { permission, initialized: true, idempotent_foreign_access: sifa } + pub fn new_accessed(permission: Permission, sifa: IdempotentForeignAccess) -> Self { + Self { permission, accessed: true, idempotent_foreign_access: sifa } } - /// Check if the location has been initialized, i.e. if it has + /// Check if the location has been accessed, i.e. if it has /// ever been accessed through a child pointer. - pub fn is_initialized(&self) -> bool { - self.initialized + pub fn is_accessed(&self) -> bool { + self.accessed } /// Check if the state can exist as the initial permission of a pointer. /// - /// Do not confuse with `is_initialized`, the two are almost orthogonal - /// as apart from `Active` which is not initial and must be initialized, + /// Do not confuse with `is_accessed`, the two are almost orthogonal + /// as apart from `Active` which is not initial and must be accessed, /// any other permission can have an arbitrary combination of being - /// initial/initialized. + /// initial/accessed. /// FIXME: when the corresponding `assert` in `tree_borrows/mod.rs` finally /// passes and can be uncommented, remove this `#[allow(dead_code)]`. #[cfg_attr(not(test), allow(dead_code))] @@ -96,8 +96,8 @@ impl LocationState { /// Apply the effect of an access to one location, including /// - applying `Permission::perform_access` to the inner `Permission`, - /// - emitting protector UB if the location is initialized, - /// - updating the initialized status (child accesses produce initialized locations). + /// - emitting protector UB if the location is accessed, + /// - updating the accessed status (child accesses produce accessed locations). fn perform_access( &mut self, access_kind: AccessKind, @@ -107,14 +107,14 @@ impl LocationState { let old_perm = self.permission; let transition = Permission::perform_access(access_kind, rel_pos, old_perm, protected) .ok_or(TransitionError::ChildAccessForbidden(old_perm))?; - self.initialized |= !rel_pos.is_foreign(); + self.accessed |= !rel_pos.is_foreign(); self.permission = transition.applied(old_perm).unwrap(); - // Why do only initialized locations cause protector errors? + // Why do only accessed locations cause protector errors? // Consider two mutable references `x`, `y` into disjoint parts of // the same allocation. A priori, these may actually both be used to // access the entire allocation, as long as only reads occur. However, // a write to `y` needs to somehow record that `x` can no longer be used - // on that location at all. For these uninitialized locations (i.e., locations + // on that location at all. For these non-accessed locations (i.e., locations // that haven't been accessed with `x` yet), we track the "future initial state": // it defaults to whatever the initial state of the tag is, // but the access to `y` moves that "future initial state" of `x` to `Disabled`. @@ -122,8 +122,8 @@ impl LocationState { // So clearly protectors shouldn't fire for such "future initial state" transitions. // // See the test `two_mut_protected_same_alloc` in `tests/pass/tree_borrows/tree-borrows.rs` - // for an example of safe code that would be UB if we forgot to check `self.initialized`. - if protected && self.initialized && transition.produces_disabled() { + // for an example of safe code that would be UB if we forgot to check `self.accessed`. + if protected && self.accessed && transition.produces_disabled() { return Err(TransitionError::ProtectedDisabled(old_perm)); } Ok(transition) @@ -158,11 +158,11 @@ impl LocationState { self.idempotent_foreign_access.can_skip_foreign_access(happening_now); if self.permission.is_disabled() { // A foreign access to a `Disabled` tag will have almost no observable effect. - // It's a theorem that `Disabled` node have no protected initialized children, + // It's a theorem that `Disabled` node have no protected accessed children, // and so this foreign access will never trigger any protector. - // (Intuition: You're either protected initialized, and thus can't become Disabled - // or you're already Disabled protected, but not initialized, and then can't - // become initialized since that requires a child access, which Disabled blocks.) + // (Intuition: You're either protected accessed, and thus can't become Disabled + // or you're already Disabled protected, but not accessed, and then can't + // become accessed since that requires a child access, which Disabled blocks.) // Further, the children will never be able to read or write again, since they // have a `Disabled` parent. So this only affects diagnostics, such that the // blocking write will still be identified directly, just at a different tag. @@ -218,7 +218,7 @@ impl LocationState { impl fmt::Display for LocationState { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { write!(f, "{}", self.permission)?; - if !self.initialized { + if !self.accessed { write!(f, "?")?; } Ok(()) @@ -599,12 +599,15 @@ impl Tree { let rperms = { let mut perms = UniValMap::default(); // We manually set it to `Active` on all in-bounds positions. - // We also ensure that it is initialized, so that no `Active` but - // not yet initialized nodes exist. Essentially, we pretend there + // We also ensure that it is accessed, so that no `Active` but + // not yet accessed nodes exist. Essentially, we pretend there // was a write that initialized these to `Active`. perms.insert( root_idx, - LocationState::new_init(Permission::new_active(), IdempotentForeignAccess::None), + LocationState::new_accessed( + Permission::new_active(), + IdempotentForeignAccess::None, + ), ); RangeMap::new(size, perms) }; @@ -782,14 +785,14 @@ impl<'tcx> Tree { /// /// If `access_range_and_kind` is `None`, this is interpreted as the special /// access that is applied on protector release: - /// - the access will be applied only to initialized locations of the allocation, + /// - the access will be applied only to accessed locations of the allocation, /// - it will not be visible to children, /// - it will be recorded as a `FnExit` diagnostic access /// - and it will be a read except if the location is `Active`, i.e. has been written to, /// in which case it will be a write. /// /// `LocationState::perform_access` will take care of raising transition - /// errors and updating the `initialized` status of each location, + /// errors and updating the `accessed` status of each location, /// this traversal adds to that: /// - inserting into the map locations that do not exist yet, /// - trimming the traversal, @@ -882,7 +885,7 @@ impl<'tcx> Tree { } } else { // This is a special access through the entire allocation. - // It actually only affects `initialized` locations, so we need + // It actually only affects `accessed` locations, so we need // to filter on those before initiating the traversal. // // In addition this implicit access should not be visible to children, @@ -892,10 +895,10 @@ impl<'tcx> Tree { // why this is important. for (perms_range, perms) in self.rperms.iter_mut_all() { let idx = self.tag_mapping.get(&tag).unwrap(); - // Only visit initialized permissions + // Only visit accessed permissions if let Some(p) = perms.get(idx) && let Some(access_kind) = p.permission.protector_end_access() - && p.initialized + && p.accessed { let access_cause = diagnostics::AccessCause::FnExit(access_kind); TreeVisitor { nodes: &mut self.nodes, tag_mapping: &self.tag_mapping, perms } @@ -1059,7 +1062,7 @@ impl Tree { impl Node { pub fn default_location_state(&self) -> LocationState { - LocationState::new_uninit( + LocationState::new_non_accessed( self.default_initial_perm, self.default_initial_idempotent_foreign_access, ) diff --git a/src/tools/miri/src/borrow_tracker/tree_borrows/tree/tests.rs b/src/tools/miri/src/borrow_tracker/tree_borrows/tree/tests.rs index dbfa9807e3b5a..bb3fc2d80b3fa 100644 --- a/src/tools/miri/src/borrow_tracker/tree_borrows/tree/tests.rs +++ b/src/tools/miri/src/borrow_tracker/tree_borrows/tree/tests.rs @@ -9,10 +9,10 @@ use crate::borrow_tracker::tree_borrows::exhaustive::{Exhaustive, precondition}; impl Exhaustive for LocationState { fn exhaustive() -> Box> { // We keep `latest_foreign_access` at `None` as that's just a cache. - Box::new(<(Permission, bool)>::exhaustive().map(|(permission, initialized)| { + Box::new(<(Permission, bool)>::exhaustive().map(|(permission, accessed)| { Self { permission, - initialized, + accessed, idempotent_foreign_access: IdempotentForeignAccess::default(), } })) @@ -76,8 +76,8 @@ fn as_protected(b: bool) -> &'static str { if b { " (protected)" } else { "" } } -fn as_lazy_or_init(b: bool) -> &'static str { - if b { "initialized" } else { "lazy" } +fn as_lazy_or_accessed(b: bool) -> &'static str { + if b { "accessed" } else { "lazy" } } /// Test that tree compacting (as performed by the GC) is sound. @@ -106,7 +106,7 @@ fn tree_compacting_is_sound() { as_foreign_or_child(rel), kind, parent.permission(), - as_lazy_or_init(child.is_initialized()), + as_lazy_or_accessed(child.is_accessed()), child.permission(), as_protected(child_protected), np.permission(), @@ -122,7 +122,7 @@ fn tree_compacting_is_sound() { as_foreign_or_child(rel), kind, parent.permission(), - as_lazy_or_init(child.is_initialized()), + as_lazy_or_accessed(child.is_accessed()), child.permission(), as_protected(child_protected), nc.permission() @@ -435,19 +435,19 @@ mod spurious_read { Ok(Self { x, y, ..self }) } - /// Perform a read on the given pointer if its state is `initialized`. + /// Perform a read on the given pointer if its state is `accessed`. /// Must be called just after reborrowing a pointer, and just after /// removing a protector. - fn read_if_initialized(self, ptr: PtrSelector) -> Result { - let initialized = match ptr { - PtrSelector::X => self.x.state.initialized, - PtrSelector::Y => self.y.state.initialized, + fn read_if_accessed(self, ptr: PtrSelector) -> Result { + let accessed = match ptr { + PtrSelector::X => self.x.state.accessed, + PtrSelector::Y => self.y.state.accessed, PtrSelector::Other => panic!( - "the `initialized` status of `PtrSelector::Other` is unknown, do not pass it to `read_if_initialized`" + "the `accessed` status of `PtrSelector::Other` is unknown, do not pass it to `read_if_accessed`" ), }; - if initialized { + if accessed { self.perform_test_access(&TestAccess { ptr, kind: AccessKind::Read }) } else { Ok(self) @@ -457,13 +457,13 @@ mod spurious_read { /// Remove the protector of `x`, including the implicit read on function exit. fn end_protector_x(self) -> Result { let x = self.x.end_protector(); - Self { x, ..self }.read_if_initialized(PtrSelector::X) + Self { x, ..self }.read_if_accessed(PtrSelector::X) } /// Remove the protector of `y`, including the implicit read on function exit. fn end_protector_y(self) -> Result { let y = self.y.end_protector(); - Self { y, ..self }.read_if_initialized(PtrSelector::Y) + Self { y, ..self }.read_if_accessed(PtrSelector::Y) } fn retag_y(self, new_y: LocStateProt) -> Result { @@ -473,7 +473,7 @@ mod spurious_read { } // `xy_rel` changes to "mutually foreign" now: `y` can no longer be a parent of `x`. Self { y: new_y, xy_rel: RelPosXY::MutuallyForeign, ..self } - .read_if_initialized(PtrSelector::Y) + .read_if_accessed(PtrSelector::Y) } fn perform_test_event(self, evt: &TestEvent) -> Result { @@ -602,14 +602,14 @@ mod spurious_read { xy_rel: RelPosXY::MutuallyForeign, x: LocStateProt { // For the tests, the strongest idempotent foreign access does not matter, so we use `Default::default` - state: LocationState::new_init( + state: LocationState::new_accessed( Permission::new_frozen(), IdempotentForeignAccess::default(), ), prot: true, }, y: LocStateProt { - state: LocationState::new_uninit( + state: LocationState::new_non_accessed( Permission::new_reserved(/* freeze */ true, /* protected */ true), IdempotentForeignAccess::default(), ), @@ -650,8 +650,8 @@ mod spurious_read { for xy_rel in RelPosXY::exhaustive() { for (x_retag_perm, y_current_perm) in <(LocationState, LocationState)>::exhaustive() { - // We can only do spurious reads for initialized locations anyway. - precondition!(x_retag_perm.initialized); + // We can only do spurious reads for accessed locations anyway. + precondition!(x_retag_perm.accessed); // And `x` just got retagged, so it must be initial. precondition!(x_retag_perm.permission.is_initial()); // As stated earlier, `x` is always protected in the patterns we consider here. @@ -696,7 +696,7 @@ mod spurious_read { fn initial_state(&self) -> Result { let (x, y) = self.retag_permissions(); let state = LocStateProtPair { xy_rel: self.xy_rel, x, y }; - state.read_if_initialized(PtrSelector::X) + state.read_if_accessed(PtrSelector::X) } } From 3e126f25d11bc0a5a5d76f618efdf9b588dba4ac Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Fri, 30 May 2025 16:47:28 +0200 Subject: [PATCH 22/28] cargo-miri: recognize --verbose alongside -v --- src/tools/miri/cargo-miri/src/phases.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/tools/miri/cargo-miri/src/phases.rs b/src/tools/miri/cargo-miri/src/phases.rs index 4857f62cd3a1c..a5e019a8ea972 100644 --- a/src/tools/miri/cargo-miri/src/phases.rs +++ b/src/tools/miri/cargo-miri/src/phases.rs @@ -90,7 +90,7 @@ pub fn phase_cargo_miri(mut args: impl Iterator) { "`cargo miri` supports the following subcommands: `run`, `test`, `nextest`, `clean`, and `setup`." ), }; - let verbose = num_arg_flag("-v"); + let verbose = num_arg_flag("-v") + num_arg_flag("--verbose"); let quiet = has_arg_flag("-q") || has_arg_flag("--quiet"); // Determine the involved architectures. From 34cdfb75c1c77a9aee0d26e1c046390ea42a2515 Mon Sep 17 00:00:00 2001 From: The Miri Cronjob Bot Date: Sat, 31 May 2025 04:54:03 +0000 Subject: [PATCH 23/28] Preparing for merge from rustc --- src/tools/miri/rust-version | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/tools/miri/rust-version b/src/tools/miri/rust-version index 6b5ec06c13617..33841e47b78d8 100644 --- a/src/tools/miri/rust-version +++ b/src/tools/miri/rust-version @@ -1 +1 @@ -38081f22c2d7380f272aa1d7fa9b935637701c2d +7a7bcbbcdbf2845164a94377d0e0efebb737ffd3 From 669ebcad1270cda1e26e891812b6d853651fa21f Mon Sep 17 00:00:00 2001 From: Nia Espera Date: Sat, 31 May 2025 07:55:24 +0200 Subject: [PATCH 24/28] accidentally committed file --- src/tools/miri/no_alloc | 46 ----------------------------------------- 1 file changed, 46 deletions(-) delete mode 100644 src/tools/miri/no_alloc diff --git a/src/tools/miri/no_alloc b/src/tools/miri/no_alloc deleted file mode 100644 index 89e333c15dd3a..0000000000000 --- a/src/tools/miri/no_alloc +++ /dev/null @@ -1,46 +0,0 @@ -{ - "backtraces": { - "mean": 2.3829520464, - "stddev": 0.07651981051526706 - }, - "big-allocs": { - "mean": 0.1673647059473684, - "stddev": 0.013478818300072831 - }, - "mse": { - "mean": 0.8133679916000001, - "stddev": 0.050075600632164104 - }, - "range-iteration": { - "mean": 4.243566763599999, - "stddev": 0.03380701243732224 - }, - "serde1": { - "mean": 2.589135003, - "stddev": 0.0700829518878718 - }, - "serde2": { - "mean": 5.955532229, - "stddev": 0.20599769835375004 - }, - "slice-chunked": { - "mean": 0.4702839168333333, - "stddev": 0.017522346103318015 - }, - "slice-get-unchecked": { - "mean": 0.8169163450000001, - "stddev": 0.013873697449548328 - }, - "string-replace": { - "mean": 0.5258388794, - "stddev": 0.032950972318742694 - }, - "unicode": { - "mean": 3.4562345727999997, - "stddev": 0.11276913990962134 - }, - "zip-equal": { - "mean": 3.0626452212, - "stddev": 0.0554291549675083 - } -} \ No newline at end of file From de289e45fddfc309ee2dc63739e08f00f40d7a60 Mon Sep 17 00:00:00 2001 From: The Miri Cronjob Bot Date: Sun, 1 Jun 2025 04:57:44 +0000 Subject: [PATCH 25/28] Preparing for merge from rustc --- src/tools/miri/rust-version | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/tools/miri/rust-version b/src/tools/miri/rust-version index 33841e47b78d8..553d410b2bc7f 100644 --- a/src/tools/miri/rust-version +++ b/src/tools/miri/rust-version @@ -1 +1 @@ -7a7bcbbcdbf2845164a94377d0e0efebb737ffd3 +337c11e5932275e7d450c1f2e26f289f0ddfa717 From 9f8e157a1f1942dcde6489b047aedcadc5a887a3 Mon Sep 17 00:00:00 2001 From: Noratrieb <48135649+Noratrieb@users.noreply.github.com> Date: Sun, 1 Jun 2025 11:43:55 +0200 Subject: [PATCH 26/28] Fix tokio/file-io.rs test relying on `read`/`write` not being short The test did `write` and `read` and hoped that it would read/write everything, which doesn't always happen and caused CI failures. Switch to `write_all` and `read_to_end` to make it more reliable. --- src/tools/miri/tests/pass-dep/tokio/file-io.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/tools/miri/tests/pass-dep/tokio/file-io.rs b/src/tools/miri/tests/pass-dep/tokio/file-io.rs index 6e88b907f5dc8..e4e9234997039 100644 --- a/src/tools/miri/tests/pass-dep/tokio/file-io.rs +++ b/src/tools/miri/tests/pass-dep/tokio/file-io.rs @@ -20,7 +20,7 @@ async fn test_create_and_write() -> io::Result<()> { let mut file = File::create(&path).await?; // Write 10 bytes to the file. - file.write(b"some bytes").await?; + file.write_all(b"some bytes").await?; assert_eq!(file.metadata().await.unwrap().len(), 10); remove_file(&path).unwrap(); @@ -31,10 +31,10 @@ async fn test_create_and_read() -> io::Result<()> { let bytes = b"more bytes"; let path = utils::prepare_with_content("foo.txt", bytes); let mut file = OpenOptions::new().read(true).open(&path).await.unwrap(); - let mut buffer = [0u8; 10]; + let mut buffer = vec![]; // Read the whole file. - file.read(&mut buffer[..]).await?; + file.read_to_end(&mut buffer).await?; assert_eq!(&buffer, b"more bytes"); remove_file(&path).unwrap(); From b88cbed741c7de3371e627004ba8fde62578c176 Mon Sep 17 00:00:00 2001 From: Noratrieb <48135649+Noratrieb@users.noreply.github.com> Date: Sun, 1 Jun 2025 15:43:14 +0200 Subject: [PATCH 27/28] Make sure to sync on file-io.rs tokio test Tokio `AsyncWriteExt::write` doesn't actually ensure that the contents have written, it just *starts* the write operation. To ensure that the file has actually been written, we need to `sync_all` first. --- src/tools/miri/tests/pass-dep/tokio/file-io.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/src/tools/miri/tests/pass-dep/tokio/file-io.rs b/src/tools/miri/tests/pass-dep/tokio/file-io.rs index e4e9234997039..7d05260c95519 100644 --- a/src/tools/miri/tests/pass-dep/tokio/file-io.rs +++ b/src/tools/miri/tests/pass-dep/tokio/file-io.rs @@ -21,6 +21,7 @@ async fn test_create_and_write() -> io::Result<()> { // Write 10 bytes to the file. file.write_all(b"some bytes").await?; + file.sync_all().await?; // tokio doesn't necessarily complete writes until you sync. assert_eq!(file.metadata().await.unwrap().len(), 10); remove_file(&path).unwrap(); From 0884c683ad3fef0b8c02ed7fdf7aa3ee874a7e3d Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Sun, 1 Jun 2025 16:27:27 +0200 Subject: [PATCH 28/28] tweak comment and use a weaker fence --- src/tools/miri/tests/pass-dep/tokio/file-io.rs | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/tools/miri/tests/pass-dep/tokio/file-io.rs b/src/tools/miri/tests/pass-dep/tokio/file-io.rs index 7d05260c95519..067753203bbd9 100644 --- a/src/tools/miri/tests/pass-dep/tokio/file-io.rs +++ b/src/tools/miri/tests/pass-dep/tokio/file-io.rs @@ -21,7 +21,10 @@ async fn test_create_and_write() -> io::Result<()> { // Write 10 bytes to the file. file.write_all(b"some bytes").await?; - file.sync_all().await?; // tokio doesn't necessarily complete writes until you sync. + // For tokio's file I/O, `await` does not have its usual semantics of waiting until the + // operation is completed, so we have to wait some more to make sure the write is completed. + file.flush().await?; + // Check that 10 bytes have been written. assert_eq!(file.metadata().await.unwrap().len(), 10); remove_file(&path).unwrap();