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

A way forward for pointer equality in const eval #73398

Merged
merged 7 commits into from
Jun 23, 2020
Merged
Show file tree
Hide file tree
Changes from 5 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
2 changes: 1 addition & 1 deletion src/liballoc/raw_vec.rs
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,7 @@ impl<T> RawVec<T, Global> {
/// `#[rustc_force_min_const_fn]` attribute which requires conformance
/// with `min_const_fn` but does not necessarily allow calling it in
/// `stable(...) const fn` / user code not enabling `foo` when
/// `#[rustc_const_unstable(feature = "foo", ..)]` is present.
/// `#[rustc_const_unstable(feature = "foo", issue = "01234")]` is present.
pub const NEW: Self = Self::new();

/// Creates the biggest possible `RawVec` (on the system heap)
Expand Down
16 changes: 13 additions & 3 deletions src/libcore/intrinsics.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1012,7 +1012,7 @@ extern "rust-intrinsic" {
///
/// The stabilized version of this intrinsic is
/// [`std::any::type_name`](../../std/any/fn.type_name.html)
#[rustc_const_unstable(feature = "const_type_name", issue = "none")]
#[rustc_const_unstable(feature = "const_type_name", issue = "63084")]
pub fn type_name<T: ?Sized>() -> &'static str;

/// Gets an identifier which is globally unique to the specified type. This
Expand All @@ -1021,7 +1021,7 @@ extern "rust-intrinsic" {
///
/// The stabilized version of this intrinsic is
/// [`std::any::TypeId::of`](../../std/any/struct.TypeId.html#method.of)
#[rustc_const_unstable(feature = "const_type_id", issue = "none")]
#[rustc_const_unstable(feature = "const_type_id", issue = "41875")]
pub fn type_id<T: ?Sized + 'static>() -> u64;

/// A guard for unsafe functions that cannot ever be executed if `T` is uninhabited:
Expand Down Expand Up @@ -1931,7 +1931,7 @@ extern "rust-intrinsic" {
pub fn nontemporal_store<T>(ptr: *mut T, val: T);

/// See documentation of `<*const T>::offset_from` for details.
#[rustc_const_unstable(feature = "const_ptr_offset_from", issue = "none")]
#[rustc_const_unstable(feature = "const_ptr_offset_from", issue = "41079")]
pub fn ptr_offset_from<T>(ptr: *const T, base: *const T) -> isize;

/// Internal hook used by Miri to implement unwinding.
Expand All @@ -1948,6 +1948,16 @@ extern "rust-intrinsic" {
#[cfg(not(bootstrap))]
#[lang = "count_code_region"]
pub fn count_code_region(index: u32);

/// See documentation of `<*const T>::guaranteed_eq` for details.
#[rustc_const_unstable(feature = "const_raw_ptr_comparison", issue = "53020")]
#[cfg(not(bootstrap))]
pub fn ptr_guaranteed_eq<T>(ptr: *const T, other: *const T) -> bool;
oli-obk marked this conversation as resolved.
Show resolved Hide resolved

/// See documentation of `<*const T>::guaranteed_ne` for details.
#[rustc_const_unstable(feature = "const_raw_ptr_comparison", issue = "53020")]
#[cfg(not(bootstrap))]
pub fn ptr_guaranteed_ne<T>(ptr: *const T, other: *const T) -> bool;
}

// Some functions are defined here because they accidentally got made
Expand Down
1 change: 1 addition & 0 deletions src/libcore/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -87,6 +87,7 @@
#![feature(const_generics)]
#![feature(const_ptr_offset)]
#![feature(const_ptr_offset_from)]
#![cfg_attr(not(bootstrap), feature(const_raw_ptr_comparison))]
#![feature(const_result)]
#![feature(const_slice_from_raw_parts)]
#![feature(const_slice_ptr_len)]
Expand Down
66 changes: 66 additions & 0 deletions src/libcore/ptr/const_ptr.rs
Original file line number Diff line number Diff line change
Expand Up @@ -295,6 +295,72 @@ impl<T: ?Sized> *const T {
intrinsics::ptr_offset_from(self, origin)
}

/// Returns whether two pointers are guaranteed equal.
///
/// At runtime this function behaves like `self == other`.
/// However, in some contexts (e.g., compile-time evaluation),
/// it is not always possible to determine equality of two pointers, so this function may
/// spuriously return `false` for pointers that later actually turn out to be equal.
/// But when it returns `true`, the pointers are guaranteed to be equal.
///
/// This function is the mirror of [`guaranteed_ne`], but not its inverse. There are pointer
/// comparisons for which both functions return `false`.
///
/// [`guaranteed_ne`]: #method.guaranteed_ne
///
/// The return value may change depending on the compiler version and unsafe code may not
/// rely on the result of this function for soundness. It is suggested to only use this function
Copy link
Member

Choose a reason for hiding this comment

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

Can unsafe code rely on a return value of true (similar to relying on std::mem::needs_drop returning false)?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, it can. The comment says

But when it returns true, the pointers are guaranteed to be equal.

I thought that was clear enough?

/// for performance optimizations where spurious `false` return values by this function do not
/// affect the outcome, but just the performance.
/// The consequences of using this method to make runtime and compile-time code behave
/// differently have not been explored. This method should not be used to introduce such
/// differences, and it should also not be stabilized before we have a better understanding
/// of this issue.
/// ```
#[unstable(feature = "const_raw_ptr_comparison", issue = "53020")]
#[rustc_const_unstable(feature = "const_raw_ptr_comparison", issue = "53020")]
#[inline]
#[cfg(not(bootstrap))]
pub const fn guaranteed_eq(self, other: *const T) -> bool
where
T: Sized,
{
intrinsics::ptr_guaranteed_eq(self, other)
}

/// Returns whether two pointers are guaranteed not equal.
oli-obk marked this conversation as resolved.
Show resolved Hide resolved
///
/// At runtime this function behaves like `self != other`.
/// However, in some contexts (e.g., compile-time evaluation),
/// it is not always possible to determine the inequality of two pointers, so this function may
/// spuriously return `false` for pointers that later actually turn out to be inequal.
/// But when it returns `true`, the pointers are guaranteed to be inequal.
///
/// This function is the mirror of [`guaranteed_eq`], but not its inverse. There are pointer
/// comparisons for which both functions return `false`.
///
/// [`guaranteed_eq`]: #method.guaranteed_eq
///
/// The return value may change depending on the compiler version and unsafe code may not
/// rely on the result of this function for soundness. It is suggested to only use this function
/// for performance optimizations where spurious `false` return values by this function do not
/// affect the outcome, but just the performance.
/// The consequences of using this method to make runtime and compile-time code behave
/// differently have not been explored. This method should not be used to introduce such
/// differences, and it should also not be stabilized before we have a better understanding
/// of this issue.
/// ```
#[unstable(feature = "const_raw_ptr_comparison", issue = "53020")]
#[rustc_const_unstable(feature = "const_raw_ptr_comparison", issue = "53020")]
#[inline]
#[cfg(not(bootstrap))]
pub const fn guaranteed_ne(self, other: *const T) -> bool
where
T: Sized,
{
intrinsics::ptr_guaranteed_ne(self, other)
}

/// Calculates the distance between two pointers. The returned value is in
/// units of T: the distance in bytes is divided by `mem::size_of::<T>()`.
///
Expand Down
66 changes: 66 additions & 0 deletions src/libcore/ptr/mut_ptr.rs
Original file line number Diff line number Diff line change
Expand Up @@ -273,6 +273,72 @@ impl<T: ?Sized> *mut T {
if self.is_null() { None } else { Some(&mut *self) }
}

/// Returns whether two pointers are guaranteed equal.
///
/// At runtime this function behaves like `self == other`.
/// However, in some contexts (e.g., compile-time evaluation),
/// it is not always possible to determine equality of two pointers, so this function may
/// spuriously return `false` for pointers that later actually turn out to be equal.
/// But when it returns `true`, the pointers are guaranteed to be equal.
///
/// This function is the mirror of [`guaranteed_ne`], but not its inverse. There are pointer
/// comparisons for which both functions return `false`.
///
/// [`guaranteed_ne`]: #method.guaranteed_ne
///
/// The return value may change depending on the compiler version and unsafe code may not
/// rely on the result of this function for soundness. It is suggested to only use this function
/// for performance optimizations where spurious `false` return values by this function do not
/// affect the outcome, but just the performance.
/// The consequences of using this method to make runtime and compile-time code behave
/// differently have not been explored. This method should not be used to introduce such
/// differences, and it should also not be stabilized before we have a better understanding
/// of this issue.
/// ```
#[unstable(feature = "const_raw_ptr_comparison", issue = "53020")]
#[rustc_const_unstable(feature = "const_raw_ptr_comparison", issue = "53020")]
#[inline]
#[cfg(not(bootstrap))]
pub const fn guaranteed_eq(self, other: *mut T) -> bool
where
T: Sized,
{
intrinsics::ptr_guaranteed_eq(self as *const _, other as *const _)
}

/// Returns whether two pointers are guaranteed not equal.
///
/// At runtime this function behaves like `self != other`.
/// However, in some contexts (e.g., compile-time evaluation),
/// it is not always possible to determine the inequality of two pointers, so this function may
/// spuriously return `false` for pointers that later actually turn out to be inequal.
/// But when it returns `true`, the pointers are guaranteed to be inequal.
///
/// This function is the mirror of [`guaranteed_eq`], but not its inverse. There are pointer
/// comparisons for which both functions return `false`.
///
/// [`guaranteed_eq`]: #method.guaranteed_eq
///
/// The return value may change depending on the compiler version and unsafe code may not
/// rely on the result of this function for soundness. It is suggested to only use this function
/// for performance optimizations where spurious `false` return values by this function do not
/// affect the outcome, but just the performance.
/// The consequences of using this method to make runtime and compile-time code behave
/// differently have not been explored. This method should not be used to introduce such
/// differences, and it should also not be stabilized before we have a better understanding
/// of this issue.
/// ```
#[unstable(feature = "const_raw_ptr_comparison", issue = "53020")]
#[rustc_const_unstable(feature = "const_raw_ptr_comparison", issue = "53020")]
#[inline]
#[cfg(not(bootstrap))]
pub const unsafe fn guaranteed_ne(self, other: *mut T) -> bool
where
T: Sized,
{
intrinsics::ptr_guaranteed_ne(self as *const _, other as *const _)
}

/// Calculates the distance between two pointers. The returned value is in
/// units of T: the distance in bytes is divided by `mem::size_of::<T>()`.
///
Expand Down
50 changes: 48 additions & 2 deletions src/libcore/slice/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5946,7 +5946,8 @@ where
}
}

// Use an equal-pointer optimization when types are `Eq`
// Remove after boostrap bump
#[cfg(bootstrap)]
impl<A> SlicePartialEq<A> for [A]
where
A: PartialEq<A> + Eq,
Expand All @@ -5964,7 +5965,8 @@ where
}
}

