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::swap_nonoverlapping in const #137280

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

RalfJung
Copy link
Member

@RalfJung RalfJung commented Feb 19, 2025

Closes #133668

The blocking issue mentioned there is resolved by documentation. We may in the future actually support such code, but that is blocked on rust-lang/const-eval#72 which is non-trivial to implement. Meanwhile, this completes stabilization of all const fn in ptr. :)

Here's a version of the problematic example to play around with:
https://play.rust-lang.org/?version=nightly&mode=debug&edition=2021&gist=6c390452379fb593e109b8f8ee854d2a

Should be FCP'd with both @rust-lang/libs-api and @rust-lang/lang since swap_nonoverlapping is documented to work as an "untyped" operation but due to the limitation mentioned above, that's not entirely true during const evaluation. I expect this limitation will only be hit in niche corner cases, so the benefits of having this function work most of the time outweigh the downsides of users running into this problem. (Note that unsafe code could already hit this limitation before this PR by doing cursed pointer casts, but having it hidden inside swap_nonoverlapping feels a bit different.)

@rustbot
Copy link
Collaborator

rustbot commented Feb 19, 2025

r? @joboet

rustbot has assigned @joboet.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. 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 Feb 19, 2025
@rustbot
Copy link
Collaborator

rustbot commented Feb 19, 2025

Some changes occurred to the intrinsics. Make sure the CTFE / Miri interpreter
gets adapted for the changes, if necessary.

cc @rust-lang/miri, @rust-lang/wg-const-eval

@RalfJung RalfJung added the I-libs-api-nominated Nominated for discussion during a libs-api team meeting. label Feb 19, 2025
@RalfJung RalfJung force-pushed the const_swap_nonoverlapping branch from c820135 to 749c3e6 Compare February 19, 2025 17:17
@traviscross traviscross added T-lang Relevant to the language team, which will review and decide on the PR/issue. I-lang-nominated Nominated for discussion during a lang team meeting. and removed T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Feb 19, 2025
@traviscross

This comment was marked as duplicate.

@rfcbot

This comment was marked as duplicate.

@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 Feb 19, 2025
/// # Const evaluation limitations
///
/// If this function is invoked during const-evaluation, the current implementation has a small (and
/// rarely relevant) limitation: if `count` is at least 2 and the data pointed to by `x` or `y`
Copy link
Member

Choose a reason for hiding this comment

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

I suppose it's a "current" thing not a promise, but specifically things like "at least 2" seem oddly specific.

Are we sure it's impossible to, say, create a failing version using a big packed struct that arranges to have an unaligned pointer that carries provenance but is unaligned so that it's copied in multiple parts by the implementation?

(I don't consider this an FCP blocker.)

Copy link
Member Author

@RalfJung RalfJung Feb 20, 2025

Choose a reason for hiding this comment

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

Yes we are. The const implementation will always swap entire T at once (this got fixed in #134689). We even have a test covering this:

fn test_const_swap_ptr() {
// The `swap` functions are implemented in the library, they are not primitives.
// Only `swap_nonoverlapping` takes a count; pointers that cross multiple elements
// are *not* supported.
// We put the pointer at an odd offset in the type and copy them as an array of bytes,
// which should catch most of the ways that the library implementation can get it wrong.
#[cfg(target_pointer_width = "32")]
type HalfPtr = i16;
#[cfg(target_pointer_width = "64")]
type HalfPtr = i32;
#[repr(C, packed)]
#[allow(unused)]
struct S {
f1: HalfPtr,
// Crucially this field is at an offset that is not a multiple of the pointer size.
ptr: &'static i32,
// Make sure the entire type does not have a power-of-2 size:
// make it 3 pointers in size. This used to hit a bug in `swap_nonoverlapping`.
f2: [HalfPtr; 3],
}
// Ensure the entire thing is usize-aligned, so in principle this
// looks like it could be eligible for a `usize` copying loop.
#[cfg_attr(target_pointer_width = "32", repr(align(4)))]
#[cfg_attr(target_pointer_width = "64", repr(align(8)))]
struct A(S);
const {
let mut s1 = A(S { ptr: &1, f1: 0, f2: [0; 3] });
let mut s2 = A(S { ptr: &666, f1: 0, f2: [0; 3] });
// Swap ptr1 and ptr2, as an array.
type T = [u8; mem::size_of::<A>()];
unsafe {
ptr::swap(ptr::from_mut(&mut s1).cast::<T>(), ptr::from_mut(&mut s2).cast::<T>());
}
// Make sure they still work.
assert!(*s1.0.ptr == 666);
assert!(*s2.0.ptr == 1);
// Swap them back, again as an array.
// (This is where we'd use a `u8` type and a `count` of `size_of::<T>()` if
// it were not for the limitation of `swap_nonoverlapping` around pointers crossing
// multiple elements.)
unsafe {
ptr::swap_nonoverlapping(
ptr::from_mut(&mut s1).cast::<T>(),
ptr::from_mut(&mut s2).cast::<T>(),
1,
);
}
// Make sure they still work.
assert!(*s1.0.ptr == 1);
assert!(*s2.0.ptr == 666);
};

"at least 2" is technically redundant since the text already says a pointer must span the boundary between two chunks being swapped, and if there's only one chunk there is no boundary. But I felt it helped make the point more clear.

@the8472

This comment was marked as resolved.

@rfcbot

This comment was marked as resolved.

@rfcbot rfcbot removed 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 Feb 19, 2025
@the8472 the8472 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 Feb 19, 2025
@the8472

This comment was marked as resolved.

@traviscross

This comment was marked as resolved.

@traviscross
Copy link
Contributor

@rfcbot fcp merge

@rfcbot
Copy link

rfcbot commented Feb 19, 2025

Team member @traviscross 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!

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.

Page not found · GitHub · GitHub
Skip to content
404 “This is not the web page you are looking for”
@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 Feb 19, 2025
@RalfJung RalfJung force-pushed the const_swap_nonoverlapping branch from 749c3e6 to 99480fc Compare February 20, 2025 07:35
@Amanieu Amanieu removed the I-libs-api-nominated Nominated for discussion during a libs-api team meeting. label Feb 25, 2025
@bors
Copy link
Contributor

bors commented Mar 1, 2025

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

@joboet joboet added S-waiting-on-fcp Status: PR is in FCP and is awaiting for FCP to complete. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 5, 2025
@RalfJung
Copy link
Member Author

@BurntSushi @joshtriplett @nikomatsakis @tmandry friendly FCP checkbox reminder :)

@RalfJung RalfJung force-pushed the const_swap_nonoverlapping branch from 99480fc to 168267d Compare March 19, 2025 16:05
/// let ptr = data2.as_ptr().cast::<*const i32>().read_unaligned();
/// assert!(*ptr == 42);
/// } }
/// ```
Copy link
Member

Choose a reason for hiding this comment

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

Can you explicitly state that this may change in the future somewhere in the docs? I know the "current" wording seemingly implies that, but I'd rather do some very clear CYA here.

Also, the docs here seem to leave it a bit mysterious as to what "fail" means. Is it a panic? Silent logical error? Something else? Could that be clarified as well?

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. I-lang-nominated Nominated for discussion during a lang team meeting. proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. S-waiting-on-fcp Status: PR is in FCP and is awaiting for FCP to complete. 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 const_swap_nonoverlapping
10 participants