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

Under Miri, disable debug asserts for things Miri always checks #101079

Closed
wants to merge 1 commit into from

Conversation

saethlin
Copy link
Member

@saethlin saethlin commented Aug 27, 2022

Context: rust-lang/miri#2497, Miri currently disables debug assertions in the standard library out of concern for how much slower code runs under Miri. A significant contributor there are these checks, which are entirely redundant and occur before Miri's, resulting in a much worse user experience if they are actually hit. So there is no reason to have them enabled under Miri.

However, this leaves some checks still enabled. Most notably, the bounds check(s) on slice::get_unchecked is staying put. This precondition originates in the requirements of Stacked Borrows, which can be disabled, and also has nothing to say about ZSTs, where the library API forbids out-of-bounds indexing, even for ZSTs.

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

r? @m-ou-se

(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 Aug 27, 2022
@saethlin
Copy link
Member Author

r? @ghost

@saethlin
Copy link
Member Author

@rustbot author

@rustbot rustbot 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 Aug 27, 2022
@saethlin saethlin force-pushed the redundant-miri-checks branch 2 times, most recently from 30d6ce8 to 680f167 Compare August 27, 2022 22:52
@rust-log-analyzer

This comment has been minimized.

@saethlin saethlin force-pushed the redundant-miri-checks branch from 680f167 to 388dd8b Compare August 27, 2022 23:12
@bors
Copy link
Contributor

bors commented Sep 5, 2022

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

@saethlin saethlin force-pushed the redundant-miri-checks branch from 388dd8b to 3ea7fa3 Compare September 5, 2022 18:38
@saethlin saethlin force-pushed the redundant-miri-checks branch from 3ea7fa3 to da79af7 Compare September 15, 2022 02:55
@bors
Copy link
Contributor

bors commented Oct 26, 2022

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

@saethlin saethlin force-pushed the redundant-miri-checks branch from da79af7 to 7710d6c Compare October 26, 2022 13:46
@bors
Copy link
Contributor

bors commented Oct 27, 2022

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

@saethlin saethlin force-pushed the redundant-miri-checks branch from 7710d6c to 80b6a04 Compare October 27, 2022 14:23
@saethlin
Copy link
Member Author

I'm not sure the magnitude of the effect here is worth the complexity of implementation. Maybe if we had a different implementation strategy it might make sense, but the scary heavyweight debug assertions for container invariants aren't/can't be covered by this strategy.

@saethlin saethlin closed this Dec 26, 2022
@saethlin saethlin deleted the redundant-miri-checks branch March 15, 2023 00:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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
Development

Successfully merging this pull request may close these issues.

6 participants