// Use memcmp for bytewise equality when the types allow
// Remove after boostrap bump
#[cfg(bootstrap)]
impl<A> SlicePartialEq<A> for [A]
where
A: PartialEq<A> + BytewiseEquality,
Expand All @@ -5983,6 +5985,50 @@ where
}
}

// Use an equal-pointer optimization when types are `Eq`
#[cfg(not(bootstrap))]
impl<A> SlicePartialEq<A> for [A]
where
A: PartialEq<A> + Eq,
{
default fn equal(&self, other: &[A]) -> bool {
if self.len() != other.len() {
return false;
}

// While performance would suffer if `guaranteed_eq` just returned `false`
// for all arguments, correctness and return value of this function are not affected.
if self.as_ptr().guaranteed_eq(other.as_ptr()) {
oli-obk marked this conversation as resolved.
Show resolved Hide resolved
return true;
}

self.iter().zip(other.iter()).all(|(x, y)| x == y)
}
}

// Use memcmp for bytewise equality when the types allow
#[cfg(not(bootstrap))]
impl<A> SlicePartialEq<A> for [A]
where
A: PartialEq<A> + BytewiseEquality,
{
fn equal(&self, other: &[A]) -> bool {
if self.len() != other.len() {
return false;
}

// While performance would suffer if `guaranteed_eq` just returned `false`
// for all arguments, correctness and return value of this function are not affected.
if self.as_ptr().guaranteed_eq(other.as_ptr()) {
return true;
}
unsafe {
let size = mem::size_of_val(self);
memcmp(self.as_ptr() as *const u8, other.as_ptr() as *const u8, size) == 0
}
}
}

