Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Get rid of some #![allow(static_mut_refs)] #121303

Merged
merged 1 commit into from
Feb 23, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
39 changes: 17 additions & 22 deletions library/panic_unwind/src/seh.rs
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@
use alloc::boxed::Box;
use core::any::Any;
use core::mem::{self, ManuallyDrop};
use core::ptr;
use core::ptr::{addr_of, addr_of_mut};
use libc::{c_int, c_uint, c_void};

// NOTE(nbdd0121): The `canary` field is part of stable ABI.
Expand Down Expand Up @@ -135,7 +135,7 @@ mod imp {
macro_rules! ptr {
(0) => (0);
($e:expr) => {
(($e as usize) - (&imp::__ImageBase as *const _ as usize)) as u32
(($e as usize) - (addr_of!(imp::__ImageBase) as usize)) as u32
}
}
}
Expand Down Expand Up @@ -220,7 +220,7 @@ extern "C" {
// This is fine since the MSVC runtime uses string comparison on the type name
// to match TypeDescriptors rather than pointer equality.
static mut TYPE_DESCRIPTOR: _TypeDescriptor = _TypeDescriptor {
pVFTable: unsafe { &TYPE_INFO_VTABLE } as *const _ as *const _,
Copy link
Member

@RalfJung RalfJung Feb 23, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Strange, did this one not trigger the warning? It doesn't seem to be inside any of the allow.

Copy link
Contributor Author

@GrigorenkoPV GrigorenkoPV Feb 23, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it said something about external statics iirc

(If you mean why I left the unsafe block in place)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Strange, did this one not trigger the warning? It doesn't seem to be inside any of the allow.

Ah, I guess this is because it is not a mut static:
https://github.com/rust-lang/rust/pull/121303/files#diff-1308e363d41cbb1f3f93d560e000263610240cc456596918347038d303dac7b6L214

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah I see, you just made them all use addr_of for good measure. Makes sense!

Copy link
Contributor Author

@GrigorenkoPV GrigorenkoPV Feb 23, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah I see, you just made them all use addr_of for good measure. Makes sense!

I think there are many more places where & ... as *const ... is used. Would it make sense to implement a lint (probably allow-by-default) suggesting to replace those with addr_of!? Or maybe at least for me to go through the libs and change those manually?

UPD: Apparently (and honestly unsurprisingly) there is already a clippy lint for that: borrow_as_ptr.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah those should all be addr_of ideally.

pVFTable: unsafe { addr_of!(TYPE_INFO_VTABLE) } as *const _,
spare: core::ptr::null_mut(),
name: TYPE_NAME,
};
Expand Down Expand Up @@ -261,9 +261,6 @@ cfg_if::cfg_if! {
}
}

