use checked casts and arithmetic in Miri engine #70226

RalfJung · 2020-03-21T13:11:23Z

This is unfortunately pretty annoying because we have to cast back and forth between u64 and usize more often that should be necessary, and that cast is considered fallible.

For example, should this really be usize?

Also, LayoutDetails uses usize for field indices, but in Miri we use u64 to be able to also handle array indexing. Maybe methods like mplace_field should be suitably generalized to accept both u64 and usize?

r? @oli-obk Cc @eddyb

eddyb · 2020-03-21T15:28:13Z

This is unfortunately pretty annoying because we have to cast back and forth between u64 and usize more often that should be necessary, and that cast is considered fallible.

Only u64 -> usize should be fallible, usize -> u64 shouldn't, but you're saying they both implement TryFrom and neither From?

Did we really future-proof the standard library for 128-bit address spaces?
Actually, that makes sense - the compiler has to care about what's currently implemented, but the standard library must be future-compatible with anything we might want to support.

RalfJung · 2020-03-21T16:18:26Z

Actually, that makes sense - the compiler has to care about what's currently implemented, but the standard library must be future-compatible with anything we might want to support.

Yeah, exactly that. And that is quite annoying. Maybe the compiler should have its own From that permits usize->u64.

Anyway, I made this a bit less annoying for Miri by now having both usize and u64-based projection methods (place_field/place_index, respectively).

RalfJung · 2020-03-21T16:19:54Z

src/librustc_mir/interpret/cast.rs

-                let width = t.bit_width().unwrap_or_else(|| self.pointer_size().bits() as usize);
-                let v = f.to_u128(width).value;
+                let width = t.bit_width().unwrap_or_else(|| self.pointer_size().bits());
+                let v = f.to_u128(usize::try_from(width).unwrap()).value;


This is apfloat using usize instead of u64 for sizes...

I mean, it could be an u8 just as well, the max value here is 128.

IMO it would make sense to consistently use the same type for sizes, that avoids casts. Note how changing bit_width to return u64 got rid of a bunch of casts both in Miri and codegen.

src/librustc/mir/interpret/allocation.rs

eddyb · 2020-03-22T18:10:07Z

src/librustc_target/abi/mod.rs


    #[inline]
-    pub fn from_bits(bits: u64) -> Size {
+    pub fn from_bits(bits: impl TryInto<u64>) -> Size {
+        let bits = bits.try_into().ok().unwrap();


Doesn't this result in a worse panic message than if you remove the .ok()?

Yeah but it doesn't compile without the ok() as we don't know if the error type implements Debug.

eddyb · 2020-03-22T18:11:21Z

src/librustc_target/abi/mod.rs

-    pub const fn from_bytes(bytes: u64) -> Size {
-        Size { raw: bytes }
+    pub fn from_bytes(bytes: impl TryInto<u64>) -> Size {
+        Size { raw: bytes.try_into().ok().unwrap() }


Same here (removing the .ok()).

RalfJung · 2020-03-22T18:46:20Z

src/librustc_mir/interpret/place.rs

+    pub fn mplace_index(
+        &self,
+        base: MPlaceTy<'tcx, M::PointerTag>,
+        index: u64,
+    ) -> InterpResult<'tcx, MPlaceTy<'tcx, M::PointerTag>> {


This is partially a duplication of logic in mplace_field -- but note that that already was a duplication of logic in the layout code. We cannot use the layout code as it is written for usize.

oli-obk · 2020-03-22T22:01:19Z

src/librustc/mir/interpret/allocation.rs

        Allocation {
-            bytes: vec![0; size.bytes() as usize],


maybe give Size a method bytes_usize which does this check for you so it isn't spread around everywhere. Make it #[track_caller] though

I added bytes_usize, but was worried track_caller would be bad for performance.

src/librustc/mir/interpret/allocation.rs

src/librustc/mir/interpret/value.rs

oli-obk · 2020-03-22T22:12:05Z

src/librustc/mir/interpret/allocation.rs

-            "cannot handle this access on this host architecture"
-        );
-        let end = end.bytes() as usize;
+        let end = Size::add(offset, size); // This does overflow checking.


I don't like the Size::bin_op style very much. Why are the regular binary operators not ok?

When I see a + I have to assume it is not overflow-checked. Figuring out the type of the operands can be non-trivial. So the only way to write overflow-clean Rust code for sure is to make sure there is, syntactically, no +/*/*-anywhere in the code. Hence the explicitSize::{add,mul}`.

The alternative is to just turn on overflow checks in release mode.

I still don't like it. This is much less readable and most of the time the types are a line or two above in the arguments.

I'm not gonna block the PR on it, but I want someone else to also say it's ok to write lisp-math in rustc if infix math would produce the exact same machine code.

Now that I did the audit, I think I'm fine going back to infix notation. I will add a comment though in places where the type is not immediately obvious.

The alternative is to just turn on overflow checks in release mode.

Would be great if that could be controlled on a per-module basis. Given that this is a MIR building thing, I see no reason why per-module should not be possible.

we could probably start out with a rustc_overflow_checks attribute, seems straight forward enough to do.

we could probably start out with a rustc_overflow_checks attribute, seems straight forward enough to do.

#70358

eddyb · 2020-03-23T00:24:34Z

@oli-obk's comment (#70226 (comment)) reminded me there used to be this pattern:

let x: u128 = ...; // Some potentially large value...
let x_as_u64 = x as u64; // ... that we want to fit in a smaller type
assert_eq!(x_as_u64 as u128, x);
// Use `x_as_u64` now that it's been validated.

I believe that u64::try_from(x).unwrap() is an appropriate replacement.
So a bunch of assert_eq!s can probably be removed now.

RalfJung · 2020-03-24T08:55:21Z

@eddyb Indeed as part of this PR I replaced all instances of this pattern that I found by try_from. If there are any left, I overlooked them.

bors · 2020-03-25T00:02:21Z

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

oli-obk · 2020-03-25T14:28:42Z

r=me after a rebase

…rand of overflowing_signed_offset

RalfJung · 2020-03-25T14:56:58Z

Rebased.
@bors r=oli-obk

bors · 2020-03-25T14:56:59Z

📌 Commit 7400955 has been approved by oli-obk

bors · 2020-03-25T18:06:19Z

⌛ Testing commit 7400955 with merge cf1bd7d3721a751f3f7737ba56e6e73bee07b40e...

@oli-obk

use checked casts and arithmetic in Miri engine This is unfortunately pretty annoying because we have to cast back and forth between `u64` and `usize` more often that should be necessary, and that cast is considered fallible. For example, should [this](https://doc.rust-lang.org/nightly/nightly-rustc/rustc/mir/interpret/value/enum.ConstValue.html) really be `usize`? Also, `LayoutDetails` uses `usize` for field indices, but in Miri we use `u64` to be able to also handle array indexing. Maybe methods like `mplace_field` should be suitably generalized to accept both `u64` and `usize`? r? @oli-obk Cc @eddyb

Dylan-DPC-zz · 2020-03-25T18:49:22Z

@bors retry yielding to rollup (which also contains this one)

bors · 2020-03-25T19:39:00Z

⌛ Testing commit 7400955 with merge d38c9793e9a3fda2dc9d8a052f3998e9cc57c1ad...

Dylan-DPC-zz · 2020-03-25T19:42:18Z

@bors retry yielding to rollup

@ghost

Rollup of 5 pull requests Successful merges: - rust-lang#70226 (use checked casts and arithmetic in Miri engine) - rust-lang#70319 (correctly normalize constants) - rust-lang#70352 (Add long error explanation for E0710 ) - rust-lang#70366 (Implement Fuse with Option) - rust-lang#70379 (fix incorrect type name in doc comments) Failed merges: - rust-lang#70375 (avoid catching InterpError) r? @ghost

@ghost

update miri Usually I'd wait until rust-lang#70226 lands which will break Miri again, but... the queue is empty (!), so whatever.^^ r? @ghost Cc @oli-obk Fixes rust-lang#70346

rust-highfive assigned oli-obk Mar 21, 2020

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

RalfJung commented Mar 21, 2020

View reviewed changes

RalfJung commented Mar 22, 2020

View reviewed changes

src/librustc/mir/interpret/allocation.rs Outdated Show resolved Hide resolved

eddyb reviewed Mar 22, 2020

View reviewed changes

This comment has been minimized.

Sign in to view

RalfJung commented Mar 22, 2020

View reviewed changes

RalfJung force-pushed the checked branch from 934d00b to 0a1c02c Compare March 22, 2020 18:49

oli-obk reviewed Mar 22, 2020

View reviewed changes

src/librustc/mir/interpret/allocation.rs Outdated Show resolved Hide resolved

oli-obk reviewed Mar 22, 2020

View reviewed changes

src/librustc/mir/interpret/value.rs Outdated Show resolved Hide resolved

oli-obk reviewed Mar 22, 2020

View reviewed changes

RalfJung force-pushed the checked branch from fdf9d40 to 9cddfa6 Compare March 24, 2020 09:19

RalfJung mentioned this pull request Mar 25, 2020

update miri #70391

Merged

RalfJung added 8 commits March 25, 2020 15:48

miri: simplify shift operator overflow checking

5be304b

use checked casts and arithmetic in Miri engine

1ddbdc6

make bit_width return u64, consistently with other sizes in the compiler

9de6008

avoid double-cast in mplace_field

cd15b65

miri: avoid a bunch of casts by offering usized-based field indexing

d7e2650

remove unnecessary cast

f16b491

make Size::from* methods generic in the integer type they accept

0bc108a

use Size addition instead of checked int addition

afcb634

RalfJung added 3 commits March 25, 2020 15:53

add helper method for ptr ops on Scalar; reduce unnecessary large ope…

1d67ca0

…rand of overflowing_signed_offset

go back to infix ops for Size

b7db732

add usize methods for Size getters

7400955

RalfJung force-pushed the checked branch from 55e72ca to 7400955 Compare March 25, 2020 14:54

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 25, 2020

RalfJung mentioned this pull request Mar 25, 2020

add 'fn write_u16s' to Memory #70397

Merged

This was referenced Mar 25, 2020

Rollup of 5 pull requests #70403

Closed

Rollup of 5 pull requests #70404

Merged

bors merged commit 97f0a9e into rust-lang:master Mar 25, 2020

RalfJung deleted the checked branch March 25, 2020 23:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

use checked casts and arithmetic in Miri engine #70226

use checked casts and arithmetic in Miri engine #70226

RalfJung commented Mar 21, 2020

eddyb commented Mar 21, 2020 •

edited

Loading

RalfJung commented Mar 21, 2020

RalfJung Mar 21, 2020

eddyb Mar 21, 2020

RalfJung Mar 21, 2020

eddyb Mar 22, 2020

RalfJung Mar 22, 2020

eddyb Mar 22, 2020

This comment has been minimized.

RalfJung Mar 22, 2020

oli-obk Mar 22, 2020

RalfJung Mar 24, 2020

oli-obk Mar 22, 2020

RalfJung Mar 24, 2020

oli-obk Mar 24, 2020

oli-obk Mar 24, 2020

RalfJung Mar 24, 2020 •

edited

Loading

oli-obk Mar 24, 2020

RalfJung Mar 24, 2020

eddyb commented Mar 23, 2020

RalfJung commented Mar 24, 2020

bors commented Mar 25, 2020

oli-obk commented Mar 25, 2020

RalfJung commented Mar 25, 2020

bors commented Mar 25, 2020

bors commented Mar 25, 2020

Dylan-DPC-zz commented Mar 25, 2020

bors commented Mar 25, 2020

Dylan-DPC-zz commented Mar 25, 2020

use checked casts and arithmetic in Miri engine #70226

use checked casts and arithmetic in Miri engine #70226

Conversation

RalfJung commented Mar 21, 2020

eddyb commented Mar 21, 2020 • edited Loading

RalfJung commented Mar 21, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment has been minimized.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RalfJung Mar 24, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eddyb commented Mar 23, 2020

RalfJung commented Mar 24, 2020

bors commented Mar 25, 2020

oli-obk commented Mar 25, 2020

RalfJung commented Mar 25, 2020

bors commented Mar 25, 2020

bors commented Mar 25, 2020

Dylan-DPC-zz commented Mar 25, 2020

bors commented Mar 25, 2020

Dylan-DPC-zz commented Mar 25, 2020

eddyb commented Mar 21, 2020 •

edited

Loading

RalfJung Mar 24, 2020 •

edited

Loading