From f14161ea3535e39f4ac22edad0b626ea78110535 Mon Sep 17 00:00:00 2001 From: yvt Date: Tue, 26 Apr 2022 23:23:00 +0900 Subject: [PATCH 1/6] Add a test for volatile loads and stores --- tests/run/volatile2.rs | 119 +++++++++++++++++++++++++++++++++++++++++ 1 file changed, 119 insertions(+) create mode 100644 tests/run/volatile2.rs diff --git a/tests/run/volatile2.rs b/tests/run/volatile2.rs new file mode 100644 index 00000000000..c1bc442454e --- /dev/null +++ b/tests/run/volatile2.rs @@ -0,0 +1,119 @@ +// Compiler: +// +// Run-time: +// status: 0 + +#![no_std] +#![feature(core_intrinsics, start)] + +#[panic_handler] +fn panic_handler(_: &core::panic::PanicInfo) -> ! { + core::intrinsics::abort(); +} + +mod libc { + #[link(name = "c")] + extern "C" { + pub fn puts(s: *const u8) -> i32; + + pub fn sigaction(signum: i32, act: *const sigaction, oldact: *mut sigaction) -> i32; + pub fn mmap(addr: *mut (), len: usize, prot: i32, flags: i32, fd: i32, offset: i64) -> *mut (); + pub fn mprotect(addr: *mut (), len: usize, prot: i32) -> i32; + } + + pub const PROT_READ: i32 = 1; + pub const PROT_WRITE: i32 = 2; + pub const MAP_PRIVATE: i32 = 0x0002; + pub const MAP_ANONYMOUS: i32 = 0x0020; + pub const MAP_FAILED: *mut u8 = !0 as *mut u8; + + /// glibc sigaction + #[repr(C)] + pub struct sigaction { + pub sa_sigaction: Option, + pub sa_mask: [u32; 32], + pub sa_flags: i32, + pub sa_restorer: Option, + } + + pub const SA_SIGINFO: i32 = 0x00000004; + pub const SIGSEGV: i32 = 11; +} + +static mut COUNT: u32 = 0; +static mut STORAGE: *mut u8 = core::ptr::null_mut(); +const PAGE_SIZE: usize = 1 << 15; + +#[start] +fn main(_argc: isize, _argv: *const *const u8) -> isize { + unsafe { + // Register a segfault handler + libc::sigaction( + libc::SIGSEGV, + &libc::sigaction { + sa_sigaction: Some(segv_handler), + sa_flags: libc::SA_SIGINFO, + ..core::mem::zeroed() + }, + core::ptr::null_mut(), + ); + + STORAGE = libc::mmap( + core::ptr::null_mut(), + PAGE_SIZE * 2, + 0, + libc::MAP_PRIVATE | libc::MAP_ANONYMOUS, + -1, + 0, + ).cast(); + if STORAGE == libc::MAP_FAILED { + libc::puts(b"error: mmap failed\0".as_ptr()); + return 1; + } + + let p_count = (&mut COUNT) as *mut u32; + p_count.write_volatile(0); + + // Trigger segfaults + STORAGE.add(0).write_volatile(1); + STORAGE.add(PAGE_SIZE).write_volatile(1); + STORAGE.add(0).write_volatile(1); + STORAGE.add(PAGE_SIZE).write_volatile(1); + STORAGE.add(0).write_volatile(1); + STORAGE.add(PAGE_SIZE).write_volatile(1); + + // The segfault handler should have been called for every + // `write_volatile` in `STORAGE`. If the compiler ignores volatility, + // some of these writes will be combined, causing a different number of + // segfaults. + // + // This `p_count` read is done by a volatile read. If the compiler + // ignores volatility, the compiler will speculate that `*p_count` is + // unchanged and remove this check, failing the test. + if p_count.read_volatile() != 6 { + libc::puts(b"error: segfault count mismatch\0".as_ptr()); + return 1; + } + + 0 + } +} + +unsafe extern "C" fn segv_handler(_: i32, _: *mut (), _: *mut ()) { + let p_count = (&mut COUNT) as *mut u32; + p_count.write_volatile(p_count.read_volatile() + 1); + let count = p_count.read_volatile(); + + // Toggle the protected page so that the handler will be called for + // each `write_volatile` + libc::mprotect( + STORAGE.cast(), + PAGE_SIZE, + if count % 2 == 1 { libc::PROT_READ | libc::PROT_WRITE } else { 0 }, + ); + libc::mprotect( + STORAGE.add(PAGE_SIZE).cast(), + PAGE_SIZE, + if count % 2 == 0 { libc::PROT_READ | libc::PROT_WRITE } else { 0 }, + ); +} From 6a1ecf3ef57784c75a4808aa294b73a1d6adc616 Mon Sep 17 00:00:00 2001 From: Antoni Boucher Date: Sun, 17 Nov 2024 19:30:43 -0500 Subject: [PATCH 2/6] Add read_volatile to volatile test --- libgccjit.version | 2 +- tests/run/volatile2.rs | 36 +++++++++++++++--------------------- 2 files changed, 16 insertions(+), 22 deletions(-) diff --git a/libgccjit.version b/libgccjit.version index f6b512a0280..26503725ed8 100644 --- a/libgccjit.version +++ b/libgccjit.version @@ -1 +1 @@ -29901846ff610daab8a80436cfe36e93b4b5aa1e +50d1270fd6409407f38b982e606df1dba4bf58ed diff --git a/tests/run/volatile2.rs b/tests/run/volatile2.rs index c1bc442454e..a177b817ab3 100644 --- a/tests/run/volatile2.rs +++ b/tests/run/volatile2.rs @@ -3,14 +3,6 @@ // Run-time: // status: 0 -#![no_std] -#![feature(core_intrinsics, start)] - -#[panic_handler] -fn panic_handler(_: &core::panic::PanicInfo) -> ! { - core::intrinsics::abort(); -} - mod libc { #[link(name = "c")] extern "C" { @@ -44,8 +36,7 @@ static mut COUNT: u32 = 0; static mut STORAGE: *mut u8 = core::ptr::null_mut(); const PAGE_SIZE: usize = 1 << 15; -#[start] -fn main(_argc: isize, _argv: *const *const u8) -> isize { +fn main() { unsafe { // Register a segfault handler libc::sigaction( @@ -67,8 +58,7 @@ fn main(_argc: isize, _argv: *const *const u8) -> isize { 0, ).cast(); if STORAGE == libc::MAP_FAILED { - libc::puts(b"error: mmap failed\0".as_ptr()); - return 1; + panic!("error: mmap failed"); } let p_count = (&mut COUNT) as *mut u32; @@ -81,21 +71,25 @@ fn main(_argc: isize, _argv: *const *const u8) -> isize { STORAGE.add(PAGE_SIZE).write_volatile(1); STORAGE.add(0).write_volatile(1); STORAGE.add(PAGE_SIZE).write_volatile(1); + STORAGE.add(0).read_volatile(); + STORAGE.add(PAGE_SIZE).read_volatile(); + STORAGE.add(0).read_volatile(); + STORAGE.add(PAGE_SIZE).read_volatile(); + STORAGE.add(0).read_volatile(); + STORAGE.add(PAGE_SIZE).read_volatile(); + STORAGE.add(0).write_volatile(1); + STORAGE.add(PAGE_SIZE).write_volatile(1); - // The segfault handler should have been called for every - // `write_volatile` in `STORAGE`. If the compiler ignores volatility, - // some of these writes will be combined, causing a different number of - // segfaults. + // The segfault handler should have been called for every `write_volatile` and + // `read_volatile` in `STORAGE`. If the compiler ignores volatility, some of these writes + // will be combined, causing a different number of segfaults. // // This `p_count` read is done by a volatile read. If the compiler // ignores volatility, the compiler will speculate that `*p_count` is // unchanged and remove this check, failing the test. - if p_count.read_volatile() != 6 { - libc::puts(b"error: segfault count mismatch\0".as_ptr()); - return 1; + if p_count.read_volatile() != 14 { + panic!("error: segfault count mismatch: {}", p_count.read_volatile()); } - - 0 } } From a583963bd3008e32596779264f03353d8a1e2f15 Mon Sep 17 00:00:00 2001 From: yvt Date: Wed, 27 Apr 2022 00:45:17 +0900 Subject: [PATCH 3/6] Implement volatile store --- src/builder.rs | 28 +++++++++++++++++++++------- 1 file changed, 21 insertions(+), 7 deletions(-) diff --git a/src/builder.rs b/src/builder.rs index 9936bc1f5f2..459110bf9db 100644 --- a/src/builder.rs +++ b/src/builder.rs @@ -1103,21 +1103,35 @@ impl<'a, 'gcc, 'tcx> BuilderMethods<'a, 'tcx> for Builder<'a, 'gcc, 'tcx> { fn store_with_flags( &mut self, - val: RValue<'gcc>, + mut val: RValue<'gcc>, ptr: RValue<'gcc>, align: Align, - _flags: MemFlags, + flags: MemFlags, ) -> RValue<'gcc> { let ptr = self.check_store(val, ptr); let destination = ptr.dereference(self.location); // NOTE: libgccjit does not support specifying the alignment on the assignment, so we cast // to type so it gets the proper alignment. let destination_type = destination.to_rvalue().get_type().unqualified(); - let aligned_type = destination_type.get_aligned(align.bytes()).make_pointer(); - let aligned_destination = self.cx.context.new_bitcast(self.location, ptr, aligned_type); - let aligned_destination = aligned_destination.dereference(self.location); - self.llbb().add_assignment(self.location, aligned_destination, val); - // TODO(antoyo): handle align and flags. + let align = if flags.contains(MemFlags::UNALIGNED) { 1 } else { align.bytes() }; + let mut modified_destination_type = destination_type.get_aligned(align); + if flags.contains(MemFlags::VOLATILE) { + modified_destination_type = modified_destination_type.make_volatile(); + } + + // FIXME: The type checking in `add_assignment` removes only one + // qualifier from each side. So the writes `int → volatile int` and + // `int → int __attribute__((aligned(1)))` are considered legal but + // `int → volatile int __attribute__((aligned(1)))` is not. This seems + // like a bug in libgccjit. The easiest way to work around this is to + // bitcast `val` to have the matching qualifiers. + val = self.cx.context.new_bitcast(None, val, modified_destination_type); + + let modified_ptr = + self.cx.context.new_bitcast(None, ptr, modified_destination_type.make_pointer()); + let modified_destination = modified_ptr.dereference(None); + self.llbb().add_assignment(None, modified_destination, val); + // TODO: handle `MemFlags::NONTEMPORAL`. // NOTE: dummy value here since it's never used. FIXME(antoyo): API should not return a value here? self.cx.context.new_rvalue_zero(self.type_i32()) } From 337a67de4942a0d7b04e0de665304fd0f7729ef6 Mon Sep 17 00:00:00 2001 From: Antoni Boucher Date: Mon, 18 Nov 2024 18:37:45 -0500 Subject: [PATCH 4/6] Add username to FIXME and TODO --- src/builder.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/builder.rs b/src/builder.rs index 459110bf9db..2839ff302a3 100644 --- a/src/builder.rs +++ b/src/builder.rs @@ -1119,7 +1119,7 @@ impl<'a, 'gcc, 'tcx> BuilderMethods<'a, 'tcx> for Builder<'a, 'gcc, 'tcx> { modified_destination_type = modified_destination_type.make_volatile(); } - // FIXME: The type checking in `add_assignment` removes only one + // FIXME(antoyo): The type checking in `add_assignment` removes only one // qualifier from each side. So the writes `int → volatile int` and // `int → int __attribute__((aligned(1)))` are considered legal but // `int → volatile int __attribute__((aligned(1)))` is not. This seems @@ -1131,7 +1131,7 @@ impl<'a, 'gcc, 'tcx> BuilderMethods<'a, 'tcx> for Builder<'a, 'gcc, 'tcx> { self.cx.context.new_bitcast(None, ptr, modified_destination_type.make_pointer()); let modified_destination = modified_ptr.dereference(None); self.llbb().add_assignment(None, modified_destination, val); - // TODO: handle `MemFlags::NONTEMPORAL`. + // TODO(antoyo): handle `MemFlags::NONTEMPORAL`. // NOTE: dummy value here since it's never used. FIXME(antoyo): API should not return a value here? self.cx.context.new_rvalue_zero(self.type_i32()) } From c2e6d38b8a44f92133c2cc44dace96fccc357d39 Mon Sep 17 00:00:00 2001 From: Antoni Boucher Date: Mon, 18 Nov 2024 18:39:41 -0500 Subject: [PATCH 5/6] Remove unnecessary bitcast --- src/builder.rs | 12 ++---------- 1 file changed, 2 insertions(+), 10 deletions(-) diff --git a/src/builder.rs b/src/builder.rs index 2839ff302a3..15ebcf42b3f 100644 --- a/src/builder.rs +++ b/src/builder.rs @@ -1103,7 +1103,7 @@ impl<'a, 'gcc, 'tcx> BuilderMethods<'a, 'tcx> for Builder<'a, 'gcc, 'tcx> { fn store_with_flags( &mut self, - mut val: RValue<'gcc>, + val: RValue<'gcc>, ptr: RValue<'gcc>, align: Align, flags: MemFlags, @@ -1119,16 +1119,8 @@ impl<'a, 'gcc, 'tcx> BuilderMethods<'a, 'tcx> for Builder<'a, 'gcc, 'tcx> { modified_destination_type = modified_destination_type.make_volatile(); } - // FIXME(antoyo): The type checking in `add_assignment` removes only one - // qualifier from each side. So the writes `int → volatile int` and - // `int → int __attribute__((aligned(1)))` are considered legal but - // `int → volatile int __attribute__((aligned(1)))` is not. This seems - // like a bug in libgccjit. The easiest way to work around this is to - // bitcast `val` to have the matching qualifiers. - val = self.cx.context.new_bitcast(None, val, modified_destination_type); - let modified_ptr = - self.cx.context.new_bitcast(None, ptr, modified_destination_type.make_pointer()); + self.cx.context.new_cast(None, ptr, modified_destination_type.make_pointer()); let modified_destination = modified_ptr.dereference(None); self.llbb().add_assignment(None, modified_destination, val); // TODO(antoyo): handle `MemFlags::NONTEMPORAL`. From b9cb8ebca6fdbb4217ddd9d90c7136e38c5e99d4 Mon Sep 17 00:00:00 2001 From: Antoni Boucher Date: Mon, 18 Nov 2024 18:55:57 -0500 Subject: [PATCH 6/6] Add missing location info --- src/builder.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/builder.rs b/src/builder.rs index 15ebcf42b3f..97a1a175f20 100644 --- a/src/builder.rs +++ b/src/builder.rs @@ -1120,9 +1120,9 @@ impl<'a, 'gcc, 'tcx> BuilderMethods<'a, 'tcx> for Builder<'a, 'gcc, 'tcx> { } let modified_ptr = - self.cx.context.new_cast(None, ptr, modified_destination_type.make_pointer()); - let modified_destination = modified_ptr.dereference(None); - self.llbb().add_assignment(None, modified_destination, val); + self.cx.context.new_cast(self.location, ptr, modified_destination_type.make_pointer()); + let modified_destination = modified_ptr.dereference(self.location); + self.llbb().add_assignment(self.location, modified_destination, val); // TODO(antoyo): handle `MemFlags::NONTEMPORAL`. // NOTE: dummy value here since it's never used. FIXME(antoyo): API should not return a value here? self.cx.context.new_rvalue_zero(self.type_i32())