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

Fix MIR check for MIN % -1 in exact_div #69138

Closed
wants to merge 1 commit into from

Conversation

elichai
Copy link
Contributor

@elichai elichai commented Feb 13, 2020

Hi,
I hope I understood the bug correctly, because I'm not too familiar with the code here.

I've found this while playing with Miri, it seems like this if condition is no longer executed: https://github.com/rust-lang/rust/blob/master/src/librustc_mir/interpret/intrinsics.rs#L414
Because this commit 28f85c6 made it return 0 in the MIN % -1 case, while the if explicitly tests if it's not 0

So I reverted to the old behavior which returned the left side (MIN) instead of 0.

r? @RalfJung

FWIW, Before this the exact_div4 test in Miri failed and after this it passes

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Feb 13, 2020
@RalfJung
Copy link
Member

RalfJung commented Feb 13, 2020

Thanks a lot for helping to fix Miri! However, I'm afraid I already took care of this one... #69126

So I reverted to the old behavior which returned the left side (MIN) instead of 0.

The old behavior was wrong, though. int_min % -1 returns 0 when you actually run it.

@elichai
Copy link
Contributor Author

elichai commented Feb 13, 2020

@RalfJung Ha. That PR wasn't there when I started diving into rustc_mir 😉

int_min % -1 returns 0 when you actually run it.

I think I don't quite understand what the mir::interpret is (ie when exactly does it run and on what) but maybe we can take this convo offline :)

@RalfJung
Copy link
Member

RalfJung commented Feb 13, 2020

I suggest https://rust-lang.github.io/rustc-guide/const-eval.html (and its sub-section) to start with :)

@elichai elichai closed this Feb 13, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants