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

floating point to integer casts can cause undefined behaviour #10184

Closed
thestinger opened this issue Oct 31, 2013 · 234 comments
Closed

floating point to integer casts can cause undefined behaviour #10184

thestinger opened this issue Oct 31, 2013 · 234 comments

Comments

@thestinger
Copy link
Contributor

@thestinger thestinger commented Oct 31, 2013

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
Copy link
Contributor

@brson brson commented Oct 31, 2013

Nominating

@pnkfelix
Copy link
Member

@pnkfelix pnkfelix commented Nov 7, 2013

accepted for P-high, same reasoning as #10183

@pcwalton
Copy link
Contributor

@pcwalton pcwalton commented Nov 22, 2013

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
Copy link
Member

@pnkfelix pnkfelix commented Dec 19, 2013

changing to P-high, same reasoning as #10183

@nrc
Copy link
Member

@nrc nrc commented Sep 12, 2014

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
Copy link
Contributor

@pcwalton pcwalton commented Sep 12, 2014

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

@zwarich
Copy link

@zwarich zwarich commented Sep 12, 2014

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
Copy link
Contributor

@pcwalton pcwalton commented Sep 12, 2014

Oh, that's easy enough then.

@nrc
Copy link
Member

@nrc nrc commented Apr 23, 2015

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

@bluss
Copy link
Member

@bluss bluss commented May 7, 2015

These casts are not checked by rustc with debug assertions.

@Aatch
Copy link
Contributor

@Aatch Aatch commented Sep 13, 2015

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
Copy link
Member

@bluss bluss commented Sep 13, 2015

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
Copy link
Member

@steveklabnik steveklabnik commented Oct 8, 2015

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

@steveklabnik
Copy link
Member

@steveklabnik steveklabnik commented Oct 29, 2015

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

@steveklabnik
Copy link
Member

@steveklabnik steveklabnik commented Oct 29, 2015

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

@nagisa
Copy link
Contributor

@nagisa nagisa commented Feb 21, 2016

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
Copy link
Contributor

@nikomatsakis nikomatsakis commented Feb 25, 2016

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.

Centril added a commit to Centril/rust that referenced this issue Apr 2, 2020
…ts, r=SimonSapin

Stabilize float::to_int_unchecked

This renames and stabilizes unsafe floating point to integer casts, which are intended to be the substitute for the currently unsound `as` behavior, once that changes to safe-but-slower saturating casts. As such, I believe this also likely unblocks rust-lang#10184 (our oldest I-unsound issue!), as once this rolls out to stable it would be far easier IMO to change the behavior of `as` to be safe by default.

This does not stabilize the trait or the associated method, as they are deemed internal implementation details (and consumers should not, generally, want to expose them, as in practice all callers likely know statically/without generics what the return type is).

Closes rust-lang#67058
@mawis
Copy link

@mawis mawis commented Apr 10, 2020

One option that’s already implemented and available in Nightly with the -Z saturating-casts compiler flag is to define them to return respectively: $Int::MAX, $Int::MIN, and zero. But it’s still possible pick some other behavior.

The behavior I would expect to get for f.trunc() > $Int::MAX and f.trunc() < $Int::MIN is the same as when the floating point number imaginary gets converted to an infinite sized integer number and then the lowest significant bits of that are returned (as in the conversion of integer types). Technically this would be some bits of the significant shifted to the left depending on the exponent (for positive numbers, negative numbers need inversion according to the two's complement).

So for example I would expect really big numbers to convert to 0.

It seems to be harder/more arbitrary to define what infinity and NaN converts to.

@CryZe
Copy link
Contributor

@CryZe CryZe commented Apr 10, 2020

@RalfJung
Copy link
Member

@RalfJung RalfJung commented Apr 10, 2020

@CryZe so if I read that correctly, that matches -Z saturating-casts (and what Miri already implements)?

@sunfishcode
Copy link
Member

@sunfishcode sunfishcode commented Apr 10, 2020

@RalfJung That's correct.

@RalfJung
Copy link
Member

@RalfJung RalfJung commented Apr 11, 2020

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
Copy link
Member

@sunfishcode sunfishcode commented Apr 11, 2020

@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
Copy link
Member

@RalfJung RalfJung commented Apr 11, 2020

@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
Copy link
Member

@sunfishcode sunfishcode commented Apr 11, 2020

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
Copy link
Member

@RalfJung RalfJung commented Apr 12, 2020

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
Copy link
Member

@Mark-Simulacrum Mark-Simulacrum commented Apr 18, 2020

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
Copy link
Contributor

@federicomenaquintero federicomenaquintero commented Apr 22, 2020

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

@kennytm
Copy link
Member

@kennytm kennytm commented Apr 22, 2020

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

Dylan-DPC added a commit to Dylan-DPC/rust that referenced this issue May 5, 2020
…nikic

Define UB in float-to-int casts to saturate

This closes rust-lang#10184 by defining the behavior there to saturate infinities and values exceeding the integral range (on the lower or upper end). `NaN` is sent to zero.
@bors bors closed this in 14d608f May 6, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.