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

Workaround for Windows denormal bug in float_to_str_bytes_common #14080

Closed
wants to merge 1 commit into from
Closed

Workaround for Windows denormal bug in float_to_str_bytes_common #14080

wants to merge 1 commit into from

Conversation

Rufflewind
Copy link
Contributor

On Windows (MinGW32), 2.0f64.powf(-1074.0) returns zero instead of a denormal number, which screws up everything later on in float_to_str_bytes_common. Changing it to use powi works around the problem but breaks the build on Linux (and presumably other systems).

Fixes #13439.

@alexcrichton
Copy link
Member

Could a test be added for this, or could a test be un-ignored? Also, if this fails on mingw32, does it pass on mingw-w64?

@Rufflewind
Copy link
Contributor Author

Sorry, took some time to get Mingw-w64 set up. Here's what I found.

Turns out if I run:

println!("{} {}", 2f64.powf(-1074.0) != 0.0, 2f64.powi(-1074) != 0.0);

I can get different results depending on the system:

  • On Linux, I get true false.
  • On vanilla Mingw, I get false true, which causes the exponential-notation test to fail.
  • On Mingw-w64, I get true true.

So the patch here should work for all the above cases. (Though, if Rust doesn't support vanilla Mingw then this pull request wouldn't make any difference.)

@adrientetar
Copy link
Contributor

Can you add a brief comment on the file explaining this?

On Windows (MinGW32), `2.0f64.powf(-1074.0)` returns zero instead of a
denormal number, which screws up everything later on in
`float_to_str_bytes_common`.  Changing it to use `powi` works around the
problem.
@Rufflewind
Copy link
Contributor Author

Done.

@adrientetar
Copy link
Contributor

Can you add a test as well?

@Rufflewind
Copy link
Contributor Author

Currently, the exponential-notation test fails because of this. Would you want another test that's more direct?

@adrientetar
Copy link
Contributor

Well if it tests for all cases above it should be fine. @alexcrichton: is this ready to land?

@klutzy
Copy link
Contributor

klutzy commented May 25, 2014

I'm curious why exponential-notation test fails on your machine. The test passed on buildbot even when we used old mingw, right?
I'm ok if this change doesn't break anything but fix some unknown bug, but in the long term we have to investigate what exactly causes problem.

@Rufflewind
Copy link
Contributor Author

I'm not really sure. What implementation of powf does Rust use? (LLVM?) I can try to dig further.

@klutzy
Copy link
Contributor

klutzy commented May 26, 2014

powf and powi are llvm intrinsics (@llvm.{pow, powi}.f64). On win32 they are converted to calll _pow (mingw function) and fldz (cpu instrunction).
I guess some mingw has some bugs on floating points like #8663. Could you check your mingw version? grep VERSION /mingw/include/_mingw.h may show that.

@Rufflewind
Copy link
Contributor Author

#define __MINGW_VERSION 4.0
#define __MINGW32_VERSION 3.20

Not sure which one is the "real" version. Also, it's gcc 4.8.1 if that helps.

@klutzy
Copy link
Contributor

klutzy commented May 26, 2014

Yeah, it's 4.0.x - mingw 4.0 added __MINGW_VERSION then deprecated __MINGW32_VERSION.

mingw 3.x uses pow of msvcrt (which returns != 0.0), but latest mingw 4.0.3 uses its own implementation which returns 0.0.
mingw-w64 also implements pow independently; it returns != 0.0.

Anyway, I'm not sure if we want mingw-specific workaround. We migrated to mingw-w64 partially because mingw has buggy floating point implementation. Should we still support for mingw? I don't have concrete answer now.

@Rufflewind
Copy link
Contributor Author

Ah I see. Well, dropping Mingw support would simplify things, and my experience has been that Mingw-compiled Rust seems quite a bit slower than Mingw-w64-compiled Rust.

@alexcrichton
Copy link
Member

The test case that this appears to be fixing seems to be running successfully on our windows bots, running mingw-w64.

If this is a bug in the mingw implementation of floating point, then it looks like the bug is with upstream mingw rather than with us, so I'd rather not deviate from the correct implementation (mingw-w64) in all cases where we're compiling on windows.

For that reason, I'm going to close this for now. If I misinterpreted what was said, though, feel free to reopen!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

make check failures on windows 7
4 participants