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

Make copy[_nonoverlapping] const #79684

Merged
merged 6 commits into from
Dec 30, 2020
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
23 changes: 23 additions & 0 deletions compiler/rustc_mir/src/interpret/intrinsics.rs
Original file line number Diff line number Diff line change
Expand Up @@ -322,6 +322,29 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
let result = Scalar::from_uint(truncated_bits, layout.size);
self.write_scalar(result, dest)?;
}
sym::copy | sym::copy_nonoverlapping => {
let elem_ty = instance.substs.type_at(0);
let elem_layout = self.layout_of(elem_ty)?;
let count = self.read_scalar(args[2])?.to_machine_usize(self)?;
let elem_align = elem_layout.align.abi;

let size = elem_layout.size.checked_mul(count, self).ok_or_else(|| {
err_ub_format!("overflow computing total size of `{}`", intrinsic_name)
})?;
let src = self.read_scalar(args[0])?.check_init()?;
let src = self.memory.check_ptr_access(src, size, elem_align)?;
let dest = self.read_scalar(args[1])?.check_init()?;
let dest = self.memory.check_ptr_access(dest, size, elem_align)?;

if let (Some(src), Some(dest)) = (src, dest) {
self.memory.copy(
src,
dest,
size,
intrinsic_name == sym::copy_nonoverlapping,
)?;
}
}
sym::offset => {
let ptr = self.read_scalar(args[0])?.check_init()?;
let offset_count = self.read_scalar(args[1])?.to_machine_isize(self)?;
Expand Down
17 changes: 11 additions & 6 deletions library/core/src/intrinsics.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1846,20 +1846,22 @@ pub(crate) fn is_nonoverlapping<T>(src: *const T, dst: *const T, count: usize) -
/// [`Vec::append`]: ../../std/vec/struct.Vec.html#method.append
#[doc(alias = "memcpy")]
#[stable(feature = "rust1", since = "1.0.0")]
#[rustc_const_unstable(feature = "const_intrinsic_copy", issue = "none")]
#[inline]
pub unsafe fn copy_nonoverlapping<T>(src: *const T, dst: *mut T, count: usize) {
pub const unsafe fn copy_nonoverlapping<T>(src: *const T, dst: *mut T, count: usize) {
extern "rust-intrinsic" {
fn copy_nonoverlapping<T>(src: *const T, dst: *mut T, count: usize);
}

if cfg!(debug_assertions)
// FIXME: Perform these checks only at run time
/*if cfg!(debug_assertions)
RalfJung marked this conversation as resolved.
Show resolved Hide resolved
&& !(is_aligned_and_not_null(src)
&& is_aligned_and_not_null(dst)
&& is_nonoverlapping(src, dst, count))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is it possible to make this work in const fn? Is there any overlap with what is done in compiler/rustc_mir/src/interpret/intrinsics.rs, or is that only affecting CTFE?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

is_aligned_and_not_null() and is_nonoverlapping() both seems to do some *const _ as usize plus math which I believe is a no no during CTFE?

Copy link
Contributor

Choose a reason for hiding this comment

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

For is_nonoverlapping you could create another intrinsic that implements the logic in the const evaluator, but then you'd have to implement a normal codegen version of that, too.

The other two can actually be implement in a const-evaluable way with https://doc.rust-lang.org/std/primitive.pointer.html#method.align_offset

That said... Since the const-eval intrinsic checks all these invariants anyway, what we can do is to finally introduce a way to run different code at runtime vs at compile-time rust-lang/const-eval#7 (comment)

This is as good as a PR to do that as any. Do you want to tackle that or is it more than you wanted on your plate right now? It may be more effective to do just the new intrinsic in a separate PR so we have some unit tests before we use it in copy_nonoverlapping

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am really new working on the compiler and do not really know what to do. If you think it might be a not-too-great first thing to implement then I will happily start with something else. Otherwise, if I knew where to start and given some (probably lots of) pointers along the way... Maybe I could give it a try, but I would not want to promise anything and am not sure about when and how much time I can spend on it. I certainly would not want to stand in the way for anyone else who want to give it a go.

Copy link
Contributor

@oli-obk oli-obk Dec 4, 2020

Choose a reason for hiding this comment

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

I just did a dive into the compiler to look at what needs to be done and while the const eval parts are fairly self contained, the codegen parts are not, so... I don't think we can make copy_nonoverlapping const fn for now, but copy could still be doable by replacing the implementation of the debug assertion functions at

pub(crate) fn is_aligned_and_not_null<T>(ptr: *const T) -> bool {
with a const fn capable one. This should be doable by using guaranteed_ne for the null pointer check and align_offset for the alignment check. (you should be able to experiment with this on the playground, so you don't have to keep recompiling libcore).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Would it be useful to continue working on this PR draft, or open a more restricted one, as an example use case/background for the problem and one potential solution?

Copy link
Member

Choose a reason for hiding this comment

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

For this PR, I see two options:

  • Leave it as "something we can do once we have a story for const-dependent dispatch".
  • Comment out the debug assertions for now. Their usefulness is anyway limited since the libstd everyone uses is compiled without debug assertions.

I guess the question is one of evaluating the relative usefulness of these new const operations vs the assertions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For this PR, I see two options:

  • Leave it as "something we can do once we have a story for const-dependent dispatch".
  • Comment out the debug assertions for now. Their usefulness is anyway limited since the libstd everyone uses is compiled without debug assertions.

I guess the question is one of evaluating the relative usefulness of these new const operations vs the assertions.

I have constified ptr::read[_unaligned] and ptr::write locally by commenting out the call to is_nonoverlapping and align_offset. I assume we can always remove the comments to bring back the checks and wait for const-dependent dispatch should we go for the latter option, right? ptr::read[_unaligned] requires #79621, should I rebase and push my changes (does not include the unleash_the_miri_inside_of_you_here attribute)?

Copy link
Contributor

Choose a reason for hiding this comment

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

ptr::read[_unaligned] requires #79621, should I rebase and push my changes (does not include the unleash_the_miri_inside_of_you_here attribute)?

yes, that sounds right

Copy link
Member

Choose a reason for hiding this comment

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

by commenting out the call to is_nonoverlapping and align_offset

Do you mean is_aligned_and_not_null? Which align_offset call?

{
// Not panicking to keep codegen impact smaller.
abort();
}
}*/

// SAFETY: the safety contract for `copy_nonoverlapping` must be
// upheld by the caller.
Expand Down Expand Up @@ -1928,16 +1930,19 @@ pub unsafe fn copy_nonoverlapping<T>(src: *const T, dst: *mut T, count: usize) {
/// ```
#[doc(alias = "memmove")]
#[stable(feature = "rust1", since = "1.0.0")]
#[rustc_const_unstable(feature = "const_intrinsic_copy", issue = "none")]
#[inline]
pub unsafe fn copy<T>(src: *const T, dst: *mut T, count: usize) {
pub const unsafe fn copy<T>(src: *const T, dst: *mut T, count: usize) {
extern "rust-intrinsic" {
#[rustc_const_unstable(feature = "const_intrinsic_copy", issue = "none")]
fn copy<T>(src: *const T, dst: *mut T, count: usize);
}

if cfg!(debug_assertions) && !(is_aligned_and_not_null(src) && is_aligned_and_not_null(dst)) {
// FIXME: Perform these checks only at run time
/*if cfg!(debug_assertions) && !(is_aligned_and_not_null(src) && is_aligned_and_not_null(dst)) {
usbalbin marked this conversation as resolved.
Show resolved Hide resolved
// Not panicking to keep codegen impact smaller.
abort();
}
}*/

// SAFETY: the safety contract for `copy` must be upheld by the caller.
unsafe { copy(src, dst, count) }
Expand Down
2 changes: 2 additions & 0 deletions library/core/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,7 @@
#![feature(const_assert_type)]
#![feature(const_discriminant)]
#![feature(const_cell_into_inner)]
#![feature(const_intrinsic_copy)]
#![feature(const_checked_int_methods)]
#![feature(const_euclidean_int_methods)]
#![feature(const_float_classify)]
Expand All @@ -93,6 +94,7 @@
#![feature(const_precise_live_drops)]
#![feature(const_ptr_offset)]
#![feature(const_ptr_offset_from)]
#![feature(const_ptr_read)]
#![feature(const_raw_ptr_comparison)]
#![feature(const_raw_ptr_deref)]
#![feature(const_slice_from_raw_parts)]
Expand Down
3 changes: 2 additions & 1 deletion library/core/src/mem/maybe_uninit.rs
Original file line number Diff line number Diff line change
Expand Up @@ -575,8 +575,9 @@ impl<T> MaybeUninit<T> {
/// // they both get dropped!
/// ```
#[unstable(feature = "maybe_uninit_extra", issue = "63567")]
#[rustc_const_unstable(feature = "maybe_uninit_extra", issue = "63567")]
#[inline(always)]
pub unsafe fn assume_init_read(&self) -> T {
pub const unsafe fn assume_init_read(&self) -> T {
// SAFETY: the caller must guarantee that `self` is initialized.
// Reading from `self.as_ptr()` is safe since `self` should be initialized.
unsafe {
Expand Down
6 changes: 4 additions & 2 deletions library/core/src/ptr/const_ptr.rs
Original file line number Diff line number Diff line change
Expand Up @@ -725,8 +725,9 @@ impl<T: ?Sized> *const T {
///
/// [`ptr::read`]: crate::ptr::read()
#[stable(feature = "pointer_methods", since = "1.26.0")]
#[rustc_const_unstable(feature = "const_ptr_read", issue = "80377")]
#[inline]
pub unsafe fn read(self) -> T
pub const unsafe fn read(self) -> T
where
T: Sized,
{
Expand Down Expand Up @@ -763,8 +764,9 @@ impl<T: ?Sized> *const T {
///
/// [`ptr::read_unaligned`]: crate::ptr::read_unaligned()
#[stable(feature = "pointer_methods", since = "1.26.0")]
#[rustc_const_unstable(feature = "const_ptr_read", issue = "80377")]
#[inline]
pub unsafe fn read_unaligned(self) -> T
pub const unsafe fn read_unaligned(self) -> T
where
T: Sized,
{
Expand Down
6 changes: 4 additions & 2 deletions library/core/src/ptr/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -685,7 +685,8 @@ pub unsafe fn replace<T>(dst: *mut T, mut src: T) -> T {
/// [valid]: self#safety
#[inline]
#[stable(feature = "rust1", since = "1.0.0")]
pub unsafe fn read<T>(src: *const T) -> T {
#[rustc_const_unstable(feature = "const_ptr_read", issue = "80377")]
pub const unsafe fn read<T>(src: *const T) -> T {
// `copy_nonoverlapping` takes care of debug_assert.
let mut tmp = MaybeUninit::<T>::uninit();
// SAFETY: the caller must guarantee that `src` is valid for reads.
Expand Down Expand Up @@ -784,7 +785,8 @@ pub unsafe fn read<T>(src: *const T) -> T {
/// ```
#[inline]
#[stable(feature = "ptr_unaligned", since = "1.17.0")]
pub unsafe fn read_unaligned<T>(src: *const T) -> T {
#[rustc_const_unstable(feature = "const_ptr_read", issue = "80377")]
pub const unsafe fn read_unaligned<T>(src: *const T) -> T {
// `copy_nonoverlapping` takes care of debug_assert.
let mut tmp = MaybeUninit::<T>::uninit();
// SAFETY: the caller must guarantee that `src` is valid for reads.
Expand Down
6 changes: 4 additions & 2 deletions library/core/src/ptr/mut_ptr.rs
Original file line number Diff line number Diff line change
Expand Up @@ -832,8 +832,9 @@ impl<T: ?Sized> *mut T {
///
/// [`ptr::read`]: crate::ptr::read()
#[stable(feature = "pointer_methods", since = "1.26.0")]
#[rustc_const_unstable(feature = "const_ptr_read", issue = "80377")]
#[inline]
pub unsafe fn read(self) -> T
pub const unsafe fn read(self) -> T
where
T: Sized,
{
Expand Down Expand Up @@ -870,8 +871,9 @@ impl<T: ?Sized> *mut T {
///
/// [`ptr::read_unaligned`]: crate::ptr::read_unaligned()
#[stable(feature = "pointer_methods", since = "1.26.0")]
#[rustc_const_unstable(feature = "const_ptr_read", issue = "80377")]
#[inline]
pub unsafe fn read_unaligned(self) -> T
pub const unsafe fn read_unaligned(self) -> T
where
T: Sized,
{
Expand Down
51 changes: 51 additions & 0 deletions library/core/tests/const_ptr.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,51 @@
// Aligned to two bytes
const DATA: [u16; 2] = [u16::from_ne_bytes([0x01, 0x23]), u16::from_ne_bytes([0x45, 0x67])];

const fn unaligned_ptr() -> *const u16 {
// Since DATA.as_ptr() is aligned to two bytes, adding 1 byte to that produces an unaligned *const u16
unsafe { (DATA.as_ptr() as *const u8).add(1) as *const u16 }
}

#[test]
fn read() {
use core::ptr;

const FOO: i32 = unsafe { ptr::read(&42 as *const i32) };
assert_eq!(FOO, 42);

const ALIGNED: i32 = unsafe { ptr::read_unaligned(&42 as *const i32) };
assert_eq!(ALIGNED, 42);

const UNALIGNED_PTR: *const u16 = unaligned_ptr();

const UNALIGNED: u16 = unsafe { ptr::read_unaligned(UNALIGNED_PTR) };
assert_eq!(UNALIGNED, u16::from_ne_bytes([0x23, 0x45]));
oli-obk marked this conversation as resolved.
Show resolved Hide resolved
}

#[test]
fn const_ptr_read() {
const FOO: i32 = unsafe { (&42 as *const i32).read() };
assert_eq!(FOO, 42);

const ALIGNED: i32 = unsafe { (&42 as *const i32).read_unaligned() };
assert_eq!(ALIGNED, 42);

const UNALIGNED_PTR: *const u16 = unaligned_ptr();

const UNALIGNED: u16 = unsafe { UNALIGNED_PTR.read_unaligned() };
assert_eq!(UNALIGNED, u16::from_ne_bytes([0x23, 0x45]));
}

#[test]
fn mut_ptr_read() {
const FOO: i32 = unsafe { (&42 as *const i32 as *mut i32).read() };
assert_eq!(FOO, 42);

const ALIGNED: i32 = unsafe { (&42 as *const i32 as *mut i32).read_unaligned() };
assert_eq!(ALIGNED, 42);

const UNALIGNED_PTR: *mut u16 = unaligned_ptr() as *mut u16;

const UNALIGNED: u16 = unsafe { UNALIGNED_PTR.read_unaligned() };
assert_eq!(UNALIGNED, u16::from_ne_bytes([0x23, 0x45]));
}
7 changes: 7 additions & 0 deletions library/core/tests/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,8 @@
#![feature(const_assume)]
#![feature(const_cell_into_inner)]
#![feature(const_maybe_uninit_assume_init)]
#![feature(const_ptr_read)]
#![feature(const_ptr_offset)]
#![feature(core_intrinsics)]
#![feature(core_private_bignum)]
#![feature(core_private_diy_float)]
Expand All @@ -34,6 +36,7 @@
#![feature(raw)]
#![feature(sort_internals)]
#![feature(slice_partition_at_index)]
#![feature(maybe_uninit_extra)]
#![feature(maybe_uninit_write_slice)]
#![feature(min_specialization)]
#![feature(step_trait)]
Expand Down Expand Up @@ -82,6 +85,10 @@ mod cell;
mod char;
mod clone;
mod cmp;

#[cfg(not(bootstrap))]
mod const_ptr;

mod fmt;
mod hash;
mod intrinsics;
Expand Down
7 changes: 7 additions & 0 deletions library/core/tests/mem.rs
Original file line number Diff line number Diff line change
Expand Up @@ -267,3 +267,10 @@ fn uninit_write_slice_cloned_no_drop() {

forget(src);
}

#[test]
#[cfg(not(bootstrap))]
fn uninit_const_assume_init_read() {
const FOO: u32 = unsafe { MaybeUninit::new(42).assume_init_read() };
assert_eq!(FOO, 42);
}
16 changes: 16 additions & 0 deletions src/test/ui/const-ptr/out_of_bounds_read.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
// error-pattern: any use of this value will cause an error

#![feature(const_ptr_read)]
#![feature(const_ptr_offset)]

fn main() {
use std::ptr;

const DATA: [u32; 1] = [42];

const PAST_END_PTR: *const u32 = unsafe { DATA.as_ptr().add(1) };

const _READ: u32 = unsafe { ptr::read(PAST_END_PTR) };
const _CONST_READ: u32 = unsafe { PAST_END_PTR.read() };
const _MUT_READ: u32 = unsafe { (PAST_END_PTR as *mut u32).read() };
}
54 changes: 54 additions & 0 deletions src/test/ui/const-ptr/out_of_bounds_read.stderr
Original file line number Diff line number Diff line change
@@ -0,0 +1,54 @@
error: any use of this value will cause an error
--> $SRC_DIR/core/src/intrinsics.rs:LL:COL
|
LL | unsafe { copy_nonoverlapping(src, dst, count) }
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
| |
| memory access failed: pointer must be in-bounds at offset 8, but is outside bounds of alloc6 which has size 4
| inside `copy_nonoverlapping::<u32>` at $SRC_DIR/core/src/intrinsics.rs:LL:COL
| inside `std::ptr::read::<u32>` at $SRC_DIR/core/src/ptr/mod.rs:LL:COL
| inside `_READ` at $DIR/out_of_bounds_read.rs:13:33
|
::: $DIR/out_of_bounds_read.rs:13:5
|
LL | const _READ: u32 = unsafe { ptr::read(PAST_END_PTR) };
| ------------------------------------------------------
|
= note: `#[deny(const_err)]` on by default

error: any use of this value will cause an error
--> $SRC_DIR/core/src/intrinsics.rs:LL:COL
|
LL | unsafe { copy_nonoverlapping(src, dst, count) }
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
| |
| memory access failed: pointer must be in-bounds at offset 8, but is outside bounds of alloc6 which has size 4
| inside `copy_nonoverlapping::<u32>` at $SRC_DIR/core/src/intrinsics.rs:LL:COL
| inside `std::ptr::read::<u32>` at $SRC_DIR/core/src/ptr/mod.rs:LL:COL
| inside `ptr::const_ptr::<impl *const u32>::read` at $SRC_DIR/core/src/ptr/const_ptr.rs:LL:COL
| inside `_CONST_READ` at $DIR/out_of_bounds_read.rs:14:39
|
::: $DIR/out_of_bounds_read.rs:14:5
|
LL | const _CONST_READ: u32 = unsafe { PAST_END_PTR.read() };
| --------------------------------------------------------

error: any use of this value will cause an error
oli-obk marked this conversation as resolved.
Show resolved Hide resolved
--> $SRC_DIR/core/src/intrinsics.rs:LL:COL
|
LL | unsafe { copy_nonoverlapping(src, dst, count) }
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
| |
| memory access failed: pointer must be in-bounds at offset 8, but is outside bounds of alloc6 which has size 4
| inside `copy_nonoverlapping::<u32>` at $SRC_DIR/core/src/intrinsics.rs:LL:COL
| inside `std::ptr::read::<u32>` at $SRC_DIR/core/src/ptr/mod.rs:LL:COL
| inside `ptr::mut_ptr::<impl *mut u32>::read` at $SRC_DIR/core/src/ptr/mut_ptr.rs:LL:COL
| inside `_MUT_READ` at $DIR/out_of_bounds_read.rs:15:37
|
::: $DIR/out_of_bounds_read.rs:15:5
|
LL | const _MUT_READ: u32 = unsafe { (PAST_END_PTR as *mut u32).read() };
| --------------------------------------------------------------------

error: aborting due to 3 previous errors