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 floating point casts have undefined behaviour #15536

Closed
huonw opened this issue Jul 8, 2014 · 20 comments

Comments

Projects
None yet
@huonw
Copy link
Member

commented Jul 8, 2014

If the value cannot fit within the destination type, ty2, then the results are undefined.

http://llvm.org/docs/LangRef.html#fptrunc-to-instruction

e.g. 1e300f64 as f32

cc #10184, #10185

@huonw

This comment has been minimized.

Copy link
Member Author

commented Jul 8, 2014

Nominating

@pcwalton

This comment has been minimized.

Copy link
Contributor

commented Jul 8, 2014

I think this is not a 1.0 issue. There is no reasonable backwards incompatible language change we can make here to fix it. The only thing to do is to change the LLVM semantics here to not be UB, but rather be an undefined value.

@zwarich

This comment has been minimized.

Copy link

commented Jul 10, 2014

@pcwalton the LLVM semantics here are that the result is an undefined value, not that the instruction exhibits undefined behavior. Otherwise you wouldn't be able to hoist an fptrunc out of a loop.

@pcwalton

This comment has been minimized.

Copy link
Contributor

commented Jul 10, 2014

OK, then I believe this is not a bug, just something to remember to note in the spec.

@pnkfelix

This comment has been minimized.

Copy link
Member

commented Jul 10, 2014

Assigning P-high, not 1.0 milestone, by the same reasoning as #10184 and #10185.

@pnkfelix pnkfelix added P-high and removed I-nominated labels Jul 10, 2014

@thestinger

This comment has been minimized.

Copy link
Contributor

commented Jul 10, 2014

@zwarich: Rust allows undefined behaviour if you have an undefined value. An undefined value has no live range so it doesn't correspond to a reserved register with a preserved value and can vary between reads. That means something like xs[undef] can do an out-of-bounds index, because there's nothing forcing it to have the same value between the check and the read.

@zwarich

This comment has been minimized.

Copy link

commented Jul 20, 2014

@thestinger That's a good point, but that just makes it like the other bugs that aren't in 1.0.

@thestinger

This comment has been minimized.

Copy link
Contributor

commented Jul 20, 2014

Programs can be written depending on the value of undefined behaviour, and changing it to either fail or provide a specific result will break those programs. This isn't C where undefined behaviour is a problem in user code.

@bombless

This comment has been minimized.

Copy link
Contributor

commented Apr 29, 2015

We may need a UB label for such issues IMO

@ranma42

This comment has been minimized.

Copy link
Contributor

commented Jan 26, 2016

Can anybody reproduce this undefined behaviour? In

#![crate_type="rlib"]

const f:f64 = 1.0E+300;

pub static ffull:f64 = f;
pub static ftrunc:f32 = f as f32;
pub static f2i:u64 = f as u64;
pub static i2f:f32 = 0xFFFFFFFFFFFFFFFFu64 as f32;

ftrunc is rounded to +inf, while (based on this issue and LLVM documentation) I would have expected an undef. In the actual LLVM code fptrunc seems to be more well-behaved than what is stated in the docs (apparently it tries to respect the IEEE754 standard, with some minor quirks around NaNs).

I asked for additional information on llvm-dev

@nodakai

This comment has been minimized.

Copy link
Contributor

commented Jan 31, 2016

@ranma42 From what I understood from the reference, the LLVM undef can turn into any value depending on each context. So we cannot complain if it happened to turn into 0x7FF0000000000000. I think the reasoning here is the same with select undef, %X, %Y can be optimized into either of %X or %Y (but not undef.)

@nrc nrc added P-medium and removed P-high labels Mar 31, 2016

@pnkfelix

This comment has been minimized.

Copy link
Member

commented Mar 31, 2016

(moving this back to P-medium and letting the I-wrong reflect the severity; the conclusion of the lang team was that there are lots of open bugs that reflect wrong behavior, and we are not going to prioritize this one as P-high.)

@DemiMarie

This comment has been minimized.

Copy link
Contributor

commented Jun 26, 2016

My thought is to define this as resulting as infinity.

@retep998

This comment has been minimized.

Copy link
Member

commented Jun 27, 2016

@drbo But doing that could be potentially more expensive than just making the result unspecified but without causing undefined behavior.

@rkruppe

This comment has been minimized.

Copy link
Member

commented Mar 31, 2018

I believe this ("if it doesn't fit in the destination type, you get UB/undef") is and always has been FUD caused by individual LLVM developers misunderstanding IEEE 754, just like with signalling NaNs.