// FIXME: Use `SyncUnsafeCell` instead of allowing `static_mut_refs` lint
#[cfg_attr(bootstrap, allow(static_mut_ref))]
#[cfg_attr(not(bootstrap), allow(static_mut_refs))]
pub unsafe fn panic(data: Box<dyn Any + Send>) -> u32 {
use core::intrinsics::atomic_store_seqcst;

Expand All @@ -274,8 +271,9 @@ pub unsafe fn panic(data: Box<dyn Any + Send>) -> u32 {
// The ManuallyDrop is needed here since we don't want Exception to be
// dropped when unwinding. Instead it will be dropped by exception_cleanup
// which is invoked by the C++ runtime.
let mut exception = ManuallyDrop::new(Exception { canary: &TYPE_DESCRIPTOR, data: Some(data) });
let throw_ptr = &mut exception as *mut _ as *mut _;
let mut exception =
ManuallyDrop::new(Exception { canary: addr_of!(TYPE_DESCRIPTOR), data: Some(data) });
let throw_ptr = addr_of_mut!(exception) as *mut _;

// This... may seems surprising, and justifiably so. On 32-bit MSVC the
// pointers between these structure are just that, pointers. On 64-bit MSVC,
Expand All @@ -298,45 +296,42 @@ pub unsafe fn panic(data: Box<dyn Any + Send>) -> u32 {
// In any case, we basically need to do something like this until we can
// express more operations in statics (and we may never be able to).
atomic_store_seqcst(
&mut THROW_INFO.pmfnUnwind as *mut _ as *mut u32,
addr_of_mut!(THROW_INFO.pmfnUnwind) as *mut u32,
ptr!(exception_cleanup) as u32,
);
atomic_store_seqcst(
&mut THROW_INFO.pCatchableTypeArray as *mut _ as *mut u32,
ptr!(&CATCHABLE_TYPE_ARRAY as *const _) as u32,
addr_of_mut!(THROW_INFO.pCatchableTypeArray) as *mut u32,
ptr!(addr_of!(CATCHABLE_TYPE_ARRAY)) as u32,
);
atomic_store_seqcst(
&mut CATCHABLE_TYPE_ARRAY.arrayOfCatchableTypes[0] as *mut _ as *mut u32,
ptr!(&CATCHABLE_TYPE as *const _) as u32,
addr_of_mut!(CATCHABLE_TYPE_ARRAY.arrayOfCatchableTypes[0]) as *mut u32,
ptr!(addr_of!(CATCHABLE_TYPE)) as u32,
);
atomic_store_seqcst(
&mut CATCHABLE_TYPE.pType as *mut _ as *mut u32,
ptr!(&TYPE_DESCRIPTOR as *const _) as u32,
addr_of_mut!(CATCHABLE_TYPE.pType) as *mut u32,
ptr!(addr_of!(TYPE_DESCRIPTOR)) as u32,
);
atomic_store_seqcst(
&mut CATCHABLE_TYPE.copyFunction as *mut _ as *mut u32,
addr_of_mut!(CATCHABLE_TYPE.copyFunction) as *mut u32,
ptr!(exception_copy) as u32,
);

extern "system-unwind" {
fn _CxxThrowException(pExceptionObject: *mut c_void, pThrowInfo: *mut u8) -> !;
}

_CxxThrowException(throw_ptr, &mut THROW_INFO as *mut _ as *mut _);
_CxxThrowException(throw_ptr, addr_of_mut!(THROW_INFO) as *mut _);
}

// FIXME: Use `SyncUnsafeCell` instead of allowing `static_mut_refs` lint
#[cfg_attr(bootstrap, allow(static_mut_ref))]
#[cfg_attr(not(bootstrap), allow(static_mut_refs))]
pub unsafe fn cleanup(payload: *mut u8) -> Box<dyn Any + Send> {
// A null payload here means that we got here from the catch (...) of
// __rust_try. This happens when a non-Rust foreign exception is caught.
if payload.is_null() {
super::__rust_foreign_exception();
}
let exception = payload as *mut Exception;
let canary = ptr::addr_of!((*exception).canary).read();
if !ptr::eq(canary, &TYPE_DESCRIPTOR) {
let canary = addr_of!((*exception).canary).read();
if !core::ptr::eq(canary, addr_of!(TYPE_DESCRIPTOR)) {
// A foreign Rust exception.
super::__rust_foreign_exception();
}
Expand Down
3 changes: 0 additions & 3 deletions library/std/src/panicking.rs
Original file line number Diff line number Diff line change
Expand Up @@ -337,9 +337,6 @@ pub mod panic_count {
#[doc(hidden)]
#[cfg(not(feature = "panic_immediate_abort"))]
#[unstable(feature = "update_panic_count", issue = "none")]
// FIXME: Use `SyncUnsafeCell` instead of allowing `static_mut_refs` lint
#[cfg_attr(bootstrap, allow(static_mut_ref))]
#[cfg_attr(not(bootstrap), allow(static_mut_refs))]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like no fix was needed for these. This does make me wonder why they were ever added... 🤷

pub mod panic_count {
use crate::cell::Cell;
use crate::sync::atomic::{AtomicUsize, Ordering};
Expand Down
2 changes: 0 additions & 2 deletions library/std/src/thread/local.rs
Original file line number Diff line number Diff line change
Expand Up @@ -180,8 +180,6 @@ impl<T: 'static> fmt::Debug for LocalKey<T> {
#[stable(feature = "rust1", since = "1.0.0")]
#[cfg_attr(not(test), rustc_diagnostic_item = "thread_local_macro")]
#[allow_internal_unstable(thread_local_internals)]
// FIXME: Use `SyncUnsafeCell` instead of allowing `static_mut_refs` lint
#[cfg_attr(not(bootstrap), allow(static_mut_refs))]
macro_rules! thread_local {
// empty (base case for the recursion)
() => {};
Expand Down
6 changes: 3 additions & 3 deletions src/tools/miri/tests/fail/tls/tls_static_dealloc.rs
Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
//! Ensure that thread-local statics get deallocated when the thread dies.

#![feature(thread_local)]
// FIXME: Use `SyncUnsafeCell` instead of allowing `static_mut_refs` lint
#![allow(static_mut_refs)]

use std::ptr::addr_of;

#[thread_local]
static mut TLS: u8 = 0;
Expand All @@ -12,7 +12,7 @@ unsafe impl Send for SendRaw {}

fn main() {
unsafe {
let dangling_ptr = std::thread::spawn(|| SendRaw(&TLS as *const u8)).join().unwrap();
let dangling_ptr = std::thread::spawn(|| SendRaw(addr_of!(TLS))).join().unwrap();
let _val = *dangling_ptr.0; //~ ERROR: has been freed
}
}
6 changes: 3 additions & 3 deletions src/tools/miri/tests/pass/static_mut.rs
Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
use std::ptr::addr_of;

static mut FOO: i32 = 42;

// FIXME: Use `SyncUnsafeCell` instead of allowing `static_mut_refs` lint
#[allow(static_mut_refs)]
static BAR: Foo = Foo(unsafe { &FOO as *const _ });
static BAR: Foo = Foo(unsafe { addr_of!(FOO) });

#[allow(dead_code)]
struct Foo(*const i32);
Expand Down
17 changes: 8 additions & 9 deletions src/tools/miri/tests/pass/tls/tls_static.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,9 +8,8 @@
//! test, we also check that thread-locals act as per-thread statics.

#![feature(thread_local)]
// FIXME: Use `SyncUnsafeCell` instead of allowing `static_mut_refs` lint
#![allow(static_mut_refs)]

use std::ptr::addr_of_mut;
use std::thread;

#[thread_local]
Expand All @@ -23,8 +22,8 @@ static mut C: u8 = 0;
#[thread_local]
static READ_ONLY: u8 = 42;

unsafe fn get_a_ref() -> *mut u8 {
&mut A
unsafe fn get_a_ptr() -> *mut u8 {
addr_of_mut!(A)
}

struct Sender(*mut u8);
Expand All @@ -35,12 +34,12 @@ fn main() {
let _val = READ_ONLY;

let ptr = unsafe {
let x = get_a_ref();
let x = get_a_ptr();
*x = 5;
assert_eq!(A, 5);
B = 15;
C = 25;
Sender(&mut A)
Sender(addr_of_mut!(A))
};

thread::spawn(move || unsafe {
Expand All @@ -51,18 +50,18 @@ fn main() {
assert_eq!(C, 25);
B = 14;
C = 24;
let y = get_a_ref();
let y = get_a_ptr();
assert_eq!(*y, 0);
*y = 4;
assert_eq!(*ptr.0, 5);
assert_eq!(A, 4);
assert_eq!(*get_a_ref(), 4);
assert_eq!(*get_a_ptr(), 4);
})
.join()
.unwrap();

unsafe {
assert_eq!(*get_a_ref(), 5);
assert_eq!(*get_a_ptr(), 5);
assert_eq!(A, 5);
assert_eq!(B, 15);
assert_eq!(C, 24);
Expand Down
Loading