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

Tracking issue for comparing raw pointers in constants #53020

Open
oli-obk opened this issue Aug 3, 2018 · 10 comments
Open

Tracking issue for comparing raw pointers in constants #53020

oli-obk opened this issue Aug 3, 2018 · 10 comments
Labels
A-const-eval Area: constant evaluation (mir interpretation) A-const-fn Area: const fn foo(..) {..}. Pure functions which can be applied at compile time. B-unstable Blocker: Implemented in the nightly compiler and unstable. C-tracking-issue Category: A tracking issue for an RFC or an unstable feature. S-tracking-design-concerns Status: There are blocking ❌ design concerns. S-tracking-needs-summary Status: It's hard to tell what's been done and what hasn't! Someone should do some investigation. T-lang Relevant to the language team, which will review and decide on the PR/issue.

Comments

@oli-obk
Copy link
Contributor

oli-obk commented Aug 3, 2018

Comparing raw pointers in constants is forbidden (hard error) in const contexts.
The const_compare_raw_pointers feature gate enables the guaranteed_eq and guaranteed_ne methods on raw pointers. The root problem with pointer comparisons at compile time is that in many cases we can't know for sure whether two pointers are equal or inequal. While it's fairly obvious that a pointer to a static and a local variable won't be equal, we can't ever tell whether two local variables are equal (their memory may alias at runtime) or whether any pointer is equal to a specific integer. In order to make it obvious that there is a discrepancy between runtime and compile-time, the guaranteed_* methods only return true when we can be sure that two things are (in)equal. At runtime these methods just return the result of the actual pointer comparison.

This permits const fn to do performance optimization tricks like the one done in slice comparison (if length is equal and pointer is equal, don't compare the slice contents, just return that it's equal).

Since we aren't sure yet what the semantics of functions that return different values at runtime and at compile-time are, we'll keep this function unstable until we've had that discussion.

Unresolved questions

  • should guaranteed_eq return an Option<bool> to allow the caller to know whether they got unclear results? What would the use-case for this be?
@oli-obk oli-obk added A-const-fn Area: const fn foo(..) {..}. Pure functions which can be applied at compile time. WG-compiler-const A-const-eval Area: constant evaluation (mir interpretation) labels Aug 3, 2018
@Centril Centril added T-lang Relevant to the language team, which will review and decide on the PR/issue. C-tracking-issue Category: A tracking issue for an RFC or an unstable feature. labels Dec 1, 2018
@jonas-schievink jonas-schievink added the B-unstable Blocker: Implemented in the nightly compiler and unstable. label Nov 26, 2019
@RalfJung

This comment has been minimized.

@oli-obk

This comment has been minimized.

Manishearth added a commit to Manishearth/rust that referenced this issue Jun 21, 2020
…,RalfJung,nagisa

A way forward for pointer equality in const eval

r? @varkor on the first commit and @RalfJung on the second commit

cc rust-lang#53020
Manishearth added a commit to Manishearth/rust that referenced this issue Jun 22, 2020
…,RalfJung,nagisa

A way forward for pointer equality in const eval

r? @varkor on the first commit and @RalfJung on the second commit

cc rust-lang#53020
Manishearth added a commit to Manishearth/rust that referenced this issue Jun 22, 2020
…,RalfJung,nagisa

A way forward for pointer equality in const eval

r? @varkor on the first commit and @RalfJung on the second commit

cc rust-lang#53020
Manishearth added a commit to Manishearth/rust that referenced this issue Jun 23, 2020
…,RalfJung,nagisa

A way forward for pointer equality in const eval

r? @varkor on the first commit and @RalfJung on the second commit

cc rust-lang#53020
@joshtriplett joshtriplett added S-tracking-needs-summary Status: It's hard to tell what's been done and what hasn't! Someone should do some investigation. S-tracking-design-concerns Status: There are blocking ❌ design concerns. labels May 11, 2022
@thomcc
Copy link
Member

thomcc commented May 26, 2022

should guaranteed_eq return an Option to allow the caller to know whether they got unclear results? What would the use-case for this be?

