Nuke slice_as{,_mut}_ptr methods of MaybeUninit #103133

SUPERCILEX · 2022-10-17T03:39:28Z

These methods are of questionable value: they mix together removing bounds checks with converting the MaybeUninit slice into a raw pointer. These two behaviors should be explicitly invoked separately to improve code clarity and safety. There is evidence of unnecessary bounds checking removal in the stdlib uses that are updated in this PR.

In rust-lang/libs-team#122, there is discussion of a need for generalized containers. However, that's going to be a long ways off, so in the meantime this PR adds an into_inner method on MaybeUninit raw pointers to unwrap the raw pointer. These aren't strictly necessary and could be replaced by cast, but then you don't get and type system guarentees.

rustbot · 2022-10-17T03:39:31Z

Hey! It looks like you've submitted a new PR for the library teams!

If this PR contains changes to any rust-lang/rust public library APIs then please comment with @rustbot label +T-libs-api -T-libs to tag it appropriately. If this PR contains changes to any unstable APIs please edit the PR description to add a link to the relevant API Change Proposal or create one if you haven't already. If you're unsure where your change falls no worries, just leave it as is and the reviewer will take a look and make a decision to forward on if necessary.

Examples of T-libs-api changes:

Stabilizing library features
Introducing insta-stable changes such as new implementations of existing stable traits on existing stable types
Introducing new or changing existing unstable library APIs (excluding permanently unstable features / features without a tracking issue)
Changing public documentation in ways that create new stability guarantees
Changing observable runtime behavior of library APIs

rust-highfive · 2022-10-17T03:39:31Z

r? @m-ou-se

(rust-highfive has picked a reviewer for you, use r? to override)

SUPERCILEX · 2022-10-17T03:41:07Z

library/core/src/fmt/num.rs

@@ -480,7 +476,7 @@ impl_Exp!(i128, u128 as u128 via to_u128 named exp_u128);

 /// Helper function for writing a u64 into `buf` going from last to first, with `curr`.
 fn parse_u64_into<const N: usize>(mut n: u64, buf: &mut [MaybeUninit<u8>; N], curr: &mut usize) {
-    let buf_ptr = MaybeUninit::slice_as_mut_ptr(buf);
+    let buf_ptr = buf.as_mut_ptr().inner();


This could potentially be replaced with a bunch of MaybeUninit::writes, but there are so many of them that I don't want to risk bounds checking perf or use get_unchecked a million times. Though maybe using get_unchecked a million times is actually better because it shows the unsafety?

add is already unsafe, so I guess probs not needed.

SUPERCILEX · 2022-10-17T03:50:25Z

r? @RalfJung

RalfJung · 2022-10-17T06:11:19Z

I'm not a libs API person, so I'm the wrong reviewer here.
r? libs

SUPERCILEX · 2022-10-17T06:13:20Z

Right, keep forgetting, sorry! :)

RalfJung · 2022-10-17T06:12:08Z

compiler/rustc_serialize/src/opaque.rs

