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

Document soundness of Integer -> Pointer -> Integer conversions in const contexts. #113510

Merged
merged 1 commit into from
Sep 5, 2023

Conversation

onestacked
Copy link
Contributor

@onestacked onestacked commented Jul 9, 2023

see this zulip thread

r? @RalfJung

With this slice Iterator's should be able to be made const once the const Trait reimplementation is done.

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Jul 9, 2023
@RalfJung
Copy link
Member

RalfJung commented Jul 9, 2023

LGTM. However, I think we need the lang team to bless this. Nominating for their discussion.
Cc @rust-lang/wg-const-eval @rust-lang/lang

@RalfJung RalfJung added A-const-eval Area: Constant evaluation (MIR interpretation) I-lang-nominated Nominated for discussion during a lang team meeting. T-lang Relevant to the language 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 9, 2023
@onestacked
Copy link
Contributor Author

@RalfJung Are there any const operations that can be done on the pointer (between the transmutes) that would make this unsound?
I can't think of any, but maybe it would be good to restrict this in some way for now.

@RalfJung
Copy link
Member

No, in const you can never turn an integer value into a pointer value.

@joshtriplett joshtriplett removed the I-lang-nominated Nominated for discussion during a lang team meeting. label Aug 1, 2023
@joshtriplett
Copy link
Member

@rfcbot merge

@scottmcm has a suggestion/concern he's going to raise, but since we know this needs an FCP I'm going ahead and starting one.

@rfcbot
Copy link

rfcbot commented Aug 1, 2023

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

Concerns:

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!

cc @rust-lang/lang-advisors: FCP proposed for lang, please feel free to register concerns.
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 Aug 1, 2023
@scottmcm
Copy link
Member

scottmcm commented Aug 1, 2023

Absolutely in favour in the abstract 👍

I do think, though, that we want it to be able to general idea of transmuting, rather than limiting the text to allowing it only via mem::transmute and ptr::invalid. (For example, mem::transmute_copy feels like it ought not to be UB.)

I've proposed some alternative text above to kick of discussion about the best way to phrase such a guarantee.
@rfcbot concern concept-vs-method

@rfcbot reviewed

@scottmcm
Copy link
Member

scottmcm commented Aug 1, 2023

@rfcbot resolve concept-vs-method

Though, other folks, please review the text to ensure it's still something with which you're ok.

@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 Aug 1, 2023
@rfcbot
Copy link

rfcbot commented Aug 1, 2023

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

Comment on lines 1133 to 1136
/// Transmuting pointers *to* integers in a `const` context is [undefined behavior][ub],
/// unless the pointer was originally created by transmuting *from* an integer.
/// (That includes this function specifically and helpers like [`invalid`][crate::ptr::invalid],
/// but also semantically-equivalent conversions such as punning through `repr(C)` union fields.)
Copy link
Member

@RalfJung RalfJung Aug 2, 2023

Choose a reason for hiding this comment

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

Suggested change
/// Transmuting pointers *to* integers in a `const` context is [undefined behavior][ub],
/// unless the pointer was originally created by transmuting *from* an integer.
/// (That includes this function specifically and helpers like [`invalid`][crate::ptr::invalid],
/// but also semantically-equivalent conversions such as punning through `repr(C)` union fields.)
/// Transmuting pointers *to* integers in a `const` context is [undefined behavior][ub],
/// unless the pointer was originally created *from* an integer.
/// (The pointer can be created from an integer via integer-to-pointer casts, helpers like [`invalid`][crate::ptr::invalid],
/// this function, or semantically-equivalent conversions such as punning through `repr(C)` union fields.)

We don't want to say that invalid is a transmute. We just want to say that transmute(ptr::invalid(N)): *mut T is allowed, as is transmute(Nusize): *mut T.

Copy link
Member

@RalfJung RalfJung Aug 2, 2023

Choose a reason for hiding this comment

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

Wait, does "that includes" refer to "created by" or to "transmuting to"? It must be "created by" as otherwise invalid can't be in the list, but then this doesn't cover union field punning from invalid to an integer.

IMO this section shouldn't talk specifically about that anyway, that should fall out of the general principle that transmute and union field punning are equivalent.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe I should mention only [invalid][crate::ptr::invalid] explicitly?

Something like:

unless the pointer was originally converted *from* an integer.
(for example with this function or [`invalid`][crate::ptr::invalid])

Copy link
Member

Choose a reason for hiding this comment

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

I'm pretty sure we want to allow transmuting 16 as *const u8 back to an integer. There is really no reason not to.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I would say that is converted from and integer and therefore allowed.
Not sure how to write a concise description that makes it clear all the possibilities for this conversion are allowed.

Copy link
Member

Choose a reason for hiding this comment

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

The parenthetical explains what it means to convert from an integer. So then if you think casting is converting from an integer, it should be listed. :)

@onestacked
Copy link
Contributor Author

Applied @RalfJung's suggestions.

@scottmcm scottmcm added the I-lang-nominated Nominated for discussion during a lang team meeting. label Aug 3, 2023
@scottmcm
Copy link
Member

scottmcm commented Aug 3, 2023

(Re-nominated to encourage discussion of the exact text on Tuesday.)

@rfcbot rfcbot added the finished-final-comment-period The final comment period is finished for this PR / Issue. label Aug 11, 2023
@rfcbot rfcbot added 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 Aug 11, 2023
@rfcbot
Copy link

rfcbot commented Aug 11, 2023

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.

@apiraino apiraino removed the to-announce Announce this issue on triage meeting label Aug 18, 2023
@RalfJung
Copy link
Member

RalfJung commented Sep 5, 2023

Oh, sorry, I didn't realize this was assigned to me...

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Sep 5, 2023

📌 Commit dc4e026 has been approved by RalfJung

It is now in the queue for this repository.

@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 Sep 5, 2023
bors added a commit to rust-lang-ci/rust that referenced this pull request Sep 5, 2023
…iaskrgr

Rollup of 7 pull requests

Successful merges:

 - rust-lang#113510 (Document soundness of Integer -> Pointer -> Integer conversions in `const` contexts.)
 - rust-lang#114412 (document our assumptions about symbols provided by the libc)
 - rust-lang#114813 (explain why we can mutate the FPU control word)
 - rust-lang#115523 (improve `AttrTokenStream`)
 - rust-lang#115536 (interpret: make MemPlace, Place, Operand types private to the interpreter)
 - rust-lang#115540 (Support debuginfo for custom MIR.)
 - rust-lang#115563 (llvm-wrapper: adapt for LLVM API change)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit a23f216 into rust-lang:master Sep 5, 2023
@rustbot rustbot added this to the 1.74.0 milestone Sep 5, 2023
@onestacked onestacked deleted the const_ptr_transmute_docs branch September 6, 2023 14:41
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) 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. I-lang-nominated Nominated for discussion during a lang team meeting. 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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants