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.

Sign up for GitHub

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

Jump to bottom

[WIP] Constified slice::sort_unstable, sort_internals #102279

Closed
wants to merge 0 commits into from
Closed

[WIP] Constified slice::sort_unstable, sort_internals #102279

wants to merge 0 commits into from

Conversation

onestacked
Copy link
Contributor

@onestacked onestacked commented Sep 25, 2022 •
edited
Loading

Constifies most of the sort related functions in core.

TODO:

cc @RalfJung @fee1-dead

@rustbot rustbot added the T-libs Relevant to the library team, which will review and decide on the PR/issue. label Sep 25, 2022
@rust-highfive
Copy link
Collaborator

r? @Mark-Simulacrum

(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 Sep 25, 2022
onestacked
onestacked commented Sep 25, 2022
View reviewed changes
library/core/src/slice/sort.rs Outdated
// Mainly lhs and rhs being in the same allocated Object
// and a multiple of their size apart.
unsafe { lhs.offset_from(rhs) }.eq(&zero)
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should those helper functions themself be unsafe?

Copy link
Member

Choose a reason for hiding this comment

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

Yes they should, and they should document their precondition.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But the the same safety comment would be duplicated 9 times for lt, 4 times for eq and 10 times for width.

Copy link
Member

Choose a reason for hiding this comment

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

That indicates that the code is repeatedly doing the same subtle safety-relevant reasoning. This doesn't get better by not talking about it, it only gets better by restructuring the code in a way that avoids doing subtle safety-relevant reasoning in many places. (I don't know if that is possible, I don't have the time to do a detailed code review -- sorry.)

library/core/src/slice/sort.rs Outdated
// (r.addr() - l.addr()) / mem::size_of::<T>()

// SAFETY: To be investigated as noted above.
(unsafe { r.offset_from(l) as usize }) / mem::size_of::<T>()
Copy link
Contributor Author

Choose a reason for hiding this comment

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

How should the const safety of this be verified?

What should the safety comment be exactly?

cc @RalfJung

Copy link
Member

Choose a reason for hiding this comment

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

You need to explain why these pointers are in the same allocation and their distance is an integer multiple of their size.

I am not familiar with this code unfortunately and won't be able to do a detailed review.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

cc @Gankra since the comment about maybe migrating was by them.

@onestacked onestacked mentioned this pull request Sep 26, 2022
Open
3 tasks
@onestacked
Copy link
Contributor Author

The current sort relies on comparing with null pointers, which is (currently) UB.

I could add checks using ptr::is_null, but that would have some runtime cost.
I could use const_eval_select to use normal pointer comparison in RT
and add additional null checks at CT and then use ptr::offset_from.

I could investigate if i can adapt the algorithm to avoid null pointers but I don't know if that will be possible without large changes to the algorithm itself.

@onestacked
Copy link
Contributor Author

I went with adding null pointer checks at CT using const_eval_select.
I think that it should be sound now, but a thorough review by someone who knows this code and const unsafety well is probably needed.

@bors
Copy link
Contributor

bors commented Nov 19, 2022

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

@bors
Copy link
Contributor

bors commented Jan 21, 2023

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

@Mark-Simulacrum Mark-Simulacrum 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 Feb 12, 2023
@bors
Copy link
Contributor

bors commented Feb 13, 2023

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

@onestacked onestacked closed this Mar 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@RalfJung RalfJung RalfJung left review comments

Assignees

@Mark-Simulacrum Mark-Simulacrum

Labels
S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@onestacked @rust-highfive @bors @RalfJung @Mark-Simulacrum @rustbot