From e932ea50ba21b355b08a5572a20cb53d2c74aacb Mon Sep 17 00:00:00 2001 From: Mateusz Gienieczko Date: Sat, 21 May 2022 16:10:08 +0200 Subject: [PATCH 1/8] Add failing page_size test. --- test-cargo-miri/Cargo.lock | 33 +++++++++++++++++++ test-cargo-miri/Cargo.toml | 1 + test-cargo-miri/test.cross-target.stdout.ref | 6 ++-- test-cargo-miri/test.default.stdout.ref | 6 ++-- .../test.filter.cross-target.stdout.ref | 2 +- test-cargo-miri/test.filter.stdout.ref | 2 +- test-cargo-miri/test.test-target.stdout.ref | 5 +-- test-cargo-miri/tests/test.rs | 20 ++++++++--- 8 files changed, 61 insertions(+), 14 deletions(-) diff --git a/test-cargo-miri/Cargo.lock b/test-cargo-miri/Cargo.lock index 7f06fdf28dec..4bece389dc87 100644 --- a/test-cargo-miri/Cargo.lock +++ b/test-cargo-miri/Cargo.lock @@ -22,6 +22,7 @@ dependencies = [ "issue_1705", "issue_1760", "issue_rust_86261", + "page_size", "rand", "serde_derive", ] @@ -123,6 +124,16 @@ dependencies = [ "libc", ] +[[package]] +name = "page_size" +version = "0.4.2" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "eebde548fbbf1ea81a99b128872779c437752fb99f217c45245e1a61dcd9edcd" +dependencies = [ + "libc", + "winapi", +] + [[package]] name = "ppv-lite86" version = "0.2.10" @@ -233,3 +244,25 @@ name = "wasi" version = "0.10.2+wasi-snapshot-preview1" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "fd6fbd9a79829dd1ad0cc20627bf1ed606756a7f77edff7b66b7064f9cb327c6" + +[[package]] +name = "winapi" +version = "0.3.9" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "5c839a674fcd7a98952e593242ea400abe93992746761e38641405d28b00f419" +dependencies = [ + "winapi-i686-pc-windows-gnu", + "winapi-x86_64-pc-windows-gnu", +] + +[[package]] +name = "winapi-i686-pc-windows-gnu" +version = "0.4.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "ac3b87c63620426dd9b991e5ce0329eff545bccbbb34f3be09ff6fb6ab51b7b6" + +[[package]] +name = "winapi-x86_64-pc-windows-gnu" +version = "0.4.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "712e227841d057c1ee1cd2fb22fa7e5a5461ae8e48fa2ca79ec42cfc1931183f" diff --git a/test-cargo-miri/Cargo.toml b/test-cargo-miri/Cargo.toml index 39ce1757f0e4..2193d354d5d2 100644 --- a/test-cargo-miri/Cargo.toml +++ b/test-cargo-miri/Cargo.toml @@ -22,6 +22,7 @@ rand = { version = "0.8", features = ["small_rng"] } getrandom_1 = { package = "getrandom", version = "0.1" } getrandom_2 = { package = "getrandom", version = "0.2" } serde_derive = "1.0" # not actually used, but exercises some unique code path (`--extern` .so file) +page_size = "0.4.1" [lib] test = false # test that this is respected (will show in the output) diff --git a/test-cargo-miri/test.cross-target.stdout.ref b/test-cargo-miri/test.cross-target.stdout.ref index aa4a5839b145..3673e5549d8c 100644 --- a/test-cargo-miri/test.cross-target.stdout.ref +++ b/test-cargo-miri/test.cross-target.stdout.ref @@ -5,7 +5,7 @@ test result: ok. 2 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out imported main -running 7 tests -..i.... -test result: ok. 6 passed; 0 failed; 1 ignored; 0 measured; 0 filtered out +running 8 tests +..i..... +test result: ok. 7 passed; 0 failed; 1 ignored; 0 measured; 0 filtered out diff --git a/test-cargo-miri/test.default.stdout.ref b/test-cargo-miri/test.default.stdout.ref index ee8625357509..a59108efb332 100644 --- a/test-cargo-miri/test.default.stdout.ref +++ b/test-cargo-miri/test.default.stdout.ref @@ -5,9 +5,9 @@ test result: ok. 2 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out imported main -running 7 tests -..i.... -test result: ok. 6 passed; 0 failed; 1 ignored; 0 measured; 0 filtered out +running 8 tests +..i..... +test result: ok. 7 passed; 0 failed; 1 ignored; 0 measured; 0 filtered out running 4 tests diff --git a/test-cargo-miri/test.filter.cross-target.stdout.ref b/test-cargo-miri/test.filter.cross-target.stdout.ref index b38aac9aa868..9fb7670d0649 100644 --- a/test-cargo-miri/test.filter.cross-target.stdout.ref +++ b/test-cargo-miri/test.filter.cross-target.stdout.ref @@ -8,5 +8,5 @@ imported main running 1 test test simple1 ... ok -test result: ok. 1 passed; 0 failed; 0 ignored; 0 measured; 6 filtered out +test result: ok. 1 passed; 0 failed; 0 ignored; 0 measured; 7 filtered out diff --git a/test-cargo-miri/test.filter.stdout.ref b/test-cargo-miri/test.filter.stdout.ref index d14bb8796e85..4b598960a096 100644 --- a/test-cargo-miri/test.filter.stdout.ref +++ b/test-cargo-miri/test.filter.stdout.ref @@ -8,7 +8,7 @@ imported main running 1 test test simple1 ... ok -test result: ok. 1 passed; 0 failed; 0 ignored; 0 measured; 6 filtered out +test result: ok. 1 passed; 0 failed; 0 ignored; 0 measured; 7 filtered out running 0 tests diff --git a/test-cargo-miri/test.test-target.stdout.ref b/test-cargo-miri/test.test-target.stdout.ref index 6655eb840929..ca069b702eba 100644 --- a/test-cargo-miri/test.test-target.stdout.ref +++ b/test-cargo-miri/test.test-target.stdout.ref @@ -1,12 +1,13 @@ -running 7 tests +running 8 tests test cargo_env ... ok test do_panic - should panic ... ok test does_not_work_on_miri ... ignored test entropy_rng ... ok test fail_index_check - should panic ... ok +test page_size ... ok test simple1 ... ok test simple2 ... ok -test result: ok. 6 passed; 0 failed; 1 ignored; 0 measured; 0 filtered out +test result: ok. 7 passed; 0 failed; 1 ignored; 0 measured; 0 filtered out diff --git a/test-cargo-miri/tests/test.rs b/test-cargo-miri/tests/test.rs index f0ff10ff6c0d..6758e99703cc 100644 --- a/test-cargo-miri/tests/test.rs +++ b/test-cargo-miri/tests/test.rs @@ -1,4 +1,4 @@ -use rand::{SeedableRng, Rng, rngs::SmallRng}; +use rand::{rngs::SmallRng, Rng, SeedableRng}; // Having more than 1 test does seem to make a difference // (i.e., this calls ptr::swap which having just one test does not). @@ -49,14 +49,26 @@ fn cargo_env() { } #[test] -#[should_panic(expected="Explicit panic")] -fn do_panic() { // In large, friendly letters :) +#[should_panic(expected = "Explicit panic")] +fn do_panic() { + // In large, friendly letters :) panic!("Explicit panic from test!"); } #[test] #[allow(unconditional_panic)] -#[should_panic(expected="the len is 0 but the index is 42")] +#[should_panic(expected = "the len is 0 but the index is 42")] fn fail_index_check() { [][42] } + +#[test] +fn page_size() { + let page_size = page_size::get(); + + assert!( + page_size.next_power_of_two() == page_size, + "page size not a power of two: {}", + page_size + ); +} From c4ee368acb065b783f3bc094077b4a5cdd1d7a27 Mon Sep 17 00:00:00 2001 From: Mateusz Gienieczko Date: Sat, 21 May 2022 18:17:25 +0200 Subject: [PATCH 2/8] Set page size in GetSystemInfo. --- src/shims/windows/foreign_items.rs | 15 +++++++++++++-- 1 file changed, 13 insertions(+), 2 deletions(-) diff --git a/src/shims/windows/foreign_items.rs b/src/shims/windows/foreign_items.rs index c49362d52b31..8fc599dd2d0b 100644 --- a/src/shims/windows/foreign_items.rs +++ b/src/shims/windows/foreign_items.rs @@ -119,9 +119,20 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx system_info.ptr, iter::repeat(0u8).take(system_info.layout.size.bytes() as usize), )?; - // Set number of processors. let dword_size = Size::from_bytes(4); - let num_cpus = this.mplace_field(&system_info, 6)?; + + // In WinApi SYSTEM_INFO's first field is a 4-byte union, but num_cpus + // inlines it as two 2-byte fields. In num_cpus case all fields are offset by 1 + // compared to WinApi. See https://github.com/rust-lang/miri/issues/2136#issuecomment-1133661262. + let first_field = this.mplace_field(&system_info, 0)?; + let offset = if first_field.layout.size.bytes() == 2 { 1 } else { 0 }; + + let page_size = this.mplace_field(&system_info, 1 + offset)?; + let num_cpus = this.mplace_field(&system_info, 5 + offset)?; + + // Set page size. + this.write_scalar(Scalar::from_int(PAGE_SIZE, dword_size), &page_size.into())?; + // Set number of processors. this.write_scalar(Scalar::from_int(NUM_CPUS, dword_size), &num_cpus.into())?; } From 63e98aee0cc005c27431a0eca317fecf6706b072 Mon Sep 17 00:00:00 2001 From: Mateusz Gienieczko Date: Sat, 21 May 2022 19:44:12 +0200 Subject: [PATCH 3/8] Change GetSystemInfo to explicit offset. --- src/shims/windows/foreign_items.rs | 35 +++++++++++++++++++----------- 1 file changed, 22 insertions(+), 13 deletions(-) diff --git a/src/shims/windows/foreign_items.rs b/src/shims/windows/foreign_items.rs index 8fc599dd2d0b..f9cf755c5b4f 100644 --- a/src/shims/windows/foreign_items.rs +++ b/src/shims/windows/foreign_items.rs @@ -111,6 +111,8 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx // Querying system information "GetSystemInfo" => { + use crate::rustc_middle::ty::{layout::LayoutOf, TyKind, UintTy}; + let [system_info] = this.check_shim(abi, Abi::System { unwind: false }, link_name, args)?; let system_info = this.deref_operand(system_info)?; @@ -119,21 +121,28 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx system_info.ptr, iter::repeat(0u8).take(system_info.layout.size.bytes() as usize), )?; - let dword_size = Size::from_bytes(4); - - // In WinApi SYSTEM_INFO's first field is a 4-byte union, but num_cpus - // inlines it as two 2-byte fields. In num_cpus case all fields are offset by 1 - // compared to WinApi. See https://github.com/rust-lang/miri/issues/2136#issuecomment-1133661262. - let first_field = this.mplace_field(&system_info, 0)?; - let offset = if first_field.layout.size.bytes() == 2 { 1 } else { 0 }; - - let page_size = this.mplace_field(&system_info, 1 + offset)?; - let num_cpus = this.mplace_field(&system_info, 5 + offset)?; - + // Set selected fields. + let dword_ty = this.tcx.mk_ty(TyKind::Uint(UintTy::U32)); + let dword_layout = this.layout_of(dword_ty)?; // Set page size. - this.write_scalar(Scalar::from_int(PAGE_SIZE, dword_size), &page_size.into())?; + let page_size = system_info.offset( + Size::from_bytes(4), + MemPlaceMeta::None, + dword_layout, + &this.tcx, + )?; + this.write_scalar( + Scalar::from_int(PAGE_SIZE, dword_layout.size), + &page_size.into(), + )?; // Set number of processors. - this.write_scalar(Scalar::from_int(NUM_CPUS, dword_size), &num_cpus.into())?; + let num_cpus = system_info.offset( + Size::from_bytes(32), + MemPlaceMeta::None, + dword_layout, + &this.tcx, + )?; + this.write_scalar(Scalar::from_int(NUM_CPUS, dword_layout.size), &num_cpus.into())?; } // Thread-local storage From 2fa53c03857095945b1e6ebfeec94cb39077297b Mon Sep 17 00:00:00 2001 From: Mateusz Gienieczko Date: Sat, 21 May 2022 21:42:25 +0200 Subject: [PATCH 4/8] Dynamic offset calculation in GetSystemInfo. --- src/shims/windows/foreign_items.rs | 36 ++++++++++++++++++++++++++++-- 1 file changed, 34 insertions(+), 2 deletions(-) diff --git a/src/shims/windows/foreign_items.rs b/src/shims/windows/foreign_items.rs index f9cf755c5b4f..f2852d6bcca0 100644 --- a/src/shims/windows/foreign_items.rs +++ b/src/shims/windows/foreign_items.rs @@ -8,6 +8,7 @@ use rustc_target::spec::abi::Abi; use crate::*; use shims::foreign_items::EmulateByNameResult; use shims::windows::sync::EvalContextExt as _; +use smallvec::SmallVec; impl<'mir, 'tcx: 'mir> EvalContextExt<'mir, 'tcx> for crate::MiriEvalContext<'mir, 'tcx> {} pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx> { @@ -122,11 +123,42 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx iter::repeat(0u8).take(system_info.layout.size.bytes() as usize), )?; // Set selected fields. + let word_ty = this.tcx.mk_ty(TyKind::Uint(UintTy::U16)); let dword_ty = this.tcx.mk_ty(TyKind::Uint(UintTy::U32)); + let usize_ty = this.tcx.mk_ty(TyKind::Uint(UintTy::Usize)); + let word_layout = this.layout_of(word_ty)?; let dword_layout = this.layout_of(dword_ty)?; + let usize_layout = this.layout_of(usize_ty)?; + + // Using `mplace_field` is error-prone, see: https://github.com/rust-lang/miri/issues/2136. + // Pointer fields have different sizes on different targets. + // To avoid all these issue we calculate the offsets dynamically. + let field_sizes = [ + word_layout.size, // 0, wProcessorArchitecture : WORD + word_layout.size, // 1, wReserved : WORD + dword_layout.size, // 2, dwPageSize : DWORD + usize_layout.size, // 3, lpMinimumApplicationAddress : LPVOID + usize_layout.size, // 4, lpMaximumApplicationAddress : LPVOID + usize_layout.size, // 5, dwActiveProcessorMask : DWORD_PTR + dword_layout.size, // 6, dwNumberOfProcessors : DWORD + dword_layout.size, // 7, dwProcessorType : DWORD + dword_layout.size, // 8, dwAllocationGranularity : DWORD + word_layout.size, // 9, wProcessorLevel : WORD + word_layout.size, // 10, wProcessorRevision : WORD + ]; + let field_offsets: SmallVec<[Size; 11]> = field_sizes + .iter() + .copied() + .scan(Size::ZERO, |a, x| { + let res = Some(*a); + *a += x; + res + }) + .collect(); + // Set page size. let page_size = system_info.offset( - Size::from_bytes(4), + field_offsets[2], MemPlaceMeta::None, dword_layout, &this.tcx, @@ -137,7 +169,7 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx )?; // Set number of processors. let num_cpus = system_info.offset( - Size::from_bytes(32), + field_offsets[6], MemPlaceMeta::None, dword_layout, &this.tcx, From b7d032c2199ec09a89b7aeafd7ad3b7e82cac7e7 Mon Sep 17 00:00:00 2001 From: Mateusz Gienieczko Date: Sun, 22 May 2022 00:59:49 +0200 Subject: [PATCH 5/8] Fix comment formatting. --- test-cargo-miri/tests/test.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test-cargo-miri/tests/test.rs b/test-cargo-miri/tests/test.rs index 6758e99703cc..ba027381fa5b 100644 --- a/test-cargo-miri/tests/test.rs +++ b/test-cargo-miri/tests/test.rs @@ -50,8 +50,8 @@ fn cargo_env() { #[test] #[should_panic(expected = "Explicit panic")] -fn do_panic() { - // In large, friendly letters :) +fn do_panic() // In large, friendly letters :) +{ panic!("Explicit panic from test!"); } From a40ff562a0c20f8588c9a47816515d2ed3dc2284 Mon Sep 17 00:00:00 2001 From: Mateusz Gienieczko Date: Sun, 22 May 2022 01:00:59 +0200 Subject: [PATCH 6/8] Add `i16` and `u16` primitive layout. --- src/machine.rs | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/machine.rs b/src/machine.rs index 7ab15b9f9ceb..9e95c632c7f4 100644 --- a/src/machine.rs +++ b/src/machine.rs @@ -178,9 +178,11 @@ pub struct AllocExtra { pub struct PrimitiveLayouts<'tcx> { pub unit: TyAndLayout<'tcx>, pub i8: TyAndLayout<'tcx>, + pub i16: TyAndLayout<'tcx>, pub i32: TyAndLayout<'tcx>, pub isize: TyAndLayout<'tcx>, pub u8: TyAndLayout<'tcx>, + pub u16: TyAndLayout<'tcx>, pub u32: TyAndLayout<'tcx>, pub usize: TyAndLayout<'tcx>, pub bool: TyAndLayout<'tcx>, @@ -194,9 +196,11 @@ impl<'mir, 'tcx: 'mir> PrimitiveLayouts<'tcx> { Ok(Self { unit: layout_cx.layout_of(tcx.mk_unit())?, i8: layout_cx.layout_of(tcx.types.i8)?, + i16: layout_cx.layout_of(tcx.types.i16)?, i32: layout_cx.layout_of(tcx.types.i32)?, isize: layout_cx.layout_of(tcx.types.isize)?, u8: layout_cx.layout_of(tcx.types.u8)?, + u16: layout_cx.layout_of(tcx.types.u16)?, u32: layout_cx.layout_of(tcx.types.u32)?, usize: layout_cx.layout_of(tcx.types.usize)?, bool: layout_cx.layout_of(tcx.types.bool)?, From bd731508d46e232cb674cef93c41dbcaf391ae45 Mon Sep 17 00:00:00 2001 From: Mateusz Gienieczko Date: Sun, 22 May 2022 01:01:12 +0200 Subject: [PATCH 7/8] Use precomputed layouts. --- src/shims/windows/foreign_items.rs | 13 ++++--------- 1 file changed, 4 insertions(+), 9 deletions(-) diff --git a/src/shims/windows/foreign_items.rs b/src/shims/windows/foreign_items.rs index f2852d6bcca0..1a9b2300f723 100644 --- a/src/shims/windows/foreign_items.rs +++ b/src/shims/windows/foreign_items.rs @@ -112,8 +112,6 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx // Querying system information "GetSystemInfo" => { - use crate::rustc_middle::ty::{layout::LayoutOf, TyKind, UintTy}; - let [system_info] = this.check_shim(abi, Abi::System { unwind: false }, link_name, args)?; let system_info = this.deref_operand(system_info)?; @@ -123,16 +121,13 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx iter::repeat(0u8).take(system_info.layout.size.bytes() as usize), )?; // Set selected fields. - let word_ty = this.tcx.mk_ty(TyKind::Uint(UintTy::U16)); - let dword_ty = this.tcx.mk_ty(TyKind::Uint(UintTy::U32)); - let usize_ty = this.tcx.mk_ty(TyKind::Uint(UintTy::Usize)); - let word_layout = this.layout_of(word_ty)?; - let dword_layout = this.layout_of(dword_ty)?; - let usize_layout = this.layout_of(usize_ty)?; + let word_layout = this.machine.layouts.u16; + let dword_layout = this.machine.layouts.u32; + let usize_layout = this.machine.layouts.usize; // Using `mplace_field` is error-prone, see: https://github.com/rust-lang/miri/issues/2136. // Pointer fields have different sizes on different targets. - // To avoid all these issue we calculate the offsets dynamically. + // To avoid all these issue we calculate the offsets ourselves. let field_sizes = [ word_layout.size, // 0, wProcessorArchitecture : WORD word_layout.size, // 1, wReserved : WORD From 9a5c9a54817804e2cd9ca49a83be8dad85111412 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Sun, 22 May 2022 07:59:18 +0200 Subject: [PATCH 8/8] comment on test --- test-cargo-miri/tests/test.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/test-cargo-miri/tests/test.rs b/test-cargo-miri/tests/test.rs index ba027381fa5b..1a8b3c72565d 100644 --- a/test-cargo-miri/tests/test.rs +++ b/test-cargo-miri/tests/test.rs @@ -66,6 +66,7 @@ fn fail_index_check() { fn page_size() { let page_size = page_size::get(); + // In particular, this checks that it is not 0. assert!( page_size.next_power_of_two() == page_size, "page size not a power of two: {}",