@@ -346,7 +341,7 @@ impl FileEncoder {
            // room to write the input to the buffer.
            unsafe {
                let src = buf.as_ptr();
-                let dst = MaybeUninit::slice_as_mut_ptr(&mut self.buf).add(buffered);
+                let dst = self.buf[buffered].as_mut_ptr();


This adds a runtime bounds check where there was none before. Same for all the other places where add became [...].

Correct. But the unsafety is centered around copy_nonoverlapping and presumably llvm can optimize out the bounds checking since it's done right above?

Happy to remove the bounds checking if that's what reviewers want, but either way get_unchecked is the method designed to express the desire to avoid bounds checking, not pointer addition IMO.

I don't know if the bounds checks here are a problem, I only know I took care to not introduce any new bounds checks when this code was originally ported to MaybeUninit.

I went back through and removed all the bounds checks that wouldn't be optimized out by the compiler. Anything remaining should be trivially removable.

RalfJung · 2022-10-17T07:06:57Z

library/core/src/mem/maybe_uninit.rs

+
+// --- FIXME ---
+// Stabilize these as From implementations.
+// The only reason they aren't From is because that's insta-stable.


IMO into is way to implicit for use around unsafe code. Unsafe code needs clearly stated intent, so just saying "please convert this to something else" is not explicit enough.

So IMO we actually want dedicated methods here, and we need to bikeshed their name. I don't like inner but I also don't have better suggestions at the moment.

I went with into_inner to be consistent with other APIs.

I don't know any API this is consistent with? into_inner is usually for methods of the form Type<T> -> T; I find it extremely surprising to see it here for a raw pointer method.

Something like as_inner_ptr or so maybe?

I guess I don't see *const Type<T> -> *const T as being too different from Type<T> -> T. I guess the difference is that one is a type cast whereas the other is providing a view into a type.

as_ is supposed to be for borrowed types—are we counting pointers as a kind of borrow for naming purposes? If so as_inner_ptr seems great or maybe even just as_inner.

I don't see *const Type -> *const T as being too different from Type -> T

Why not? It makes a huge difference whether ownership is consume vs whether there is a form of "borrowing". We never use into_inner for methods like &Type<T> -> &T or &mut Type<T> -> &mut T. These functions here are very much like them, except they are borrowing via raw pointers, not references.

One similar function we have is UnsafeCell::raw_get. On MaybeUninit we already have as_(mut_)ptr that take references; now here we have the exact same methods using raw pointers so the naming should be similar. raw_as_ptr sounds odd though...

Gotya, I wasn't really thinking of pointers as references for naming but you're totally right that makes more sense. raw_as_ptr does seems a little weird, but it would be consistent... will rename.

Noratrieb · 2022-10-17T07:12:05Z

If you have to add a completely new method to actually replace these methods, then maybe they aren't ready for removal imo. You should add the new method through the correct processes first, and then we can maybe remove these later.

Also I agree with Ralf that these should not be From impls at all.

SUPERCILEX · 2022-10-17T20:20:08Z

Agreed. See rust-lang/libs-team#122 (comment). Let's let this sit until the other MaybeUninit stuff is resolved and I'll give this some more thought.

@rustbot author

JohnCSimon · 2023-06-17T14:14:49Z

@SUPERCILEX
ping from triage - can you post your status on this PR? There hasn't been an update in a few months. Is thanks still blocked? Thanks!

SUPERCILEX · 2023-06-26T18:24:44Z

Alrighty, I created rust-lang/libs-team#245 because I think creating a small, focused ACP will help move things forward.

SUPERCILEX · 2023-06-27T18:49:52Z

@rustbot ready

ACP resolution: rust-lang/libs-team#245 (comment)

compiler/rustc_serialize/src/opaque.rs

Signed-off-by: Alex Saveau <saveau.alexandre@gmail.com>

workingjubilee · 2023-07-26T02:49:14Z

Do we need FCP or something for posterity, or should this just be r+'d? Can I even
r? libs-api

RalfJung · 2023-07-26T06:17:08Z

I don't think I like this stripped down version - every new use of 'cast' is a step backwards IMO. These are hard to review since one cannot easily tell what they are being cast *from*.

RalfJung · 2023-07-26T07:05:08Z

There is evidence of unnecessary bounds checking removal in the stdlib uses that are updated in this PR.

Which ones did you think were unnecessary? As far as I am aware, most/all of these are ported from old pre-MaybeUninit code that already did not have bounds checks. So the removal was entirely deliberate, and not related to the signature of these methods.

FWIW I agree that with a raw ptr projection method (*mut MaybeUninit<T> -> *mut T) this becomes clearer, albeit more verbose. But reverting to cast strikes me as a bad intermediate step. It makes the code harder to read and it is unlikely to be ported to the raw ptr projection method once that exists (since how would we even find the places where it can be used).

SUPERCILEX · 2023-08-02T19:45:03Z

Which ones did you think were unnecessary?

Take a look at all the places I used slicing operators in num.rs for this commit: f2e9b40 (#103133). All of those should have probably eliminatable bounds checks.

But reverting to cast strikes me as a bad intermediate step.

Yeah, that's fair. So do those two changes need to happen together?

RalfJung · 2023-08-02T20:11:19Z

"should have probably" is fairly weak. This is all unsafe code because it is considered performance-sensitive. And this code is old, it used unchecked accesses already before it used MaybeUninit:

https://github.com/rust-lang/rust/blob/c11e514e9dfd44eebe435408b32de4193d87860c/src/libcore/fmt/num.rs

Notice all the offset. They were added in ebf9e1a.

So this is definitely not evidence that the current API leads to omitted bounds checks.

So do those two changes need to happen together?

I would personally prefer that. I don't have the power to block changes in this part of the compiler though.

SUPERCILEX · 2023-08-02T20:21:29Z

"should have probably"

Typo that autocorrect keeps trying to put in, sorry. Should have provably. As in if they don't get optimized out it's an llvm bug.

I would personally prefer that.

Yeah I think that's reasonable unfortunately. I'll have time to look into this in a month or two again.

apiraino · 2023-08-31T13:54:48Z

Based on the prev. comment switching to waiting on author to incorporate changes. Feel free to request a review with @rustbot ready, thanks!

@rustbot author

bors · 2023-09-21T00:18:10Z

☔ The latest upstream changes (presumably #115542) made this pull request unmergeable. Please resolve the merge conflicts.

JohnCSimon · 2023-11-12T18:21:30Z

@SUPERCILEX
ping from triage - can you post your status on this PR? There hasn't been an update in a few months. Thanks!

FYI: when a PR is ready for review, send a message containing
@rustbot ready to switch to S-waiting-on-review so the PR is in the reviewer's backlog.

SUPERCILEX · 2023-11-22T22:04:51Z

This PR is waiting on me to figure out what to do with *{const,mut} MaybeUninit<T> to *{const,mut} T ptr casts. Hoping to have time to think about this in December/January.

Dylan-DPC · 2024-02-05T11:13:06Z

@SUPERCILEX any updates on this?

SUPERCILEX · 2024-04-03T01:08:48Z

I'll close for now. I still think these methods should be removed, but sadly don't have the time to see that work through right now.

rust-highfive assigned m-ou-se Oct 17, 2022

rustbot added T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Oct 17, 2022

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 17, 2022

SUPERCILEX commented Oct 17, 2022

View reviewed changes

SUPERCILEX force-pushed the kill-slice-ptr-uninit branch from cf04e53 to accf4dc Compare October 17, 2022 03:43

rust-highfive assigned RalfJung and unassigned m-ou-se Oct 17, 2022

SUPERCILEX mentioned this pull request Oct 17, 2022

Tracking issue for #![feature(maybe_uninit_slice)] #63569

Open

This comment has been minimized.

Sign in to view

SUPERCILEX force-pushed the kill-slice-ptr-uninit branch 4 times, most recently from 512e93b to 0bbb561 Compare October 17, 2022 04:49

rust-highfive assigned Mark-Simulacrum and unassigned RalfJung Oct 17, 2022

RalfJung reviewed Oct 17, 2022

View reviewed changes

rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 17, 2022

Dylan-DPC added S-blocked Status: Blocked on something else such as an RFC or other implementation work. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Dec 13, 2022

SUPERCILEX force-pushed the kill-slice-ptr-uninit branch from 4684c50 to 4c97cda Compare June 27, 2023 18:00

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jun 27, 2023

ChrisDenton reviewed Jun 30, 2023

View reviewed changes

compiler/rustc_serialize/src/opaque.rs Outdated Show resolved Hide resolved

Remove type safe MaybeUninit ptr conversion

b602bbf

Signed-off-by: Alex Saveau <saveau.alexandre@gmail.com>

SUPERCILEX force-pushed the kill-slice-ptr-uninit branch from 4c97cda to b602bbf Compare June 30, 2023 21:04

workingjubilee added T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. and removed T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Jul 26, 2023

rustbot assigned dtolnay and unassigned Mark-Simulacrum Jul 26, 2023

dtolnay changed the title ~~Nuke slice_as{,_mut}_ptr methods~~ Nuke slice_as{,_mut}_ptr methods of MaybeUninit Jul 31, 2023

rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 31, 2023

dtolnay added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Nov 23, 2023

SUPERCILEX closed this Apr 3, 2024

Nuke slice_as{,_mut}_ptr methods of MaybeUninit #103133

Nuke slice_as{,_mut}_ptr methods of MaybeUninit #103133

Conversation

SUPERCILEX commented Oct 17, 2022 • edited Loading

rustbot commented Oct 17, 2022

rust-highfive commented Oct 17, 2022

SUPERCILEX Oct 17, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SUPERCILEX commented Oct 17, 2022

This comment has been minimized.

RalfJung commented Oct 17, 2022

SUPERCILEX commented Oct 17, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Noratrieb commented Oct 17, 2022

SUPERCILEX commented Oct 17, 2022

JohnCSimon commented Jun 17, 2023

SUPERCILEX commented Jun 26, 2023

SUPERCILEX commented Jun 27, 2023 • edited Loading

workingjubilee commented Jul 26, 2023

RalfJung commented Jul 26, 2023 via email

RalfJung commented Jul 26, 2023 • edited Loading

SUPERCILEX commented Aug 2, 2023

RalfJung commented Aug 2, 2023

SUPERCILEX commented Aug 2, 2023

apiraino commented Aug 31, 2023

bors commented Sep 21, 2023

JohnCSimon commented Nov 12, 2023

SUPERCILEX commented Nov 22, 2023

Dylan-DPC commented Feb 5, 2024

SUPERCILEX commented Apr 3, 2024

SUPERCILEX commented Oct 17, 2022 •

edited

Loading

SUPERCILEX Oct 17, 2022 •

edited

Loading

SUPERCILEX commented Jun 27, 2023 •

edited

Loading

RalfJung commented Jul 26, 2023 •

edited

Loading