From 7d09ce84ee0b9b63e44ec90241ff9e6584068164 Mon Sep 17 00:00:00 2001 From: "dependabot[bot]" <49699333+dependabot[bot]@users.noreply.github.com> Date: Wed, 20 Aug 2025 09:50:54 +0000 Subject: Bump slab from 0.4.10 to 0.4.11 in /tests/deps Bumps [slab](https://github.com/tokio-rs/slab) from 0.4.10 to 0.4.11. - [Release notes](https://github.com/tokio-rs/slab/releases) - [Changelog](https://github.com/tokio-rs/slab/blob/master/CHANGELOG.md) - [Commits](https://github.com/tokio-rs/slab/compare/v0.4.10...v0.4.11) --- updated-dependencies: - dependency-name: slab dependency-version: 0.4.11 dependency-type: indirect ... Signed-off-by: dependabot[bot] --- src/tools/miri/tests/deps/Cargo.lock | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) (limited to 'src') diff --git a/src/tools/miri/tests/deps/Cargo.lock b/src/tools/miri/tests/deps/Cargo.lock index 4b783ebdc4e..65ca4215c60 100644 --- a/src/tools/miri/tests/deps/Cargo.lock +++ b/src/tools/miri/tests/deps/Cargo.lock @@ -296,9 +296,9 @@ dependencies = [ [[package]] name = "slab" -version = "0.4.10" +version = "0.4.11" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "04dc19736151f35336d325007ac991178d504a119863a2fcb3758cdb5e52c50d" +checksum = "7a2ae44ef20feb57a68b23d846850f861394c2e02dc425a50098ae8c90267589" [[package]] name = "socket2" -- cgit 1.4.1-3-g733a5 From 07aff76dda8fe022052de2ad7624df7de3306529 Mon Sep 17 00:00:00 2001 From: The Miri Cronjob Bot Date: Thu, 21 Aug 2025 04:53:54 +0000 Subject: Prepare for merging from rust-lang/rust This updates the rust-version file to 125ff8a788c5d6a66917f499abdc00051afe6886. --- src/tools/miri/rust-version | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'src') diff --git a/src/tools/miri/rust-version b/src/tools/miri/rust-version index 59adc572eaa..85ce9ed79f4 100644 --- a/src/tools/miri/rust-version +++ b/src/tools/miri/rust-version @@ -1 +1 @@ -f605b57042ffeb320d7ae44490113a827139b766 +125ff8a788c5d6a66917f499abdc00051afe6886 -- cgit 1.4.1-3-g733a5 From f702219ba61d94a50ecb581a4e0ab51dc459e99e Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Thu, 21 Aug 2025 16:12:43 +0200 Subject: update rustc-build-sysroot --- src/tools/miri/cargo-miri/Cargo.lock | 4 ++-- src/tools/miri/cargo-miri/Cargo.toml | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) (limited to 'src') diff --git a/src/tools/miri/cargo-miri/Cargo.lock b/src/tools/miri/cargo-miri/Cargo.lock index b3f5dafab64..ea9c04a3cb5 100644 --- a/src/tools/miri/cargo-miri/Cargo.lock +++ b/src/tools/miri/cargo-miri/Cargo.lock @@ -429,9 +429,9 @@ dependencies = [ [[package]] name = "rustc-build-sysroot" -version = "0.5.9" +version = "0.5.10" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "fdb13874a0e55baf4ac3d49d38206aecb31a55b75d6c4d04fd850b53942c8cc8" +checksum = "dd41ead66a69880951b2f7df3139db401d44451b4da123344d27eaa791b89c95" dependencies = [ "anyhow", "rustc_version", diff --git a/src/tools/miri/cargo-miri/Cargo.toml b/src/tools/miri/cargo-miri/Cargo.toml index 77cb1df8e74..64b56ea114e 100644 --- a/src/tools/miri/cargo-miri/Cargo.toml +++ b/src/tools/miri/cargo-miri/Cargo.toml @@ -18,7 +18,7 @@ directories = "6" rustc_version = "0.4" serde_json = "1.0.40" cargo_metadata = "0.21" -rustc-build-sysroot = "0.5.8" +rustc-build-sysroot = "0.5.10" # Enable some feature flags that dev-dependencies need but dependencies # do not. This makes `./miri install` after `./miri build` faster. -- cgit 1.4.1-3-g733a5 From 5c3f317187c86fe06cd40a81b7072323a2f8ec55 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Thu, 21 Aug 2025 21:20:28 +0200 Subject: CI: also test on powerpc --- src/tools/miri/.github/workflows/ci.yml | 5 +++++ 1 file changed, 5 insertions(+) (limited to 'src') diff --git a/src/tools/miri/.github/workflows/ci.yml b/src/tools/miri/.github/workflows/ci.yml index 7d79c384f85..c0fed96d4e6 100644 --- a/src/tools/miri/.github/workflows/ci.yml +++ b/src/tools/miri/.github/workflows/ci.yml @@ -41,6 +41,11 @@ jobs: multiarch: s390x gcc_cross: s390x-linux-gnu qemu: true + - host_target: powerpc64le-unknown-linux-gnu + os: ubuntu-latest + multiarch: ppc64el + gcc_cross: powerpc64le-linux-gnu + qemu: true - host_target: aarch64-apple-darwin os: macos-latest - host_target: i686-pc-windows-msvc -- cgit 1.4.1-3-g733a5 From 9f15771d16d14490481e53445f4eb7a49442c52e Mon Sep 17 00:00:00 2001 From: The Miri Cronjob Bot Date: Fri, 22 Aug 2025 04:54:12 +0000 Subject: Prepare for merging from rust-lang/rust This updates the rust-version file to 8e3710ef31a0b2cdf5a1c2f3929b7735d1e28c20. --- src/tools/miri/rust-version | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'src') diff --git a/src/tools/miri/rust-version b/src/tools/miri/rust-version index 85ce9ed79f4..d0757e58bf9 100644 --- a/src/tools/miri/rust-version +++ b/src/tools/miri/rust-version @@ -1 +1 @@ -125ff8a788c5d6a66917f499abdc00051afe6886 +8e3710ef31a0b2cdf5a1c2f3929b7735d1e28c20 -- cgit 1.4.1-3-g733a5 From 5068317842a770371d6fcbca358fdb237bd30865 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Fri, 22 Aug 2025 10:15:57 +0200 Subject: add some ZST reborrow tests --- .../pass/both_borrows/basic_aliasing_model.rs | 22 +++++++++++++++++++++- 1 file changed, 21 insertions(+), 1 deletion(-) (limited to 'src') 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 6a625e597df..82976326a8d 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 @@ -23,7 +23,8 @@ fn main() { not_unpin_not_protected(); write_does_not_invalidate_all_aliases(); box_into_raw_allows_interior_mutable_alias(); - cell_inside_struct() + cell_inside_struct(); + zst(); } // Make sure that reading from an `&mut` does, like reborrowing to `&`, @@ -287,3 +288,22 @@ fn cell_inside_struct() { // Writing to `field1`, which is reserved, should also be allowed. (*a).field1 = 88; } + +/// ZST reborrows on various kinds of dangling pointers are valid. +fn zst() { + unsafe { + // Integer pointer. + let ptr = ptr::without_provenance_mut::<()>(15); + let _ref = &mut *ptr; + + // Out-of-bounds pointer. + let mut b = Box::new(0u8); + let ptr = (&raw mut *b).wrapping_add(15) as *mut (); + let _ref = &mut *ptr; + + // Deallocated pointer. + let ptr = &raw mut *b as *mut (); + drop(b); + let _ref = &mut *ptr; + } +} -- cgit 1.4.1-3-g733a5 From 804b41e949b07e907175b6f0a7f8c74ef5f9f9ee Mon Sep 17 00:00:00 2001 From: Stypox Date: Tue, 12 Aug 2025 22:25:54 +0200 Subject: Account for time spent tracing, use RDTSC for faster time --- src/tools/miri/src/bin/log/mod.rs | 1 + src/tools/miri/src/bin/log/tracing_chrome.rs | 177 +++++++++++--------- .../miri/src/bin/log/tracing_chrome_instant.rs | 183 +++++++++++++++++++++ 3 files changed, 283 insertions(+), 78 deletions(-) create mode 100644 src/tools/miri/src/bin/log/tracing_chrome_instant.rs (limited to 'src') diff --git a/src/tools/miri/src/bin/log/mod.rs b/src/tools/miri/src/bin/log/mod.rs index f3b2fdb5348..22f74dd46b5 100644 --- a/src/tools/miri/src/bin/log/mod.rs +++ b/src/tools/miri/src/bin/log/mod.rs @@ -1,2 +1,3 @@ pub mod setup; mod tracing_chrome; +mod tracing_chrome_instant; diff --git a/src/tools/miri/src/bin/log/tracing_chrome.rs b/src/tools/miri/src/bin/log/tracing_chrome.rs index 3379816550c..310887a13a5 100644 --- a/src/tools/miri/src/bin/log/tracing_chrome.rs +++ b/src/tools/miri/src/bin/log/tracing_chrome.rs @@ -7,12 +7,15 @@ //! (`git log -- path/to/tracing_chrome.rs`), but in summary: //! - the file attributes were changed and `extern crate` was added at the top //! - if a tracing span has a field called "tracing_separate_thread", it will be given a separate -//! span ID even in [TraceStyle::Threaded] mode, to make it appear on a separate line when viewing -//! the trace in . This is the syntax to trigger this behavior: +//! span ID even in [TraceStyle::Threaded] mode, to make it appear on a separate line when viewing +//! the trace in . This is the syntax to trigger this behavior: //! ```rust //! tracing::info_span!("my_span", tracing_separate_thread = tracing::field::Empty, /* ... */) //! ``` -//! - use i64 instead of u64 for the "id" in [ChromeLayer::get_root_id] to be compatible with Perfetto +//! - use i64 instead of u64 for the "id" in [ChromeLayer::get_root_id] to be compatible with +//! Perfetto +//! - use [ChromeLayer::with_elapsed_micros_subtracting_tracing] to make time measurements faster on +//! Linux x86/x86_64 and to subtract time spent tracing from the timestamps in the trace file //! //! Depending on the tracing-chrome crate from crates.io is unfortunately not possible, since it //! depends on `tracing_core` which conflicts with rustc_private's `tracing_core` (meaning it would @@ -50,9 +53,22 @@ use std::{ thread::JoinHandle, }; +use crate::log::tracing_chrome_instant::TracingChromeInstant; + +/// Contains thread-local data for threads that send tracing spans or events. +struct ThreadData { + /// A unique ID for this thread, will populate "tid" field in the output trace file. + tid: usize, + /// A clone of [ChromeLayer::out] to avoid the expensive operation of accessing a mutex + /// every time. This is used to send [Message]s to the thread that saves trace data to file. + out: Sender, + /// The instant in time this thread was started. All events happening on this thread will be + /// saved to the trace file with a timestamp (the "ts" field) measured relative to this instant. + start: TracingChromeInstant, +} + thread_local! { - static OUT: RefCell>> = const { RefCell::new(None) }; - static TID: RefCell> = const { RefCell::new(None) }; + static THREAD_DATA: RefCell> = const { RefCell::new(None) }; } type NameFn = Box) -> String + Send + Sync>; @@ -64,7 +80,6 @@ where S: Subscriber + for<'span> LookupSpan<'span> + Send + Sync, { out: Arc>>, - start: std::time::Instant, max_tid: AtomicUsize, include_args: bool, include_locations: bool, @@ -323,7 +338,6 @@ where { fn new(mut builder: ChromeLayerBuilder) -> (ChromeLayer, FlushGuard) { let (tx, rx) = mpsc::channel(); - OUT.with(|val| val.replace(Some(tx.clone()))); let out_writer = builder .out_writer @@ -443,7 +457,6 @@ where }; let layer = ChromeLayer { out: Arc::new(Mutex::new(tx)), - start: std::time::Instant::now(), max_tid: AtomicUsize::new(0), name_fn: builder.name_fn.take(), cat_fn: builder.cat_fn.take(), @@ -456,22 +469,7 @@ where (layer, guard) } - fn get_tid(&self) -> (usize, bool) { - TID.with(|value| { - let tid = *value.borrow(); - match tid { - Some(tid) => (tid, false), - None => { - let tid = self.max_tid.fetch_add(1, Ordering::SeqCst); - value.replace(Some(tid)); - (tid, true) - } - } - }) - } - - fn get_callsite(&self, data: EventOrSpan) -> Callsite { - let (tid, new_thread) = self.get_tid(); + fn get_callsite(&self, data: EventOrSpan, tid: usize) -> Callsite { let name = self.name_fn.as_ref().map(|name_fn| name_fn(&data)); let target = self.cat_fn.as_ref().map(|cat_fn| cat_fn(&data)); let meta = match data { @@ -502,14 +500,6 @@ where (None, None) }; - if new_thread { - let name = match std::thread::current().name() { - Some(name) => name.to_owned(), - None => tid.to_string(), - }; - self.send_message(Message::NewThread(tid, name)); - } - Callsite { tid, name, @@ -548,31 +538,55 @@ where } } - fn enter_span(&self, span: SpanRef, ts: f64) { - let callsite = self.get_callsite(EventOrSpan::Span(&span)); + fn enter_span(&self, span: SpanRef, ts: f64, tid: usize, out: &Sender) { + let callsite = self.get_callsite(EventOrSpan::Span(&span), tid); let root_id = self.get_root_id(span); - self.send_message(Message::Enter(ts, callsite, root_id)); + let _ignored = out.send(Message::Enter(ts, callsite, root_id)); } - fn exit_span(&self, span: SpanRef, ts: f64) { - let callsite = self.get_callsite(EventOrSpan::Span(&span)); + fn exit_span(&self, span: SpanRef, ts: f64, tid: usize, out: &Sender) { + let callsite = self.get_callsite(EventOrSpan::Span(&span), tid); let root_id = self.get_root_id(span); - self.send_message(Message::Exit(ts, callsite, root_id)); + let _ignored = out.send(Message::Exit(ts, callsite, root_id)); } - fn get_ts(&self) -> f64 { - self.start.elapsed().as_nanos() as f64 / 1000.0 - } + /// Helper function that measures how much time is spent while executing `f` and accounts for it + /// in subsequent calls, with the aim to reduce biases in the data collected by `tracing_chrome` + /// by subtracting the time spent inside tracing functions from the timeline. This makes it so + /// that the time spent inside the `tracing_chrome` functions does not impact the timestamps + /// inside the trace file (i.e. `ts`), even if such functions are slow (e.g. because they need + /// to format arguments on the same thread those arguments are collected on, otherwise memory + /// safety would be broken). + /// + /// `f` is called with the microseconds elapsed since the current thread was started (**not** + /// since the program start!), with the current thread ID (i.e. `tid`), and with a [Sender] that + /// can be used to send a [Message] to the thread that collects [Message]s and saves them to the + /// trace file. + #[inline(always)] + fn with_elapsed_micros_subtracting_tracing(&self, f: impl Fn(f64, usize, &Sender)) { + THREAD_DATA.with(|value| { + let mut thread_data = value.borrow_mut(); + let (ThreadData { tid, out, start }, new_thread) = match thread_data.as_mut() { + Some(thread_data) => (thread_data, false), + None => { + let tid = self.max_tid.fetch_add(1, Ordering::SeqCst); + let out = self.out.lock().unwrap().clone(); + let start = TracingChromeInstant::setup_for_thread_and_start(tid); + *thread_data = Some(ThreadData { tid, out, start }); + (thread_data.as_mut().unwrap(), true) + } + }; - fn send_message(&self, message: Message) { - OUT.with(move |val| { - if val.borrow().is_some() { - let _ignored = val.borrow().as_ref().unwrap().send(message); - } else { - let out = self.out.lock().unwrap().clone(); - let _ignored = out.send(message); - val.replace(Some(out)); - } + start.with_elapsed_micros_subtracting_tracing(|ts| { + if new_thread { + let name = match std::thread::current().name() { + Some(name) => name.to_owned(), + None => tid.to_string(), + }; + let _ignored = out.send(Message::NewThread(*tid, name)); + } + f(ts, *tid, out); + }); }); } } @@ -586,52 +600,58 @@ where return; } - let ts = self.get_ts(); - self.enter_span(ctx.span(id).expect("Span not found."), ts); + self.with_elapsed_micros_subtracting_tracing(|ts, tid, out| { + self.enter_span(ctx.span(id).expect("Span not found."), ts, tid, out); + }); } fn on_record(&self, id: &span::Id, values: &span::Record<'_>, ctx: Context<'_, S>) { if self.include_args { - let span = ctx.span(id).unwrap(); - let mut exts = span.extensions_mut(); + self.with_elapsed_micros_subtracting_tracing(|_, _, _| { + let span = ctx.span(id).unwrap(); + let mut exts = span.extensions_mut(); - let args = exts.get_mut::(); + let args = exts.get_mut::(); - if let Some(args) = args { - let args = Arc::make_mut(&mut args.args); - values.record(&mut JsonVisitor { object: args }); - } + if let Some(args) = args { + let args = Arc::make_mut(&mut args.args); + values.record(&mut JsonVisitor { object: args }); + } + }); } } fn on_event(&self, event: &Event<'_>, _ctx: Context<'_, S>) { - let ts = self.get_ts(); - let callsite = self.get_callsite(EventOrSpan::Event(event)); - self.send_message(Message::Event(ts, callsite)); + self.with_elapsed_micros_subtracting_tracing(|ts, tid, out| { + let callsite = self.get_callsite(EventOrSpan::Event(event), tid); + let _ignored = out.send(Message::Event(ts, callsite)); + }); } fn on_exit(&self, id: &span::Id, ctx: Context<'_, S>) { if let TraceStyle::Async = self.trace_style { return; } - let ts = self.get_ts(); - self.exit_span(ctx.span(id).expect("Span not found."), ts); + self.with_elapsed_micros_subtracting_tracing(|ts, tid, out| { + self.exit_span(ctx.span(id).expect("Span not found."), ts, tid, out); + }); } fn on_new_span(&self, attrs: &span::Attributes<'_>, id: &span::Id, ctx: Context<'_, S>) { - if self.include_args { - let mut args = Object::new(); - attrs.record(&mut JsonVisitor { object: &mut args }); - ctx.span(id).unwrap().extensions_mut().insert(ArgsWrapper { - args: Arc::new(args), - }); - } - if let TraceStyle::Threaded = self.trace_style { - return; - } + self.with_elapsed_micros_subtracting_tracing(|ts, tid, out| { + if self.include_args { + let mut args = Object::new(); + attrs.record(&mut JsonVisitor { object: &mut args }); + ctx.span(id).unwrap().extensions_mut().insert(ArgsWrapper { + args: Arc::new(args), + }); + } + if let TraceStyle::Threaded = self.trace_style { + return; + } - let ts = self.get_ts(); - self.enter_span(ctx.span(id).expect("Span not found."), ts); + self.enter_span(ctx.span(id).expect("Span not found."), ts, tid, out); + }); } fn on_close(&self, id: span::Id, ctx: Context<'_, S>) { @@ -639,8 +659,9 @@ where return; } - let ts = self.get_ts(); - self.exit_span(ctx.span(&id).expect("Span not found."), ts); + self.with_elapsed_micros_subtracting_tracing(|ts, tid, out| { + self.exit_span(ctx.span(&id).expect("Span not found."), ts, tid, out); + }); } } diff --git a/src/tools/miri/src/bin/log/tracing_chrome_instant.rs b/src/tools/miri/src/bin/log/tracing_chrome_instant.rs new file mode 100644 index 00000000000..f400bc20a7b --- /dev/null +++ b/src/tools/miri/src/bin/log/tracing_chrome_instant.rs @@ -0,0 +1,183 @@ +//! Code in this class was in part inspired by +//! . +//! A useful resource is also +//! , +//! although this file does not implement TSC synchronization but insteads pins threads to CPUs, +//! since the former is not reliable (i.e. it might lead to non-monotonic time measurements). +//! Another useful resource for future improvements might be measureme's time measurement utils: +//! . +//! Documentation about how the Linux kernel chooses a clock source can be found here: +//! . +#![cfg(feature = "tracing")] + +/// This alternative `TracingChromeInstant` implementation was made entirely to suit the needs of +/// [crate::log::tracing_chrome], and shouldn't be used for anything else. It featues two functions: +/// - [TracingChromeInstant::setup_for_thread_and_start], which sets up the current thread to do +/// proper time tracking and returns a point in time to use as "t=0", and +/// - [TracingChromeInstant::with_elapsed_micros_subtracting_tracing], which allows +/// obtaining how much time elapsed since [TracingChromeInstant::setup_for_thread_and_start] was +/// called while accounting for (and subtracting) the time spent inside tracing-related functions. +/// +/// This measures time using [std::time::Instant], except for x86/x86_64 Linux machines, where +/// [std::time::Instant] is too slow (~1.5us) and thus `rdtsc` is used instead (~5ns). +pub enum TracingChromeInstant { + WallTime { + /// The time at which this instant was created, shifted forward to account + /// for time spent in tracing functions as explained in + /// [TracingChromeInstant::with_elapsed_micros_subtracting_tracing]'s comments. + start_instant: std::time::Instant, + }, + #[cfg(all(target_os = "linux", any(target_arch = "x86", target_arch = "x86_64")))] + Tsc { + /// The value in the TSC counter when this instant was created, shifted forward to account + /// for time spent in tracing functions as explained in + /// [TracingChromeInstant::with_elapsed_micros_subtracting_tracing]'s comments. + start_tsc: u64, + /// The period of the TSC counter in microseconds. + tsc_to_microseconds: f64, + }, +} + +impl TracingChromeInstant { + /// Can be thought of as the same as [std::time::Instant::now()], but also does some setup to + /// make TSC stable in case TSC is available. This is supposed to be called (at most) once per + /// thread since the thread setup takes a few milliseconds. + /// + /// WARNING: If TSC is available, `incremental_thread_id` is used to pick to which CPU to pin + /// the current thread. Thread IDs should be assigned contiguously starting from 0. Be aware + /// that the current thread will be restricted to one CPU for the rest of the execution! + pub fn setup_for_thread_and_start(incremental_thread_id: usize) -> TracingChromeInstant { + #[cfg(all(target_os = "linux", any(target_arch = "x86", target_arch = "x86_64")))] + if *tsc::IS_TSC_AVAILABLE.get_or_init(tsc::is_tsc_available) { + // We need to lock this thread to a specific CPU, because CPUs' TSC timers might be out + // of sync. + tsc::set_cpu_affinity(incremental_thread_id); + + // Can only use tsc_to_microseconds() and rdtsc() after having set the CPU affinity! + // We compute tsc_to_microseconds anew for every new thread just in case some CPU core + // has a different TSC frequency. + let tsc_to_microseconds = tsc::tsc_to_microseconds(); + let start_tsc = tsc::rdtsc(); + return TracingChromeInstant::Tsc { start_tsc, tsc_to_microseconds }; + } + + let _ = incremental_thread_id; // otherwise we get a warning when the TSC branch is disabled + TracingChromeInstant::WallTime { start_instant: std::time::Instant::now() } + } + + /// Calls `f` with the time elapsed in microseconds since this [TracingChromeInstant] was built + /// by [TracingChromeInstant::setup_for_thread_and_start], while subtracting all time previously + /// spent executing other `f`s passed to this function. This behavior allows subtracting time + /// spent in functions that log tracing data (which `f` is supposed to be) from the tracing time + /// measurements. + /// + /// Note: microseconds are used as the time unit since that's what Chrome trace files should + /// contain, see the definition of the "ts" field in + /// . + #[inline(always)] + pub fn with_elapsed_micros_subtracting_tracing(&mut self, f: impl Fn(f64)) { + match self { + TracingChromeInstant::WallTime { start_instant } => { + // Obtain the current time (before executing `f`). + let instant_before_f = std::time::Instant::now(); + + // Using the current time (`instant_before_f`) and the `start_instant` stored in + // `self`, calculate the elapsed time (in microseconds) since this instant was + // instantiated, accounting for any time that was previously spent executing `f`. + // The "accounting" part is not computed in this line, but is rather done by + // shifting forward the `start_instant` down below. + let ts = (instant_before_f - *start_instant).as_nanos() as f64 / 1000.0; + + // Run the function (supposedly a function internal to the tracing infrastructure). + f(ts); + + // Measure how much time was spent executing `f` and shift `start_instant` forward + // by that amount. This "removes" that time from the trace. + *start_instant += std::time::Instant::now() - instant_before_f; + } + + #[cfg(all(target_os = "linux", any(target_arch = "x86", target_arch = "x86_64")))] + TracingChromeInstant::Tsc { start_tsc, tsc_to_microseconds } => { + // the comments above also apply here, since it's the same logic + let tsc_before_f = tsc::rdtsc(); + let ts = ((tsc_before_f - *start_tsc) as f64) * (*tsc_to_microseconds); + f(ts); + *start_tsc += tsc::rdtsc() - tsc_before_f; + } + } + } +} + +#[cfg(all(target_os = "linux", any(target_arch = "x86", target_arch = "x86_64")))] +mod tsc { + + pub static IS_TSC_AVAILABLE: std::sync::OnceLock = std::sync::OnceLock::new(); + + /// Reads the timestamp-counter register. Will give monotonic answers only when called from the + /// same thread, because the TSC of different CPUs might be out of sync. + #[inline(always)] + pub(super) fn rdtsc() -> u64 { + #[cfg(target_arch = "x86")] + use core::arch::x86::_rdtsc; + #[cfg(target_arch = "x86_64")] + use core::arch::x86_64::_rdtsc; + + unsafe { _rdtsc() } + } + + /// Estimates the frequency of the TSC counter by waiting 10ms in a busy loop and + /// looking at how much the TSC increased in the meantime. + pub(super) fn tsc_to_microseconds() -> f64 { + const BUSY_WAIT: std::time::Duration = std::time::Duration::from_millis(10); + let tsc_start = rdtsc(); + let instant_start = std::time::Instant::now(); + while instant_start.elapsed() < BUSY_WAIT { + // `thread::sleep()` is not very precise at waking up the program at the right time, + // so use a busy loop instead. + core::hint::spin_loop(); + } + let tsc_end = rdtsc(); + (BUSY_WAIT.as_nanos() as f64) / 1000.0 / ((tsc_end - tsc_start) as f64) + } + + /// Checks whether the TSC counter is available and runs at a constant rate independently + /// of CPU frequency even across different power states of the CPU (i.e. checks for the + /// `invariant_tsc` CPUID flag). + pub(super) fn is_tsc_available() -> bool { + #[cfg(target_arch = "x86")] + use core::arch::x86::__cpuid; + #[cfg(target_arch = "x86_64")] + use core::arch::x86_64::__cpuid; + + // implemented like https://docs.rs/raw-cpuid/latest/src/raw_cpuid/extended.rs.html#965-967 + const LEAF: u32 = 0x80000007; // this is the leaf for "advanced power management info" + let cpuid = unsafe { __cpuid(LEAF) }; + (cpuid.edx & (1 << 8)) != 0 // EDX bit 8 indicates invariant TSC + } + + /// Forces the current thread to run on a single CPU, which ensures the TSC counter is monotonic + /// (since TSCs of different CPUs might be out-of-sync). `incremental_thread_id` is used to pick + /// to which CPU to pin the current thread, and should be an incremental number that starts from + /// 0. + pub(super) fn set_cpu_affinity(incremental_thread_id: usize) { + let cpu_id = match std::thread::available_parallelism() { + Ok(available_parallelism) => incremental_thread_id % available_parallelism, + _ => panic!("Could not determine CPU count to properly set CPU affinity"), + }; + + let mut set = unsafe { std::mem::zeroed::() }; + unsafe { libc::CPU_SET(cpu_id, &mut set) }; + + // Set the current thread's core affinity. + if unsafe { + libc::sched_setaffinity( + 0, // Defaults to current thread + size_of::(), + &set as *const _, + ) + } != 0 + { + panic!("Could not set CPU affinity") + } + } +} -- cgit 1.4.1-3-g733a5 From c68450a473e0d6895b8372c38b38591b9baa7472 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Sat, 23 Aug 2025 22:00:57 +0200 Subject: tree borrows: refactor new-permission logic --- .../miri/src/borrow_tracker/tree_borrows/mod.rs | 149 ++++++++------------- .../miri/src/borrow_tracker/tree_borrows/perms.rs | 17 +-- .../src/borrow_tracker/tree_borrows/tree/tests.rs | 2 +- .../frozen-lazy-write-to-surrounding.rs | 9 ++ .../frozen-lazy-write-to-surrounding.stderr | 21 +++ .../tree_borrows/cell-lazy-write-to-surrounding.rs | 10 +- 6 files changed, 98 insertions(+), 110 deletions(-) create mode 100644 src/tools/miri/tests/fail/tree_borrows/frozen-lazy-write-to-surrounding.rs create mode 100644 src/tools/miri/tests/fail/tree_borrows/frozen-lazy-write-to-surrounding.stderr (limited to 'src') 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 ad2a67160f4..a3650448628 100644 --- a/src/tools/miri/src/borrow_tracker/tree_borrows/mod.rs +++ b/src/tools/miri/src/borrow_tracker/tree_borrows/mod.rs @@ -125,81 +125,64 @@ pub struct NewPermission { /// Whether a read access should be performed on the non-frozen /// part on a retag. nonfreeze_access: bool, + /// Permission for memory outside the range. + outside_perm: Permission, /// Whether this pointer is part of the arguments of a function call. /// `protector` is `Some(_)` for all pointers marked `noalias`. protector: Option, } impl<'tcx> NewPermission { - /// Determine NewPermission of the reference from the type of the pointee. - fn from_ref_ty( + /// Determine NewPermission of the reference/Box from the type of the pointee. + /// + /// A `ref_mutability` of `None` indicates a `Box` type. + fn new( pointee: Ty<'tcx>, - mutability: Mutability, - kind: RetagKind, + ref_mutability: Option, + retag_kind: RetagKind, cx: &crate::MiriInterpCx<'tcx>, ) -> Option { let ty_is_unpin = pointee.is_unpin(*cx.tcx, cx.typing_env()); - let is_protected = kind == RetagKind::FnEntry; - let protector = is_protected.then_some(ProtectorKind::StrongProtector); - - Some(match mutability { - Mutability::Mut if ty_is_unpin => - 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 ty_is_freeze = pointee.is_freeze(*cx.tcx, cx.typing_env()); + let is_protected = retag_kind == RetagKind::FnEntry; - /// Compute permission for `Box`-like type (`Box` always, and also `Unique` if enabled). - /// These pointers allow deallocation so need a different kind of protector not handled - /// by `from_ref_ty`. - fn from_unique_ty( - ty: Ty<'tcx>, - kind: RetagKind, - cx: &crate::MiriInterpCx<'tcx>, - ) -> Option { - let pointee = ty.builtin_deref(true).unwrap(); - 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 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, - } + if matches!(ref_mutability, Some(Mutability::Mut) | None if !ty_is_unpin) { + // Mutable reference / Box to pinning type: retagging is a NOP. + // FIXME: with `UnsafePinned`, this should do proper per-byte tracking. + return None; + } + + let freeze_perm = match ref_mutability { + // Shared references are frozen. + Some(Mutability::Not) => Permission::new_frozen(), + // Mutable references and Boxes are reserved. + _ => Permission::new_reserved_frz(), + }; + let nonfreeze_perm = match ref_mutability { + // Shared references are "transparent". + Some(Mutability::Not) => Permission::new_cell(), + // *Protected* mutable references and boxes are reserved without regarding for interior mutability. + _ if is_protected => Permission::new_reserved_frz(), + // Unprotected mutable references and boxes start in `ReservedIm`. + _ => Permission::new_reserved_im(), + }; + + // Everything except for `Cell` gets an initial access. + let initial_access = |perm: &Permission| !perm.is_cell(); + + Some(NewPermission { + freeze_perm, + freeze_access: initial_access(&freeze_perm), + nonfreeze_perm, + nonfreeze_access: initial_access(&nonfreeze_perm), + outside_perm: if ty_is_freeze { freeze_perm } else { nonfreeze_perm }, + protector: is_protected.then_some(if ref_mutability.is_some() { + // Strong protector for references + ProtectorKind::StrongProtector + } else { + // Weak protector for boxes + ProtectorKind::WeakProtector + }), }) } } @@ -313,15 +296,11 @@ trait EvalContextPrivExt<'tcx>: crate::MiriInterpCxExt<'tcx> { let span = this.machine.current_span(); - // Store initial permissions and their corresponding range. + // Store initial permissions for the "inside" part. let mut perms_map: DedupRangeMap = DedupRangeMap::new( ptr_size, 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 - // 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 @@ -350,14 +329,13 @@ trait EvalContextPrivExt<'tcx>: crate::MiriInterpCxExt<'tcx> { .get_tree_borrows_params() .precise_interior_mut; - let default_perm = if !precise_interior_mut { - // NOTE: Using `ty_is_freeze` doesn't give the same result as going through the range - // and computing `has_unsafe_cell`. This is because of zero-sized `UnsafeCell`, for which - // `has_unsafe_cell` is false, but `!ty_is_freeze` is true. + // Set "inside" permissions. + if !precise_interior_mut { let ty_is_freeze = place.layout.ty.is_freeze(*this.tcx, this.typing_env()); let (perm, access) = if ty_is_freeze { (new_perm.freeze_perm, new_perm.freeze_access) } else { + // Just pretend the entire thing is an `UnsafeCell`. (new_perm.nonfreeze_perm, new_perm.nonfreeze_access) }; let sifa = perm.strongest_idempotent_foreign_access(protected); @@ -370,12 +348,8 @@ trait EvalContextPrivExt<'tcx>: crate::MiriInterpCxExt<'tcx> { for (_loc_range, loc) in perms_map.iter_mut_all() { *loc = new_loc; } - - perm } else { 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) @@ -401,9 +375,6 @@ trait EvalContextPrivExt<'tcx>: crate::MiriInterpCxExt<'tcx> { interp_ok(()) })?; - - // Allow lazily writing to surrounding data if we found an `UnsafeCell`. - if has_unsafe_cell { new_perm.nonfreeze_perm } else { new_perm.freeze_perm } }; let alloc_extra = this.get_alloc_extra(alloc_id)?; @@ -447,7 +418,7 @@ trait EvalContextPrivExt<'tcx>: crate::MiriInterpCxExt<'tcx> { orig_tag, new_tag, perms_map, - default_perm, + new_perm.outside_perm, protected, span, )?; @@ -514,7 +485,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { let this = self.eval_context_mut(); let new_perm = match val.layout.ty.kind() { &ty::Ref(_, pointee, mutability) => - NewPermission::from_ref_ty(pointee, mutability, kind, this), + NewPermission::new(pointee, Some(mutability), kind, this), _ => None, }; if let Some(new_perm) = new_perm { @@ -571,8 +542,9 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { fn visit_box(&mut self, box_ty: Ty<'tcx>, place: &PlaceTy<'tcx>) -> InterpResult<'tcx> { // Only boxes for the global allocator get any special treatment. if box_ty.is_box_global(*self.ecx.tcx) { + let pointee = place.layout.ty.builtin_deref(true).unwrap(); let new_perm = - NewPermission::from_unique_ty(place.layout.ty, self.kind, self.ecx); + NewPermission::new(pointee, /* not a ref */ None, self.kind, self.ecx); self.retag_ptr_inplace(place, new_perm)?; } interp_ok(()) @@ -591,7 +563,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { match place.layout.ty.kind() { &ty::Ref(_, pointee, mutability) => { let new_perm = - NewPermission::from_ref_ty(pointee, mutability, self.kind, self.ecx); + NewPermission::new(pointee, Some(mutability), self.kind, self.ecx); self.retag_ptr_inplace(place, new_perm)?; } ty::RawPtr(_, _) => { @@ -643,14 +615,11 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { // 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_perm: Permission::new_reserved_frz(), freeze_access: true, - nonfreeze_perm: Permission::new_reserved( - /* ty_is_freeze */ false, /* protected */ true, - ), + nonfreeze_perm: Permission::new_reserved_frz(), nonfreeze_access: true, + outside_perm: Permission::new_reserved_frz(), protector: Some(ProtectorKind::StrongProtector), }; 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 38863ca0734..390435e58d1 100644 --- a/src/tools/miri/src/borrow_tracker/tree_borrows/perms.rs +++ b/src/tools/miri/src/borrow_tracker/tree_borrows/perms.rs @@ -272,28 +272,15 @@ impl Permission { /// Default initial permission of a reborrowed mutable reference that is either /// protected or not interior mutable. - fn new_reserved_frz() -> Self { + pub fn new_reserved_frz() -> Self { Self { inner: ReservedFrz { conflicted: false } } } /// Default initial permission of an unprotected interior mutable reference. - fn new_reserved_im() -> Self { + pub fn new_reserved_im() -> Self { Self { inner: ReservedIM } } - /// Wrapper around `new_reserved_frz` and `new_reserved_im` that decides - /// which to call based on the interior mutability and the retag kind (whether there - /// 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() } - } - /// Default initial permission of a reborrowed shared reference. pub fn new_frozen() -> Self { Self { inner: Frozen } 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 bb3fc2d80b3..d9b3696e4f8 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 @@ -610,7 +610,7 @@ mod spurious_read { }, y: LocStateProt { state: LocationState::new_non_accessed( - Permission::new_reserved(/* freeze */ true, /* protected */ true), + Permission::new_reserved_frz(), IdempotentForeignAccess::default(), ), prot: true, diff --git a/src/tools/miri/tests/fail/tree_borrows/frozen-lazy-write-to-surrounding.rs b/src/tools/miri/tests/fail/tree_borrows/frozen-lazy-write-to-surrounding.rs new file mode 100644 index 00000000000..7d51050f32b --- /dev/null +++ b/src/tools/miri/tests/fail/tree_borrows/frozen-lazy-write-to-surrounding.rs @@ -0,0 +1,9 @@ +//@compile-flags: -Zmiri-tree-borrows + +fn main() { + // Since the "inside" part is `!Freeze`, the permission to mutate is gone. + let pair = ((), 1); + let x = &pair.0; + let ptr = (&raw const *x).cast::().cast_mut(); + unsafe { ptr.write(0) }; //~ERROR: /write access .* forbidden/ +} diff --git a/src/tools/miri/tests/fail/tree_borrows/frozen-lazy-write-to-surrounding.stderr b/src/tools/miri/tests/fail/tree_borrows/frozen-lazy-write-to-surrounding.stderr new file mode 100644 index 00000000000..e9800468c57 --- /dev/null +++ b/src/tools/miri/tests/fail/tree_borrows/frozen-lazy-write-to-surrounding.stderr @@ -0,0 +1,21 @@ +error: Undefined Behavior: write access through at ALLOC[0x0] is forbidden + --> tests/fail/tree_borrows/frozen-lazy-write-to-surrounding.rs:LL:CC + | +LL | unsafe { ptr.write(0) }; + | ^^^^^^^^^^^^ Undefined Behavior occurred here + | + = 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: see https://github.com/rust-lang/unsafe-code-guidelines/blob/master/wip/tree-borrows.md for further information + = help: the accessed tag has state Frozen which forbids this child write access +help: the accessed tag was created here, in the initial state Frozen + --> tests/fail/tree_borrows/frozen-lazy-write-to-surrounding.rs:LL:CC + | +LL | let x = &pair.0; + | ^^^^^^^ + = note: BACKTRACE (of the first span): + = note: inside `main` at tests/fail/tree_borrows/frozen-lazy-write-to-surrounding.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/tree_borrows/cell-lazy-write-to-surrounding.rs b/src/tools/miri/tests/pass/tree_borrows/cell-lazy-write-to-surrounding.rs index abe08f2cd22..7352784ac7a 100644 --- 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 @@ -14,9 +14,11 @@ fn main() { 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); + + // As long as the "inside" part is `!Freeze`, the permission to mutate the "outside" is preserved. + let pair = (Cell::new(()), 1); + let x = &pair.0; + let ptr = (&raw const *x).cast::().cast_mut(); + unsafe { ptr.write(0) }; } -- cgit 1.4.1-3-g733a5 From 416f988fafe11841095aab78c29a4a0101c12c84 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Sat, 23 Aug 2025 22:08:41 +0200 Subject: refactor tb_rebor: reduce code duplication --- .../miri/src/borrow_tracker/tree_borrows/mod.rs | 60 ++++++++-------------- 1 file changed, 22 insertions(+), 38 deletions(-) (limited to 'src') 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 a3650448628..04b951cd77c 100644 --- a/src/tools/miri/src/borrow_tracker/tree_borrows/mod.rs +++ b/src/tools/miri/src/borrow_tracker/tree_borrows/mod.rs @@ -296,12 +296,6 @@ trait EvalContextPrivExt<'tcx>: crate::MiriInterpCxExt<'tcx> { let span = this.machine.current_span(); - // Store initial permissions for the "inside" part. - let mut perms_map: DedupRangeMap = DedupRangeMap::new( - ptr_size, - LocationState::new_accessed(Permission::new_disabled(), IdempotentForeignAccess::None), // this will be overwritten - ); - // 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 @@ -329,58 +323,48 @@ trait EvalContextPrivExt<'tcx>: crate::MiriInterpCxExt<'tcx> { .get_tree_borrows_params() .precise_interior_mut; - // Set "inside" permissions. - if !precise_interior_mut { - let ty_is_freeze = place.layout.ty.is_freeze(*this.tcx, this.typing_env()); - let (perm, access) = if ty_is_freeze { + // Compute initial "inside" permissions. + let loc_state = |frozen: bool| -> LocationState { + let (perm, access) = if frozen { (new_perm.freeze_perm, new_perm.freeze_access) } else { - // Just pretend the entire thing is an `UnsafeCell`. (new_perm.nonfreeze_perm, new_perm.nonfreeze_access) }; let sifa = perm.strongest_idempotent_foreign_access(protected); - let new_loc = if access { + if access { LocationState::new_accessed(perm, sifa) } else { LocationState::new_non_accessed(perm, sifa) - }; - - for (_loc_range, loc) in perms_map.iter_mut_all() { - *loc = new_loc; } + }; + let perms_map = if !precise_interior_mut { + // For `!Freeze` types, just pretend the entire thing is an `UnsafeCell`. + let ty_is_freeze = place.layout.ty.is_freeze(*this.tcx, this.typing_env()); + let state = loc_state(ty_is_freeze); + DedupRangeMap::new(ptr_size, state) } else { + // The initial state will be overwritten by the visitor below. + let mut perms_map: DedupRangeMap = DedupRangeMap::new( + ptr_size, + LocationState::new_accessed( + Permission::new_disabled(), + IdempotentForeignAccess::None, + ), + ); this.visit_freeze_sensitive(place, ptr_size, |range, 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) - }; - 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_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. - let new_loc = if access { - LocationState::new_accessed(perm, sifa) - } else { - LocationState::new_non_accessed(perm, sifa) - }; - - // Store initial permissions. + let state = loc_state(frozen); for (_loc_range, loc) in perms_map.iter_mut(range.start, range.size) { - *loc = new_loc; + *loc = state; } - interp_ok(()) })?; + perms_map }; let alloc_extra = this.get_alloc_extra(alloc_id)?; let mut tree_borrows = alloc_extra.borrow_tracker_tb().borrow_mut(); - for (perm_range, perm) in perms_map.iter_mut_all() { + for (perm_range, perm) in perms_map.iter_all() { if perm.is_accessed() { // Some reborrows incur a read access to the parent. // Adjust range to be relative to allocation start (rather than to `place`). -- cgit 1.4.1-3-g733a5 From 8c2d2c07ff7af6e9d10e00833d8e82a483f2acae Mon Sep 17 00:00:00 2001 From: The Miri Cronjob Bot Date: Sun, 24 Aug 2025 04:54:24 +0000 Subject: Prepare for merging from rust-lang/rust This updates the rust-version file to f6d23413c399fb530be362ebcf25a4e788e16137. --- src/tools/miri/rust-version | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'src') diff --git a/src/tools/miri/rust-version b/src/tools/miri/rust-version index d0757e58bf9..3450f18334a 100644 --- a/src/tools/miri/rust-version +++ b/src/tools/miri/rust-version @@ -1 +1 @@ -8e3710ef31a0b2cdf5a1c2f3929b7735d1e28c20 +f6d23413c399fb530be362ebcf25a4e788e16137 -- cgit 1.4.1-3-g733a5 From 9e18b7b0e1b014833ec49a10fc3544d400ca1acc Mon Sep 17 00:00:00 2001 From: The Miri Cronjob Bot Date: Sun, 24 Aug 2025 05:02:39 +0000 Subject: fmt --- src/tools/miri/src/machine.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) (limited to 'src') diff --git a/src/tools/miri/src/machine.rs b/src/tools/miri/src/machine.rs index 0b2ce900414..0136de55216 100644 --- a/src/tools/miri/src/machine.rs +++ b/src/tools/miri/src/machine.rs @@ -1077,7 +1077,8 @@ impl<'tcx> Machine<'tcx> for MiriMachine<'tcx> { .target_features .iter() .filter(|&feature| { - feature.kind != TargetFeatureKind::Implied && !ecx.tcx.sess.target_features.contains(&feature.name) + feature.kind != TargetFeatureKind::Implied + && !ecx.tcx.sess.target_features.contains(&feature.name) }) .fold(String::new(), |mut s, feature| { if !s.is_empty() { -- cgit 1.4.1-3-g733a5 From 22ef90c8049cf8f07ce42e09f01ca791b829594e Mon Sep 17 00:00:00 2001 From: The Miri Cronjob Bot Date: Mon, 25 Aug 2025 04:55:35 +0000 Subject: Prepare for merging from rust-lang/rust This updates the rust-version file to a1dbb443527bd126452875eb5d5860c1d001d761. --- src/tools/miri/rust-version | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'src') diff --git a/src/tools/miri/rust-version b/src/tools/miri/rust-version index 3450f18334a..f412399cc8c 100644 --- a/src/tools/miri/rust-version +++ b/src/tools/miri/rust-version @@ -1 +1 @@ -f6d23413c399fb530be362ebcf25a4e788e16137 +a1dbb443527bd126452875eb5d5860c1d001d761 -- cgit 1.4.1-3-g733a5 From b08ebc5b204af82c1687f27047370209653c7983 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Mon, 25 Aug 2025 12:47:29 +0200 Subject: TB: fix SIFA comment --- .../miri/src/borrow_tracker/tree_borrows/mod.rs | 24 +++++++++++----------- 1 file changed, 12 insertions(+), 12 deletions(-) (limited to 'src') 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 04b951cd77c..bed65440dc9 100644 --- a/src/tools/miri/src/borrow_tracker/tree_borrows/mod.rs +++ b/src/tools/miri/src/borrow_tracker/tree_borrows/mod.rs @@ -298,18 +298,18 @@ trait EvalContextPrivExt<'tcx>: crate::MiriInterpCxExt<'tcx> { // 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). + // 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 the type is `!Freeze`, 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 via the assert below, and thus everything is taken care of. + // * If the type is `Freeze`, 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(); -- cgit 1.4.1-3-g733a5 From a10bdf93b153211e5daf1b1c147b955e690e30ef Mon Sep 17 00:00:00 2001 From: The Miri Cronjob Bot Date: Wed, 27 Aug 2025 04:53:08 +0000 Subject: Prepare for merging from rust-lang/rust This updates the rust-version file to 269d5b56bcfdf2be82213e72ef9a2e4c592a8c6b. --- src/tools/miri/rust-version | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'src') diff --git a/src/tools/miri/rust-version b/src/tools/miri/rust-version index f412399cc8c..5e833540002 100644 --- a/src/tools/miri/rust-version +++ b/src/tools/miri/rust-version @@ -1 +1 @@ -a1dbb443527bd126452875eb5d5860c1d001d761 +269d5b56bcfdf2be82213e72ef9a2e4c592a8c6b -- cgit 1.4.1-3-g733a5 From 8743a04b89dde55dd956014a5793f27a801efab5 Mon Sep 17 00:00:00 2001 From: Stypox Date: Fri, 22 Aug 2025 21:07:09 +0200 Subject: Add documentation for tracing --- .../miri/doc/img/perfetto_aggregate_statistics.png | Bin 0 -> 77886 bytes .../doc/img/perfetto_aggregate_statistics_sql.png | Bin 0 -> 181156 bytes src/tools/miri/doc/img/perfetto_span.png | Bin 0 -> 165716 bytes .../miri/doc/img/perfetto_subname_statistics.png | Bin 0 -> 102585 bytes src/tools/miri/doc/img/perfetto_timeline.png | Bin 0 -> 613708 bytes .../doc/img/perfetto_visualize_argument_values.png | Bin 0 -> 68014 bytes ...fetto_visualize_argument_values_misbehaving.png | Bin 0 -> 68856 bytes .../img/perfetto_visualize_argument_values_sql.png | Bin 0 -> 261657 bytes src/tools/miri/doc/tracing.md | 292 +++++++++++++++++++++ 9 files changed, 292 insertions(+) create mode 100644 src/tools/miri/doc/img/perfetto_aggregate_statistics.png create mode 100644 src/tools/miri/doc/img/perfetto_aggregate_statistics_sql.png create mode 100644 src/tools/miri/doc/img/perfetto_span.png create mode 100644 src/tools/miri/doc/img/perfetto_subname_statistics.png create mode 100644 src/tools/miri/doc/img/perfetto_timeline.png create mode 100644 src/tools/miri/doc/img/perfetto_visualize_argument_values.png create mode 100644 src/tools/miri/doc/img/perfetto_visualize_argument_values_misbehaving.png create mode 100644 src/tools/miri/doc/img/perfetto_visualize_argument_values_sql.png create mode 100644 src/tools/miri/doc/tracing.md (limited to 'src') diff --git a/src/tools/miri/doc/img/perfetto_aggregate_statistics.png b/src/tools/miri/doc/img/perfetto_aggregate_statistics.png new file mode 100644 index 00000000000..d4fd3826f47 Binary files /dev/null and b/src/tools/miri/doc/img/perfetto_aggregate_statistics.png differ diff --git a/src/tools/miri/doc/img/perfetto_aggregate_statistics_sql.png b/src/tools/miri/doc/img/perfetto_aggregate_statistics_sql.png new file mode 100644 index 00000000000..bda92d3885a Binary files /dev/null and b/src/tools/miri/doc/img/perfetto_aggregate_statistics_sql.png differ diff --git a/src/tools/miri/doc/img/perfetto_span.png b/src/tools/miri/doc/img/perfetto_span.png new file mode 100644 index 00000000000..1a7184f22ae Binary files /dev/null and b/src/tools/miri/doc/img/perfetto_span.png differ diff --git a/src/tools/miri/doc/img/perfetto_subname_statistics.png b/src/tools/miri/doc/img/perfetto_subname_statistics.png new file mode 100644 index 00000000000..8c86b07e925 Binary files /dev/null and b/src/tools/miri/doc/img/perfetto_subname_statistics.png differ diff --git a/src/tools/miri/doc/img/perfetto_timeline.png b/src/tools/miri/doc/img/perfetto_timeline.png new file mode 100644 index 00000000000..49f8a1fac1d Binary files /dev/null and b/src/tools/miri/doc/img/perfetto_timeline.png differ diff --git a/src/tools/miri/doc/img/perfetto_visualize_argument_values.png b/src/tools/miri/doc/img/perfetto_visualize_argument_values.png new file mode 100644 index 00000000000..1dcbacaf9cb Binary files /dev/null and b/src/tools/miri/doc/img/perfetto_visualize_argument_values.png differ diff --git a/src/tools/miri/doc/img/perfetto_visualize_argument_values_misbehaving.png b/src/tools/miri/doc/img/perfetto_visualize_argument_values_misbehaving.png new file mode 100644 index 00000000000..beeba8a4a3a Binary files /dev/null and b/src/tools/miri/doc/img/perfetto_visualize_argument_values_misbehaving.png differ diff --git a/src/tools/miri/doc/img/perfetto_visualize_argument_values_sql.png b/src/tools/miri/doc/img/perfetto_visualize_argument_values_sql.png new file mode 100644 index 00000000000..c7b163b0a57 Binary files /dev/null and b/src/tools/miri/doc/img/perfetto_visualize_argument_values_sql.png differ diff --git a/src/tools/miri/doc/tracing.md b/src/tools/miri/doc/tracing.md new file mode 100644 index 00000000000..d7114af947d --- /dev/null +++ b/src/tools/miri/doc/tracing.md @@ -0,0 +1,292 @@ +# Documentation for the tracing infrastructure in Miri + +Miri can be traced to understand how much time is spent in its various components (e.g. borrow tracker, data race checker, etc.). When tracing is enabled, running Miri will create a `.json` trace file that can be opened and analyzed in [Perfetto](https://ui.perfetto.dev/). For any questions regarding this documentation you may contact [Stypox](https://rust-lang.zulipchat.com/#narrow/dm/627563-Stypox) on Zulip. + +## Obtaining a trace file + +### From the Miri codebase + +All of the tracing functionality in Miri is gated by the `"tracing"` feature flag to ensure it does not create any overhead when unneeded. To compile Miri with this feature enabled, you can pass `--features=tracing` to `./miri`. Then, to make running Miri actually produce a trace file, you also need to set the `MIRI_TRACING` environment variable. For example: + +```sh +MIRI_TRACING=1 ./miri run --features=tracing ./tests/pass/hello.rs +``` + +### From the rustc codebase + +If you are building Miri from within the rustc tree, you need to enable the `"tracing"` feature by adding this line to `bootstrap.toml`: + +```toml +build.tool.miri.features = ["tracing"] +``` + +And then you could run the following: + +```sh +MIRI_TRACING=1 ./x.py run miri --stage 1 --args ./src/tools/miri/tests/pass/hello.rs +``` + +### The trace file + +After running Miri with tracing enabled you will get a `.json` trace file that contains a list of all events and spans that occurred throughout the execution. The file follows [this format](https://docs.google.com/document/d/1CvAClvFfyA5R-PhYUmn5OOQtYMH4h6I0nSsKchNAySU/preview). + +## Analyzing a trace file + +To analyze traces you can use [Perfetto UI](https://ui.perfetto.dev/), a trace analyzer made by Google that was originally a part of the Chrome browser. Just open Perfetto and drag and drop the `.json` file there. Official documentation for the controls in the UI can be found [here](https://perfetto.dev/docs/visualization/perfetto-ui). + +### The timeline + +You will see the boxes "Global Legacy Events" and "Process 1" on the left of the workspace: after clicking on either of them their timeline will expand and you will be able to zoom in and look at individual spans (and events). + +- "Process 1" contains tracing spans for the various components of Miri, all in a single timeline line (e.g. borrow tracker, data race checker, etc.) +- "Global Legacy Events" contains auxiliary spans on two separate lines that allow understanding what code is being executed at any point in time: + - "frame": what is the current stack frame in the interpreted program + - "step": what statement/terminator in the MIR of the interpreted program is being executed + +Spans are represented as colored boxes in the timeline, while instantaneous events are represented by tiny arrows. (Events exist because rustc and Miri also use the `tracing` crate for debug logging, and those logs turn into events in the trace.) + +![](./img/perfetto_timeline.png) + +### Span/event data + +You can click on a span or an event to get more information about it, including some arguments that were passed when the span/event was entered/fired. In the following screenshot you can see the details of a "layouting" span that was generated by the following line in Miri's code: + +```rust +let _trace = enter_trace_span!(M, layouting::fn_abi_of_instance, ?instance, ?extra_args); +``` + +![](./img/perfetto_span.png) + +### SQL tables + +Perfetto supports querying the span/event database using SQL queries (see the [docs](https://perfetto.dev/docs/analysis/perfetto-sql-syntax)). Just type `:` in the search bar at the top to enter SQL mode, and then you will be able to enter SQL queries there. The relevant SQL tables are: +- `slices`: contains all spans and events; events can be distinguished from spans since their `dur` is 0. Relevant columns are: + - `id`: a unique primary-key ID for the span (assigned by Perfetto, not present in the trace file) + - `ts` and `dur`: the beginning and duration of the span, in nanoseconds + - `name`: the name of the span + - `parent_id`: the parent span ID, or null if there is no parent (assigned by Perfetto based on the timing at which spans occur, i.e. two nested spans must be one the child of the other) + - `arg_set_id`: a foreign key into the table of arguments (1-to-N) +- `args`: contains all of the arguments of the various events/spans. Relevant columns are: + - `arg_set_id`: the key used to join the slices and args tables + - `key`: the name of the argument prepended with "args." + - `display_value`: the value of the argument + +Some useful queries are provided in the following sections. + +### Enhancing the timeline + +On the "Process 1" timeline line there are some spans with the same name, that are actually generated from different places in Miri's code. In those cases the span name indicates the component that was invoked (e.g. the data race checker), but not the specific function that was run. To inspect the specific function, we store a "subname" in an argument with the same name as the span, which unfortunately can be seen only after clicking on the span. + +To make it quicker to look at subnames, you can add a new timeline line that specifically shows the subnames for spans with a specific name. To do so: +1. select any span with the name you care about (call this name `$NAME`) +2. click on the dropdown highlighted in blue next on the argument with name `$NAME` (or `args.$NAME`) +3. click on "Visualize argument values" +4. a new timeline line will appear with only spans originally named `$NAME`, but now with the subname displayed instead + +The following screenshot shows the 4 steps for spans named "data_race": + +![](./img/perfetto_visualize_argument_values.png) + +### Visualizing which "frame" or "step" is being executed + +Unfortunately the instructions in [Enhancing the timeline](#enhancing-the-timeline) only work well with spans under "Process 1", but misbehave with spans under "Global Legacy Events" (see the screenshot below). This might be a bug in Perfetto, but nevertheless a workaround is available: + +1. click on the search bar at the top and write `:` to enter SQL mode +2. copy-paste the following SQL, replace "SPAN_NAME" at the end with either "frame" or "step" (i.e. one of the two span names under "Global Legacy Events"), and press Enter to execute it: + ```sql + select slices.id, ts, dur, track_id, category, args.string_value as name, depth, stack_id, parent_stack_id, parent_id, slices.arg_set_id, thread_ts, thread_instruction_count, thread_instruction_delta, cat, slice_id + from slices inner join args using (arg_set_id) + where args.key = "args." || name and name = "SPAN_NAME" + ``` +3. at the top-right of the box at the bottom, click on "Show debug track" +4. press on "Show" in the popup that just appeared +5. a new debug track will appear with the names of steps or frames + +What the SQL does is to select only spans with the name "SPAN_NAME" and keep all of the span fields untouched, except for the name which is replaced with the subname. As explained in [Enhancing the timeline](#enhancing-the-timeline), remember that the subname is stored in an argument with the same name as the span. + +![](./img/perfetto_visualize_argument_values_sql.png) + + + +### Compute aggregate statistics + +The simplest way to get aggregate statistics about a time range is to: + +1. select a time range by drag-clicking along a trace line +2. click on the "Current Selection" tab at the bottom if it's not already open +3. see various tables/visualizations of how much time is spent in each span by clicking on "Slices", "Pivot Table" or "Slice Flamegraph" + +Note that the numbers shown in the "Slices" and "Pivot Table" tabs also include nested spans, so they cannot be used to compute statistics such as "X% of time is spent in spans named Y" because two spans named Y might be nested and their duration would be counted twice. For such statistics use the method in [Compute aggregate statistics (enhanced)](#compute-aggregate-statistics-enhanced). + +![](./img/perfetto_aggregate_statistics.png) + +### Compute aggregate statistics (enhanced) + +The following (long but not complicated) query can be used to find out how much time is spent in spans (grouped by their name). Only spans without a parent are considered towards the computations (see `where parent_id is null`): so for example if `validate_operand` in turn calls `layouting` (which generates a nested/child span), only the `validate_operand` statistics are increased. This query also excludes auxiliary spans (see `name != "frame" and name != "step"`). + +Note that this query does not allow selecting a time range, but that can be done by adding a condition, e.g. `ts + dur > MIN_T and ts < MAX_T` would match only spans that intersect the range `(MIN_T, MAX_T)`. Remember that the time unit is nanoseconds. + +```sql +select "TOTAL PROGRAM DURATION" as name, count(*), max(ts + dur) as "sum(dur)", 100.0 as "%", null as "min(dur)", null as "max(dur)", null as "avg(dur)", null as "stddev(dur)" +from slices + +union select "TOTAL OVER ALL SPANS (excluding events)" as name, count(*), sum(dur), cast(cast(sum(dur) as float) / (select max(ts + dur) from slices) * 1000 as int) / 10.0 as "%", min(dur), max(dur), cast(avg(dur) as int) as "avg(dur)", cast(sqrt(avg(dur*dur)-avg(dur)*avg(dur)) as int) as "stddev(dur)" +from slices +where parent_id is null and name != "frame" and name != "step" and dur > 0 + +union select name, count(*), sum(dur), cast(cast(sum(dur) as float) / (select max(ts + dur) from slices) * 1000 as int) / 10.0 as "%", min(dur), max(dur), cast(avg(dur) as int) as "avg(dur)", cast(sqrt(avg(dur*dur)-avg(dur)*avg(dur)) as int) as "stddev(dur)" +from slices +where parent_id is null and name != "frame" and name != "step" +group by name +order by sum(dur) desc, count(*) desc +``` + +This is the kind of table you would get out: + +![](./img/perfetto_aggregate_statistics_sql.png) + +### Statistics about subnames of a span + +Use the following SQL to see statistics about the subnames of spans with the same name (replace "SPAN_NAME" with the name of the span you want to see subname statistics of): + +```sql +select args.string_value as name, count(*), sum(dur), min(dur), max(dur), cast(avg(dur) as int) as "avg(dur)", cast(sqrt(avg(dur*dur)-avg(dur)*avg(dur)) as int) as "stddev(dur)" +from slices inner join args using (arg_set_id) +where args.key = "args." || name and name = "SPAN_NAME" +group by args.string_value +order by count(*) desc +``` + +For example, this is the table of how much time is spent in each borrow tracker function: + +![](./img/perfetto_subname_statistics.png) + +### Finding long periods of time without any tracing + +The following SQL finds the longest periods of time where time is being spent, with the ability to click on IDs in the table of results to quickly reach the corresponding place. This can be useful to spot things that use up a significant amount of time but that are not yet covered by tracing calls. + +```sql +with ordered as ( + select s1.*, row_number() over (order by s1.ts) as rn + from slices as s1 + where s1.parent_id is null and s1.dur > 0 and s1.name != "frame" and s1.name != "step" +) +select a.ts+a.dur as ts, b.ts-a.ts-a.dur as dur, a.id, a.track_id, a.category, a.depth, a.stack_id, a.parent_stack_id, a.parent_id, a.arg_set_id, a.thread_ts, a.thread_instruction_count, a.thread_instruction_delta, a.cat, a.slice_id, "empty" as name +from ordered as a inner join ordered as b on a.rn=b.rn-1 +order by b.ts-a.ts-a.dur desc +``` + +### Saving Perfetto's state as a preset + +Unfortunately Perfetto does not seem to support saving the UI state as a preset that can be used to repeat the same analysis on multiple traces. You have to click through the various menus or run the various SQL queries every time to setup the UI as you want. + +## Adding new tracing calls to the code + +### The "tracing" feature + +Miri is highly interconnected with `rustc_const_eval`, and therefore collecting proper trace data about Miri also involves adding some tracing calls within `rustc_const_eval`'s codebase. As explained in [Obtaining a trace file](#obtaining-a-trace-file), tracing calls are disabled (and optimized out) when Miri's "tracing" feature is not enabled. However, while it is possible to check for the feature from Miri's codebase, it's not possible to do so from `rustc_const_eval` (since it's a separate crate, and it's even in a precompiled `.rlib` in case of out-of-tree builds). + +The solution to make it possible to check whether tracing is enabled at compile time even in `rustc_const_eval` was to add a function with this signature to the `Machine` trait: +```rust +fn enter_trace_span(span: impl FnOnce() -> tracing::Span) -> impl EnteredTraceSpan +``` + +where `EnteredTraceSpan` is just a marker trait implemented by `()` and `tracing::span::EnteredSpan`. This function returns `()` by default (without calling the `span` closure), except in `MiriMachine` where if tracing is enabled it will return `span().entered()`. + +The code in `rustc_const_eval` calls this function when it wants to do tracing, and the compiler will (hopefully) optimize out tracing calls when tracing is disabled. + +### The `enter_trace_span!()` macro + +To add tracing to a section of code in Miri or in `rustc_const_eval`, you can use the `enter_trace_span!()` macro, which takes care of the details explained in [The "tracing" feature](#the-tracing-feature). + +The `enter_trace_span!()` macro accepts the same syntax as `tracing::span!()` ([documentation](https://docs.rs/tracing/latest/tracing/#using-the-macros)) except for a few customizations, and returns an already entered trace span. The returned value is a drop guard that will exit the span when dropped, so **make sure to give it a proper scope** by storing it in a variable like this: + +```rust +let _trace = enter_trace_span!("My span"); +``` + +When calling this macro from `rustc_const_eval` you need to pass a type implementing the `Machine` trait as the first argument (since it will be used to call `Machine::enter_trace_span()`). This is usually available in various parts of `rustc_const_eval` under the name `M`, since most of `rustc_const_eval`'s code is `Machine`-agnostic. + +```rust +let _trace = enter_trace_span!("My span"); // from Miri +let _trace = enter_trace_span!(M, "My span"); // from rustc_const_eval +``` + +You can make sense of the syntaxes explained below also by looking at this Perfetto screenshot from [Span/event data](#spanevent-data). + +![](./img/perfetto_span.png) + +### Syntax accepted by `tracing::span!()` + +The full documentation for the `tracing::span!()` syntax can be found [here](https://docs.rs/tracing/latest/tracing/#using-the-macros) under "Using the Macros". A few possibly confusing syntaxes are listed here: +```rust +// logs a span named "hello" with a field named "arg" of value 42 (works only because +// 42 implements the tracing::Value trait, otherwise use one of the options below) +let _trace = enter_trace_span!(M, "hello", arg = 42); +// logs a field called "my_display_var" using the Display implementation +let _trace = enter_trace_span!(M, "hello", %my_display_var); +// logs a field called "my_debug_var" using the Debug implementation +let _trace = enter_trace_span!(M, "hello", ?my_debug_var); +``` + +### `NAME::SUBNAME` syntax + +In addition to the syntax accepted by `tracing::span!()`, the `enter_trace_span!()` macro optionally allows passing the span name (i.e. the first macro argument) in the form `NAME::SUBNAME` (without quotes) to indicate that the span has name "NAME" (usually the name of the component) and has an additional more specific name "SUBNAME" (usually the function name). The latter is passed to the tracing crate as a span field with the name "NAME". This allows not being distracted by subnames when looking at the trace in Perfetto, but when deeper introspection is needed within a component, it's still possible to view the subnames directly with a few steps (see [Enhancing the timeline](#enhancing-the-timeline)). +```rust +// for example, the first will expand to the second +let _trace = enter_trace_span!(M, borrow_tracker::on_stack_pop); +let _trace = enter_trace_span!(M, "borrow_tracker", borrow_tracker = "on_stack_pop"); +``` + +### `tracing_separate_thread` parameter + +Miri saves traces using the the `tracing_chrome` `tracing::Layer` so that they can be visualized in Perfetto. To instruct `tracing_chrome` to put some spans on a separate trace thread/line than other spans when viewed in Perfetto, you can pass `tracing_separate_thread = tracing::field::Empty` to the tracing macros. This is useful to separate out spans which just indicate the current step or program frame being processed by the interpreter. As explained in [The timeline](#the-timeline), those spans end up under the "Global Legacy Events" track. You should use a value of `tracing::field::Empty` so that other tracing layers (e.g. the logger) will ignore the `tracing_separate_thread` field. For example: +```rust +let _trace = enter_trace_span!(M, step::eval_statement, tracing_separate_thread = tracing::field::Empty); +``` + +### Executing something else when tracing is disabled + +The `EnteredTraceSpan` trait contains a `or_if_tracing_disabled()` function that you can use to e.g. log a line as an alternative to the tracing span for when tracing is disabled. For example: +```rust +let _trace = enter_trace_span!(M, step::eval_statement) + .or_if_tracing_disabled(|| tracing::info!("eval_statement")); +``` + +## Implementation details + +Here we explain how tracing is implemented internally. + +The events and spans generated throughout the codebase are collected by [the `tracing` crate](https://crates.io/crates/tracing), which then dispatches them to the code that writes to the trace file, but also to the logger if logging is enabled. + +### Choice of tracing library + +The crate that was chosen for collecting traces is [tracing](https://crates.io/crates/tracing), since: +- it is very well maintained +- it supports various different trace formats through plug-and-play `Layer`s (in Miri we are using `tracing_chrome` to export traces for perfetto, see [The `tracing_chrome` layer](#the-tracing_chrome-layer)) +- spans and events are collected with not just their name, but also file, line, module, and any number of custom arguments +- it was already used in Miri and rustc as a logging framework + +One major drawback of the tracing crate is, however, its big overhead. Entering and exiting a span takes on the order of 100ns, and many of Miri's spans are shorter than that, so their measurements are completely off and the program execution increases significantly. E.g. at the point of writing this documentation, enabling tracing makes Miri 5x slower. Note that this used to be even worse, see [Time measurements](#time-measurements). + +### The `tracing_chrome` layer + +Miri uses [tracing-chrome](https://github.com/thoren-d/tracing-chrome) as the `Layer` that collects spans and events from the tracing crate and saves them to a file that can be opened in Perfetto. Although the crate [is published](https://crates.io/crates/tracing-chrome) on crates.io, it was not possible to depend on it from Miri, because it would bring in a separate compilation of the `tracing` crate. This is because Miri does not directly depend on `tracing`, and instead uses rustc's version through rustc-private, and apparently cargo can't realize that the same library is being built again when rustc-private is involved. + +So the solution was to copy-paste [the only file](https://github.com/thoren-d/tracing-chrome/blob/develop/src/lib.rs) in tracing-chrome into Miri. Nevertheless, this gave the possibility to make some changes to tracing-chrome, which you can read about in documentation at the top of [the file](https://github.com/rust-lang/miri/blob/master/src/bin/log/tracing_chrome.rs) that was copied to Miri. + +### Time measurements + +tracing-chrome originally used `std::time::Instant` to measure time, however on some x86/x86_64 Linux systems it might be unbearably slow since the underlying system call (`clock_gettime`) would take ≈1.3µs. Read more [here](https://btorpey.github.io/blog/2014/02/18/clock-sources-in-linux/) about how the Linux kernel chooses the clock source. + +Therefore, on x86/x86_64 Linux systems with a CPU that has an invariant TSC counter, we read from that instead to measure time, which takes only ≈13ns. There are unfortunately a lot of caveats to this approach though, as explained [in the code](https://github.com/rust-lang/miri/blob/master/src/bin/log/tracing_chrome_instant.rs) and [in the PR](https://github.com/rust-lang/miri/pull/4524). The most impactful one is that: every thread spawned in Miri that wants to trace something (which requires measuring time) needs to pin itself to a single CPU core (using `sched_setaffinity`). + +## Other useful stuff + +### Making a flamegraph + +After compiling Miri, you can run the following command to make a flamegraph using Linux' `perf`. It can be useful to spot functions that use up a significant amount of time but that are not yet covered by tracing calls. + +```sh +perf record --call-graph dwarf -F 999 ./miri/target/debug/miri --edition 2021 --sysroot ~/.cache/miri ./tests/pass/hashmap.rs && perf script | inferno-collapse-perf | inferno-flamegraph > flamegraph.svg +``` -- cgit 1.4.1-3-g733a5 From 01ed1052d8d243b6e022ed9c26c386752b178779 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Thu, 28 Aug 2025 14:58:44 +0200 Subject: unix read/write: fix zero-size handling --- src/tools/miri/src/shims/unix/fd.rs | 28 ++++++++++++++++++++++++---- src/tools/miri/tests/pass/shims/fs.rs | 4 +++- src/tools/miri/tests/utils/libc.rs | 5 +---- 3 files changed, 28 insertions(+), 9 deletions(-) (limited to 'src') diff --git a/src/tools/miri/src/shims/unix/fd.rs b/src/tools/miri/src/shims/unix/fd.rs index e226a55d8b1..9fbecffc55d 100644 --- a/src/tools/miri/src/shims/unix/fd.rs +++ b/src/tools/miri/src/shims/unix/fd.rs @@ -264,11 +264,19 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { return this.set_last_error_and_return(LibcError("EBADF"), dest); }; + // Handle the zero-sized case. The man page says: + // > If count is zero, read() may detect the errors described below. In the absence of any + // > errors, or if read() does not check for errors, a read() with a count of 0 returns zero + // > and has no other effects. + if count == 0 { + this.write_null(dest)?; + return interp_ok(()); + } // Non-deterministically decide to further reduce the count, simulating a partial read (but - // never to 0, that has different behavior). + // never to 0, that would indicate EOF). let count = if fd.nondet_short_accesses() && count >= 2 && this.machine.rng.get_mut().random() { - count / 2 + count / 2 // since `count` is at least 2, the result is still at least 1 } else { count }; @@ -338,8 +346,20 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { return this.set_last_error_and_return(LibcError("EBADF"), dest); }; - // Non-deterministically decide to further reduce the count, simulating a partial write (but - // never to 0, that has different behavior). + // Handle the zero-sized case. The man page says: + // > If count is zero and fd refers to a regular file, then write() may return a failure + // > status if one of the errors below is detected. If no errors are detected, or error + // > detection is not performed, 0 is returned without causing any other effect. If count + // > is zero and fd refers to a file other than a regular file, the results are not + // > specified. + if count == 0 { + // For now let's not open the can of worms of what exactly "not specified" could mean... + this.write_null(dest)?; + return interp_ok(()); + } + // Non-deterministically decide to further reduce the count, simulating a partial write. + // We avoid reducing the write size to 0: the docs seem to be entirely fine with that, + // but the standard library is not (https://github.com/rust-lang/rust/issues/145959). let count = if fd.nondet_short_accesses() && count >= 2 && this.machine.rng.get_mut().random() { count / 2 diff --git a/src/tools/miri/tests/pass/shims/fs.rs b/src/tools/miri/tests/pass/shims/fs.rs index e7f11c54704..022dcc5dcba 100644 --- a/src/tools/miri/tests/pass/shims/fs.rs +++ b/src/tools/miri/tests/pass/shims/fs.rs @@ -72,7 +72,9 @@ fn test_file() { // Writing to a file opened for reading should error (and not stop interpretation). std does not // categorize the error so we don't check for details. - file.write(&[]).unwrap_err(); + file.write(&[0]).unwrap_err(); + // However, writing 0 bytes can succeed or fail. + let _ignore = file.write(&[]); // Removing file should succeed. remove_file(&path).unwrap(); diff --git a/src/tools/miri/tests/utils/libc.rs b/src/tools/miri/tests/utils/libc.rs index 1a3cd067c04..4757a5a268c 100644 --- a/src/tools/miri/tests/utils/libc.rs +++ b/src/tools/miri/tests/utils/libc.rs @@ -34,10 +34,7 @@ pub unsafe fn write_all( if res < 0 { return res; } - if res == 0 { - // EOF? - break; - } + // Apparently a return value of 0 is just a short write, nothing special (unlike reads). written_so_far += res as libc::size_t; } return written_so_far as libc::ssize_t; -- cgit 1.4.1-3-g733a5 From 497df6006d55b3440220eed50ddccc054b431770 Mon Sep 17 00:00:00 2001 From: Rune Tynan Date: Mon, 2 Jun 2025 11:02:00 -0700 Subject: Add duplicate handle test + make null lpTargetHandle an abort, not an unsupported. --- src/tools/miri/src/shims/windows/handle.rs | 8 ++-- src/tools/miri/tests/pass-dep/shims/windows-fs.rs | 47 ++++++++++++++++++++--- 2 files changed, 46 insertions(+), 9 deletions(-) (limited to 'src') diff --git a/src/tools/miri/src/shims/windows/handle.rs b/src/tools/miri/src/shims/windows/handle.rs index 1e30bf25ed9..2ef328f7add 100644 --- a/src/tools/miri/src/shims/windows/handle.rs +++ b/src/tools/miri/src/shims/windows/handle.rs @@ -285,9 +285,11 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { } if this.ptr_is_null(target_handle_ptr)? { - throw_unsup_format!( - "`DuplicateHandle` `lpTargetHandle` parameter is null, which is unsupported" - ); + throw_machine_stop!(TerminationInfo::Abort( + "`DuplicateHandle` `lpTargetHandle` parameter must not be null, as otherwise the \ + newly created handle is leaked" + .to_string() + )); } if options != this.eval_windows("c", "DUPLICATE_SAME_ACCESS") { diff --git a/src/tools/miri/tests/pass-dep/shims/windows-fs.rs b/src/tools/miri/tests/pass-dep/shims/windows-fs.rs index 4ca19046b67..7b756603d92 100644 --- a/src/tools/miri/tests/pass-dep/shims/windows-fs.rs +++ b/src/tools/miri/tests/pass-dep/shims/windows-fs.rs @@ -2,20 +2,20 @@ //@compile-flags: -Zmiri-disable-isolation #![allow(nonstandard_style)] -use std::io::{ErrorKind, Read, Write}; +use std::io::{ErrorKind, Read, Seek, SeekFrom, Write}; use std::os::windows::ffi::OsStrExt; -use std::os::windows::io::AsRawHandle; +use std::os::windows::io::{AsRawHandle, FromRawHandle}; use std::path::Path; -use std::{fs, ptr}; +use std::{fs, mem, ptr}; #[path = "../../utils/mod.rs"] mod utils; use windows_sys::Wdk::Storage::FileSystem::{NtReadFile, NtWriteFile}; use windows_sys::Win32::Foundation::{ - CloseHandle, ERROR_ACCESS_DENIED, ERROR_ALREADY_EXISTS, ERROR_IO_DEVICE, GENERIC_READ, - GENERIC_WRITE, GetLastError, RtlNtStatusToDosError, STATUS_ACCESS_DENIED, - STATUS_IO_DEVICE_ERROR, STATUS_SUCCESS, SetLastError, + CloseHandle, DUPLICATE_SAME_ACCESS, DuplicateHandle, ERROR_ACCESS_DENIED, ERROR_ALREADY_EXISTS, + ERROR_IO_DEVICE, FALSE, GENERIC_READ, GENERIC_WRITE, GetLastError, RtlNtStatusToDosError, + STATUS_ACCESS_DENIED, STATUS_IO_DEVICE_ERROR, STATUS_SUCCESS, SetLastError, }; use windows_sys::Win32::Storage::FileSystem::{ BY_HANDLE_FILE_INFORMATION, CREATE_ALWAYS, CREATE_NEW, CreateFileW, DeleteFileW, @@ -24,6 +24,7 @@ use windows_sys::Win32::Storage::FileSystem::{ FILE_SHARE_WRITE, GetFileInformationByHandle, OPEN_ALWAYS, OPEN_EXISTING, SetFilePointerEx, }; use windows_sys::Win32::System::IO::IO_STATUS_BLOCK; +use windows_sys::Win32::System::Threading::GetCurrentProcess; fn main() { unsafe { @@ -36,6 +37,7 @@ fn main() { test_ntstatus_to_dos(); test_file_read_write(); test_file_seek(); + test_dup_handle(); } } @@ -273,6 +275,39 @@ unsafe fn test_file_read_write() { assert_eq!(GetLastError(), 1234); } +unsafe fn test_dup_handle() { + let temp = utils::tmp().join("test_dup.txt"); + + let mut file1 = fs::File::options().read(true).write(true).create(true).open(&temp).unwrap(); + + file1.write_all(b"Hello, World!\n").unwrap(); + file1.seek(SeekFrom::Start(0)).unwrap(); + + let first_handle = file1.as_raw_handle(); + + let cur_proc = GetCurrentProcess(); + let mut second_handle = mem::zeroed(); + let res = DuplicateHandle( + cur_proc, + first_handle, + cur_proc, + &mut second_handle, + 0, + FALSE, + DUPLICATE_SAME_ACCESS, + ); + assert!(res != 0); + + let mut buf1 = [0; 5]; + file1.read(&mut buf1).unwrap(); + assert_eq!(&buf1, b"Hello"); + + let mut file2 = fs::File::from_raw_handle(second_handle); + let mut buf2 = [0; 5]; + file2.read(&mut buf2).unwrap(); + assert_eq!(&buf2, b", Wor"); +} + unsafe fn test_file_seek() { let temp = utils::tmp().join("test_file_seek.txt"); let mut file = fs::File::options().create(true).write(true).read(true).open(&temp).unwrap(); -- cgit 1.4.1-3-g733a5 From 615450e1ac5b9cbae08e211bc01ca0068648c5ef Mon Sep 17 00:00:00 2001 From: "dependabot[bot]" <49699333+dependabot[bot]@users.noreply.github.com> Date: Fri, 29 Aug 2025 20:57:03 +0000 Subject: Bump tracing-subscriber from 0.3.19 to 0.3.20 Bumps [tracing-subscriber](https://github.com/tokio-rs/tracing) from 0.3.19 to 0.3.20. - [Release notes](https://github.com/tokio-rs/tracing/releases) - [Commits](https://github.com/tokio-rs/tracing/compare/tracing-subscriber-0.3.19...tracing-subscriber-0.3.20) --- updated-dependencies: - dependency-name: tracing-subscriber dependency-version: 0.3.20 dependency-type: indirect ... Signed-off-by: dependabot[bot] --- src/tools/miri/Cargo.lock | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) (limited to 'src') diff --git a/src/tools/miri/Cargo.lock b/src/tools/miri/Cargo.lock index b46f0f83420..4df17c83c7e 100644 --- a/src/tools/miri/Cargo.lock +++ b/src/tools/miri/Cargo.lock @@ -1569,9 +1569,9 @@ dependencies = [ [[package]] name = "tracing-subscriber" -version = "0.3.19" +version = "0.3.20" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "e8189decb5ac0fa7bc8b96b7cb9b2701d60d48805aca84a238004d665fcc4008" +checksum = "2054a14f5307d601f88daf0553e1cbf472acc4f2c51afab632431cdcd72124d5" dependencies = [ "sharded-slab", "thread_local", -- cgit 1.4.1-3-g733a5 From 435de36225382f763cc7ce6d744365c75c318596 Mon Sep 17 00:00:00 2001 From: The Miri Cronjob Bot Date: Sat, 30 Aug 2025 04:52:26 +0000 Subject: Prepare for merging from rust-lang/rust This updates the rust-version file to e004014d1bf4c29928a0f0f9f7d0964d43606cbd. --- src/tools/miri/rust-version | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'src') diff --git a/src/tools/miri/rust-version b/src/tools/miri/rust-version index 5e833540002..695959e2e68 100644 --- a/src/tools/miri/rust-version +++ b/src/tools/miri/rust-version @@ -1 +1 @@ -269d5b56bcfdf2be82213e72ef9a2e4c592a8c6b +e004014d1bf4c29928a0f0f9f7d0964d43606cbd -- cgit 1.4.1-3-g733a5 From f2ff4c2598609992d476e3d937233534fd137571 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Sat, 30 Aug 2025 14:29:17 +0200 Subject: native-lib mode: avoid unsoundness due to mrpotect --- src/tools/miri/src/shims/native_lib/trace/child.rs | 30 ++++++++++------------ 1 file changed, 13 insertions(+), 17 deletions(-) (limited to 'src') diff --git a/src/tools/miri/src/shims/native_lib/trace/child.rs b/src/tools/miri/src/shims/native_lib/trace/child.rs index b998ba822dd..95b0617a026 100644 --- a/src/tools/miri/src/shims/native_lib/trace/child.rs +++ b/src/tools/miri/src/shims/native_lib/trace/child.rs @@ -90,14 +90,6 @@ impl Supervisor { // Unwinding might be messed up due to partly protected memory, so let's abort if something // breaks inside here. let res = std::panic::abort_unwind(|| { - // SAFETY: We do not access machine memory past this point until the - // supervisor is ready to allow it. - // FIXME: this is sketchy, as technically the memory is still in the Rust Abstract Machine, - // and the compiler would be allowed to reorder accesses below this block... - unsafe { - Self::protect_pages(alloc.pages(), mman::ProtFlags::PROT_NONE).unwrap(); - } - // Send over the info. // NB: if we do not wait to receive a blank confirmation response, it is // possible that the supervisor is alerted of the SIGSTOP *before* it has @@ -110,16 +102,14 @@ impl Supervisor { // count. signal::raise(signal::SIGSTOP).unwrap(); - let res = f(); + // SAFETY: We have coordinated with the supervisor to ensure that this memory will keep + // working as normal, just with extra tracing. So even if the compiler moves memory + // accesses down to after the `mprotect`, they won't actually segfault. + unsafe { + Self::protect_pages(alloc.pages(), mman::ProtFlags::PROT_NONE).unwrap(); + } - // We can't use IPC channels here to signal that FFI mode has ended, - // since they might allocate memory which could get us stuck in a SIGTRAP - // with no easy way out! While this could be worked around, it is much - // simpler and more robust to simply use the signals which are left for - // arbitrary usage. Since this will block until we are continued by the - // supervisor, we can assume past this point that everything is back to - // normal. - signal::raise(signal::SIGUSR1).unwrap(); + let res = f(); // SAFETY: We set memory back to normal, so this is safe. unsafe { @@ -130,6 +120,12 @@ impl Supervisor { .unwrap(); } + // Signal the supervisor that we are done. Will block until the supervisor continues us. + // This will also shut down the segfault handler, so it's important that all memory is + // reset back to normal above. There must not be a window in time where accessing the + // pages we protected above actually causes the program to abort. + signal::raise(signal::SIGUSR1).unwrap(); + res }); -- cgit 1.4.1-3-g733a5 From 0308a15f64ffd03510a1456c6e492b87fef2097e Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Sat, 30 Aug 2025 14:50:53 +0200 Subject: reduce some code duplication and update some comments --- .../miri/src/shims/native_lib/trace/parent.rs | 30 ++++++---------------- 1 file changed, 8 insertions(+), 22 deletions(-) (limited to 'src') diff --git a/src/tools/miri/src/shims/native_lib/trace/parent.rs b/src/tools/miri/src/shims/native_lib/trace/parent.rs index 83f6c7a13fc..acb94395b57 100644 --- a/src/tools/miri/src/shims/native_lib/trace/parent.rs +++ b/src/tools/miri/src/shims/native_lib/trace/parent.rs @@ -132,10 +132,10 @@ impl Iterator for ChildListener { return Some(ExecEvent::Syscall(pid)); }, // Child with the given pid was stopped by the given signal. - // It's somewhat dubious when this is returned instead of - // WaitStatus::Stopped, but for our purposes they are the - // same thing. - wait::WaitStatus::PtraceEvent(pid, signal, _) => + // It's somewhat unclear when which of these two is returned; + // we just treat them the same. + wait::WaitStatus::Stopped(pid, signal) + | wait::WaitStatus::PtraceEvent(pid, signal, _) => if self.attached { // This is our end-of-FFI signal! if signal == signal::SIGUSR1 { @@ -148,19 +148,6 @@ impl Iterator for ChildListener { // Just pass along the signal. ptrace::cont(pid, signal).unwrap(); }, - // Child was stopped at the given signal. Same logic as for - // WaitStatus::PtraceEvent. - wait::WaitStatus::Stopped(pid, signal) => - if self.attached { - if signal == signal::SIGUSR1 { - self.attached = false; - return Some(ExecEvent::End); - } else { - return Some(ExecEvent::Status(pid, signal)); - } - } else { - ptrace::cont(pid, signal).unwrap(); - }, _ => (), }, // This case should only trigger when all children died. @@ -250,7 +237,7 @@ pub fn sv_loop( // We can't trust simply calling `Pid::this()` in the child process to give the right // PID for us, so we get it this way. curr_pid = wait_for_signal(None, signal::SIGSTOP, InitialCont::No).unwrap(); - + // Continue until next syscall. ptrace::syscall(curr_pid, None).unwrap(); } // Child wants to end tracing. @@ -289,8 +276,7 @@ pub fn sv_loop( } } }, - // Child entered a syscall; we wait for exits inside of this, so it - // should never trigger on return from a syscall we care about. + // Child entered or exited a syscall. For now we ignore this and just continue. ExecEvent::Syscall(pid) => { ptrace::syscall(pid, None).unwrap(); } @@ -344,8 +330,8 @@ fn wait_for_signal( return Err(ExecEnd(Some(code))); } wait::WaitStatus::Signaled(_, _, _) => return Err(ExecEnd(None)), - wait::WaitStatus::Stopped(pid, signal) => (signal, pid), - wait::WaitStatus::PtraceEvent(pid, signal, _) => (signal, pid), + wait::WaitStatus::Stopped(pid, signal) + | wait::WaitStatus::PtraceEvent(pid, signal, _) => (signal, pid), // This covers PtraceSyscall and variants that are impossible with // the flags set (e.g. WaitStatus::StillAlive). _ => { -- cgit 1.4.1-3-g733a5 From fbd8b96a1264cbb4eed65f2c2e8b86c882f4305e Mon Sep 17 00:00:00 2001 From: Nia Espera Date: Sun, 31 Aug 2025 14:00:13 +0200 Subject: native-lib: more resilient grabbing of instruction bytes Co-authored-by: Ralf Jung --- .../miri/src/shims/native_lib/trace/parent.rs | 52 +++++++++++----------- 1 file changed, 26 insertions(+), 26 deletions(-) (limited to 'src') diff --git a/src/tools/miri/src/shims/native_lib/trace/parent.rs b/src/tools/miri/src/shims/native_lib/trace/parent.rs index acb94395b57..3ae98259ab3 100644 --- a/src/tools/miri/src/shims/native_lib/trace/parent.rs +++ b/src/tools/miri/src/shims/native_lib/trace/parent.rs @@ -18,6 +18,11 @@ const ARCH_WORD_SIZE: usize = 4; #[cfg(target_arch = "x86_64")] const ARCH_WORD_SIZE: usize = 8; +// x86 max instruction length is 15 bytes: +// https://www.intel.com/content/www/us/en/developer/articles/technical/intel-sdm.html +// See vol. 3B section 24.25. +const ARCH_MAX_INSTR_SIZE: usize = 15; + /// The address of the page set to be edited, initialised to a sentinel null /// pointer. static PAGE_ADDR: AtomicPtr = AtomicPtr::new(std::ptr::null_mut()); @@ -472,7 +477,27 @@ fn handle_segfault( let stack_ptr = ch_stack.strict_add(CALLBACK_STACK_SIZE / 2); let regs_bak = ptrace::getregs(pid).unwrap(); let mut new_regs = regs_bak; - let ip_prestep = regs_bak.ip(); + + // Read at least one instruction from the ip. It's possible that the instruction + // that triggered the segfault was short and at the end of the mapped text area, + // so some of these reads may fail; in that case, just write empty bytes. If all + // reads failed, the disassembler will report an error. + let instr = (0..(ARCH_MAX_INSTR_SIZE.div_ceil(ARCH_WORD_SIZE))) + .flat_map(|ofs| { + // This reads one word of memory; we divided by `ARCH_WORD_SIZE` above to compensate for that. + ptrace::read( + pid, + std::ptr::without_provenance_mut( + regs_bak.ip().strict_add(ARCH_WORD_SIZE.strict_mul(ofs)), + ), + ) + .unwrap_or_default() + .to_ne_bytes() + }) + .collect::>(); + + // Now figure out the size + type of access and log it down. + capstone_disassemble(&instr, addr, cs, acc_events).expect("Failed to disassemble instruction"); // Move the instr ptr into the deprotection code. #[expect(clippy::as_conversions)] @@ -512,33 +537,8 @@ fn handle_segfault( ptrace::write(pid, std::ptr::with_exposed_provenance_mut(a), 0).unwrap(); } - // Save registers and grab the bytes that were executed. This would - // be really nasty if it was a jump or similar but those thankfully - // won't do memory accesses and so can't trigger this! let regs_bak = ptrace::getregs(pid).unwrap(); new_regs = regs_bak; - let ip_poststep = regs_bak.ip(); - - // Ensure that we've actually gone forwards. - assert!(ip_poststep > ip_prestep); - // But not by too much. 64 bytes should be "big enough" on ~any architecture. - assert!(ip_prestep.strict_add(64) > ip_poststep); - - // We need to do reads/writes in word-sized chunks. - let diff = (ip_poststep.strict_sub(ip_prestep)).div_ceil(ARCH_WORD_SIZE); - let instr = (ip_prestep..ip_prestep.strict_add(diff)).fold(vec![], |mut ret, ip| { - // This only needs to be a valid pointer in the child process, not ours. - ret.append( - &mut ptrace::read(pid, std::ptr::without_provenance_mut(ip)) - .unwrap() - .to_ne_bytes() - .to_vec(), - ); - ret - }); - - // Now figure out the size + type of access and log it down. - capstone_disassemble(&instr, addr, cs, acc_events).expect("Failed to disassemble instruction"); // Reprotect everything and continue. #[expect(clippy::as_conversions)] -- cgit 1.4.1-3-g733a5 From 4dbadd05f853414501401984e7888564df7291d4 Mon Sep 17 00:00:00 2001 From: Nia Espera Date: Sun, 13 Jul 2025 18:40:27 +0200 Subject: native-lib: pass structs to native code --- src/tools/miri/Cargo.toml | 2 +- src/tools/miri/src/shims/native_lib/ffi.rs | 54 ++++ src/tools/miri/src/shims/native_lib/mod.rs | 305 ++++++++++++--------- .../miri/tests/native-lib/aggregate_arguments.c | 42 +++ .../tests/native-lib/fail/multi_struct_alloc.rs | 21 ++ .../native-lib/fail/multi_struct_alloc.stderr | 15 + .../tests/native-lib/fail/struct_not_extern_c.rs | 19 ++ .../native-lib/fail/struct_not_extern_c.stderr | 14 + .../miri/tests/native-lib/fail/uninit_struct.rs | 27 ++ .../tests/native-lib/fail/uninit_struct.stderr | 15 + .../tests/native-lib/pass/aggregate_arguments.rs | 51 ++++ .../miri/tests/native-lib/pass/ptr_read_access.rs | 20 ++ src/tools/miri/tests/native-lib/ptr_read_access.c | 10 + src/tools/miri/tests/ui.rs | 1 + 14 files changed, 467 insertions(+), 129 deletions(-) create mode 100644 src/tools/miri/src/shims/native_lib/ffi.rs create mode 100644 src/tools/miri/tests/native-lib/aggregate_arguments.c create mode 100644 src/tools/miri/tests/native-lib/fail/multi_struct_alloc.rs create mode 100644 src/tools/miri/tests/native-lib/fail/multi_struct_alloc.stderr create mode 100644 src/tools/miri/tests/native-lib/fail/struct_not_extern_c.rs create mode 100644 src/tools/miri/tests/native-lib/fail/struct_not_extern_c.stderr create mode 100644 src/tools/miri/tests/native-lib/fail/uninit_struct.rs create mode 100644 src/tools/miri/tests/native-lib/fail/uninit_struct.stderr create mode 100644 src/tools/miri/tests/native-lib/pass/aggregate_arguments.rs (limited to 'src') diff --git a/src/tools/miri/Cargo.toml b/src/tools/miri/Cargo.toml index 99111092d39..924dfed2bca 100644 --- a/src/tools/miri/Cargo.toml +++ b/src/tools/miri/Cargo.toml @@ -39,7 +39,7 @@ features = ['unprefixed_malloc_on_supported_platforms'] [target.'cfg(unix)'.dependencies] libc = "0.2" # native-lib dependencies -libffi = { version = "4.0.0", optional = true } +libffi = { version = "4.1.1", optional = true } libloading = { version = "0.8", optional = true } serde = { version = "1.0.219", features = ["derive"], optional = true } diff --git a/src/tools/miri/src/shims/native_lib/ffi.rs b/src/tools/miri/src/shims/native_lib/ffi.rs new file mode 100644 index 00000000000..b2615cedaea --- /dev/null +++ b/src/tools/miri/src/shims/native_lib/ffi.rs @@ -0,0 +1,54 @@ +use libffi::low::CodePtr; +use libffi::middle::{Arg as ArgPtr, Cif, Type as FfiType}; + +/// Perform the actual FFI call. +/// +/// SAFETY: The safety invariants of the foreign function being called must be +/// upheld (if any). +pub unsafe fn call(fun: CodePtr, args: &mut [OwnedArg]) -> R { + let mut arg_tys = vec![]; + let mut arg_ptrs = vec![]; + for arg in args { + arg_tys.push(arg.take_ty()); + arg_ptrs.push(arg.ptr()) + } + let cif = Cif::new(arg_tys, R::reify().into_middle()); + // SAFETY: Caller upholds that the function is safe to call, and since we + // were passed a slice reference we know the `OwnedArg`s won't have been + // dropped by this point. + unsafe { cif.call(fun, &arg_ptrs) } +} + +/// An argument for an FFI call. +#[derive(Debug, Clone)] +pub struct OwnedArg { + /// The type descriptor for this argument. + ty: Option, + /// Corresponding bytes for the value. + bytes: Box<[u8]>, +} + +impl OwnedArg { + /// Instantiates an argument from a type descriptor and bytes. + pub fn new(ty: FfiType, bytes: Box<[u8]>) -> Self { + Self { ty: Some(ty), bytes } + } + + /// Gets the libffi type descriptor for this argument. Should only be + /// called once on a given `OwnedArg`. + fn take_ty(&mut self) -> FfiType { + self.ty.take().unwrap() + } + + /// Instantiates a libffi argument pointer pointing to this argument's bytes. + /// NB: Since `libffi::middle::Arg` ignores the lifetime of the reference + /// it's derived from, it is up to the caller to ensure the `OwnedArg` is + /// not dropped before unsafely calling `libffi::middle::Cif::call()`! + fn ptr(&self) -> ArgPtr { + // FIXME: Using `&self.bytes[0]` to reference the whole array is + // definitely unsound under SB, but we're waiting on + // https://github.com/libffi-rs/libffi-rs/commit/112a37b3b6ffb35bd75241fbcc580de40ba74a73 + // to land in a release so that we don't need to do this. + ArgPtr::new(&self.bytes[0]) + } +} diff --git a/src/tools/miri/src/shims/native_lib/mod.rs b/src/tools/miri/src/shims/native_lib/mod.rs index 74b9b704fea..156d6171c73 100644 --- a/src/tools/miri/src/shims/native_lib/mod.rs +++ b/src/tools/miri/src/shims/native_lib/mod.rs @@ -2,14 +2,15 @@ use std::ops::Deref; -use libffi::high::call as ffi; use libffi::low::CodePtr; -use rustc_abi::{BackendRepr, HasDataLayout, Size}; -use rustc_middle::mir::interpret::Pointer; -use rustc_middle::ty::{self as ty, IntTy, UintTy}; +use libffi::middle::Type as FfiType; +use rustc_abi::{HasDataLayout, Size}; +use rustc_middle::ty::{self as ty, IntTy, Ty, UintTy}; use rustc_span::Symbol; use serde::{Deserialize, Serialize}; +mod ffi; + #[cfg_attr( not(all( target_os = "linux", @@ -20,6 +21,7 @@ use serde::{Deserialize, Serialize}; )] pub mod trace; +use self::ffi::OwnedArg; use crate::*; /// The final results of an FFI trace, containing every relevant event detected @@ -70,12 +72,12 @@ impl AccessRange { impl<'tcx> EvalContextExtPriv<'tcx> for crate::MiriInterpCx<'tcx> {} trait EvalContextExtPriv<'tcx>: crate::MiriInterpCxExt<'tcx> { /// Call native host function and return the output as an immediate. - fn call_native_with_args<'a>( + fn call_native_with_args( &mut self, link_name: Symbol, dest: &MPlaceTy<'tcx>, ptr: CodePtr, - libffi_args: Vec>, + libffi_args: &mut [OwnedArg], ) -> InterpResult<'tcx, (crate::ImmTy<'tcx>, Option)> { let this = self.eval_context_mut(); #[cfg(target_os = "linux")] @@ -93,55 +95,55 @@ trait EvalContextExtPriv<'tcx>: crate::MiriInterpCxExt<'tcx> { // Unsafe because of the call to native code. // Because this is calling a C function it is not necessarily sound, // but there is no way around this and we've checked as much as we can. - let x = unsafe { ffi::call::(ptr, libffi_args.as_slice()) }; + let x = unsafe { ffi::call::(ptr, libffi_args) }; Scalar::from_i8(x) } ty::Int(IntTy::I16) => { - let x = unsafe { ffi::call::(ptr, libffi_args.as_slice()) }; + let x = unsafe { ffi::call::(ptr, libffi_args) }; Scalar::from_i16(x) } ty::Int(IntTy::I32) => { - let x = unsafe { ffi::call::(ptr, libffi_args.as_slice()) }; + let x = unsafe { ffi::call::(ptr, libffi_args) }; Scalar::from_i32(x) } ty::Int(IntTy::I64) => { - let x = unsafe { ffi::call::(ptr, libffi_args.as_slice()) }; + let x = unsafe { ffi::call::(ptr, libffi_args) }; Scalar::from_i64(x) } ty::Int(IntTy::Isize) => { - let x = unsafe { ffi::call::(ptr, libffi_args.as_slice()) }; + let x = unsafe { ffi::call::(ptr, libffi_args) }; Scalar::from_target_isize(x.try_into().unwrap(), this) } // uints ty::Uint(UintTy::U8) => { - let x = unsafe { ffi::call::(ptr, libffi_args.as_slice()) }; + let x = unsafe { ffi::call::(ptr, libffi_args) }; Scalar::from_u8(x) } ty::Uint(UintTy::U16) => { - let x = unsafe { ffi::call::(ptr, libffi_args.as_slice()) }; + let x = unsafe { ffi::call::(ptr, libffi_args) }; Scalar::from_u16(x) } ty::Uint(UintTy::U32) => { - let x = unsafe { ffi::call::(ptr, libffi_args.as_slice()) }; + let x = unsafe { ffi::call::(ptr, libffi_args) }; Scalar::from_u32(x) } ty::Uint(UintTy::U64) => { - let x = unsafe { ffi::call::(ptr, libffi_args.as_slice()) }; + let x = unsafe { ffi::call::(ptr, libffi_args) }; Scalar::from_u64(x) } ty::Uint(UintTy::Usize) => { - let x = unsafe { ffi::call::(ptr, libffi_args.as_slice()) }; + let x = unsafe { ffi::call::(ptr, libffi_args) }; Scalar::from_target_usize(x.try_into().unwrap(), this) } // Functions with no declared return type (i.e., the default return) // have the output_type `Tuple([])`. ty::Tuple(t_list) if (*t_list).deref().is_empty() => { - unsafe { ffi::call::<()>(ptr, libffi_args.as_slice()) }; + unsafe { ffi::call::<()>(ptr, libffi_args) }; return interp_ok(ImmTy::uninit(dest.layout)); } ty::RawPtr(..) => { - let x = unsafe { ffi::call::<*const ()>(ptr, libffi_args.as_slice()) }; - let ptr = Pointer::new(Provenance::Wildcard, Size::from_bytes(x.addr())); + let x = unsafe { ffi::call::<*const ()>(ptr, libffi_args) }; + let ptr = StrictPointer::new(Provenance::Wildcard, Size::from_bytes(x.addr())); Scalar::from_pointer(ptr, this) } _ => @@ -267,6 +269,158 @@ trait EvalContextExtPriv<'tcx>: crate::MiriInterpCxExt<'tcx> { interp_ok(()) } + + /// Extract the value from the result of reading an operand from the machine + /// and convert it to a `OwnedArg`. + fn op_to_ffi_arg(&self, v: &OpTy<'tcx>, tracing: bool) -> InterpResult<'tcx, OwnedArg> { + let this = self.eval_context_ref(); + + // This should go first so that we emit unsupported before doing a bunch + // of extra work for types that aren't supported yet. + let ty = this.ty_to_ffitype(v.layout.ty)?; + + // Now grab the bytes of the argument. + let bytes = match v.as_mplace_or_imm() { + either::Either::Left(mplace) => { + // Get the alloc id corresponding to this mplace, alongside + // a pointer that's offset to point to this particular + // mplace (not one at the base addr of the allocation). + let mplace_ptr = mplace.ptr(); + let sz = mplace.layout.size.bytes_usize(); + if sz == 0 { + throw_unsup_format!("attempting to pass a ZST over FFI"); + } + let (id, ofs, _) = this.ptr_get_alloc_id(mplace_ptr, sz.try_into().unwrap())?; + let ofs = ofs.bytes_usize(); + // Expose all provenances in the allocation within the byte + // range of the struct, if any. + let alloc = this.get_alloc_raw(id)?; + let alloc_ptr = this.get_alloc_bytes_unchecked_raw(id)?; + for prov in alloc.provenance().get_range(this, (ofs..ofs.strict_add(sz)).into()) { + this.expose_provenance(prov)?; + } + // SAFETY: We know for sure that at alloc_ptr + ofs the next layout.size + // bytes are part of this allocation and initialised. They might be marked + // as uninit in Miri, but all bytes returned by `MiriAllocBytes` are + // initialised. + unsafe { + Box::from(std::slice::from_raw_parts( + alloc_ptr.add(ofs), + mplace.layout.size.bytes_usize(), + )) + } + } + either::Either::Right(imm) => { + // A little helper to write scalars to our byte array. + let write_scalar = |this: &MiriInterpCx<'tcx>, sc: Scalar, bytes: &mut [u8]| { + // If a scalar is a pointer, then expose its provenance. + if let interpret::Scalar::Ptr(p, _) = sc { + // This relies on the `expose_provenance` in the `visit_reachable_allocs` callback + // below to expose the actual interpreter-level allocation. + this.expose_and_warn(Some(p.provenance), tracing)?; + } + // `bytes[0]` should be the first byte we want to write to. + write_target_uint( + this.data_layout().endian, + &mut bytes[..sc.size().bytes_usize()], + sc.to_scalar_int()?.to_bits_unchecked(), + ) + .unwrap(); + interp_ok(()) + }; + + let mut bytes: Box<[u8]> = + (0..imm.layout.size.bytes_usize()).map(|_| 0u8).collect(); + + match *imm { + Immediate::Scalar(sc) => write_scalar(this, sc, &mut bytes)?, + Immediate::ScalarPair(sc_first, sc_second) => { + // The first scalar has an offset of zero. + let ofs_second = { + let rustc_abi::BackendRepr::ScalarPair(a, b) = imm.layout.backend_repr + else { + span_bug!( + this.cur_span(), + "op_to_ffi_arg: invalid scalar pair layout: {:#?}", + imm.layout + ) + }; + a.size(this).align_to(b.align(this).abi).bytes_usize() + }; + + write_scalar(this, sc_first, &mut bytes)?; + write_scalar(this, sc_second, &mut bytes[ofs_second..])?; + } + Immediate::Uninit => + span_bug!(this.cur_span(), "op_to_ffi_arg: argument is uninit: {:#?}", imm), + } + + bytes + } + }; + interp_ok(OwnedArg::new(ty, bytes)) + } + + /// Parses an ADT to construct the matching libffi type. + fn adt_to_ffitype( + &self, + orig_ty: Ty<'_>, + adt_def: ty::AdtDef<'tcx>, + args: &'tcx ty::List>, + ) -> InterpResult<'tcx, FfiType> { + // TODO: Certain non-C reprs should be okay also. + if !adt_def.repr().c() { + throw_unsup_format!("passing a non-#[repr(C)] struct over FFI: {orig_ty}") + } + // TODO: unions, etc. + if !adt_def.is_struct() { + throw_unsup_format!( + "unsupported argument type for native call: {orig_ty} is an enum or union" + ); + } + + let this = self.eval_context_ref(); + let mut fields = vec![]; + for field in adt_def.all_fields() { + fields.push(this.ty_to_ffitype(field.ty(*this.tcx, args))?); + } + + interp_ok(FfiType::structure(fields)) + } + + /// Gets the matching libffi type for a given Ty. + fn ty_to_ffitype(&self, ty: Ty<'tcx>) -> InterpResult<'tcx, FfiType> { + interp_ok(match ty.kind() { + ty::Int(IntTy::I8) => FfiType::i8(), + ty::Int(IntTy::I16) => FfiType::i16(), + ty::Int(IntTy::I32) => FfiType::i32(), + ty::Int(IntTy::I64) => FfiType::i64(), + ty::Int(IntTy::Isize) => FfiType::isize(), + // the uints + ty::Uint(UintTy::U8) => FfiType::u8(), + ty::Uint(UintTy::U16) => FfiType::u16(), + ty::Uint(UintTy::U32) => FfiType::u32(), + ty::Uint(UintTy::U64) => FfiType::u64(), + ty::Uint(UintTy::Usize) => FfiType::usize(), + ty::RawPtr(..) => FfiType::pointer(), + ty::Adt(adt_def, args) => self.adt_to_ffitype(ty, *adt_def, args)?, + _ => throw_unsup_format!("unsupported argument type for native call: {}", ty), + }) + } + + fn expose_and_warn(&self, prov: Option, tracing: bool) -> InterpResult<'tcx> { + let this = self.eval_context_ref(); + if let Some(prov) = prov { + // The first time this happens, print a warning. + if !this.machine.native_call_mem_warned.replace(true) { + // Newly set, so first time we get here. + this.emit_diagnostic(NonHaltingDiagnostic::NativeCallSharedMem { tracing }); + } + + this.expose_provenance(prov)?; + }; + interp_ok(()) + } } impl<'tcx> EvalContextExt<'tcx> for crate::MiriInterpCx<'tcx> {} @@ -295,36 +449,11 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { // Do we have ptrace? let tracing = trace::Supervisor::is_enabled(); - // Get the function arguments, and convert them to `libffi`-compatible form. - let mut libffi_args = Vec::::with_capacity(args.len()); + // Get the function arguments, copy them, and prepare the type descriptions. + let mut libffi_args = Vec::::with_capacity(args.len()); for arg in args.iter() { - if !matches!(arg.layout.backend_repr, BackendRepr::Scalar(_)) { - throw_unsup_format!("only scalar argument types are supported for native calls") - } - let imm = this.read_immediate(arg)?; - libffi_args.push(imm_to_carg(&imm, this)?); - // If we are passing a pointer, expose its provenance. Below, all exposed memory - // (previously exposed and new exposed) will then be properly prepared. - if matches!(arg.layout.ty.kind(), ty::RawPtr(..)) { - let ptr = imm.to_scalar().to_pointer(this)?; - let Some(prov) = ptr.provenance else { - // Pointer without provenance may not access any memory anyway, skip. - continue; - }; - // The first time this happens, print a warning. - if !this.machine.native_call_mem_warned.replace(true) { - // Newly set, so first time we get here. - this.emit_diagnostic(NonHaltingDiagnostic::NativeCallSharedMem { tracing }); - } - - this.expose_provenance(prov)?; - } + libffi_args.push(this.op_to_ffi_arg(arg, tracing)?); } - // Convert arguments to `libffi::high::Arg` type. - let libffi_args = libffi_args - .iter() - .map(|arg| arg.arg_downcast()) - .collect::>>(); // Prepare all exposed memory (both previously exposed, and just newly exposed since a // pointer was passed as argument). Uninitialised memory is left as-is, but any data @@ -367,7 +496,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { // Call the function and store output, depending on return type in the function signature. let (ret, maybe_memevents) = - this.call_native_with_args(link_name, dest, code_ptr, libffi_args)?; + this.call_native_with_args(link_name, dest, code_ptr, &mut libffi_args)?; if tracing { this.tracing_apply_accesses(maybe_memevents.unwrap())?; @@ -377,83 +506,3 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { interp_ok(true) } } - -#[derive(Debug, Clone)] -/// Enum of supported arguments to external C functions. -// We introduce this enum instead of just calling `ffi::arg` and storing a list -// of `libffi::high::Arg` directly, because the `libffi::high::Arg` just wraps a reference -// to the value it represents: https://docs.rs/libffi/latest/libffi/high/call/struct.Arg.html -// and we need to store a copy of the value, and pass a reference to this copy to C instead. -enum CArg { - /// 8-bit signed integer. - Int8(i8), - /// 16-bit signed integer. - Int16(i16), - /// 32-bit signed integer. - Int32(i32), - /// 64-bit signed integer. - Int64(i64), - /// isize. - ISize(isize), - /// 8-bit unsigned integer. - UInt8(u8), - /// 16-bit unsigned integer. - UInt16(u16), - /// 32-bit unsigned integer. - UInt32(u32), - /// 64-bit unsigned integer. - UInt64(u64), - /// usize. - USize(usize), - /// Raw pointer, stored as C's `void*`. - RawPtr(*mut std::ffi::c_void), -} - -impl<'a> CArg { - /// Convert a `CArg` to a `libffi` argument type. - fn arg_downcast(&'a self) -> libffi::high::Arg<'a> { - match self { - CArg::Int8(i) => ffi::arg(i), - CArg::Int16(i) => ffi::arg(i), - CArg::Int32(i) => ffi::arg(i), - CArg::Int64(i) => ffi::arg(i), - CArg::ISize(i) => ffi::arg(i), - CArg::UInt8(i) => ffi::arg(i), - CArg::UInt16(i) => ffi::arg(i), - CArg::UInt32(i) => ffi::arg(i), - CArg::UInt64(i) => ffi::arg(i), - CArg::USize(i) => ffi::arg(i), - CArg::RawPtr(i) => ffi::arg(i), - } - } -} - -/// Extract the scalar value from the result of reading a scalar from the machine, -/// and convert it to a `CArg`. -fn imm_to_carg<'tcx>(v: &ImmTy<'tcx>, cx: &impl HasDataLayout) -> InterpResult<'tcx, CArg> { - interp_ok(match v.layout.ty.kind() { - // If the primitive provided can be converted to a type matching the type pattern - // then create a `CArg` of this primitive value with the corresponding `CArg` constructor. - // the ints - ty::Int(IntTy::I8) => CArg::Int8(v.to_scalar().to_i8()?), - ty::Int(IntTy::I16) => CArg::Int16(v.to_scalar().to_i16()?), - ty::Int(IntTy::I32) => CArg::Int32(v.to_scalar().to_i32()?), - ty::Int(IntTy::I64) => CArg::Int64(v.to_scalar().to_i64()?), - ty::Int(IntTy::Isize) => - CArg::ISize(v.to_scalar().to_target_isize(cx)?.try_into().unwrap()), - // the uints - ty::Uint(UintTy::U8) => CArg::UInt8(v.to_scalar().to_u8()?), - ty::Uint(UintTy::U16) => CArg::UInt16(v.to_scalar().to_u16()?), - ty::Uint(UintTy::U32) => CArg::UInt32(v.to_scalar().to_u32()?), - ty::Uint(UintTy::U64) => CArg::UInt64(v.to_scalar().to_u64()?), - ty::Uint(UintTy::Usize) => - CArg::USize(v.to_scalar().to_target_usize(cx)?.try_into().unwrap()), - ty::RawPtr(..) => { - let s = v.to_scalar().to_pointer(cx)?.addr(); - // This relies on the `expose_provenance` in the `visit_reachable_allocs` callback - // above. - CArg::RawPtr(std::ptr::with_exposed_provenance_mut(s.bytes_usize())) - } - _ => throw_unsup_format!("unsupported argument type for native call: {}", v.layout.ty), - }) -} diff --git a/src/tools/miri/tests/native-lib/aggregate_arguments.c b/src/tools/miri/tests/native-lib/aggregate_arguments.c new file mode 100644 index 00000000000..9c29485e799 --- /dev/null +++ b/src/tools/miri/tests/native-lib/aggregate_arguments.c @@ -0,0 +1,42 @@ +#include + +// See comments in build_native_lib() +#define EXPORT __attribute__((visibility("default"))) + +/* Test: test_pass_struct */ + +typedef struct PassMe { + int32_t value; + int64_t other_value; +} PassMe; + +EXPORT int64_t pass_struct(const PassMe pass_me) { + return pass_me.value + pass_me.other_value; +} + +/* Test: test_pass_struct_complex */ + +typedef struct Part1 { + uint16_t high; + uint16_t low; +} Part1; + +typedef struct Part2 { + uint32_t bits; +} Part2; + +typedef struct ComplexStruct { + Part1 part_1; + Part2 part_2; + uint32_t part_3; +} ComplexStruct; + +EXPORT int32_t pass_struct_complex(const ComplexStruct complex, uint16_t high, uint16_t low, uint32_t bits) { + if (complex.part_1.high == high && complex.part_1.low == low + && complex.part_2.bits == bits + && complex.part_3 == bits) + return 0; + else { + return 1; + } +} diff --git a/src/tools/miri/tests/native-lib/fail/multi_struct_alloc.rs b/src/tools/miri/tests/native-lib/fail/multi_struct_alloc.rs new file mode 100644 index 00000000000..058beab0594 --- /dev/null +++ b/src/tools/miri/tests/native-lib/fail/multi_struct_alloc.rs @@ -0,0 +1,21 @@ +//@compile-flags: -Zmiri-permissive-provenance + +#[repr(C)] +#[derive(Copy, Clone)] +struct HasPointer { + ptr: *const u8, +} + +extern "C" { + fn access_struct_ptr(s: HasPointer) -> u8; +} + +fn main() { + let vals = [10u8, 20u8]; + let structs = + vec![HasPointer { ptr: &raw const vals[0] }, HasPointer { ptr: &raw const vals[1] }]; + unsafe { + access_struct_ptr(structs[1]); + let _val = *std::ptr::with_exposed_provenance::(structs[0].ptr.addr()); //~ ERROR: Undefined Behavior: attempting a read access using + }; +} diff --git a/src/tools/miri/tests/native-lib/fail/multi_struct_alloc.stderr b/src/tools/miri/tests/native-lib/fail/multi_struct_alloc.stderr new file mode 100644 index 00000000000..44656f9ee50 --- /dev/null +++ b/src/tools/miri/tests/native-lib/fail/multi_struct_alloc.stderr @@ -0,0 +1,15 @@ +error: Undefined Behavior: attempting a read access using at ALLOC[0x0], but no exposed tags have suitable permission in the borrow stack for this location + --> tests/native-lib/fail/multi_struct_alloc.rs:LL:CC + | +LL | ...al = *std::ptr::with_exposed_provenance::(structs[0].ptr.addr()); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ this error occurs as part of an access at ALLOC[0x0..0x1] + | + = help: this indicates a potential bug in the program: it performed an invalid operation, but the Stacked Borrows rules it violated are still experimental + = help: see https://github.com/rust-lang/unsafe-code-guidelines/blob/master/wip/stacked-borrows.md for further information + = note: BACKTRACE: + = note: inside `main` at tests/native-lib/fail/multi_struct_alloc.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/native-lib/fail/struct_not_extern_c.rs b/src/tools/miri/tests/native-lib/fail/struct_not_extern_c.rs new file mode 100644 index 00000000000..cf8315e0fd9 --- /dev/null +++ b/src/tools/miri/tests/native-lib/fail/struct_not_extern_c.rs @@ -0,0 +1,19 @@ +// Only works on Unix targets +//@ignore-target: windows wasm +//@only-on-host + +#![allow(improper_ctypes)] + +pub struct PassMe { + pub value: i32, + pub other_value: i64, +} + +extern "C" { + fn pass_struct(s: PassMe) -> i64; +} + +fn main() { + let pass_me = PassMe { value: 42, other_value: 1337 }; + unsafe { pass_struct(pass_me) }; //~ ERROR: unsupported operation: passing a non-#[repr(C)] struct over FFI +} diff --git a/src/tools/miri/tests/native-lib/fail/struct_not_extern_c.stderr b/src/tools/miri/tests/native-lib/fail/struct_not_extern_c.stderr new file mode 100644 index 00000000000..90e59a31da4 --- /dev/null +++ b/src/tools/miri/tests/native-lib/fail/struct_not_extern_c.stderr @@ -0,0 +1,14 @@ +error: unsupported operation: passing a non-#[repr(C)] struct over FFI: PassMe + --> tests/native-lib/fail/struct_not_extern_c.rs:LL:CC + | +LL | unsafe { pass_struct(pass_me) }; + | ^^^^^^^^^^^^^^^^^^^^ unsupported operation occurred here + | + = help: this is likely not a bug in the program; it indicates that the program performed an operation that Miri does not support + = note: BACKTRACE: + = note: inside `main` at tests/native-lib/fail/struct_not_extern_c.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/native-lib/fail/uninit_struct.rs b/src/tools/miri/tests/native-lib/fail/uninit_struct.rs new file mode 100644 index 00000000000..cf61c7f3915 --- /dev/null +++ b/src/tools/miri/tests/native-lib/fail/uninit_struct.rs @@ -0,0 +1,27 @@ +#[repr(C)] +#[derive(Copy, Clone)] +struct ComplexStruct { + part_1: Part1, + part_2: Part2, + part_3: u32, +} +#[repr(C)] +#[derive(Copy, Clone)] +struct Part1 { + high: u16, + low: u16, +} +#[repr(C)] +#[derive(Copy, Clone)] +struct Part2 { + bits: u32, +} + +extern "C" { + fn pass_struct_complex(s: ComplexStruct, high: u16, low: u16, bits: u32) -> i32; +} + +fn main() { + let arg = std::mem::MaybeUninit::::uninit(); + unsafe { pass_struct_complex(*arg.as_ptr(), 0, 0, 0) }; //~ ERROR: Undefined Behavior: constructing invalid value +} diff --git a/src/tools/miri/tests/native-lib/fail/uninit_struct.stderr b/src/tools/miri/tests/native-lib/fail/uninit_struct.stderr new file mode 100644 index 00000000000..0fe6ad9c77b --- /dev/null +++ b/src/tools/miri/tests/native-lib/fail/uninit_struct.stderr @@ -0,0 +1,15 @@ +error: Undefined Behavior: constructing invalid value at .part_1.high: encountered uninitialized memory, but expected an integer + --> tests/native-lib/fail/uninit_struct.rs:LL:CC + | +LL | unsafe { pass_struct_complex(*arg.as_ptr(), 0, 0, 0) }; + | ^^^^^^^^^^^^^ Undefined Behavior occurred here + | + = help: this indicates a bug in the program: it performed an invalid operation, and caused Undefined Behavior + = help: see https://doc.rust-lang.org/nightly/reference/behavior-considered-undefined.html for further information + = note: BACKTRACE: + = note: inside `main` at tests/native-lib/fail/uninit_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/native-lib/pass/aggregate_arguments.rs b/src/tools/miri/tests/native-lib/pass/aggregate_arguments.rs new file mode 100644 index 00000000000..15137c37d7c --- /dev/null +++ b/src/tools/miri/tests/native-lib/pass/aggregate_arguments.rs @@ -0,0 +1,51 @@ +fn main() { + test_pass_struct(); + test_pass_struct_complex(); +} + +/// Test passing a basic struct as an argument. +fn test_pass_struct() { + #[repr(C)] + struct PassMe { + value: i32, + other_value: i64, + } + + extern "C" { + fn pass_struct(s: PassMe) -> i64; + } + + let pass_me = PassMe { value: 42, other_value: 1337 }; + assert_eq!(unsafe { pass_struct(pass_me) }, 42 + 1337); +} + +/// Test passing a more complex struct as an argument. +fn test_pass_struct_complex() { + #[repr(C)] + struct ComplexStruct { + part_1: Part1, + part_2: Part2, + part_3: u32, + } + #[repr(C)] + struct Part1 { + high: u16, + low: u16, + } + #[repr(C)] + struct Part2 { + bits: u32, + } + + extern "C" { + fn pass_struct_complex(s: ComplexStruct, high: u16, low: u16, bits: u32) -> i32; + } + + let high = 0xabcd; + let low = 0xef01; + let bits = 0xabcdef01; + + let complex = + ComplexStruct { part_1: Part1 { high, low }, part_2: Part2 { bits }, part_3: bits }; + assert_eq!(unsafe { pass_struct_complex(complex, high, low, bits) }, 0); +} diff --git a/src/tools/miri/tests/native-lib/pass/ptr_read_access.rs b/src/tools/miri/tests/native-lib/pass/ptr_read_access.rs index 4f3c37f00c1..49750a734d6 100644 --- a/src/tools/miri/tests/native-lib/pass/ptr_read_access.rs +++ b/src/tools/miri/tests/native-lib/pass/ptr_read_access.rs @@ -1,12 +1,14 @@ //@revisions: trace notrace //@[trace] only-target: x86_64-unknown-linux-gnu i686-unknown-linux-gnu //@[trace] compile-flags: -Zmiri-native-lib-enable-tracing +//@compile-flags: -Zmiri-permissive-provenance fn main() { test_access_pointer(); test_access_simple(); test_access_nested(); test_access_static(); + test_access_struct_ptr(); } /// Test function that dereferences an int pointer and prints its contents from C. @@ -74,3 +76,21 @@ fn test_access_static() { assert_eq!(unsafe { access_static(&STATIC) }, 9001); } + +/// Test exposing provenance from a field within a struct. +fn test_access_struct_ptr() { + #[repr(C)] + struct HasPointer { + ptr: *const u8, + } + + extern "C" { + // Return value exists only so the access isn't optimised away. + fn access_struct_ptr(s: HasPointer) -> u8; + } + + let some_val = 42u8; + let ptr = &raw const some_val; + unsafe { access_struct_ptr(HasPointer { ptr }) }; + assert_eq!(some_val, unsafe { *std::ptr::with_exposed_provenance::(ptr.addr()) }) +} diff --git a/src/tools/miri/tests/native-lib/ptr_read_access.c b/src/tools/miri/tests/native-lib/ptr_read_access.c index 021eb6adca4..2107d2bc21f 100644 --- a/src/tools/miri/tests/native-lib/ptr_read_access.c +++ b/src/tools/miri/tests/native-lib/ptr_read_access.c @@ -55,3 +55,13 @@ EXPORT int32_t access_static(const Static *s_ptr) { EXPORT uintptr_t do_one_deref(const int32_t ***ptr) { return (uintptr_t)*ptr; } + +/* Test: test_access_struct_ptr */ + +typedef struct HasPointer { + uint8_t *ptr; +} HasPointer; + +EXPORT uint8_t access_struct_ptr(const HasPointer s) { + return *s.ptr; +} diff --git a/src/tools/miri/tests/ui.rs b/src/tools/miri/tests/ui.rs index f021d5194cd..b7286d9a367 100644 --- a/src/tools/miri/tests/ui.rs +++ b/src/tools/miri/tests/ui.rs @@ -60,6 +60,7 @@ fn build_native_lib(target: &str) -> PathBuf { native_lib_path.to_str().unwrap(), // FIXME: Automate gathering of all relevant C source files in the directory. "tests/native-lib/scalar_arguments.c", + "tests/native-lib/aggregate_arguments.c", "tests/native-lib/ptr_read_access.c", "tests/native-lib/ptr_write_access.c", // Ensure we notice serious problems in the C code. -- cgit 1.4.1-3-g733a5 From c1d16779d57b12cda18c4b8df36de98745ac10e9 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Sun, 31 Aug 2025 20:49:15 +0200 Subject: some refactoring and cleanup --- src/tools/miri/src/shims/native_lib/ffi.rs | 24 ++-- src/tools/miri/src/shims/native_lib/mod.rs | 122 ++++++++++----------- .../miri/tests/native-lib/aggregate_arguments.c | 10 ++ .../tests/native-lib/fail/multi_struct_alloc.rs | 21 ---- .../native-lib/fail/multi_struct_alloc.stderr | 15 --- .../fail/pass_struct_expose_only_range.rs | 23 ++++ .../fail/pass_struct_expose_only_range.stderr | 28 +++++ .../tests/native-lib/pass/aggregate_arguments.rs | 1 + .../miri/tests/native-lib/pass/ptr_read_access.rs | 19 ---- src/tools/miri/tests/native-lib/ptr_read_access.c | 10 -- 10 files changed, 128 insertions(+), 145 deletions(-) delete mode 100644 src/tools/miri/tests/native-lib/fail/multi_struct_alloc.rs delete mode 100644 src/tools/miri/tests/native-lib/fail/multi_struct_alloc.stderr create mode 100644 src/tools/miri/tests/native-lib/fail/pass_struct_expose_only_range.rs create mode 100644 src/tools/miri/tests/native-lib/fail/pass_struct_expose_only_range.stderr (limited to 'src') diff --git a/src/tools/miri/src/shims/native_lib/ffi.rs b/src/tools/miri/src/shims/native_lib/ffi.rs index b2615cedaea..0badf22bb76 100644 --- a/src/tools/miri/src/shims/native_lib/ffi.rs +++ b/src/tools/miri/src/shims/native_lib/ffi.rs @@ -1,18 +1,16 @@ +//! Support code for dealing with libffi. + use libffi::low::CodePtr; use libffi::middle::{Arg as ArgPtr, Cif, Type as FfiType}; /// Perform the actual FFI call. /// -/// SAFETY: The safety invariants of the foreign function being called must be -/// upheld (if any). +/// # Safety +/// +/// The safety invariants of the foreign function being called must be upheld (if any). pub unsafe fn call(fun: CodePtr, args: &mut [OwnedArg]) -> R { - let mut arg_tys = vec![]; - let mut arg_ptrs = vec![]; - for arg in args { - arg_tys.push(arg.take_ty()); - arg_ptrs.push(arg.ptr()) - } - let cif = Cif::new(arg_tys, R::reify().into_middle()); + let arg_ptrs: Vec<_> = args.iter().map(|arg| arg.ptr()).collect(); + let cif = Cif::new(args.iter_mut().map(|arg| arg.ty.take().unwrap()), R::reify().into_middle()); // SAFETY: Caller upholds that the function is safe to call, and since we // were passed a slice reference we know the `OwnedArg`s won't have been // dropped by this point. @@ -34,13 +32,7 @@ impl OwnedArg { Self { ty: Some(ty), bytes } } - /// Gets the libffi type descriptor for this argument. Should only be - /// called once on a given `OwnedArg`. - fn take_ty(&mut self) -> FfiType { - self.ty.take().unwrap() - } - - /// Instantiates a libffi argument pointer pointing to this argument's bytes. + /// Creates a libffi argument pointer pointing to this argument's bytes. /// NB: Since `libffi::middle::Arg` ignores the lifetime of the reference /// it's derived from, it is up to the caller to ensure the `OwnedArg` is /// not dropped before unsafely calling `libffi::middle::Cif::call()`! diff --git a/src/tools/miri/src/shims/native_lib/mod.rs b/src/tools/miri/src/shims/native_lib/mod.rs index 156d6171c73..da8f785e373 100644 --- a/src/tools/miri/src/shims/native_lib/mod.rs +++ b/src/tools/miri/src/shims/native_lib/mod.rs @@ -76,7 +76,7 @@ trait EvalContextExtPriv<'tcx>: crate::MiriInterpCxExt<'tcx> { &mut self, link_name: Symbol, dest: &MPlaceTy<'tcx>, - ptr: CodePtr, + fun: CodePtr, libffi_args: &mut [OwnedArg], ) -> InterpResult<'tcx, (crate::ImmTy<'tcx>, Option)> { let this = self.eval_context_mut(); @@ -95,54 +95,54 @@ trait EvalContextExtPriv<'tcx>: crate::MiriInterpCxExt<'tcx> { // Unsafe because of the call to native code. // Because this is calling a C function it is not necessarily sound, // but there is no way around this and we've checked as much as we can. - let x = unsafe { ffi::call::(ptr, libffi_args) }; + let x = unsafe { ffi::call::(fun, libffi_args) }; Scalar::from_i8(x) } ty::Int(IntTy::I16) => { - let x = unsafe { ffi::call::(ptr, libffi_args) }; + let x = unsafe { ffi::call::(fun, libffi_args) }; Scalar::from_i16(x) } ty::Int(IntTy::I32) => { - let x = unsafe { ffi::call::(ptr, libffi_args) }; + let x = unsafe { ffi::call::(fun, libffi_args) }; Scalar::from_i32(x) } ty::Int(IntTy::I64) => { - let x = unsafe { ffi::call::(ptr, libffi_args) }; + let x = unsafe { ffi::call::(fun, libffi_args) }; Scalar::from_i64(x) } ty::Int(IntTy::Isize) => { - let x = unsafe { ffi::call::(ptr, libffi_args) }; + let x = unsafe { ffi::call::(fun, libffi_args) }; Scalar::from_target_isize(x.try_into().unwrap(), this) } // uints ty::Uint(UintTy::U8) => { - let x = unsafe { ffi::call::(ptr, libffi_args) }; + let x = unsafe { ffi::call::(fun, libffi_args) }; Scalar::from_u8(x) } ty::Uint(UintTy::U16) => { - let x = unsafe { ffi::call::(ptr, libffi_args) }; + let x = unsafe { ffi::call::(fun, libffi_args) }; Scalar::from_u16(x) } ty::Uint(UintTy::U32) => { - let x = unsafe { ffi::call::(ptr, libffi_args) }; + let x = unsafe { ffi::call::(fun, libffi_args) }; Scalar::from_u32(x) } ty::Uint(UintTy::U64) => { - let x = unsafe { ffi::call::(ptr, libffi_args) }; + let x = unsafe { ffi::call::(fun, libffi_args) }; Scalar::from_u64(x) } ty::Uint(UintTy::Usize) => { - let x = unsafe { ffi::call::(ptr, libffi_args) }; + let x = unsafe { ffi::call::(fun, libffi_args) }; Scalar::from_target_usize(x.try_into().unwrap(), this) } // Functions with no declared return type (i.e., the default return) // have the output_type `Tuple([])`. ty::Tuple(t_list) if (*t_list).deref().is_empty() => { - unsafe { ffi::call::<()>(ptr, libffi_args) }; + unsafe { ffi::call::<()>(fun, libffi_args) }; return interp_ok(ImmTy::uninit(dest.layout)); } ty::RawPtr(..) => { - let x = unsafe { ffi::call::<*const ()>(ptr, libffi_args) }; + let x = unsafe { ffi::call::<*const ()>(fun, libffi_args) }; let ptr = StrictPointer::new(Provenance::Wildcard, Size::from_bytes(x.addr())); Scalar::from_pointer(ptr, this) } @@ -279,63 +279,68 @@ trait EvalContextExtPriv<'tcx>: crate::MiriInterpCxExt<'tcx> { // of extra work for types that aren't supported yet. let ty = this.ty_to_ffitype(v.layout.ty)?; - // Now grab the bytes of the argument. + // Helper to print a warning when a pointer is shared with the native code. + let expose = |prov: Provenance| -> InterpResult<'tcx> { + // The first time this happens, print a warning. + if !this.machine.native_call_mem_warned.replace(true) { + // Newly set, so first time we get here. + this.emit_diagnostic(NonHaltingDiagnostic::NativeCallSharedMem { tracing }); + } + + this.expose_provenance(prov)?; + interp_ok(()) + }; + + // Compute the byte-level representation of the argument. If there's a pointer in there, we + // expose it inside the AM. Later in `visit_reachable_allocs`, the "meta"-level provenance + // for accessing the pointee gets exposed; this is crucial to justify the C code effectively + // casting the integer in `byte` to a pointer and using that. let bytes = match v.as_mplace_or_imm() { either::Either::Left(mplace) => { // Get the alloc id corresponding to this mplace, alongside // a pointer that's offset to point to this particular // mplace (not one at the base addr of the allocation). - let mplace_ptr = mplace.ptr(); let sz = mplace.layout.size.bytes_usize(); if sz == 0 { throw_unsup_format!("attempting to pass a ZST over FFI"); } - let (id, ofs, _) = this.ptr_get_alloc_id(mplace_ptr, sz.try_into().unwrap())?; + let (id, ofs, _) = this.ptr_get_alloc_id(mplace.ptr(), sz.try_into().unwrap())?; let ofs = ofs.bytes_usize(); - // Expose all provenances in the allocation within the byte - // range of the struct, if any. + let range = ofs..ofs.strict_add(sz); + // Expose all provenances in the allocation within the byte range of the struct, if + // any. These pointers are being directly passed to native code by-value. let alloc = this.get_alloc_raw(id)?; - let alloc_ptr = this.get_alloc_bytes_unchecked_raw(id)?; - for prov in alloc.provenance().get_range(this, (ofs..ofs.strict_add(sz)).into()) { - this.expose_provenance(prov)?; - } - // SAFETY: We know for sure that at alloc_ptr + ofs the next layout.size - // bytes are part of this allocation and initialised. They might be marked - // as uninit in Miri, but all bytes returned by `MiriAllocBytes` are - // initialised. - unsafe { - Box::from(std::slice::from_raw_parts( - alloc_ptr.add(ofs), - mplace.layout.size.bytes_usize(), - )) + for prov in alloc.provenance().get_range(this, range.clone().into()) { + expose(prov)?; } + // Read the bytes that make up this argument. We cannot use the normal getter as + // those would fail if any part of the argument is uninitialized. Native code + // is kind of outside the interpreter, after all... + Box::from(alloc.inspect_with_uninit_and_ptr_outside_interpreter(range)) } either::Either::Right(imm) => { + let mut bytes: Box<[u8]> = vec![0; imm.layout.size.bytes_usize()].into(); + // A little helper to write scalars to our byte array. - let write_scalar = |this: &MiriInterpCx<'tcx>, sc: Scalar, bytes: &mut [u8]| { + let mut write_scalar = |this: &MiriInterpCx<'tcx>, sc: Scalar, pos: usize| { // If a scalar is a pointer, then expose its provenance. if let interpret::Scalar::Ptr(p, _) = sc { - // This relies on the `expose_provenance` in the `visit_reachable_allocs` callback - // below to expose the actual interpreter-level allocation. - this.expose_and_warn(Some(p.provenance), tracing)?; + expose(p.provenance)?; } - // `bytes[0]` should be the first byte we want to write to. write_target_uint( this.data_layout().endian, - &mut bytes[..sc.size().bytes_usize()], + &mut bytes[pos..][..sc.size().bytes_usize()], sc.to_scalar_int()?.to_bits_unchecked(), ) .unwrap(); interp_ok(()) }; - let mut bytes: Box<[u8]> = - (0..imm.layout.size.bytes_usize()).map(|_| 0u8).collect(); - + // Write the scalar into the `bytes` buffer. match *imm { - Immediate::Scalar(sc) => write_scalar(this, sc, &mut bytes)?, + Immediate::Scalar(sc) => write_scalar(this, sc, 0)?, Immediate::ScalarPair(sc_first, sc_second) => { - // The first scalar has an offset of zero. + // The first scalar has an offset of zero; compute the offset of the 2nd. let ofs_second = { let rustc_abi::BackendRepr::ScalarPair(a, b) = imm.layout.backend_repr else { @@ -348,11 +353,12 @@ trait EvalContextExtPriv<'tcx>: crate::MiriInterpCxExt<'tcx> { a.size(this).align_to(b.align(this).abi).bytes_usize() }; - write_scalar(this, sc_first, &mut bytes)?; - write_scalar(this, sc_second, &mut bytes[ofs_second..])?; + write_scalar(this, sc_first, 0)?; + write_scalar(this, sc_second, ofs_second)?; + } + Immediate::Uninit => { + // Nothing to write. } - Immediate::Uninit => - span_bug!(this.cur_span(), "op_to_ffi_arg: argument is uninit: {:#?}", imm), } bytes @@ -381,7 +387,7 @@ trait EvalContextExtPriv<'tcx>: crate::MiriInterpCxExt<'tcx> { let this = self.eval_context_ref(); let mut fields = vec![]; - for field in adt_def.all_fields() { + for field in &adt_def.non_enum_variant().fields { fields.push(this.ty_to_ffitype(field.ty(*this.tcx, args))?); } @@ -407,20 +413,6 @@ trait EvalContextExtPriv<'tcx>: crate::MiriInterpCxExt<'tcx> { _ => throw_unsup_format!("unsupported argument type for native call: {}", ty), }) } - - fn expose_and_warn(&self, prov: Option, tracing: bool) -> InterpResult<'tcx> { - let this = self.eval_context_ref(); - if let Some(prov) = prov { - // The first time this happens, print a warning. - if !this.machine.native_call_mem_warned.replace(true) { - // Newly set, so first time we get here. - this.emit_diagnostic(NonHaltingDiagnostic::NativeCallSharedMem { tracing }); - } - - this.expose_provenance(prov)?; - }; - interp_ok(()) - } } impl<'tcx> EvalContextExt<'tcx> for crate::MiriInterpCx<'tcx> {} @@ -472,8 +464,9 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { std::hint::black_box(alloc.get_bytes_unchecked_raw().expose_provenance()); if !tracing { - // Expose all provenances in this allocation, since the native code can do $whatever. - // Can be skipped when tracing; in that case we'll expose just the actually-read parts later. + // Expose all provenances in this allocation, since the native code can do + // $whatever. Can be skipped when tracing; in that case we'll expose just the + // actually-read parts later. for prov in alloc.provenance().provenances() { this.expose_provenance(prov)?; } @@ -483,7 +476,8 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { if info.mutbl.is_mut() { let (alloc, cx) = this.get_alloc_raw_mut(alloc_id)?; // These writes could initialize everything and wreck havoc with the pointers. - // We can skip that when tracing; in that case we'll later do that only for the memory that got actually written. + // We can skip that when tracing; in that case we'll later do that only for the + // memory that got actually written. if !tracing { alloc.process_native_write(&cx.tcx, None); } diff --git a/src/tools/miri/tests/native-lib/aggregate_arguments.c b/src/tools/miri/tests/native-lib/aggregate_arguments.c index 9c29485e799..8ad687f2aec 100644 --- a/src/tools/miri/tests/native-lib/aggregate_arguments.c +++ b/src/tools/miri/tests/native-lib/aggregate_arguments.c @@ -3,6 +3,16 @@ // See comments in build_native_lib() #define EXPORT __attribute__((visibility("default"))) +/* Test: fail/pass_struct_expose_only_range */ + +typedef struct HasPointer { + uint8_t *ptr; +} HasPointer; + +EXPORT uint8_t access_struct_ptr(const HasPointer s) { + return *s.ptr; +} + /* Test: test_pass_struct */ typedef struct PassMe { diff --git a/src/tools/miri/tests/native-lib/fail/multi_struct_alloc.rs b/src/tools/miri/tests/native-lib/fail/multi_struct_alloc.rs deleted file mode 100644 index 058beab0594..00000000000 --- a/src/tools/miri/tests/native-lib/fail/multi_struct_alloc.rs +++ /dev/null @@ -1,21 +0,0 @@ -//@compile-flags: -Zmiri-permissive-provenance - -#[repr(C)] -#[derive(Copy, Clone)] -struct HasPointer { - ptr: *const u8, -} - -extern "C" { - fn access_struct_ptr(s: HasPointer) -> u8; -} - -fn main() { - let vals = [10u8, 20u8]; - let structs = - vec![HasPointer { ptr: &raw const vals[0] }, HasPointer { ptr: &raw const vals[1] }]; - unsafe { - access_struct_ptr(structs[1]); - let _val = *std::ptr::with_exposed_provenance::(structs[0].ptr.addr()); //~ ERROR: Undefined Behavior: attempting a read access using - }; -} diff --git a/src/tools/miri/tests/native-lib/fail/multi_struct_alloc.stderr b/src/tools/miri/tests/native-lib/fail/multi_struct_alloc.stderr deleted file mode 100644 index 44656f9ee50..00000000000 --- a/src/tools/miri/tests/native-lib/fail/multi_struct_alloc.stderr +++ /dev/null @@ -1,15 +0,0 @@ -error: Undefined Behavior: attempting a read access using at ALLOC[0x0], but no exposed tags have suitable permission in the borrow stack for this location - --> tests/native-lib/fail/multi_struct_alloc.rs:LL:CC - | -LL | ...al = *std::ptr::with_exposed_provenance::(structs[0].ptr.addr()); - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ this error occurs as part of an access at ALLOC[0x0..0x1] - | - = help: this indicates a potential bug in the program: it performed an invalid operation, but the Stacked Borrows rules it violated are still experimental - = help: see https://github.com/rust-lang/unsafe-code-guidelines/blob/master/wip/stacked-borrows.md for further information - = note: BACKTRACE: - = note: inside `main` at tests/native-lib/fail/multi_struct_alloc.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/native-lib/fail/pass_struct_expose_only_range.rs b/src/tools/miri/tests/native-lib/fail/pass_struct_expose_only_range.rs new file mode 100644 index 00000000000..a2b43031a29 --- /dev/null +++ b/src/tools/miri/tests/native-lib/fail/pass_struct_expose_only_range.rs @@ -0,0 +1,23 @@ +//@compile-flags: -Zmiri-permissive-provenance + +#[repr(C)] +#[derive(Copy, Clone)] +struct HasPointer { + ptr: *const u8, +} + +extern "C" { + fn access_struct_ptr(s: HasPointer) -> u8; +} + +fn main() { + let structs = vec![HasPointer { ptr: &0 }, HasPointer { ptr: &1 }]; + unsafe { + let r = access_struct_ptr(structs[1]); + assert_eq!(r, 1); + // There are two pointers stored in the allocation backing `structs`; ensure + // we only exposed the one that was actually passed to C. + let _val = *std::ptr::with_exposed_provenance::(structs[1].ptr.addr()); // fine, ptr got sent to C + let _val = *std::ptr::with_exposed_provenance::(structs[0].ptr.addr()); //~ ERROR: memory access failed + }; +} diff --git a/src/tools/miri/tests/native-lib/fail/pass_struct_expose_only_range.stderr b/src/tools/miri/tests/native-lib/fail/pass_struct_expose_only_range.stderr new file mode 100644 index 00000000000..a8f85001c23 --- /dev/null +++ b/src/tools/miri/tests/native-lib/fail/pass_struct_expose_only_range.stderr @@ -0,0 +1,28 @@ +warning: sharing memory with a native function called via FFI + --> tests/native-lib/fail/pass_struct_expose_only_range.rs:LL:CC + | +LL | let r = access_struct_ptr(structs[1]); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ sharing memory with a native function + | + = help: when memory is shared with a native function call, Miri stops tracking initialization and provenance for that memory + = help: in particular, Miri assumes that the native call initializes all memory it has access to + = help: Miri also assumes that any part of this memory may be a pointer that is permitted to point to arbitrary exposed memory + = help: what this means is that Miri will easily miss Undefined Behavior related to incorrect usage of this shared memory, so you should not take a clean Miri run as a signal that your FFI code is UB-free + = note: BACKTRACE: + = note: inside `main` at tests/native-lib/fail/pass_struct_expose_only_range.rs:LL:CC + +error: Undefined Behavior: memory access failed: attempting to access 1 byte, but got $HEX[noalloc] which is a dangling pointer (it has no provenance) + --> tests/native-lib/fail/pass_struct_expose_only_range.rs:LL:CC + | +LL | let _val = *std::ptr::with_exposed_provenance::(structs[0].ptr.addr()); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Undefined Behavior occurred here + | + = help: this indicates a bug in the program: it performed an invalid operation, and caused Undefined Behavior + = help: see https://doc.rust-lang.org/nightly/reference/behavior-considered-undefined.html for further information + = note: BACKTRACE: + = note: inside `main` at tests/native-lib/fail/pass_struct_expose_only_range.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; 1 warning emitted + diff --git a/src/tools/miri/tests/native-lib/pass/aggregate_arguments.rs b/src/tools/miri/tests/native-lib/pass/aggregate_arguments.rs index 15137c37d7c..55acb240612 100644 --- a/src/tools/miri/tests/native-lib/pass/aggregate_arguments.rs +++ b/src/tools/miri/tests/native-lib/pass/aggregate_arguments.rs @@ -5,6 +5,7 @@ fn main() { /// Test passing a basic struct as an argument. fn test_pass_struct() { + // Exactly two fields, so that we hit the ScalarPair case. #[repr(C)] struct PassMe { value: i32, diff --git a/src/tools/miri/tests/native-lib/pass/ptr_read_access.rs b/src/tools/miri/tests/native-lib/pass/ptr_read_access.rs index 49750a734d6..bab73f7cf17 100644 --- a/src/tools/miri/tests/native-lib/pass/ptr_read_access.rs +++ b/src/tools/miri/tests/native-lib/pass/ptr_read_access.rs @@ -8,7 +8,6 @@ fn main() { test_access_simple(); test_access_nested(); test_access_static(); - test_access_struct_ptr(); } /// Test function that dereferences an int pointer and prints its contents from C. @@ -76,21 +75,3 @@ fn test_access_static() { assert_eq!(unsafe { access_static(&STATIC) }, 9001); } - -/// Test exposing provenance from a field within a struct. -fn test_access_struct_ptr() { - #[repr(C)] - struct HasPointer { - ptr: *const u8, - } - - extern "C" { - // Return value exists only so the access isn't optimised away. - fn access_struct_ptr(s: HasPointer) -> u8; - } - - let some_val = 42u8; - let ptr = &raw const some_val; - unsafe { access_struct_ptr(HasPointer { ptr }) }; - assert_eq!(some_val, unsafe { *std::ptr::with_exposed_provenance::(ptr.addr()) }) -} diff --git a/src/tools/miri/tests/native-lib/ptr_read_access.c b/src/tools/miri/tests/native-lib/ptr_read_access.c index 2107d2bc21f..021eb6adca4 100644 --- a/src/tools/miri/tests/native-lib/ptr_read_access.c +++ b/src/tools/miri/tests/native-lib/ptr_read_access.c @@ -55,13 +55,3 @@ EXPORT int32_t access_static(const Static *s_ptr) { EXPORT uintptr_t do_one_deref(const int32_t ***ptr) { return (uintptr_t)*ptr; } - -/* Test: test_access_struct_ptr */ - -typedef struct HasPointer { - uint8_t *ptr; -} HasPointer; - -EXPORT uint8_t access_struct_ptr(const HasPointer s) { - return *s.ptr; -} -- cgit 1.4.1-3-g733a5 From 988c077b068b9e6fb4e58650d7e921993a650983 Mon Sep 17 00:00:00 2001 From: The Miri Cronjob Bot Date: Mon, 1 Sep 2025 04:55:23 +0000 Subject: Prepare for merging from rust-lang/rust This updates the rust-version file to 828e45ad11ce4ab56dd64e93f1fb5dd8f0c0ae93. --- src/tools/miri/rust-version | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'src') diff --git a/src/tools/miri/rust-version b/src/tools/miri/rust-version index 695959e2e68..594b47b888f 100644 --- a/src/tools/miri/rust-version +++ b/src/tools/miri/rust-version @@ -1 +1 @@ -e004014d1bf4c29928a0f0f9f7d0964d43606cbd +828e45ad11ce4ab56dd64e93f1fb5dd8f0c0ae93 -- cgit 1.4.1-3-g733a5 From 20a3256bc3b726f5cdc43bd16e6068e4c6fec98a Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Mon, 1 Sep 2025 08:45:45 +0200 Subject: improve output for 'cargo miri test --help' --- src/tools/miri/cargo-miri/src/phases.rs | 39 +++++++++++++++++++++++---------- 1 file changed, 27 insertions(+), 12 deletions(-) (limited to 'src') diff --git a/src/tools/miri/cargo-miri/src/phases.rs b/src/tools/miri/cargo-miri/src/phases.rs index efb9053f69a..0716f4add9d 100644 --- a/src/tools/miri/cargo-miri/src/phases.rs +++ b/src/tools/miri/cargo-miri/src/phases.rs @@ -65,16 +65,6 @@ fn forward_patched_extern_arg(args: &mut impl Iterator, cmd: &mut } pub fn phase_cargo_miri(mut args: impl Iterator) { - // Check for version and help flags even when invoked as `cargo-miri`. - if has_arg_flag("--help") || has_arg_flag("-h") { - show_help(); - return; - } - if has_arg_flag("--version") || has_arg_flag("-V") { - show_version(); - return; - } - // Require a subcommand before any flags. // We cannot know which of those flags take arguments and which do not, // so we cannot detect subcommands later. @@ -85,11 +75,36 @@ pub fn phase_cargo_miri(mut args: impl Iterator) { "setup" => MiriCommand::Setup, "test" | "t" | "run" | "r" | "nextest" => MiriCommand::Forward(subcommand), "clean" => MiriCommand::Clean, - _ => + _ => { + // Check for version and help flags. + if has_arg_flag("--help") || has_arg_flag("-h") { + show_help(); + return; + } + if has_arg_flag("--version") || has_arg_flag("-V") { + show_version(); + return; + } show_error!( "`cargo miri` supports the following subcommands: `run`, `test`, `nextest`, `clean`, and `setup`." - ), + ) + } }; + if has_arg_flag("--help") || has_arg_flag("-h") { + match subcommand { + MiriCommand::Forward(verb) => { + println!("`cargo miri {verb}` supports the same flags as `cargo {verb}`:\n"); + let mut cmd = cargo(); + cmd.arg(verb); + cmd.arg("--help"); + exec(cmd); + } + _ => { + show_help(); + return; + } + } + } let verbose = num_arg_flag("-v") + num_arg_flag("--verbose"); let quiet = has_arg_flag("-q") || has_arg_flag("--quiet"); -- cgit 1.4.1-3-g733a5 From 821a44d3efb677e65b392e627f2d6647841db77e Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Mon, 1 Sep 2025 09:02:43 +0200 Subject: move some configuration enums to a more logical place --- src/tools/miri/src/eval.rs | 59 ------------------------------------------- src/tools/miri/src/lib.rs | 10 +++----- src/tools/miri/src/machine.rs | 59 +++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 63 insertions(+), 65 deletions(-) (limited to 'src') diff --git a/src/tools/miri/src/eval.rs b/src/tools/miri/src/eval.rs index 4c531a8d1f5..43d251b157b 100644 --- a/src/tools/miri/src/eval.rs +++ b/src/tools/miri/src/eval.rs @@ -32,65 +32,6 @@ pub enum MiriEntryFnType { /// will hang the program. const MAIN_THREAD_YIELDS_AT_SHUTDOWN: u32 = 256; -#[derive(Copy, Clone, Debug, PartialEq)] -pub enum AlignmentCheck { - /// Do not check alignment. - None, - /// Check alignment "symbolically", i.e., using only the requested alignment for an allocation and not its real base address. - Symbolic, - /// Check alignment on the actual physical integer address. - Int, -} - -#[derive(Copy, Clone, Debug, PartialEq)] -pub enum RejectOpWith { - /// Isolated op is rejected with an abort of the machine. - Abort, - - /// If not Abort, miri returns an error for an isolated op. - /// Following options determine if user should be warned about such error. - /// Do not print warning about rejected isolated op. - NoWarning, - - /// Print a warning about rejected isolated op, with backtrace. - Warning, - - /// Print a warning about rejected isolated op, without backtrace. - WarningWithoutBacktrace, -} - -#[derive(Copy, Clone, Debug, PartialEq)] -pub enum IsolatedOp { - /// Reject an op requiring communication with the host. By - /// default, miri rejects the op with an abort. If not, it returns - /// an error code, and prints a warning about it. Warning levels - /// are controlled by `RejectOpWith` enum. - Reject(RejectOpWith), - - /// Execute op requiring communication with the host, i.e. disable isolation. - Allow, -} - -#[derive(Debug, Copy, Clone, PartialEq, Eq)] -pub enum BacktraceStyle { - /// Prints a terser backtrace which ideally only contains relevant information. - Short, - /// Prints a backtrace with all possible information. - Full, - /// Prints only the frame that the error occurs in. - Off, -} - -#[derive(Debug, Copy, Clone, PartialEq, Eq)] -pub enum ValidationMode { - /// Do not perform any kind of validation. - No, - /// Validate the interior of the value, but not things behind references. - Shallow, - /// Fully recursively validate references. - Deep, -} - /// Configuration needed to spawn a Miri instance. #[derive(Clone)] pub struct MiriConfig { diff --git a/src/tools/miri/src/lib.rs b/src/tools/miri/src/lib.rs index 5ed6d6b346c..090f10f4813 100644 --- a/src/tools/miri/src/lib.rs +++ b/src/tools/miri/src/lib.rs @@ -138,15 +138,13 @@ pub use crate::data_structures::mono_hash_map::MonoHashMap; pub use crate::diagnostics::{ EvalContextExt as _, NonHaltingDiagnostic, TerminationInfo, report_error, }; -pub use crate::eval::{ - AlignmentCheck, BacktraceStyle, IsolatedOp, MiriConfig, MiriEntryFnType, RejectOpWith, - ValidationMode, create_ecx, eval_entry, -}; +pub use crate::eval::{MiriConfig, MiriEntryFnType, create_ecx, eval_entry}; pub use crate::helpers::{AccessKind, EvalContextExt as _, ToU64 as _, ToUsize as _}; pub use crate::intrinsics::EvalContextExt as _; pub use crate::machine::{ - AllocExtra, DynMachineCallback, FrameExtra, MachineCallback, MemoryKind, MiriInterpCx, - MiriInterpCxExt, MiriMachine, MiriMemoryKind, PrimitiveLayouts, Provenance, ProvenanceExtra, + AlignmentCheck, AllocExtra, BacktraceStyle, DynMachineCallback, FrameExtra, IsolatedOp, + MachineCallback, MemoryKind, MiriInterpCx, MiriInterpCxExt, MiriMachine, MiriMemoryKind, + PrimitiveLayouts, Provenance, ProvenanceExtra, RejectOpWith, ValidationMode, }; pub use crate::operator::EvalContextExt as _; pub use crate::provenance_gc::{EvalContextExt as _, LiveAllocs, VisitProvenance, VisitWith}; diff --git a/src/tools/miri/src/machine.rs b/src/tools/miri/src/machine.rs index 0136de55216..e27b2e89b48 100644 --- a/src/tools/miri/src/machine.rs +++ b/src/tools/miri/src/machine.rs @@ -49,6 +49,65 @@ pub const SIGRTMAX: i32 = 42; /// base address for each evaluation would produce unbounded memory usage. const ADDRS_PER_ANON_GLOBAL: usize = 32; +#[derive(Copy, Clone, Debug, PartialEq)] +pub enum AlignmentCheck { + /// Do not check alignment. + None, + /// Check alignment "symbolically", i.e., using only the requested alignment for an allocation and not its real base address. + Symbolic, + /// Check alignment on the actual physical integer address. + Int, +} + +#[derive(Copy, Clone, Debug, PartialEq)] +pub enum RejectOpWith { + /// Isolated op is rejected with an abort of the machine. + Abort, + + /// If not Abort, miri returns an error for an isolated op. + /// Following options determine if user should be warned about such error. + /// Do not print warning about rejected isolated op. + NoWarning, + + /// Print a warning about rejected isolated op, with backtrace. + Warning, + + /// Print a warning about rejected isolated op, without backtrace. + WarningWithoutBacktrace, +} + +#[derive(Copy, Clone, Debug, PartialEq)] +pub enum IsolatedOp { + /// Reject an op requiring communication with the host. By + /// default, miri rejects the op with an abort. If not, it returns + /// an error code, and prints a warning about it. Warning levels + /// are controlled by `RejectOpWith` enum. + Reject(RejectOpWith), + + /// Execute op requiring communication with the host, i.e. disable isolation. + Allow, +} + +#[derive(Debug, Copy, Clone, PartialEq, Eq)] +pub enum BacktraceStyle { + /// Prints a terser backtrace which ideally only contains relevant information. + Short, + /// Prints a backtrace with all possible information. + Full, + /// Prints only the frame that the error occurs in. + Off, +} + +#[derive(Debug, Copy, Clone, PartialEq, Eq)] +pub enum ValidationMode { + /// Do not perform any kind of validation. + No, + /// Validate the interior of the value, but not things behind references. + Shallow, + /// Fully recursively validate references. + Deep, +} + /// Extra data stored with each stack frame pub struct FrameExtra<'tcx> { /// Extra data for the Borrow Tracker. -- cgit 1.4.1-3-g733a5 From a3546491021831e17c030c7f8ec82f4715b321de Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Tue, 2 Sep 2025 15:02:35 +0200 Subject: add a flag to always apply the maximum float error --- src/tools/miri/README.md | 2 ++ src/tools/miri/src/bin/miri.rs | 4 +++- src/tools/miri/src/eval.rs | 4 ++-- src/tools/miri/src/lib.rs | 7 ++++--- src/tools/miri/src/machine.rs | 12 ++++++++++- src/tools/miri/src/math.rs | 11 +++++++++-- .../miri/tests/pass/float_extra_rounding_error.rs | 23 ++++++++++++++++++++++ 7 files changed, 54 insertions(+), 9 deletions(-) create mode 100644 src/tools/miri/tests/pass/float_extra_rounding_error.rs (limited to 'src') diff --git a/src/tools/miri/README.md b/src/tools/miri/README.md index 7ccd27d7b83..cb4a56b58ac 100644 --- a/src/tools/miri/README.md +++ b/src/tools/miri/README.md @@ -319,6 +319,8 @@ environment variable. We first document the most relevant and most commonly used Can be used without a value; in that case the range defaults to `0..64`. * `-Zmiri-many-seeds-keep-going` tells Miri to really try all the seeds in the given range, even if a failing seed has already been found. This is useful to determine which fraction of seeds fails. +* `-Zmiri-max-extra-rounding-error` tells Miri to always apply the maximum error to float operations + that do not have a guaranteed precision. The sign of the error is still non-deterministic. * `-Zmiri-no-extra-rounding-error` stops Miri from adding extra rounding errors to float operations that do not have a guaranteed precision. * `-Zmiri-num-cpus` states the number of available CPUs to be reported by miri. By default, the diff --git a/src/tools/miri/src/bin/miri.rs b/src/tools/miri/src/bin/miri.rs index ae1b25f8857..8fd8d332978 100644 --- a/src/tools/miri/src/bin/miri.rs +++ b/src/tools/miri/src/bin/miri.rs @@ -556,7 +556,9 @@ fn main() { } else if arg == "-Zmiri-deterministic-floats" { miri_config.float_nondet = false; } else if arg == "-Zmiri-no-extra-rounding-error" { - miri_config.float_rounding_error = false; + miri_config.float_rounding_error = miri::FloatRoundingErrorMode::None; + } else if arg == "-Zmiri-max-extra-rounding-error" { + miri_config.float_rounding_error = miri::FloatRoundingErrorMode::Max; } else if arg == "-Zmiri-strict-provenance" { miri_config.provenance_mode = ProvenanceMode::Strict; } else if arg == "-Zmiri-permissive-provenance" { diff --git a/src/tools/miri/src/eval.rs b/src/tools/miri/src/eval.rs index 43d251b157b..f174a448a81 100644 --- a/src/tools/miri/src/eval.rs +++ b/src/tools/miri/src/eval.rs @@ -112,7 +112,7 @@ pub struct MiriConfig { /// Whether floating-point operations can behave non-deterministically. pub float_nondet: bool, /// Whether floating-point operations can have a non-deterministic rounding error. - pub float_rounding_error: bool, + pub float_rounding_error: FloatRoundingErrorMode, } impl Default for MiriConfig { @@ -154,7 +154,7 @@ impl Default for MiriConfig { fixed_scheduling: false, force_intrinsic_fallback: false, float_nondet: true, - float_rounding_error: true, + float_rounding_error: FloatRoundingErrorMode::Random, } } } diff --git a/src/tools/miri/src/lib.rs b/src/tools/miri/src/lib.rs index 090f10f4813..0856411b8e8 100644 --- a/src/tools/miri/src/lib.rs +++ b/src/tools/miri/src/lib.rs @@ -142,9 +142,10 @@ pub use crate::eval::{MiriConfig, MiriEntryFnType, create_ecx, eval_entry}; pub use crate::helpers::{AccessKind, EvalContextExt as _, ToU64 as _, ToUsize as _}; pub use crate::intrinsics::EvalContextExt as _; pub use crate::machine::{ - AlignmentCheck, AllocExtra, BacktraceStyle, DynMachineCallback, FrameExtra, IsolatedOp, - MachineCallback, MemoryKind, MiriInterpCx, MiriInterpCxExt, MiriMachine, MiriMemoryKind, - PrimitiveLayouts, Provenance, ProvenanceExtra, RejectOpWith, ValidationMode, + AlignmentCheck, AllocExtra, BacktraceStyle, DynMachineCallback, FloatRoundingErrorMode, + FrameExtra, IsolatedOp, MachineCallback, MemoryKind, MiriInterpCx, MiriInterpCxExt, + MiriMachine, MiriMemoryKind, PrimitiveLayouts, Provenance, ProvenanceExtra, RejectOpWith, + ValidationMode, }; pub use crate::operator::EvalContextExt as _; pub use crate::provenance_gc::{EvalContextExt as _, LiveAllocs, VisitProvenance, VisitWith}; diff --git a/src/tools/miri/src/machine.rs b/src/tools/miri/src/machine.rs index e27b2e89b48..7802a58f112 100644 --- a/src/tools/miri/src/machine.rs +++ b/src/tools/miri/src/machine.rs @@ -108,6 +108,16 @@ pub enum ValidationMode { Deep, } +#[derive(Debug, Copy, Clone, PartialEq, Eq)] +pub enum FloatRoundingErrorMode { + /// Apply a random error (the default). + Random, + /// Don't apply any error. + None, + /// Always apply the maximum error (with a random sign). + Max, +} + /// Extra data stored with each stack frame pub struct FrameExtra<'tcx> { /// Extra data for the Borrow Tracker. @@ -658,7 +668,7 @@ pub struct MiriMachine<'tcx> { /// Whether floating-point operations can behave non-deterministically. pub float_nondet: bool, /// Whether floating-point operations can have a non-deterministic rounding error. - pub float_rounding_error: bool, + pub float_rounding_error: FloatRoundingErrorMode, } impl<'tcx> MiriMachine<'tcx> { diff --git a/src/tools/miri/src/math.rs b/src/tools/miri/src/math.rs index e9e5a1070c9..dd994b03f5d 100644 --- a/src/tools/miri/src/math.rs +++ b/src/tools/miri/src/math.rs @@ -15,7 +15,9 @@ pub(crate) fn apply_random_float_error( val: F, err_scale: i32, ) -> F { - if !ecx.machine.float_nondet || !ecx.machine.float_rounding_error { + if !ecx.machine.float_nondet + || matches!(ecx.machine.float_rounding_error, FloatRoundingErrorMode::None) + { return val; } @@ -24,7 +26,12 @@ pub(crate) fn apply_random_float_error( // (When read as binary, the position of the first `1` determines the exponent, // and the remaining bits fill the mantissa. `PREC` is one plus the size of the mantissa, // so this all works out.) - let r = F::from_u128(rng.random_range(0..(1 << F::PRECISION))).value; + let r = F::from_u128(match ecx.machine.float_rounding_error { + FloatRoundingErrorMode::Random => rng.random_range(0..(1 << F::PRECISION)), + FloatRoundingErrorMode::Max => (1 << F::PRECISION) - 1, // force max error + FloatRoundingErrorMode::None => unreachable!(), + }) + .value; // Multiply this with 2^(scale - PREC). The result is between 0 and // 2^PREC * 2^(scale - PREC) = 2^scale. let err = r.scalbn(err_scale.strict_sub(F::PRECISION.try_into().unwrap())); diff --git a/src/tools/miri/tests/pass/float_extra_rounding_error.rs b/src/tools/miri/tests/pass/float_extra_rounding_error.rs new file mode 100644 index 00000000000..73512399f82 --- /dev/null +++ b/src/tools/miri/tests/pass/float_extra_rounding_error.rs @@ -0,0 +1,23 @@ +//! Check that the flags to control the extra rounding error work. +//@revisions: random max none +//@[max]compile-flags: -Zmiri-max-extra-rounding-error +//@[none]compile-flags: -Zmiri-no-extra-rounding-error +#![feature(cfg_select)] + +use std::collections::HashSet; +use std::hint::black_box; + +fn main() { + let expected = cfg_select! { + random => 13, // FIXME: why is it 13? + max => 2, + none => 1, + }; + // Call `sin(0.5)` a bunch of times and see how many different values we get. + let mut values = HashSet::new(); + for _ in 0..(expected * 16) { + let val = black_box(0.5f64).sin(); + values.insert(val.to_bits()); + } + assert_eq!(values.len(), expected); +} -- cgit 1.4.1-3-g733a5 From d4f861ecb6319e1db20d20e3cc5ddab84b4aaade Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Tue, 2 Sep 2025 16:33:10 +0200 Subject: account for aarch64 windows oversleeping --- src/tools/miri/tests/pass/concurrency/sync.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) (limited to 'src') diff --git a/src/tools/miri/tests/pass/concurrency/sync.rs b/src/tools/miri/tests/pass/concurrency/sync.rs index a92359758da..142ac9cc8ca 100644 --- a/src/tools/miri/tests/pass/concurrency/sync.rs +++ b/src/tools/miri/tests/pass/concurrency/sync.rs @@ -9,7 +9,8 @@ use std::time::{Duration, Instant}; // We are expecting to sleep for 10ms. How long of a sleep we are accepting? // Even with 1000ms we still see this test fail on macOS runners. -const MAX_SLEEP_TIME_MS: u64 = 2000; +// On a aarch64-pc-windows-msvc runner, we saw 2.7s! +const MAX_SLEEP_TIME_MS: u64 = 4000; // Check if Rust barriers are working. -- cgit 1.4.1-3-g733a5 From 4daea276ee223c12de884cf6ca8ec323f439dbeb Mon Sep 17 00:00:00 2001 From: Nia Espera Date: Wed, 3 Sep 2025 01:56:03 +0200 Subject: add zed editor config --- src/tools/miri/CONTRIBUTING.md | 6 +++++ src/tools/miri/etc/rust_analyzer_zed.json | 41 +++++++++++++++++++++++++++++++ 2 files changed, 47 insertions(+) create mode 100644 src/tools/miri/etc/rust_analyzer_zed.json (limited to 'src') diff --git a/src/tools/miri/CONTRIBUTING.md b/src/tools/miri/CONTRIBUTING.md index 7d78fdddbad..073ad267476 100644 --- a/src/tools/miri/CONTRIBUTING.md +++ b/src/tools/miri/CONTRIBUTING.md @@ -255,6 +255,12 @@ when installing the Miri toolchain. Alternatively, set the `RUSTUP_TOOLCHAIN` en [`etc/rust_analyzer_helix.toml`]: https://github.com/rust-lang/miri/blob/master/etc/rust_analyzer_helix.toml +### Zed + +Copy [`etc/rust_analyzer_zed.json`] to `.zed/settings.json` in the project root directory. + +[`etc/rust_analyzer_zed.json`]: https://github.com/rust-lang/miri/blob/master/etc/rust_analyzer_zed.json + ### Advanced configuration If you are building Miri with a locally built rustc, set diff --git a/src/tools/miri/etc/rust_analyzer_zed.json b/src/tools/miri/etc/rust_analyzer_zed.json new file mode 100644 index 00000000000..839914c8b68 --- /dev/null +++ b/src/tools/miri/etc/rust_analyzer_zed.json @@ -0,0 +1,41 @@ +{ + "lsp": { + "rust-analyzer": { + "initialization_options": { + "rustc": { + "source": "discover" + }, + "linkedProjects": [ + "./Cargo.toml", + "./cargo-miri/Cargo.toml", + "./genmc-sys/Cargo.toml", + "./miri-script/Cargo.toml" + ], + "check": { + "invocationStrategy": "once", + "overrideCommand": [ + "./miri", + "clippy", // make this `check` when working with a locally built rustc + "--message-format=json" + ] + }, + "cargo": { + "extraEnv": { + "MIRI_AUTO_OPS": "no", + "MIRI_IN_RA": "1" + }, + // Contrary to what the name suggests, this also affects proc macros. + "buildScripts": { + "invocationStrategy": "once", + "overrideCommand": [ + "./miri", + "check", + "--no-default-features", + "--message-format=json" + ] + } + } + } + } + } +} -- cgit 1.4.1-3-g733a5 From ef1dab1cb6234bb32a5b82e31b8684be0cabfcd9 Mon Sep 17 00:00:00 2001 From: The Miri Cronjob Bot Date: Wed, 3 Sep 2025 04:53:32 +0000 Subject: Prepare for merging from rust-lang/rust This updates the rust-version file to 51ff895062ba60a7cba53f57af928c3fb7b0f2f4. --- src/tools/miri/rust-version | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'src') diff --git a/src/tools/miri/rust-version b/src/tools/miri/rust-version index 594b47b888f..d44488399f8 100644 --- a/src/tools/miri/rust-version +++ b/src/tools/miri/rust-version @@ -1 +1 @@ -828e45ad11ce4ab56dd64e93f1fb5dd8f0c0ae93 +51ff895062ba60a7cba53f57af928c3fb7b0f2f4 -- cgit 1.4.1-3-g733a5 From 9f0b2a2df41a517412f5e2d404cbabc73be665de Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Tue, 2 Sep 2025 17:17:19 +0200 Subject: fix mangitude of applied float error --- src/tools/miri/src/intrinsics/mod.rs | 49 +++------------- src/tools/miri/src/machine.rs | 2 +- src/tools/miri/src/math.rs | 67 ++++++++++++++-------- src/tools/miri/tests/pass/float.rs | 7 ++- .../miri/tests/pass/float_extra_rounding_error.rs | 10 +++- .../miri/tests/pass/shims/x86/rounding-error.rs | 37 ++++++++++++ 6 files changed, 103 insertions(+), 69 deletions(-) create mode 100644 src/tools/miri/tests/pass/shims/x86/rounding-error.rs (limited to 'src') diff --git a/src/tools/miri/src/intrinsics/mod.rs b/src/tools/miri/src/intrinsics/mod.rs index b5e81460773..5e46768b0e6 100644 --- a/src/tools/miri/src/intrinsics/mod.rs +++ b/src/tools/miri/src/intrinsics/mod.rs @@ -10,7 +10,7 @@ use rustc_abi::Size; use rustc_apfloat::ieee::{IeeeFloat, Semantics}; use rustc_apfloat::{self, Float, Round}; use rustc_middle::mir; -use rustc_middle::ty::{self, FloatTy, ScalarInt}; +use rustc_middle::ty::{self, FloatTy}; use rustc_span::{Symbol, sym}; use self::atomic::EvalContextExt as _; @@ -230,7 +230,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { let res = apply_random_float_error_ulp( this, res, - 2, // log2(4) + 4, ); // Clamp the result to the guaranteed range of this function according to the C standard, @@ -274,7 +274,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { let res = apply_random_float_error_ulp( this, res, - 2, // log2(4) + 4, ); // Clamp the result to the guaranteed range of this function according to the C standard, @@ -336,9 +336,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { // Apply a relative error of 4ULP to introduce some non-determinism // simulating imprecise implementations and optimizations. - apply_random_float_error_ulp( - this, res, 2, // log2(4) - ) + apply_random_float_error_ulp(this, res, 4) }); let res = this.adjust_nan(res, &[f1, f2]); this.write_scalar(res, dest)?; @@ -354,9 +352,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { // Apply a relative error of 4ULP to introduce some non-determinism // simulating imprecise implementations and optimizations. - apply_random_float_error_ulp( - this, res, 2, // log2(4) - ) + apply_random_float_error_ulp(this, res, 4) }); let res = this.adjust_nan(res, &[f1, f2]); this.write_scalar(res, dest)?; @@ -373,9 +369,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { // Apply a relative error of 4ULP to introduce some non-determinism // simulating imprecise implementations and optimizations. - apply_random_float_error_ulp( - this, res, 2, // log2(4) - ) + apply_random_float_error_ulp(this, res, 4) }); let res = this.adjust_nan(res, &[f]); this.write_scalar(res, dest)?; @@ -391,9 +385,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { // Apply a relative error of 4ULP to introduce some non-determinism // simulating imprecise implementations and optimizations. - apply_random_float_error_ulp( - this, res, 2, // log2(4) - ) + apply_random_float_error_ulp(this, res, 4) }); let res = this.adjust_nan(res, &[f]); this.write_scalar(res, dest)?; @@ -448,7 +440,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { } // Apply a relative error of 4ULP to simulate non-deterministic precision loss // due to optimizations. - let res = apply_random_float_error_to_imm(this, res, 2 /* log2(4) */)?; + let res = crate::math::apply_random_float_error_to_imm(this, res, 4)?; this.write_immediate(*res, dest)?; } @@ -486,31 +478,6 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { } } -/// Applies a random ULP floating point error to `val` and returns the new value. -/// So if you want an X ULP error, `ulp_exponent` should be log2(X). -/// -/// Will fail if `val` is not a floating point number. -fn apply_random_float_error_to_imm<'tcx>( - ecx: &mut MiriInterpCx<'tcx>, - val: ImmTy<'tcx>, - ulp_exponent: u32, -) -> InterpResult<'tcx, ImmTy<'tcx>> { - let scalar = val.to_scalar_int()?; - let res: ScalarInt = match val.layout.ty.kind() { - ty::Float(FloatTy::F16) => - apply_random_float_error_ulp(ecx, scalar.to_f16(), ulp_exponent).into(), - ty::Float(FloatTy::F32) => - apply_random_float_error_ulp(ecx, scalar.to_f32(), ulp_exponent).into(), - ty::Float(FloatTy::F64) => - apply_random_float_error_ulp(ecx, scalar.to_f64(), ulp_exponent).into(), - ty::Float(FloatTy::F128) => - apply_random_float_error_ulp(ecx, scalar.to_f128(), ulp_exponent).into(), - _ => bug!("intrinsic called with non-float input type"), - }; - - interp_ok(ImmTy::from_scalar_int(res, val.layout)) -} - /// For the intrinsics: /// - sinf32, sinf64 /// - cosf32, cosf64 diff --git a/src/tools/miri/src/machine.rs b/src/tools/miri/src/machine.rs index 7802a58f112..e0ee9541948 100644 --- a/src/tools/miri/src/machine.rs +++ b/src/tools/miri/src/machine.rs @@ -1278,7 +1278,7 @@ impl<'tcx> Machine<'tcx> for MiriMachine<'tcx> { ecx: &mut InterpCx<'tcx, Self>, val: ImmTy<'tcx>, ) -> InterpResult<'tcx, ImmTy<'tcx>> { - crate::math::apply_random_float_error_to_imm(ecx, val, 2 /* log2(4) */) + crate::math::apply_random_float_error_to_imm(ecx, val, 4) } #[inline(always)] diff --git a/src/tools/miri/src/math.rs b/src/tools/miri/src/math.rs index dd994b03f5d..604d6c0fd73 100644 --- a/src/tools/miri/src/math.rs +++ b/src/tools/miri/src/math.rs @@ -6,10 +6,6 @@ use rustc_middle::ty::{self, FloatTy, ScalarInt}; use crate::*; /// Disturbes a floating-point result by a relative error in the range (-2^scale, 2^scale). -/// -/// For a 2^N ULP error, you can use an `err_scale` of `-(F::PRECISION - 1 - N)`. -/// In other words, a 1 ULP (absolute) error is the same as a `2^-(F::PRECISION-1)` relative error. -/// (Subtracting 1 compensates for the integer bit.) pub(crate) fn apply_random_float_error( ecx: &mut crate::MiriInterpCx<'_>, val: F, @@ -17,11 +13,13 @@ pub(crate) fn apply_random_float_error( ) -> F { if !ecx.machine.float_nondet || matches!(ecx.machine.float_rounding_error, FloatRoundingErrorMode::None) + // relative errors don't do anything to zeros... avoid messing up the sign + || val.is_zero() { return val; } - let rng = ecx.machine.rng.get_mut(); + // Generate a random integer in the range [0, 2^PREC). // (When read as binary, the position of the first `1` determines the exponent, // and the remaining bits fill the mantissa. `PREC` is one plus the size of the mantissa, @@ -37,43 +35,66 @@ pub(crate) fn apply_random_float_error( let err = r.scalbn(err_scale.strict_sub(F::PRECISION.try_into().unwrap())); // give it a random sign let err = if rng.random() { -err } else { err }; - // multiple the value with (1+err) - (val * (F::from_u128(1).value + err).value).value + // Compute `val*(1+err)`, distributed out as `val + val*err` to avoid the imprecise addition + // error being amplified by multiplication. + (val + (val * err).value).value } -/// [`apply_random_float_error`] gives instructions to apply a 2^N ULP error. -/// This function implements these instructions such that applying a 2^N ULP error is less error prone. -/// So for a 2^N ULP error, you would pass N as the `ulp_exponent` argument. +/// Applies an error of `[-N, +N]` ULP to the given value. pub(crate) fn apply_random_float_error_ulp( ecx: &mut crate::MiriInterpCx<'_>, val: F, - ulp_exponent: u32, + max_error: u32, ) -> F { - let n = i32::try_from(ulp_exponent) - .expect("`err_scale_for_ulp`: exponent is too large to create an error scale"); - // we know this fits - let prec = i32::try_from(F::PRECISION).unwrap(); - let err_scale = -(prec - n - 1); - apply_random_float_error(ecx, val, err_scale) + // We could try to be clever and reuse `apply_random_float_error`, but that is hard to get right + // (see for why) so we + // implement the logic directly instead. + if !ecx.machine.float_nondet + || matches!(ecx.machine.float_rounding_error, FloatRoundingErrorMode::None) + // FIXME: also disturb zeros? That requires a lot more cases in `fixed_float_value` + // and might make the std test suite quite unhappy. + || val.is_zero() + { + return val; + } + let rng = ecx.machine.rng.get_mut(); + + let max_error = i64::from(max_error); + let error = match ecx.machine.float_rounding_error { + FloatRoundingErrorMode::Random => rng.random_range(-max_error..=max_error), + FloatRoundingErrorMode::Max => + if rng.random() { + max_error + } else { + -max_error + }, + FloatRoundingErrorMode::None => unreachable!(), + }; + // If upwards ULP and downwards ULP differ, we take the average. + let ulp = (((val.next_up().value - val).value + (val - val.next_down().value).value).value + / F::from_u128(2).value) + .value; + // Shift the value by N times the ULP + (val + (ulp * F::from_i128(error.into()).value).value).value } -/// Applies a random 16ULP floating point error to `val` and returns the new value. +/// Applies an error of `[-N, +N]` ULP to the given value. /// Will fail if `val` is not a floating point number. pub(crate) fn apply_random_float_error_to_imm<'tcx>( ecx: &mut MiriInterpCx<'tcx>, val: ImmTy<'tcx>, - ulp_exponent: u32, + max_error: u32, ) -> InterpResult<'tcx, ImmTy<'tcx>> { let scalar = val.to_scalar_int()?; let res: ScalarInt = match val.layout.ty.kind() { ty::Float(FloatTy::F16) => - apply_random_float_error_ulp(ecx, scalar.to_f16(), ulp_exponent).into(), + apply_random_float_error_ulp(ecx, scalar.to_f16(), max_error).into(), ty::Float(FloatTy::F32) => - apply_random_float_error_ulp(ecx, scalar.to_f32(), ulp_exponent).into(), + apply_random_float_error_ulp(ecx, scalar.to_f32(), max_error).into(), ty::Float(FloatTy::F64) => - apply_random_float_error_ulp(ecx, scalar.to_f64(), ulp_exponent).into(), + apply_random_float_error_ulp(ecx, scalar.to_f64(), max_error).into(), ty::Float(FloatTy::F128) => - apply_random_float_error_ulp(ecx, scalar.to_f128(), ulp_exponent).into(), + apply_random_float_error_ulp(ecx, scalar.to_f128(), max_error).into(), _ => bug!("intrinsic called with non-float input type"), }; diff --git a/src/tools/miri/tests/pass/float.rs b/src/tools/miri/tests/pass/float.rs index fe7316c6680..2be262d76a4 100644 --- a/src/tools/miri/tests/pass/float.rs +++ b/src/tools/miri/tests/pass/float.rs @@ -39,9 +39,8 @@ macro_rules! assert_approx_eq { }}; ($a:expr, $b: expr) => { - // accept up to 12ULP (4ULP for host floats and 4ULP for miri artificial error and 4 for any additional effects - // due to having multiple error sources. - assert_approx_eq!($a, $b, 12); + // accept up to 8ULP (4ULP for host floats and 4ULP for miri artificial error). + assert_approx_eq!($a, $b, 8); }; } @@ -176,6 +175,7 @@ fn assert_eq_msg(x: T, y: T, msg: impl Display) { } /// Check that floats have bitwise equality +#[track_caller] fn assert_biteq(a: F, b: F, msg: impl Display) { let ab = a.to_bits(); let bb = b.to_bits(); @@ -189,6 +189,7 @@ fn assert_biteq(a: F, b: F, msg: impl Display) { } /// Check that two floats have equality +#[track_caller] fn assert_feq(a: F, b: F, msg: impl Display) { let ab = a.to_bits(); let bb = b.to_bits(); diff --git a/src/tools/miri/tests/pass/float_extra_rounding_error.rs b/src/tools/miri/tests/pass/float_extra_rounding_error.rs index 73512399f82..24d7cf2ccee 100644 --- a/src/tools/miri/tests/pass/float_extra_rounding_error.rs +++ b/src/tools/miri/tests/pass/float_extra_rounding_error.rs @@ -9,7 +9,7 @@ use std::hint::black_box; fn main() { let expected = cfg_select! { - random => 13, // FIXME: why is it 13? + random => 9, // -4 ..= +4 ULP error max => 2, none => 1, }; @@ -20,4 +20,12 @@ fn main() { values.insert(val.to_bits()); } assert_eq!(values.len(), expected); + + if !cfg!(none) { + // Ensure the smallest and biggest value are 8 ULP apart. + // We can just subtract the raw bit representations for this. + let min = *values.iter().min().unwrap(); + let max = *values.iter().max().unwrap(); + assert_eq!(min.abs_diff(max), 8); + } } diff --git a/src/tools/miri/tests/pass/shims/x86/rounding-error.rs b/src/tools/miri/tests/pass/shims/x86/rounding-error.rs new file mode 100644 index 00000000000..bf56111b2e4 --- /dev/null +++ b/src/tools/miri/tests/pass/shims/x86/rounding-error.rs @@ -0,0 +1,37 @@ +// We're testing x86 target specific features +//@only-target: x86_64 i686 + +//! rsqrt and rcp SSE/AVX operations are approximate. We use that as license to treat them as +//! non-deterministic. Ensure that we do indeed see random results within the expected error bounds. + +#[cfg(target_arch = "x86")] +use std::arch::x86::*; +#[cfg(target_arch = "x86_64")] +use std::arch::x86_64::*; +use std::collections::HashSet; + +fn main() { + let mut vals = HashSet::new(); + for _ in 0..50 { + unsafe { + // Compute the inverse square root of 4.0, four times. + let a = _mm_setr_ps(4.0, 4.0, 4.0, 4.0); + let exact = 0.5; + let r = _mm_rsqrt_ps(a); + let r: [f32; 4] = std::mem::transmute(r); + // Check the results. + for r in r { + vals.insert(r.to_bits()); + // Ensure the relative error is less than 2^-12. + let rel_error = (r - exact) / exact; + let log_error = rel_error.abs().log2(); + assert!( + rel_error == 0.0 || log_error < -12.0, + "got an error of {rel_error} = 2^{log_error}" + ); + } + } + } + // Ensure we saw a bunch of different results. + assert!(vals.len() >= 50); +} -- cgit 1.4.1-3-g733a5 From 283985393a98761e656c86c5f6f153e9f90eb7e2 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Wed, 3 Sep 2025 10:27:20 +0200 Subject: add flag to not shorten FD reads/writes; don't shorten pipe operations --- src/tools/miri/README.md | 4 +++ src/tools/miri/src/bin/miri.rs | 2 ++ src/tools/miri/src/eval.rs | 3 +++ src/tools/miri/src/machine.rs | 5 ++++ src/tools/miri/src/shims/files.rs | 12 +++++++-- src/tools/miri/src/shims/unix/fd.rs | 30 +++++++++++++--------- .../miri/src/shims/unix/linux_like/eventfd.rs | 5 ---- src/tools/miri/src/shims/unix/unnamed_socket.rs | 8 ++++++ 8 files changed, 50 insertions(+), 19 deletions(-) (limited to 'src') diff --git a/src/tools/miri/README.md b/src/tools/miri/README.md index cb4a56b58ac..517aa343a6d 100644 --- a/src/tools/miri/README.md +++ b/src/tools/miri/README.md @@ -323,6 +323,10 @@ environment variable. We first document the most relevant and most commonly used that do not have a guaranteed precision. The sign of the error is still non-deterministic. * `-Zmiri-no-extra-rounding-error` stops Miri from adding extra rounding errors to float operations that do not have a guaranteed precision. +* `-Zmiri-no-short-fd-operations` stops Miri from artificially forcing `read`/`write` operations + to only process a part of their buffer. Note that whenever Miri uses host operations to + implement `read`/`write` (e.g. for file-backed file descriptors), the host system can still + introduce short reads/writes. * `-Zmiri-num-cpus` states the number of available CPUs to be reported by miri. By default, the number of available CPUs is `1`. Note that this flag does not affect how miri handles threads in any way. diff --git a/src/tools/miri/src/bin/miri.rs b/src/tools/miri/src/bin/miri.rs index 8fd8d332978..9b0bee72aef 100644 --- a/src/tools/miri/src/bin/miri.rs +++ b/src/tools/miri/src/bin/miri.rs @@ -559,6 +559,8 @@ fn main() { miri_config.float_rounding_error = miri::FloatRoundingErrorMode::None; } else if arg == "-Zmiri-max-extra-rounding-error" { miri_config.float_rounding_error = miri::FloatRoundingErrorMode::Max; + } else if arg == "-Zmiri-no-short-fd-operations" { + miri_config.short_fd_operations = false; } else if arg == "-Zmiri-strict-provenance" { miri_config.provenance_mode = ProvenanceMode::Strict; } else if arg == "-Zmiri-permissive-provenance" { diff --git a/src/tools/miri/src/eval.rs b/src/tools/miri/src/eval.rs index f174a448a81..caed98f04f3 100644 --- a/src/tools/miri/src/eval.rs +++ b/src/tools/miri/src/eval.rs @@ -113,6 +113,8 @@ pub struct MiriConfig { pub float_nondet: bool, /// Whether floating-point operations can have a non-deterministic rounding error. pub float_rounding_error: FloatRoundingErrorMode, + /// Whether Miri artifically introduces short reads/writes on file descriptors. + pub short_fd_operations: bool, } impl Default for MiriConfig { @@ -155,6 +157,7 @@ impl Default for MiriConfig { force_intrinsic_fallback: false, float_nondet: true, float_rounding_error: FloatRoundingErrorMode::Random, + short_fd_operations: true, } } } diff --git a/src/tools/miri/src/machine.rs b/src/tools/miri/src/machine.rs index 7802a58f112..9067c5d33cb 100644 --- a/src/tools/miri/src/machine.rs +++ b/src/tools/miri/src/machine.rs @@ -669,6 +669,9 @@ pub struct MiriMachine<'tcx> { pub float_nondet: bool, /// Whether floating-point operations can have a non-deterministic rounding error. pub float_rounding_error: FloatRoundingErrorMode, + + /// Whether Miri artifically introduces short reads/writes on file descriptors. + pub short_fd_operations: bool, } impl<'tcx> MiriMachine<'tcx> { @@ -830,6 +833,7 @@ impl<'tcx> MiriMachine<'tcx> { force_intrinsic_fallback: config.force_intrinsic_fallback, float_nondet: config.float_nondet, float_rounding_error: config.float_rounding_error, + short_fd_operations: config.short_fd_operations, } } @@ -1006,6 +1010,7 @@ impl VisitProvenance for MiriMachine<'_> { force_intrinsic_fallback: _, float_nondet: _, float_rounding_error: _, + short_fd_operations: _, } = self; threads.visit_provenance(visit); diff --git a/src/tools/miri/src/shims/files.rs b/src/tools/miri/src/shims/files.rs index 0d4642c6ad0..8c29cb040b5 100644 --- a/src/tools/miri/src/shims/files.rs +++ b/src/tools/miri/src/shims/files.rs @@ -168,8 +168,9 @@ pub trait FileDescription: std::fmt::Debug + FileDescriptionExt { } /// Determines whether this FD non-deterministically has its reads and writes shortened. - fn nondet_short_accesses(&self) -> bool { - true + fn short_fd_operations(&self) -> bool { + // We only enable this for FD kinds where we think short accesses gain useful test coverage. + false } /// Seeks to the given offset (which can be relative to the beginning, end, or current position). @@ -395,6 +396,13 @@ impl FileDescription for FileHandle { communicate_allowed && self.file.is_terminal() } + fn short_fd_operations(&self) -> bool { + // While short accesses on file-backed FDs are very rare (at least for sufficiently small + // accesses), they can realistically happen when a signal interrupts the syscall. + // FIXME: we should return `false` if this is a named pipe... + true + } + fn as_unix<'tcx>(&self, ecx: &MiriInterpCx<'tcx>) -> &dyn UnixFileDescription { assert!( ecx.target_os_is_unix(), diff --git a/src/tools/miri/src/shims/unix/fd.rs b/src/tools/miri/src/shims/unix/fd.rs index 9fbecffc55d..95e26ef5d5d 100644 --- a/src/tools/miri/src/shims/unix/fd.rs +++ b/src/tools/miri/src/shims/unix/fd.rs @@ -274,12 +274,15 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { } // Non-deterministically decide to further reduce the count, simulating a partial read (but // never to 0, that would indicate EOF). - let count = - if fd.nondet_short_accesses() && count >= 2 && this.machine.rng.get_mut().random() { - count / 2 // since `count` is at least 2, the result is still at least 1 - } else { - count - }; + let count = if this.machine.short_fd_operations + && fd.short_fd_operations() + && count >= 2 + && this.machine.rng.get_mut().random() + { + count / 2 // since `count` is at least 2, the result is still at least 1 + } else { + count + }; trace!("read: FD mapped to {fd:?}"); // We want to read at most `count` bytes. We are sure that `count` is not negative @@ -360,12 +363,15 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { // Non-deterministically decide to further reduce the count, simulating a partial write. // We avoid reducing the write size to 0: the docs seem to be entirely fine with that, // but the standard library is not (https://github.com/rust-lang/rust/issues/145959). - let count = - if fd.nondet_short_accesses() && count >= 2 && this.machine.rng.get_mut().random() { - count / 2 - } else { - count - }; + let count = if this.machine.short_fd_operations + && fd.short_fd_operations() + && count >= 2 + && this.machine.rng.get_mut().random() + { + count / 2 + } else { + count + }; let finish = { let dest = dest.clone(); diff --git a/src/tools/miri/src/shims/unix/linux_like/eventfd.rs b/src/tools/miri/src/shims/unix/linux_like/eventfd.rs index 2d35ef064db..ee7deb8d383 100644 --- a/src/tools/miri/src/shims/unix/linux_like/eventfd.rs +++ b/src/tools/miri/src/shims/unix/linux_like/eventfd.rs @@ -37,11 +37,6 @@ impl FileDescription for EventFd { "event" } - fn nondet_short_accesses(&self) -> bool { - // We always read and write exactly one `u64`. - false - } - fn close<'tcx>( self, _communicate_allowed: bool, diff --git a/src/tools/miri/src/shims/unix/unnamed_socket.rs b/src/tools/miri/src/shims/unix/unnamed_socket.rs index 817ddd7954d..7eb82851033 100644 --- a/src/tools/miri/src/shims/unix/unnamed_socket.rs +++ b/src/tools/miri/src/shims/unix/unnamed_socket.rs @@ -123,6 +123,14 @@ impl FileDescription for AnonSocket { anonsocket_write(self, ptr, len, ecx, finish) } + fn short_fd_operations(&self) -> bool { + // Pipes guarantee that sufficiently small accesses are not broken apart: + // . + // For now, we don't bother checking for the size, and just entirely disable + // short accesses on pipes. + matches!(self.fd_type, AnonSocketType::Socketpair) + } + fn as_unix<'tcx>(&self, _ecx: &MiriInterpCx<'tcx>) -> &dyn UnixFileDescription { self } -- cgit 1.4.1-3-g733a5 From 86670347189962f15061404526acab2a6bb0da8f Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Wed, 3 Sep 2025 21:22:43 +0200 Subject: fix applying an error to infinities --- src/tools/miri/src/math.rs | 4 ++++ src/tools/miri/tests/pass/float.rs | 4 ++++ 2 files changed, 8 insertions(+) (limited to 'src') diff --git a/src/tools/miri/src/math.rs b/src/tools/miri/src/math.rs index 604d6c0fd73..6427f3ca6e9 100644 --- a/src/tools/miri/src/math.rs +++ b/src/tools/miri/src/math.rs @@ -15,6 +15,8 @@ pub(crate) fn apply_random_float_error( || matches!(ecx.machine.float_rounding_error, FloatRoundingErrorMode::None) // relative errors don't do anything to zeros... avoid messing up the sign || val.is_zero() + // The logic below makes no sense if the input is already non-finite. + || !val.is_finite() { return val; } @@ -54,6 +56,8 @@ pub(crate) fn apply_random_float_error_ulp( // FIXME: also disturb zeros? That requires a lot more cases in `fixed_float_value` // and might make the std test suite quite unhappy. || val.is_zero() + // The logic below makes no sense if the input is already non-finite. + || !val.is_finite() { return val; } diff --git a/src/tools/miri/tests/pass/float.rs b/src/tools/miri/tests/pass/float.rs index 2be262d76a4..1b1163c7797 100644 --- a/src/tools/miri/tests/pass/float.rs +++ b/src/tools/miri/tests/pass/float.rs @@ -1052,6 +1052,10 @@ pub fn libm() { assert_eq!(42f64.powf(0.0), 1.0); assert_eq!(f32::INFINITY.powf(0.0), 1.0); assert_eq!(f64::INFINITY.powf(0.0), 1.0); + assert_eq!(f32::NEG_INFINITY.powi(3), f32::NEG_INFINITY); + assert_eq!(f32::NEG_INFINITY.powi(2), f32::INFINITY); + assert_eq!(f64::INFINITY.powi(3), f64::INFINITY); + assert_eq!(f64::INFINITY.powi(2), f64::INFINITY); // f*::NAN is a quiet NAN and should return 1 as well. assert_eq!(f32::NAN.powi(0), 1.0); -- cgit 1.4.1-3-g733a5