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

stabilize ptr_offset_from #74238

Merged
merged 7 commits into from
Aug 23, 2020
Merged

stabilize ptr_offset_from #74238

merged 7 commits into from
Aug 23, 2020

Conversation

RalfJung
Copy link
Member

@RalfJung RalfJung commented Jul 11, 2020

This stabilizes ptr::offset_from, and closes #41079. It also removes the deprecated wrapping_offset_from. This function was deprecated 19 days ago and was never stable; given an FCP of 10 days and some waiting time until FCP starts, that leaves at least a month between deprecation and removal which I think is fine for a nightly-only API.

Regarding the open questions in #41079:

  • Should offset_from abort instead of panic on ZSTs? -- As far as I know, there is no precedent for such aborts. We could, however, declare this UB. Given that the size is always known statically and the check thus rather cheap, UB seems excessive.
  • Should there be more methods like this with different restrictions (to allow nuw/nsw, perhaps) or that return usize (like how isize-taking offset is more conveniently done with usize-taking add these days)? -- No reason to block stabilization on that, we can always add such methods later.

Also nominating the lang team because this exposes an intrinsic.

The stabilized method is best described by its doc-comment. The documentation forgot to mention the requirement that both pointers must "have the same provenance", aka "be derived from pointers to the same allocation", which I am adding in this PR. This is a precondition that Miri already implements and that, should LLVM ever obtain a psub operation to subtract pointers, will likely be required for that operation (following the semantics in this paper).

@RalfJung RalfJung 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 Jul 11, 2020
@rust-highfive
Copy link
Collaborator

r? @kennytm

(rust_highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 11, 2020
@jonas-schievink jonas-schievink added the relnotes Marks issues that should be documented in the release notes of the next release. label Jul 11, 2020
@jonas-schievink jonas-schievink added this to the 1.46 milestone Jul 11, 2020
@RalfJung RalfJung force-pushed the offset_from branch 3 times, most recently from 8bcbf6b to db6acc1 Compare July 11, 2020 13:21
@Amanieu
Copy link
Member

Amanieu commented Jul 11, 2020

@rfcbot fcp merge

@rfcbot
Copy link

rfcbot commented Jul 11, 2020

Team member @Amanieu 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 Jul 11, 2020
@scottmcm
Copy link
Member

Also nominating the lang team because this exposes an intrinsic.

This used to just be implemented with normal code, IIRC. Is there some observable semantics that the intrinsic provides? Or is it just for optimization/miri convenience?

@RalfJung
Copy link
Member Author

Hm... so in terms of const-eval, there is definitely something observable, as the implementation with normal code just does not work. (CTFE stops when you try to subtract integers that were obtained from pointers, as this is not an operation we can support in general.)

But you are right that the part that is being stabilized here could actually be implemented (possibly less efficiently) without the intrinsic. I had not thought of that.

@oli-obk

This comment has been minimized.

@RalfJung

This comment has been minimized.

@oli-obk

This comment has been minimized.

@nikomatsakis
Copy link
Contributor

Removing nomination since we discussed in today's @rust-lang/lang meeting. No concerns were raised.

@nikomatsakis
Copy link
Contributor

cc @cramertj @pnkfelix @withoutboats @sfackler for remaining checkboxes :)

@bors
Copy link
Contributor

bors commented Jul 28, 2020

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

@nikomatsakis
Copy link
Contributor

cc @pnkfelix @cramertj and @sfackler for check-boxes on this issue

@bors
Copy link
Contributor

bors commented Aug 22, 2020

💔 Test failed - checks-actions

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Aug 22, 2020
@RalfJung
Copy link
Member Author

The wrapping_offset_from feature was conditionally enabled on SGX, but from what I can see was actually unused.

@bors r=oli-obk

@bors
Copy link
Contributor

bors commented Aug 22, 2020

📌 Commit 74a860bf2693b58b8058ba371dc93ade6b3810cc has been approved by oli-obk

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 22, 2020
Copy link
Member

@cuviper cuviper left a comment

Choose a reason for hiding this comment

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

The stable versions should be 1.47, unless you're planning a really quick beta backport...

library/core/src/ptr/const_ptr.rs Outdated Show resolved Hide resolved
library/core/src/ptr/mut_ptr.rs Outdated Show resolved Hide resolved
@oli-obk
Copy link
Contributor

oli-obk commented Aug 22, 2020

@bors r-

@bors bors 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-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Aug 22, 2020
@oli-obk
Copy link
Contributor

oli-obk commented Aug 22, 2020

also needs another rustfmt run

@scottmcm
Copy link
Member

One other thing that came to mind:

The safety requirement is currently that the pointers differ by a multiple of the size of T. And interestingly there's no requirement that the pointers actually be aligned.

That's no a soundness gap -- the current implementation doesn't depend on the pointers being aligned -- but it made me wonder if it should be more constrained than it currently is.

@RalfJung
Copy link
Member Author

The stable versions should be 1.47, unless you're planning a really quick beta backport...

Good point. This also means it should land before Tuesday or it'll be 1.48 instead.

@RalfJung
Copy link
Member Author

That's no a soundness gap -- the current implementation doesn't depend on the pointers being aligned -- but it made me wonder if it should be more constrained than it currently is.

In that case however we'd also need a version of this function that works with unaligned pointers. And I cannot imagine how pointers being aligned helps here. Can you?

RalfJung and others added 2 commits August 23, 2020 16:12
Co-authored-by: Josh Stone <cuviper@gmail.com>
@RalfJung
Copy link
Member Author

@bors r=oli-obk

@bors
Copy link
Contributor

bors commented Aug 23, 2020

📌 Commit 4129e07 has been approved by oli-obk

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Aug 23, 2020
@bors
Copy link
Contributor

bors commented Aug 23, 2020

⌛ Testing commit 4129e07 with merge 9d606d9...

@bors
Copy link
Contributor

bors commented Aug 23, 2020

☀️ Test successful - checks-actions, checks-azure
Approved by: oli-obk
Pushing 9d606d9 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Aug 23, 2020
@bors bors merged commit 9d606d9 into rust-lang:master Aug 23, 2020
@scottmcm
Copy link
Member

@RalfJung No, I don't think I can come up with a reason. And I guess with ptr::raw! and friends coming along unaligned-but-separated pointers are plausible.

All good as-is, then.

@RalfJung RalfJung deleted the offset_from branch August 24, 2020 08:05
@spastorino spastorino removed the to-announce Announce this issue on triage meeting label Aug 26, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. finished-final-comment-period The final comment period is finished for this PR / Issue. merged-by-bors This PR was explicitly merged by bors. relnotes Marks issues that should be documented in the release notes of the next release. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. 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 this pull request may close these issues.

Tracking issue for ptr::offset_from (feature: ptr_offset_from)