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 const offset_from (const_ptr_offset_from) #92980

Closed
1 of 2 tasks
RalfJung opened this issue Jan 16, 2022 · 17 comments · Fixed by #96240
Closed
1 of 2 tasks

Tracking Issue for const offset_from (const_ptr_offset_from) #92980

RalfJung opened this issue Jan 16, 2022 · 17 comments · Fixed by #96240
Assignees
Labels
C-tracking-issue Category: An issue tracking the progress of sth. like the implementation of an RFC disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. E-help-wanted Call for participation: Help is requested to fix this issue. finished-final-comment-period The final comment period is finished for this PR / Issue. S-tracking-ready-to-stabilize Status: This is ready to stabilize; it may need a stabilization report and a PR T-lang Relevant to the language team, which will review and decide on the PR/issue. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.

Comments

@RalfJung
Copy link
Member

RalfJung commented Jan 16, 2022

The feature gate for the issue is #![feature(const_ptr_offset_from)].

This tracks constness of the following function(s):

impl<T> *const T {
  const unsafe fn offset_from(self, origin: *const T) -> isize
}

About tracking issues

Tracking issues are used to record the overall progress of implementation.
They are also used as hubs connecting to other relevant issues, e.g., bugs or open design questions.
A tracking issue is however not meant for large scale discussion, questions, or bug reports about a feature.
Instead, open a dedicated issue for the specific matter and add the relevant feature gate label.

Steps

Unresolved Questions

Implementation history

@RalfJung RalfJung added the C-tracking-issue Category: An issue tracking the progress of sth. like the implementation of an RFC label Jan 16, 2022
@RalfJung
Copy link
Member Author

Cc @rust-lang/wg-const-eval

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Jan 17, 2022
…tolnay

fix const_ptr_offset_from tracking issue

The old tracking issue rust-lang#41079 was for exposing those functions at all, and got closed when they were stabilized. We had nothing tracking their `const`ness so I opened a new tracking issue: rust-lang#92980.
@kupiakos
Copy link
Contributor

kupiakos commented Mar 5, 2022

I'd like to see if we can work out the kinks and stabilize this sooner rather than later. Since addr_of! is stable, this being stabilized would enable us to do something quite powerful: stable const-time offset_of. See here for an example. With that, we can get stable const-time calculation of padding byte locations, something that could be very helpful for the zerocopy library and related firmware code.

@RalfJung
Copy link
Member Author

RalfJung commented Mar 5, 2022

See here for an example.

Note that that macro has some subtle flaws, see https://github.com/Gilnaa/memoffset for a bullet-proof (and also nightly-const-compatible) version. In particular it protects against .field triggering deref coercions, which make your version of the macro unsound.

@RalfJung
Copy link
Member Author

RalfJung commented Mar 5, 2022

But regarding this tracking issue, I think it would be good to fix rust-lang/miri#1950 before stabilizing, to ensure we actually check the UB for this function properly. (Miri and CTFE share that implementation.) Other than that, I am not aware of any blockers. I think we should do the change suggested in #92512, but that can happen post-stabilization.

@kupiakos
Copy link
Contributor

kupiakos commented Mar 5, 2022

Good call on the Deref soundness, rest assured I'm not copying playground code into production 😃

I'll see if I can spend some time looking at the change or issue.

In the meantime, I think I may have found an alternative way to calculate offset_of! in stable CTFE that doesn't depend on pointer arithmetic, it's just more expensive at compile time (obviously this contains the same Deref flaw currently)

@RalfJung
Copy link
Member Author

RalfJung commented Mar 11, 2022

Stabilization report

I would like to propose this feature for stabilization: it allows const-code to call

impl<T> *const T {
  const unsafe fn offset_from(self, origin: *const T) -> isize
}

Click here for method docs

offset_from has been stabilized for runtime code more than 1.5 years ago (see #41079). Its CTFE implementation is shared with Miri and has been working fine for a long time; #94827 works out the last kink (but even that is optional since we do not guarantee to detect all UB). This is the last feature that is missing to be able to implement offset_of! in a const-compatible way, which has been a long time coming. :)

This cycle we are also on track to const-stabilize a whole lot of other pointer offset functions (#71499), so this fits quite well.

The tests for this feature are at

In the future I would like to relax the UB restrictions of this function (#92512), but that should be backwards compatible so it does not have to block stabilization.

@rust-lang/lang can I ask for the FCP process to be initiated?

@RalfJung
Copy link
Member Author

RalfJung commented Mar 11, 2022

Or is this for @rust-lang/libs-api ? It is const-exposing an intrinsic (that could not be implemented in pure Rust), so... possibly both.

@scottmcm scottmcm added T-lang Relevant to the language team, which will review and decide on the PR/issue. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. labels Mar 11, 2022
@scottmcm
Copy link
Member

Both teams sounds reasonable, yeah.

Teams, do we want to allow offset-between-pointers calculations in const?

Stabilization report from Ralf: #92980 (comment)

@rfcbot fcp merge

One correction to the report: it's T: Sized only, due to a where clause:

#[stable(feature = "ptr_offset_from", since = "1.47.0")]
#[rustc_const_unstable(feature = "const_ptr_offset_from", issue = "92980")]
#[inline]
pub const unsafe fn offset_from(self, origin: *const T) -> isize
where
T: Sized,
{
let pointee_size = mem::size_of::<T>();
assert!(0 < pointee_size && pointee_size <= isize::MAX as usize);
// SAFETY: the caller must uphold the safety contract for `ptr_offset_from`.
unsafe { intrinsics::ptr_offset_from(self, origin) }
}

@rfcbot
Copy link

rfcbot commented Mar 11, 2022

Team member @scottmcm has proposed to merge this. The next step is review by the rest of the tagged team members:

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

@rfcbot rfcbot added proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. labels Mar 11, 2022
@RalfJung
Copy link
Member Author

One correction to the report: it's T: Sized only, due to a where clause:

Good point, fixed.

@RalfJung
Copy link
Member Author

@BurntSushi @nikomatsakis @pnkfelix @yaahc friendly reminder that there are some boxes waiting for you to check them here. :)

@nikomatsakis
Copy link
Contributor

@rfcbot reviewed

@rfcbot rfcbot added final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. and removed proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. labels Apr 9, 2022
@rfcbot
Copy link

rfcbot commented Apr 9, 2022

🔔 This is now entering its final comment period, as per the review above. 🔔

@rfcbot rfcbot added finished-final-comment-period The final comment period is finished for this PR / Issue. to-announce Announce this issue on triage meeting and removed final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. labels Apr 19, 2022
@rfcbot
Copy link

rfcbot commented Apr 19, 2022

The final comment period, with a disposition to merge, as per the review above, is now complete.

As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.

This will be merged soon.

@RalfJung
Copy link
Member Author

Any takers for a stabilization PR? :)

@yaahc yaahc added E-help-wanted Call for participation: Help is requested to fix this issue. S-tracking-ready-to-stabilize Status: This is ready to stabilize; it may need a stabilization report and a PR labels Apr 19, 2022
@yaahc
Copy link
Member

yaahc commented Apr 19, 2022

turns on bat stabilization signal

@fee1-dead
Copy link
Member

I'll prepare a stabilization PR

@rustbot claim

@apiraino apiraino removed the to-announce Announce this issue on triage meeting label Apr 21, 2022
fee1-dead added a commit to fee1-dead-contrib/rust that referenced this issue Aug 25, 2022
Stabilization has been completed [here](rust-lang#92980 (comment))
with a FCP.
compiler-errors added a commit to compiler-errors/rust that referenced this issue Aug 27, 2022
…ffset_from, r=Mark-Simulacrum

Stabilize `const_ptr_offset_from`.

Stabilization has been completed [here](rust-lang#92980 (comment)) with a FCP.

Closes rust-lang#92980.
@bors bors closed this as completed in 539e408 Aug 27, 2022
workingjubilee pushed a commit to tcdi/postgrestd that referenced this issue Sep 15, 2022
Stabilization has been completed [here](rust-lang/rust#92980 (comment))
with a FCP.
workingjubilee pushed a commit to tcdi/postgrestd that referenced this issue Sep 15, 2022
…m, r=Mark-Simulacrum

Stabilize `const_ptr_offset_from`.

Stabilization has been completed [here](rust-lang/rust#92980 (comment)) with a FCP.

Closes #92980.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-tracking-issue Category: An issue tracking the progress of sth. like the implementation of an RFC disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. E-help-wanted Call for participation: Help is requested to fix this issue. finished-final-comment-period The final comment period is finished for this PR / Issue. S-tracking-ready-to-stabilize Status: This is ready to stabilize; it may need a stabilization report and a PR T-lang Relevant to the language team, which will review and decide on the PR/issue. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants