-
Notifications
You must be signed in to change notification settings - Fork 13.4k
Deprecate Read::initializer in favor of ptr::freeze #58363
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
Changes from all commits
5e0fb23
c21e79e
e4d316e
73133ee
1671e90
0529921
695feb1
9d4f07e
055bcb6
78e2233
ec682d1
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -946,6 +946,73 @@ pub unsafe fn write_volatile<T>(dst: *mut T, src: T) { | |||||
intrinsics::volatile_store(dst, src); | ||||||
} | ||||||
|
||||||
/// Freezes `count * size_of::<T>()` bytes of memory, converting uninitialized data into | ||||||
/// arbitrary but fixed data. | ||||||
sfackler marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
/// | ||||||
/// Uninitialized memory has undefined contents, and interaction with those contents | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In #58468, I use terminology like "uninitialized memory does not have a fixed value/content", to try and distinguish it from memory that contains unknown but fixed data. Do you think it would make sense to also use such terminology here, to explain why interactions can cause UB? |
||||||
/// can easily cause undefined behavior. Freezing the memory avoids those issues by | ||||||
/// converting the memory to an initialized state without actually needing to write to | ||||||
/// all of it. This function has no effect on memory which is already initialized. | ||||||
/// | ||||||
/// While this function does not actually physically write to memory, it acts as if it | ||||||
/// does. For example, calling this method on data which is concurrently accessible | ||||||
/// elsewhere is undefined behavior just as it would be to use `ptr::write`. | ||||||
/// | ||||||
/// # Warning | ||||||
/// | ||||||
/// Take care when using this function as the uninitialized memory being frozen can | ||||||
/// contain bits and pieces of previously discarded data, including sensitive | ||||||
/// information such as cryptographic keys. | ||||||
/// | ||||||
/// # Safety | ||||||
/// | ||||||
/// Behavior is undefined if any of the following conditions are violated: | ||||||
/// | ||||||
/// * `dst` must be [valid] for writes. | ||||||
/// | ||||||
/// * Every bit representation of `T` must be a valid value. | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why is that a precondition to
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I thought that what you meant in this comment: #58363 (comment). If the validity requirements are less strict I can move this to be a more general warning about use of the function rather than a hard UB constraint. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The phrasing here seems congruent with the behavior of There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The validity requirements come in once you actually operate on or a make a copy of a value at some type. So the following is UB: let x = MaybeUninit::<bool>::uninitialized();
ptr::freeze(x.as_mut_ptr());
let x = x.into_initialized(); // UB But it's not the freeze that is UB, it's the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @RalfJung Oh... you're talking about |
||||||
/// | ||||||
/// Note that even if `T` has size `0`, the pointer must be non-NULL and properly aligned. | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Note that this formulation is used consistently throughout this file, so I'd prefer that if it gets changed that happens in a separate PR. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah; that's a good idea; I'll see if I can remember to make such a PR... :) |
||||||
/// | ||||||
/// [valid]: ../ptr/index.html#safety | ||||||
/// | ||||||
/// # Examples | ||||||
/// | ||||||
/// ```ignore (io-is-in-std) | ||||||
/// use std::io::{self, Read}; | ||||||
/// use std::mem; | ||||||
/// use std::ptr; | ||||||
/// | ||||||
/// pub fn read_le_u32<R>(reader: &mut R) -> io::Result<u32> | ||||||
/// where | ||||||
/// R: Read, | ||||||
/// { | ||||||
/// unsafe { | ||||||
/// // We're passing this buffer to an arbitrary reader and aren't | ||||||
/// // guaranteed they won't read from it, so freeze to avoid UB. | ||||||
/// let mut buf: [u8; 4] = mem::uninitialized(); | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could you add a FIXME somewhere about porting this to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. (This is still open, from what I can see) |
||||||
/// ptr::freeze(buf.as_mut_ptr(), buf.len()); | ||||||
/// reader.read_exact(&mut buf)?; | ||||||
/// | ||||||
/// Ok(u32::from_le_bytes(buf)) | ||||||
/// } | ||||||
/// } | ||||||
/// ``` | ||||||
#[inline] | ||||||
#[unstable(feature = "ptr_freeze", issue = "0")] | ||||||
#[cfg(not(stage0))] | ||||||
pub unsafe fn freeze<T>(dst: *mut T, count: usize) { | ||||||
intrinsics::freeze(dst, count) | ||||||
} | ||||||
/// | ||||||
#[inline] | ||||||
#[unstable(feature = "ptr_freeze", issue = "0")] | ||||||
#[cfg(stage0)] | ||||||
pub unsafe fn freeze<T>(dst: *mut T, count: usize) { | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this the right interface? It's currently a bit weird in that we don't actually use the count. It could alternatively just take the pointer, and say that it freezes all memory reachable through it? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Also, should this be unsafe in the first place? Since it's not actually modifying any of the pointed-to data, does it matter if it's valid or not? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Since it's already "basically stable", I wonder if this should take I'd naively think that it could be safe and probably should be, but I'm not an expert! We also briefly discussed maybe only taking There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think it should take raw pointers so people don't have to create references to uninitialized data.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could we change this to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. AFAIK there's currently no way to form a |
||||||
let _ = count; | ||||||
asm!("" :: "r"(dst) : "memory" : "volatile"); | ||||||
} | ||||||
|
||||||
#[lang = "const_ptr"] | ||||||
impl<T: ?Sized> *const T { | ||||||
/// Returns `true` if the pointer is null. | ||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -16,7 +16,8 @@ use rustc::ty::{self, Ty}; | |
use rustc::ty::layout::{self, LayoutOf, HasTyCtxt, Primitive}; | ||
use rustc_codegen_ssa::common::{IntPredicate, TypeKind}; | ||
use rustc::hir; | ||
use syntax::ast::{self, FloatTy}; | ||
use std::ffi::CStr; | ||
use syntax::ast::{self, FloatTy, AsmDialect}; | ||
use syntax::symbol::Symbol; | ||
use builder::Builder; | ||
use value::Value; | ||
|
@@ -695,6 +696,23 @@ impl IntrinsicCallMethods<'tcx> for Builder<'a, 'll, 'tcx> { | |
return; | ||
} | ||
|
||
"freeze" => { | ||
let dst = args[0].immediate(); | ||
let r = self.inline_asm_call( | ||
CStr::from_bytes_with_nul(b"\0").unwrap(), | ||
CStr::from_bytes_with_nul(b"r,~{memory}\0").unwrap(), | ||
&[dst], | ||
self.type_void(), | ||
true, | ||
false, | ||
AsmDialect::Att, | ||
); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We could optimize this when A memory clobber can be expensive since it forces the compiler to reload the values of all values that are potentially globally visible. This optimization requires There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There are inlining and (limited) constant propagation passes for MIR but the former is not enabled by default and it's not clear when it will be. I can't find any indication that your proposal will actually result in improved codegen (see this test case). Pointee types in LLVM IR are meaningless and slated for removal (if only someone would push that refactoring over the finish line), so I see no reason why the LLVM IR pointee type should have bearing on how many bytes the inline asm can write through the pointer. Now, if there was a way to indicate that the inline asm guarantees it only accesses a certain range of bytes around the address that's passed in, that would help, but AFAIK there's no such thing. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hmm, I tried this out in C: https://godbolt.org/z/Fbz1o5 It seems that GCC interprets "m" constraints very precisely, while it seems that LLVM just treats all memory constraints (even read-only memory inputs) as a general memory clobber. I suppose that in this case we can just stick to the existing formulation since there is no advantage in optimizing it. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, it seems like this is just going to be inherently a bit imprecise until LLVM lands freeze support on its end. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I cannot review this part, as I have no knowledge of these assembly annotation. @Amanieu could you confirm that this tells the compiler that the assembly block may read and/or write all memory reachable from There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
@comex put it succinctly here: rust-lang/rfcs#2360 (comment)
|
||
if r.is_none() { | ||
bug!("broken freeze ASM"); | ||
} | ||
return; | ||
} | ||
|
||
_ => bug!("unknown intrinsic '{}'", name), | ||
}; | ||
|
||
|
Uh oh!
There was an error while loading. Please reload this page.