LLVM's actual long-standing policy (now finally documented) — and the only policy that makes sense for an optimizing C compiler IMO — is that everything happens in the default floating point environment. This means among other things that rounding is always ties-to-even, the status flags are never inspected, and floating point exception handling always uses the default behavior rather than any custom handling. Consequently, it is clear as day that a floating point truncation that exceeds the range of the target type should result in positive or negative infinity. And indeed that is what LLVM's constant folding will do (specifically, what APFloat will do).

FWIW this operation is a standard operation (formatOf-convertFormat), defined in §5.4.2 of IEEE 754-2008, but it would not really matter if this was a non-standard operation since those follow the same basic rules.

@rkruppe

This comment has been minimized.

Copy link
Member

commented Mar 31, 2018

So I recommend:

  • someone with a bugs.llvm.org account files a bug with LLVM to fix this inaccuracy (and it really is just that, it's clearly contradictory with the explicit policy linked earlier)
  • we close this issue
  • we remove this warning
@rkruppe

This comment has been minimized.

Copy link
Member

commented Mar 31, 2018

@rkruppe

This comment has been minimized.

Copy link
Member

commented Apr 3, 2018

Commit https://reviews.llvm.org/rL329065 removed the quoted statement (and also the stuff about undefined rounding mode) and replaced it with

This instruction is assumed to execute in the default floating-point environment.

Since this was just a doc clarification and in reality LLVM always behaved as we wanted it to, there's no need to wait until an LLVM update to pick up any code changes related to this. So we can close this issue now.

@sfackler sfackler closed this Apr 5, 2018

nikic added a commit to nikic/rust that referenced this issue Apr 14, 2018

Remove warning about f64->f32 cast being potential UB
As discussed in rust-lang#15536, the LLVM documentation incorrect described
overflowing f64->f32 casts as being undefined behavior. LLVM never
treated them as such, and the documentation has been adjusted in
https://reviews.llvm.org/rL329065. As such, this warning can now
be removed.

Closes rust-lang#49622.

kennytm added a commit to kennytm/rust that referenced this issue Apr 16, 2018

Rollup merge of rust-lang#49965 - nikic:fix-49622, r=rkruppe
Remove warning about f64->f32 cast being potential UB

As discussed in rust-lang#15536, the LLVM documentation incorrect described overflowing f64->f32 casts as being undefined behavior. LLVM never treated them as such, and the documentation has been adjusted in https://reviews.llvm.org/rL329065. As such, this warning can now be removed.

Closes rust-lang#49622.

---

I could not find any existing test checking for this warning. Should I be adding a test for the absence of the warning instead?

kennytm added a commit to kennytm/rust that referenced this issue Apr 16, 2018

Rollup merge of rust-lang#49965 - nikic:fix-49622, r=rkruppe
Remove warning about f64->f32 cast being potential UB

As discussed in rust-lang#15536, the LLVM documentation incorrect described overflowing f64->f32 casts as being undefined behavior. LLVM never treated them as such, and the documentation has been adjusted in https://reviews.llvm.org/rL329065. As such, this warning can now be removed.

Closes rust-lang#49622.

---

I could not find any existing test checking for this warning. Should I be adding a test for the absence of the warning instead?

rkruppe added a commit to rkruppe/nomicon that referenced this issue May 1, 2018

Update description of float casts
rust-lang/rust#15536 has been closed, there's no UB there, so let's stop linking to it. And for the same reasons (LLVM's assumption of the default floating point environment), f64->f32 and int->float casts do actually have a defined rounding mode, ties-to-even.
@dhardy

This comment has been minimized.

Copy link
Contributor

commented May 11, 2019

Thanks @rkruppe for clearing this up, however the reference still says this is UB.

rkruppe added a commit to rkruppe/reference that referenced this issue May 11, 2019

Update description of float casts
rust-lang/rust#15536 has been fixed by LLVM changing its documentation, there's no UB there, so the bug has been fixed and we can stop talking about it.

For the same reasons, the rounding modes for int->float and f64->f32 are no longer unspecified in LLVM (instead, they round ties-to-even). I assume it is uncontroversial to guarantee this in Rust, as it's clearly the default choice and we probably just didn't say it because LLVM docs were disagreeing (this too has been fixed).

Closes rust-lang-nursery#570

(Note: the diff is identical to rust-lang-nursery/nomicon#65, I didn't realize the reference has the same text)
@rkruppe

This comment has been minimized.

Copy link
Member

commented May 11, 2019

@dhardy Thanks for pointing this out, filed a PR to update the reference.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.