I came here to say yes. In general there's an ongoing issue with several APIs in const eval, where under const evaluation to assume there's no reason you might need to handle the case where the function fails to produce a meaningful result.

Some examples of this are align_offset returning usize::MAX, and align_to returning the whole slice in the first item. These are much worse to use (the first is a genuine footgun, and requires branching at runtime even in cases where the failure to align would be impossible except in const) because of this behavior, and IMO it's something we should stop doing.

@RalfJung
Copy link
Member

FWIW, the align_offset contract is useful even for runtime code, since it enables more reliable detection of bad alignment code in sanitizers like Miri. But seeing all the confusion it caused, I agree this is not a great API.

align_to though I would consider less problematic. The "failure" case there is nicely covered by the types in the API I would say.

@thomcc
Copy link
Member

thomcc commented May 27, 2022

align_to though I would consider less problematic. The "failure" case there is nicely covered by the types in the API I would say.

Sort of, but this could have been done in the appropriate cases as a.align_to::<T>().unwrap_or_else(|| (a, &[], &[]). As it is if you need to handle that, you can't. I don't think "yes, most people will use this formulation of unwrap_or_else to be a good argument for it doing it for you.

As it is, it not only means that you cannot rely on the alignment occurring for non-performance cases, but you also can't rely on it happening for performance cases either. It's pretty much set up only for loops that are exactly "handle head", "handle body", "handle tail", which is the rough shape, but you often want more flexibility than is allowed by that assumption.

This ends up being very frustrating when using the API, and I think its a pattern we shouldn't repeat.

@RalfJung
Copy link
Member

RalfJung commented May 27, 2022

It's pretty much set up only for loops that are exactly "handle head", "handle body", "handle tail", which is the rough shape, but you often want more flexibility than is allowed by that assumption.

That is explicitly and exclusively the case this API was designed for. So IMO it would have been a mistake to make the API worse for that case, and expand its scope. Rather, we should have other APIs for the other usecases. (And we can still add them, if someone has good ideas for useful APIs! I don't feel like Option<(&[T], &[U], &[T])> is terribly useful though, but who knows. Option<&[U]> sounds useful.)

Anyway, that's water under the bridge. guaranteed_eq is different, it's more like align_offset, so I can see the argument for returning an Option<bool>. OTOH the "guaranteed" in the name feels redundant then? It could in principle be just eq, and then guaranteed_eq is eq().unwrap_or(false) and guaranteed_ne is !eq().unwrap_or(true) (or eq().map(Neg::neg).unwrap_or(false)).

@thomcc
Copy link
Member

thomcc commented May 27, 2022

That is explicitly and exclusively the case this API was designed for. So IMO it would have been a mistake to make the API worse for that case, and expand its scope.

FWIW, the things I'm referring to are things like being able to rely on there being less than size_of::<T>() items in some_u8s.align_to::<T>(), in the head. This kind of thing has been desirable every time I've used the function, and I've used it a lot. So I don't disagree this would meaningfully expand the scope.

An example from the stdlib of this is https://github.com/rust-lang/rust/blob/master/library/core/src/str/count.rs#L60-L69, which still exists at runtime, and causes worse codegen.

@thomcc
Copy link
Member

thomcc commented May 27, 2022

OTOH the "guaranteed" in the name feels redundant then? It could in principle be just eq, and then guaranteed_eq is eq().unwrap_or(false) and guaranteed_ne is !eq().unwrap_or(true) (or eq().map(Neg::neg).unwrap_or(false)).

I agree on this point, although it makes it explicit that the Option is representing "no guarantee". It also has the benefit, of not shadowing PartialEq::eq.

@thomcc
Copy link
Member

thomcc commented Sep 5, 2022

I tried to use this just now in some code wanting to assert that some struct definition was correct at compile time. (It failed, but I writing it that way was probably expecting too much of const eval, and I was able to rewrite it without this).

Anyway, initially my assert!(a.guaranteed_eq(b)) failed and when this happened I was left wondering if it was because my code sucked (and the struct definition was wrong), or because I had expected too much of const-eval (in which case I need to rework my assert, or give up).

It occurs to me though, that I only noticed this because my code had true be the success path -- If that was behind a false instead, it would been easy to miss. This further convinces me that guaranteed_{eq,ne} be exposed as an API that returns an Option.

While it might seem unlikely for someone to write code that does !a.guaranteed_eq(b) instead of a.guaranteed_ne(b), I could easily imagine it happening if the code had been ported from non-const code which had !a.eq(b).

Use of an option would force handling the "don't know at compile time" case in some way (ideally unwrap) and avoid a case where the API is similar enough to a non-const API that it becomes easy to do make a mistake when translating from one to the other.

@RalfJung
Copy link
Member

RalfJung commented Sep 6, 2022

We probably want to use a simpler type for the intrinsic (to make that easier to implement), but I am generally on-board with changing the type of the public functions here.

bors added a commit to rust-lang-ci/rust that referenced this issue Sep 10, 2022
The `<*const T>::guaranteed_*` methods now return an option for the unknown case

cc rust-lang#53020 (comment)

I chose `0` for "not equal" and `1` for "equal" and left `2` for the unknown case so backends can just forward to raw pointer equality and it works ✨

r? `@fee1-dead` or `@lcnr`

cc `@rust-lang/wg-const-eval`
bjorn3 pushed a commit to rust-lang/rustc_codegen_cranelift that referenced this issue Sep 12, 2022
The `<*const T>::guaranteed_*` methods now return an option for the unknown case

cc rust-lang/rust#53020 (comment)

I chose `0` for "not equal" and `1` for "equal" and left `2` for the unknown case so backends can just forward to raw pointer equality and it works ✨

r? `@fee1-dead` or `@lcnr`

cc `@rust-lang/wg-const-eval`
workingjubilee pushed a commit to tcdi/postgrestd that referenced this issue Sep 15, 2022
The `<*const T>::guaranteed_*` methods now return an option for the unknown case

cc rust-lang/rust#53020 (comment)

I chose `0` for "not equal" and `1` for "equal" and left `2` for the unknown case so backends can just forward to raw pointer equality and it works ✨

r? `@fee1-dead` or `@lcnr`

cc `@rust-lang/wg-const-eval`
github-actions bot pushed a commit to rust-lang/glacier that referenced this issue Apr 20, 2023
=== stdout ===
=== stderr ===
error: pointers cannot be reliably compared during const eval
 --> /home/runner/work/glacier/glacier/ices/105047.rs:6:12
  |
6 |     if let RCZ = &raw const *&0 {}
  |            ^^^
  |
  = note: see issue #53020 <rust-lang/rust#53020> for more information

note: erroneous constant used
 --> /home/runner/work/glacier/glacier/ices/105047.rs:6:30
  |
6 |     if let RCZ = &raw const *&0 {}
  |                              ^^

error: aborting due to previous error

==============
RalfJung pushed a commit to RalfJung/rust-analyzer that referenced this issue Apr 20, 2024
The `<*const T>::guaranteed_*` methods now return an option for the unknown case

cc rust-lang/rust#53020 (comment)

I chose `0` for "not equal" and `1` for "equal" and left `2` for the unknown case so backends can just forward to raw pointer equality and it works ✨

r? `@fee1-dead` or `@lcnr`

cc `@rust-lang/wg-const-eval`
RalfJung pushed a commit to RalfJung/rust-analyzer that referenced this issue Apr 27, 2024
The `<*const T>::guaranteed_*` methods now return an option for the unknown case

cc rust-lang/rust#53020 (comment)

I chose `0` for "not equal" and `1` for "equal" and left `2` for the unknown case so backends can just forward to raw pointer equality and it works ✨

r? `@fee1-dead` or `@lcnr`

cc `@rust-lang/wg-const-eval`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-const-eval Area: constant evaluation (mir interpretation) A-const-fn Area: const fn foo(..) {..}. Pure functions which can be applied at compile time. B-unstable Blocker: Implemented in the nightly compiler and unstable. C-tracking-issue Category: A tracking issue for an RFC or an unstable feature. S-tracking-design-concerns Status: There are blocking ❌ design concerns. S-tracking-needs-summary Status: It's hard to tell what's been done and what hasn't! Someone should do some investigation. T-lang Relevant to the language team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

6 participants