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

remove -Zmiri-panic-on-unsupported flag #3950

Merged
merged 2 commits into from
Oct 10, 2024

Conversation

RalfJung
Copy link
Member

@RalfJung RalfJung commented Oct 8, 2024

Fixes #3952, see that issue for discussion.

@RalfJung RalfJung force-pushed the handle_unsupported_foreign_item branch from 4adf9a3 to 115f00e Compare October 8, 2024 06:51
@saethlin
Copy link
Member

saethlin commented Oct 8, 2024

nextest is the better option for "continue running other tests if one test failed"

Nextest does not and cannot support doctests, so this would mean we don't have a way to continue running if one doctest fails.

@RalfJung
Copy link
Member Author

RalfJung commented Oct 8, 2024

Cargo/rustdoc already supports continuing if one doctest fails, I think?

It also runs all doctests concurrently. That's why in miri-test-libstd, doctests are so much faster than unit/integration tests. ;)

@saethlin
Copy link
Member

saethlin commented Oct 8, 2024

Ah of course it does, this is probably the same reason to get a single-threaded doctest run you need -j1 plus --test-threads=1.

@saethlin
Copy link
Member

saethlin commented Oct 8, 2024

So if all use of this flag can be replaced by nextest, why should we keep it at all? I don't think I've seen this actually happen, but injecting new unwinding sites into a bunch of unsafe code may cause UB that's only there because of unwinding that can only happen with Miri.

@RalfJung
Copy link
Member Author

RalfJung commented Oct 8, 2024

Yeah I agree, IMO we should remove the flag. That requires a bit of discussion, so I opened an issue: #3952.

However if possible I'd like to land this PR in the mean time to remove the foot-gun. I doubt unsupported errors due to syscall are super common so I would hope this is an acceptable reduction in functionality -- but OTOH maybe @landaire specifically needed it for syscall and that's why they added it in this function...

@RalfJung RalfJung changed the title remove handle_unsupported_foreign_item helper remove -Zmiri-panic-on-unsupported flag Oct 10, 2024
@RalfJung
Copy link
Member Author

All right, I now made this just entirely remove the flag. @rust-lang/miri any objections?

@RalfJung RalfJung force-pushed the handle_unsupported_foreign_item branch from 9a02955 to 2b3deec Compare October 10, 2024 06:33
@saethlin
Copy link
Member

No objections.

@oli-obk
Copy link
Contributor

oli-obk commented Oct 10, 2024

@bors r=RalfJung,saethlin,oli-obk

@bors
Copy link
Contributor

bors commented Oct 10, 2024

📌 Commit 2b3deec has been approved by RalfJung,saethlin,oli-obk

It is now in the queue for this repository.

@bors
Copy link
Contributor

bors commented Oct 10, 2024

⌛ Testing commit 2b3deec with merge bdc100f...

@bors
Copy link
Contributor

bors commented Oct 10, 2024

☀️ Test successful - checks-actions
Approved by: RalfJung,saethlin,oli-obk
Pushing bdc100f to master...

@bors bors merged commit bdc100f into rust-lang:master Oct 10, 2024
8 checks passed
@RalfJung RalfJung deleted the handle_unsupported_foreign_item branch October 11, 2024 08:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove -Zmiri-panic-on-unsupported?
4 participants