#[doc(hidden)]
// intermediate trait for specialization of slice's PartialOrd
trait SlicePartialOrd: Sized {
Expand Down
14 changes: 13 additions & 1 deletion src/librustc_codegen_llvm/intrinsic.rs
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ use log::debug;
use rustc_ast::ast;
use rustc_codegen_ssa::base::{compare_simd_types, to_immediate, wants_msvc_seh};
use rustc_codegen_ssa::common::span_invalid_monomorphization_error;
use rustc_codegen_ssa::common::TypeKind;
use rustc_codegen_ssa::common::{IntPredicate, TypeKind};
use rustc_codegen_ssa::glue;
use rustc_codegen_ssa::mir::operand::{OperandRef, OperandValue};
use rustc_codegen_ssa::mir::place::PlaceRef;
Expand Down Expand Up @@ -731,6 +731,18 @@ impl IntrinsicCallMethods<'tcx> for Builder<'a, 'll, 'tcx> {
return;
}

"ptr_guaranteed_eq" | "ptr_guaranteed_ne" => {
let a = args[0].immediate();
let b = args[1].immediate();
let a = self.ptrtoint(a, self.type_isize());
let b = self.ptrtoint(b, self.type_isize());
Copy link
Member

Choose a reason for hiding this comment

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

Is there a non-obvious reason you ptrtoint before comparing? icmp can work fine with pointer types, as long as the pointee types match, which in this case they should.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh. I didn't know that. I basically grabbed this off

"ptr_offset_from" => {
let ty = substs.type_at(0);
let pointee_size = self.size_of(ty);
// This is the same sequence that Clang emits for pointer subtraction.
// It can be neither `nsw` nor `nuw` because the input is treated as
// unsigned but then the output is treated as signed, so neither works.
let a = args[0].immediate();
let b = args[1].immediate();
let a = self.ptrtoint(a, self.type_isize());
let b = self.ptrtoint(b, self.type_isize());
let d = self.sub(a, b);
let pointee_size = self.const_usize(pointee_size.bytes());
// this is where the signed magic happens (notice the `s` in `exactsdiv`)
self.exactsdiv(d, pointee_size)
which I'm not sure whether it needs that either.

Copy link
Member

Choose a reason for hiding this comment

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

which I'm not sure whether it needs that either.

It does. LLVM supports comparison of pointer types but not subtraction.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So confusing. There's probably a good reason for that, but still... So many inconsistencies.

Copy link
Member

@RalfJung RalfJung Jun 21, 2020

Choose a reason for hiding this comment

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

FWIW, there's a plan to add a psub operation that directly subtracts pointers. ;)

(There is no pointer addition, either. Instead there is getelementptr as the specific instruction to do pointer arithmetic. That makes a lot of sense IMO.)

if name == "ptr_guaranteed_eq" {
self.icmp(IntPredicate::IntEQ, a, b)
} else {
self.icmp(IntPredicate::IntNE, a, b)
}
}

"ptr_offset_from" => {
let ty = substs.type_at(0);
let pointee_size = self.size_of(ty);
Expand Down
3 changes: 0 additions & 3 deletions src/librustc_feature/active.rs
Original file line number Diff line number Diff line change
Expand Up @@ -401,9 +401,6 @@ declare_features! (
/// Allows dereferencing raw pointers during const eval.
(active, const_raw_ptr_deref, "1.27.0", Some(51911), None),

/// Allows comparing raw pointers during const eval.
(active, const_compare_raw_pointers, "1.27.0", Some(53020), None),

/// Allows `#[doc(alias = "...")]`.
(active, doc_alias, "1.27.0", Some(50146), None),

Expand Down
5 changes: 5 additions & 0 deletions src/librustc_feature/removed.rs
Original file line number Diff line number Diff line change
Expand Up @@ -113,6 +113,11 @@ declare_features! (
Some("removed in favor of `#![feature(marker_trait_attr)]`")),
/// Allows `#[no_debug]`.
(removed, no_debug, "1.43.0", Some(29721), None, Some("removed due to lack of demand")),

/// Allows comparing raw pointers during const eval.
(removed, const_compare_raw_pointers, "1.46.0", Some(53020), None,
Some("cannot be allowed in const eval in any meaningful way")),

// -------------------------------------------------------------------------
// feature-group-end: removed features
// -------------------------------------------------------------------------
Expand Down
5 changes: 5 additions & 0 deletions src/librustc_mir/interpret/intrinsics.rs
Original file line number Diff line number Diff line change
Expand Up @@ -291,6 +291,11 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
let offset_ptr = ptr.ptr_wrapping_signed_offset(offset_bytes, self);
self.write_scalar(offset_ptr, dest)?;
}
sym::ptr_guaranteed_eq | sym::ptr_guaranteed_ne => {
Copy link
Member

Choose a reason for hiding this comment

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

I just realized that this implementation will also be used by Miri, which is a bug. Can we somehow make it be used only during CTCE?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

doesn't miri fall back to this only if it has no own impl? So we can just give miri an impl that behaves differently.

Copy link
Member

Choose a reason for hiding this comment

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

Miri prefers the CTFE impls, to make sure we test those properly.

I am working on a PR that adds these to Miri in a way that it prefers its own impls.

Copy link
Member

Choose a reason for hiding this comment

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

Here it is: rust-lang/miri#1459

// FIXME: return `true` for at least some comparisons where we can reliably
// determine the result of runtime (in)equality tests at compile-time.
self.write_scalar(Scalar::from_bool(false), dest)?;
}
sym::ptr_offset_from => {
let a = self.read_immediate(args[0])?.to_scalar()?;
let b = self.read_immediate(args[1])?.to_scalar()?;
Expand Down
20 changes: 9 additions & 11 deletions src/librustc_mir/transform/check_consts/ops.rs
Original file line number Diff line number Diff line change
Expand Up @@ -284,18 +284,16 @@ impl NonConstOp for Panic {
#[derive(Debug)]
pub struct RawPtrComparison;
impl NonConstOp for RawPtrComparison {
fn feature_gate() -> Option<Symbol> {
Some(sym::const_compare_raw_pointers)
}

fn emit_error(&self, ccx: &ConstCx<'_, '_>, span: Span) {
feature_err(
&ccx.tcx.sess.parse_sess,
sym::const_compare_raw_pointers,
span,
&format!("comparing raw pointers inside {}", ccx.const_kind()),
)
.emit();
let mut err = ccx
.tcx
.sess
.struct_span_err(span, "pointers cannot be reliably compared during const eval.");
err.note(
"see issue #53020 <https://github.com/rust-lang/rust/issues/53020> \
for more information",
);
err.emit();
}
}

Expand Down
15 changes: 0 additions & 15 deletions src/librustc_mir/transform/check_unsafety.rs
Original file line number Diff line number Diff line change
Expand Up @@ -171,21 +171,6 @@ impl<'a, 'tcx> Visitor<'tcx> for UnsafetyChecker<'a, 'tcx> {
_ => {}
}
}
// raw pointer and fn pointer operations are unsafe as it is not clear whether one
// pointer would be "less" or "equal" to another, because we cannot know where llvm
// or the linker will place various statics in memory. Without this information the
// result of a comparison of addresses would differ between runtime and compile-time.
Rvalue::BinaryOp(_, ref lhs, _)
if self.const_context && self.tcx.features().const_compare_raw_pointers =>
{
if let ty::RawPtr(_) | ty::FnPtr(..) = lhs.ty(self.body, self.tcx).kind {
self.require_unsafe(
"pointer operation",
"operations on pointers in constants",
UnsafetyViolationKind::General,
);
}
}
_ => {}
}
self.super_rvalue(rvalue, location);
Expand Down
2 changes: 2 additions & 0 deletions src/librustc_span/symbol.rs
Original file line number Diff line number Diff line change
Expand Up @@ -588,6 +588,8 @@ symbols! {
proc_macro_non_items,
proc_macro_path_invoc,
profiler_runtime,
ptr_guaranteed_eq,
ptr_guaranteed_ne,
ptr_offset_from,
pub_restricted,
pure,
Expand Down
Loading