floating point to integer casts can cause undefined behaviour

[thestinger]

Status as of 2020-04-18

We intend to stabilize the saturating-float-casts behavior for as, and have stabilized unsafe library functions that handle the previous behavior. See #71269 for the latest discussion on that stabilization process.

Status as of 2018-11-05

A flag has been implemented in the compiler, -Zsaturating-float-casts, which will cause all float to integer casts have "saturating" behavior where if it's out of bounds it's clamped to the nearest bound. A call for benchmarking of this change went out awhile ago. Results, while positive in many projects, are quite negative for some projects and indicates that we're not done here.

The next steps are figuring out how to recover performance for these cases:

• One option is to take today's as cast behavior (which is UB in some cases) and add unsafe functions for the relevant types and such.
• Another is to wait for LLVM to add a freeze concept which means that we get a garbage bit pattern, but it's at least not UB
• Another is to implement casts via inline assembly in LLVM IR, as the current codegen is not heavily optimized.

Old status

UPDATE (by @nikomatsakis): After much discussion, we've got the rudiments of a plan for how to address this problem. But we need some help with actually investigating the performance impact and working out the final details!

---

ORIGINAL ISSUE FOLLOWS:

If the value cannot fit in ty2, the results are undefined.

1.04E+17 as u8

[brson]

Nominating

[pnkfelix]

accepted for P-high, same reasoning as #10183

[pcwalton]

I don't think this is backwards incompatible at a language level. It will not cause code that was working OK to stop working. Nominating.

[pnkfelix]

changing to P-high, same reasoning as #10183

[nrc]

How do we propose to solve this and #10185? Since whether behaviour is defined or not depends on the dynamic value of the number being cast, it seems the only solution is to insert dynamic checks. We seem to agree we do not want to do that for arithmetic overflow, are we happy to do it for cast overflow?

[pcwalton]

We could add an intrinsic to LLVM that performs a "safe conversion". @zwarich may have other ideas.

[zwarich]

AFAIK the only solution at the moment is to use the target-specific intrinsics. That's what JavaScriptCore does, at least according to someone I asked.

[pcwalton]

Oh, that's easy enough then.

[nrc]

ping @pnkfelix is this covered by the new overflow checking stuff?

[bluss]

These casts are not checked by rustc with debug assertions.

[Aatch]

I'm happy to handle this, but I need a concrete solution. I personally think that it should be checked along with overflowing integer arithmetic, as it's a very similar issue. I don't really mind what we do though.

Note that this issue is currently causing an ICE when used in certain constant expressions.

[bluss]

This allows violating memory safety in safe rust, example from this forum post:

Undefs, huh? Undefs are fun. They tend to propagate. After a few minutes of wrangling..

#[inline(never)]
pub fn f(ary: &[u8; 5]) -> &[u8] {
    let idx = 1e100f64 as usize;
    &ary[idx..]
}

fn main() {
    println!("{}", f(&[1; 5])[0xdeadbeef]);
}

segfaults on my system (latest nightly) with -O.

[steveklabnik]

Marking with I-unsound given the violation of memory safety in safe rust.

[steveklabnik]

@bluss , this does not segfualt for me, just gives an assertion error. untagging since i was the one who added it

[steveklabnik]

Sigh, I forgot the -O, re-tagging.

[nagisa]

re-nominating for P-high. Apparently this was at some point P-high but got lower over time. This seems pretty important for correctness.

EDIT: didn’t react to triage comment, adding label manually.

[nikomatsakis]

It seems like the precedent from the overflow stuff (e.g. for shifting) is to just settle on some behavior. Java seems to produce the result modulo the range, which seems not unreasonable; I'm not sure just what kind of LLVM code we'd need to handle that.

[RalfJung]

Awesome, I'll copy https://github.com/WebAssembly/testsuite/blob/master/conversions.wast (with traps replaced by the specified results) to Miri's test suite then. :)

[sunfishcode]

@RalfJung Please update to the latest version of conversions.wast, which was just updated to include tests for the new saturating conversion operators. The new operators have "_sat" in their names, and they don't have trapping so you shouldn't need to replace anything.

[RalfJung]

@sunfishcode thanks for updating! I have to translate the tests to Rust anyway so I still have to replace many things. ;)

Are the _sat tests any different in terms of the values being tested? (EDIT: there's a comment there saying the values are the same.) For Rust's saturating casts I took many of these values and added them in rust-lang/miri#1321. I was too lazy to do it for all of them... but I think this means that there is nothing to change right now with the updated file.

For the UB intrinsic, the traps on the wasm side should then become compile-fail tests in Miri I think.

[sunfishcode]

The input values are all the same, the only difference is that _sat operators have expected output values on inputs where the trapping operators have expected traps.

[RalfJung]

Tests for Miri (and thus also the Rust CTFE engine) were added in rust-lang/miri#1321. I locally checked that rustc -Zmir-opt-level=0 -Zsaturating-float-casts also passes the tests in that file.
I now also implemented the unchecked intrinsic in Miri, see rust-lang/miri#1325.

[Mark-Simulacrum]

I've posted #71269 (comment) which documents the current state as I understood it and that PR also moves to stabilize the behavior of the saturating -Z flag.

Given the length of this thread I think if folks feel that I've missed anything in that comment, I would direct commentary to the PR, or, if it's minor, feel free to ping me on Zulip or Discord (simulacrum) and I can fix things up to avoid unnecessary noise on the PR thread.

I expect that someone on the language team will likely start an FCP proposal on that PR soon, and merging it will automatically close this issue out :)

[federicomenaquintero]

Are there plans for checked conversions? Something like fn i32::checked_from(f64) -> Result<i32, DoesntFit>?

[kennytm]

You'll need to consider what should i32::checked_from(4.5) return.