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

The const_raw_ptr_to_usize_cast feature has been removed #48

Closed
timothee-haudebourg opened this issue Jul 28, 2021 · 17 comments
Closed

Comments

@timothee-haudebourg
Copy link

This feature has been recently removed so the crate does not compile anymore.

   |
51 |   const_raw_ptr_to_usize_cast,
   |   ^^^^^^^^^^^^^^^^^^^^^^^^^^^ feature has been removed
   |
   = note: at compile-time, pointers do not have an integer value, so these casts cannot be properly supported
@timothee-haudebourg timothee-haudebourg changed the title The const_raw_ptr_to_usize_cast has been removed The const_raw_ptr_to_usize_cast feature has been removed Jul 28, 2021
@timothee-haudebourg
Copy link
Author

@slightlyoutofphase can we expect a fix sometime soon along with #49?

@ohmyarch
Copy link

ohmyarch commented Sep 7, 2021

Same.

@RalfJung
Copy link

I am curious how this crate even used const_raw_ptr_to_usize_cast. Almost all ways of using it would lead to an error when actually evaluating the constant.

@slightlyoutofphase
Copy link
Owner

slightlyoutofphase commented Oct 21, 2021

Hey guys, I've been dealing with an extended family emergency recently. Looking into fixing everything ASAP, now.

@RalfJung:

I am curious how this crate even used const_raw_ptr_to_usize_cast. Almost all ways of using it would lead to an error when actually evaluating the constant.

I frankly couldn't tell you off the top of my head right at this very moment. It was definitely doing something practical that I would have made sure to directly use both in the test suite and example programs, though. In any case, it appears I have a fair chunk of updating to do as far as feature flags added and removed since the last commit.

@slightlyoutofphase
Copy link
Owner

After refreshing my memory by re-reading the code, the use of const_raw_ptr_to_usize_cast was largely related to directly accounting for the possibility of ZSTs in a non-panicking way. It did work as intended in the places it was used, even under Miri.

@RalfJung
Copy link

Ah so those must have been pointers that started as integers cast to raw pointers, and later cast back. That is the only way you would not have gotten errors. (Miri and rustc execute compile-time code the same way.)

Do you still need that feature or can it be replaced by other alternatives? If there is something that you used to be able to do that is now no longer possible, please let us know!

@slightlyoutofphase
Copy link
Owner

slightlyoutofphase commented Oct 21, 2021

I'm still looking into "what's new" in the core and standard libraries in terms of functionality since I last worked on the crate, but it seems like I should be able to use functionality from there for a few things I previously had to hand-roll, as there's stuff that's now const which definitely wasn't const last time I looked at it.

I'm still not 100% sure how I'll keep the compile-time-quicksort functionality going ultimately, but I think it'll be possible. At worst it might just have to panic outright if it encounters a ZST, or something along those lines. May require a larger version bump to 0.11 for the crate, when all is said and done, but that's not a huge deal I guess.

@slightlyoutofphase
Copy link
Owner

Ok, everything is mostly working. Going to leave all these issues open until I'm 100% satisfied with the state of things, though.

@slightlyoutofphase
Copy link
Owner

slightlyoutofphase commented Dec 22, 2021

Ah so those must have been pointers that started as integers cast to raw pointers, and later cast back. That is the only way you would not have gotten errors. (Miri and rustc execute compile-time code the same way.)

Do you still need that feature or can it be replaced by other alternatives? If there is something that you used to be able to do that is now no longer possible, please let us know!

Getting back into working on this again a bit more, I do seem to basically be facing a problem where it is currently no longer possible for the following function from src/utils.rs to be const in general:

/// An internal function for calculating pointer offsets as usizes, while accounting
/// directly for possible ZSTs. This is used specifically in the iterator implementations.
#[inline(always)]
pub(crate) /*const*/ fn distance_between<T>(dest: *const T, origin: *const T) -> usize {
  match size_of::<T>() {
    0 => (dest as usize).wrapping_sub(origin as usize),
    // Safety: this function is used strictly with linear inputs
    // where dest is known to come after origin.
    _ => unsafe { ptr_offset_from(dest, origin) as usize },
  }
}

