Miri/CTFE: properly treat overflow in (signed) division/rem as UB #94512

RalfJung · 2022-03-02T01:05:53Z

To my surprise, it looks like LLVM treats overflow of signed div/rem as UB. From what I can tell, MIR Div/Rem directly lowers to the corresponding LLVM operation, so to make that correct we also have to consider these overflows UB in the CTFE/Miri interpreter engine.

r? @oli-obk

rust-highfive · 2022-03-02T01:05:56Z

Some changes occured to the CTFE / Miri engine

cc @rust-lang/miri

Some changes occured to the CTFE / Miri engine

cc @rust-lang/miri

RalfJung · 2022-03-02T01:41:33Z

library/core/tests/num/wrapping.rs

+    // see https://github.com/rust-lang/rust/pull/94512.
+    const _: () = {
+        assert!(i32::MIN.wrapping_div(-1) == i32::MIN);
+        assert!(i32::MIN.wrapping_rem(-1) == 0);


Turns out wrapping_div/rem actually does distinguish between div-by-0 and overflow, but LLVM does not (both are UB). For a second I thought wrapping_div relies on the old behavior of the interpreter, but turns out that function actually explicitly checks for this case and avoids even calling the MIR operator. So we are good.

But this is subtle enough that it warrants a specific test.

oli-obk · 2022-03-02T08:36:21Z

@bors r+ rollup

bors · 2022-03-02T08:36:22Z

📌 Commit 6739299 has been approved by oli-obk

Miri/CTFE: properly treat overflow in (signed) division/rem as UB To my surprise, it looks like LLVM treats overflow of signed div/rem as UB. From what I can tell, MIR `Div`/`Rem` directly lowers to the corresponding LLVM operation, so to make that correct we also have to consider these overflows UB in the CTFE/Miri interpreter engine. r? `@oli-obk`

Miri/CTFE: properly treat overflow in (signed) division/rem as UB To my surprise, it looks like LLVM treats overflow of signed div/rem as UB. From what I can tell, MIR `Div`/`Rem` directly lowers to the corresponding LLVM operation, so to make that correct we also have to consider these overflows UB in the CTFE/Miri interpreter engine. r? ``@oli-obk``

Miri/CTFE: properly treat overflow in (signed) division/rem as UB To my surprise, it looks like LLVM treats overflow of signed div/rem as UB. From what I can tell, MIR `Div`/`Rem` directly lowers to the corresponding LLVM operation, so to make that correct we also have to consider these overflows UB in the CTFE/Miri interpreter engine. r? ```@oli-obk```

Miri/CTFE: properly treat overflow in (signed) division/rem as UB To my surprise, it looks like LLVM treats overflow of signed div/rem as UB. From what I can tell, MIR `Div`/`Rem` directly lowers to the corresponding LLVM operation, so to make that correct we also have to consider these overflows UB in the CTFE/Miri interpreter engine. r? ````@oli-obk````

Miri/CTFE: properly treat overflow in (signed) division/rem as UB To my surprise, it looks like LLVM treats overflow of signed div/rem as UB. From what I can tell, MIR `Div`/`Rem` directly lowers to the corresponding LLVM operation, so to make that correct we also have to consider these overflows UB in the CTFE/Miri interpreter engine. r? `````@oli-obk`````

Miri/CTFE: properly treat overflow in (signed) division/rem as UB To my surprise, it looks like LLVM treats overflow of signed div/rem as UB. From what I can tell, MIR `Div`/`Rem` directly lowers to the corresponding LLVM operation, so to make that correct we also have to consider these overflows UB in the CTFE/Miri interpreter engine. r? ``````@oli-obk``````

Miri/CTFE: properly treat overflow in (signed) division/rem as UB To my surprise, it looks like LLVM treats overflow of signed div/rem as UB. From what I can tell, MIR `Div`/`Rem` directly lowers to the corresponding LLVM operation, so to make that correct we also have to consider these overflows UB in the CTFE/Miri interpreter engine. r? ```````@oli-obk```````

Dylan-DPC · 2022-03-03T00:08:06Z

failed in rollup

@bors r-

RalfJung · 2022-03-03T00:12:45Z

@bors rollup=never

The rust-log output is misleading, that is Miri failing which is fine. The actual issue (I think) is

     Running tests\compile-test.rs (build\x86_64-pc-windows-msvc\stage2-tools\x86_64-pc-windows-msvc\release\deps\compile_test-93a03ccdd9aec39c.exe)

[...]

failures:
    [ui] ui\modulo_one.rs

test result: FAILED. 682 passed; 1 failed; 9 ignored; 0 measured; 0 filtered out; finished in 16.33s

Sadly this doesn't even say which tools's test suite this is a part of.^^ But I guess it must be clippy?

RalfJung · 2022-03-03T00:21:19Z

I updated the stderr file by hand (since blessing clippy does not seem easily possible) -- hopefully that means the PR runner will now run Clippy tests.

Dylan-DPC · 2022-03-03T00:21:51Z

@bors r=oli-obk

bors · 2022-03-03T00:21:53Z

📌 Commit 24fc115 has been approved by oli-obk

RalfJung · 2022-03-03T00:23:31Z

@bors r-

I'd prefer to first see if PR CI is green.

Dylan-DPC · 2022-03-03T00:24:07Z

ah oops :D

oli-obk · 2022-03-03T09:27:33Z

@bors r+

bors · 2022-03-03T09:27:35Z

📌 Commit 24fc115 has been approved by oli-obk

bors · 2022-03-03T12:56:27Z

⌛ Testing commit 24fc115 with merge 4566094...

bors · 2022-03-03T15:20:41Z

☀️ Test successful - checks-actions
Approved by: oli-obk
Pushing 4566094 to master...

rust-highfive · 2022-03-03T15:21:29Z

📣 Toolstate changed by #94512!

Tested on commit 4566094.
Direct link to PR: #94512

💔 miri on windows: test-pass → test-fail (cc @oli-obk @RalfJung @eddyb).
💔 miri on linux: test-pass → test-fail (cc @oli-obk @RalfJung @eddyb).

@oli-obk

Tested on commit rust-lang/rust@4566094. Direct link to PR: <rust-lang/rust#94512> 💔 miri on windows: test-pass → test-fail (cc @oli-obk @RalfJung @eddyb). 💔 miri on linux: test-pass → test-fail (cc @oli-obk @RalfJung @eddyb).

adjust for div/rem overflow being UB This is the Miri side of rust-lang/rust#94512. Just some error messages change.

rust-timer · 2022-03-03T21:50:55Z

Finished benchmarking commit (4566094): comparison url.

Summary: This benchmark run did not return any relevant results. 3 results were found to be statistically significant but too small to be relevant.

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

@rustbot label: -perf-regression

Miri/CTFE: properly treat overflow in (signed) division/rem as UB To my surprise, it looks like LLVM treats overflow of signed div/rem as UB. From what I can tell, MIR `Div`/`Rem` directly lowers to the corresponding LLVM operation, so to make that correct we also have to consider these overflows UB in the CTFE/Miri interpreter engine. r? `@oli-obk`

rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Mar 2, 2022

rust-highfive assigned oli-obk Mar 2, 2022

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 2, 2022

This comment has been minimized.

Sign in to view

Miri/CTFE: properly treat overflow in (signed) division/rem as UB

6739299

RalfJung force-pushed the sdiv-ub branch from 2fd1ac7 to 6739299 Compare March 2, 2022 01:40

RalfJung commented Mar 2, 2022

View reviewed changes

RalfJung mentioned this pull request Mar 2, 2022

adjust for div/rem overflow being UB rust-lang/miri#1992

Merged

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 2, 2022

Dylan-DPC mentioned this pull request Mar 2, 2022

Rollup of 8 pull requests #94520

Closed

This was referenced Mar 2, 2022

Rollup of 8 pull requests #94521

Closed

Rollup of 7 pull requests #94525

Closed

matthiaskrgr mentioned this pull request Mar 2, 2022

Rollup of 11 pull requests #94531

Closed

matthiaskrgr mentioned this pull request Mar 2, 2022

Rollup of 10 pull requests #94532

Closed

matthiaskrgr mentioned this pull request Mar 2, 2022

Rollup of 8 pull requests #94533

Closed

matthiaskrgr mentioned this pull request Mar 2, 2022

Rollup of 9 pull requests #94535

Closed

bors 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-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Mar 3, 2022

bless clippy

24fc115

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Mar 3, 2022

bors 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-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Mar 3, 2022

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Mar 3, 2022

bors added the merged-by-bors This PR was explicitly merged by bors. label Mar 3, 2022

bors merged commit 4566094 into rust-lang:master Mar 3, 2022

rustbot added this to the 1.61.0 milestone Mar 3, 2022

rust-highfive mentioned this pull request Mar 3, 2022

miri no longer builds after rust-lang/rust#94512 #94562

Closed

bors added a commit to rust-lang/miri that referenced this pull request Mar 3, 2022

Auto merge of #1992 - RalfJung:sdiv, r=RalfJung

2f9ecde

adjust for div/rem overflow being UB This is the Miri side of rust-lang/rust#94512. Just some error messages change.

RalfJung deleted the sdiv-ub branch March 6, 2022 00:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Miri/CTFE: properly treat overflow in (signed) division/rem as UB #94512

Miri/CTFE: properly treat overflow in (signed) division/rem as UB #94512

RalfJung commented Mar 2, 2022

rust-highfive commented Mar 2, 2022

This comment has been minimized.

RalfJung Mar 2, 2022

oli-obk commented Mar 2, 2022

bors commented Mar 2, 2022

Dylan-DPC commented Mar 3, 2022

RalfJung commented Mar 3, 2022 •

edited

Loading

RalfJung commented Mar 3, 2022

Dylan-DPC commented Mar 3, 2022

bors commented Mar 3, 2022

RalfJung commented Mar 3, 2022

Dylan-DPC commented Mar 3, 2022

oli-obk commented Mar 3, 2022

bors commented Mar 3, 2022

bors commented Mar 3, 2022

bors commented Mar 3, 2022

rust-highfive commented Mar 3, 2022

rust-timer commented Mar 3, 2022

Miri/CTFE: properly treat overflow in (signed) division/rem as UB #94512

Miri/CTFE: properly treat overflow in (signed) division/rem as UB #94512

Conversation

RalfJung commented Mar 2, 2022

rust-highfive commented Mar 2, 2022

This comment has been minimized.

RalfJung Mar 2, 2022

Choose a reason for hiding this comment

oli-obk commented Mar 2, 2022

bors commented Mar 2, 2022

Dylan-DPC commented Mar 3, 2022

RalfJung commented Mar 3, 2022 • edited Loading

RalfJung commented Mar 3, 2022

Dylan-DPC commented Mar 3, 2022

bors commented Mar 3, 2022

RalfJung commented Mar 3, 2022

Dylan-DPC commented Mar 3, 2022

oli-obk commented Mar 3, 2022

bors commented Mar 3, 2022

bors commented Mar 3, 2022

bors commented Mar 3, 2022

rust-highfive commented Mar 3, 2022

rust-timer commented Mar 3, 2022

RalfJung commented Mar 3, 2022 •

edited

Loading