I'm aware that this would have (in most cases at least) produced one of those any use of this value will cause an error compilation failures if it was actually used in a specifically const context where size_of::<T>() really was 0, however, that would have been fine with me as it would not have amounted to anything useful in that scenario anyways.

Basically I only actually cared about the const-context usability of the function as far as it usefully related to non-ZSTs, but still needed it to correctly handle ZSTs without panicking the way ptr_offset_from does if / when encountering them in regular non-const contexts, if that makes any sense.

@RalfJung
Copy link

Would casting the pointer to *const u8 and then calling ptr_offset_from work? Or is the problem that the pointers are not actually in-bounds of the same allocation in the ZST case?

Cc @oli-obk @fee1-dead in case they have some more ideas

@slightlyoutofphase
Copy link
Owner

slightlyoutofphase commented Dec 22, 2021

To clarify, I guess the intrinsic form of ptr_offset_from doesn't actually "panic" on typed ZST pointers the way the normal offset_from method does, but rather just either crashes or returns a random number depending if you're in release mode or debug mode (not that any of that is the issue here specifically).

In any case, your *const u8 suggestion seems likely to amount to the same thing that the usize-cast subtraction did (which I believe was always just returning 0, as you'd expect when operating on a sizeless array of nothing at all). I'll give it a shot. Thanks!

@slightlyoutofphase
Copy link
Owner

Yep, that works perfectly. Wish I'd thought to do it that way in the first place, honestly! Thanks again. I'd say this particular issue can be consider fully solved, now.

@RalfJung
Copy link

Note the side-conditions of offset_from though: https://doc.rust-lang.org/nightly/std/primitive.pointer.html#method.offset_from.

In particular, (20usize as *const u8).offset_from(10usize as *const u8) is UB since these pointers are not both in-bounds of the same allocated object.

@slightlyoutofphase
Copy link
Owner

Yep. As far as I'm aware, I'm adhering to those properly, as all pointers being used are pointers to elements of a StaticVec's fixed-sized backing array. The test suite does a lot of stuff that no one would likely actually even do in real life while using staticvec (with and without ZST elements) specifically to give Miri as much to "work with" as possible as far as identifying potential issues of that sort.

@RalfJung
Copy link

RalfJung commented Dec 22, 2021

But how does the iterator stores its state when the type is a ZST and hence the backing array has size 0? The dest and origin pointers must be the same then so the difference is 0?

The iterator in the standard library then uses the begin and end pointers as simple counters not actually pointing to anything. offset_from would definitely not be legal with that approach.

@slightlyoutofphase
Copy link
Owner

slightlyoutofphase commented Dec 22, 2021

The way the iterators in this crate initially get set up use an approach that I believe I adapted directly from something in the standard library, where the start and end pointers get specified differently based on whether or not T is a ZST. The "constant reference" iterator returned by StaticVec::iter() for example does this to set the end pointer with a zero-sized element type:

(start_ptr as *const u8).wrapping_add(self.length) as *const T

I realized just now that I was definitely incorrect earlier when I said distance_between always returned zero for ZSTs, as of course tests like this one would never have passed were that the case.

@RalfJung
Copy link

RalfJung commented Dec 23, 2021

(start_ptr as *const u8).wrapping_add(self.length) as *const T

Yeah that leads to pointers that should not be legal with offset_from.

But you are right that Miri strangely accepts this program:

fn main() {
    let start_ptr = &() as *const ();
    let length = 10;
    let end_ptr = (start_ptr as *const u8).wrapping_add(length) as *const ();
    unsafe { (end_ptr as *const u8).offset_from(start_ptr as *const u8); }
}

That seems like a bug to me... and indeed looking at the implementation of ptr_offset_from for Miri, it checks that both pointers point to the same allocation, but it does not check that they are both in-bounds oops.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants