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
Special floating-point values aren't converted to strings correctly under Windows #4688
Comments
Comment author: @gasche I've just been hit by this issue. |
Comment author: @gasche There also is a problem with precision modifiers: it has been reported that |
Comment author: thelema Batteries has failed some tests under windows related to this, there's definitely some interest in getting this issue resolved. |
Comment author: @protz http://www.mingw.org/wiki/C99 contains some insights on the C99 issue for mingw-compiled programs under windows. |
Comment author: @protz After investigation, it turns out that this is the behavior of the underlying libc printf function that you're observing. It turns out that the MSVCRT DLL (Microsoft's implementation of the C library) prints floats that way. Both mingw32 and MSVC generate programs linke against MSVCRT, so fixing this would require writing a compatibility layer in the OCaml runtime. I don't think it's reasonable to expect that from OCaml. Thanks, jonathan |
Comment author: @dra27 With respect, it is entirely reasonable to expect this of OCaml - the limitations of an older-standard C runtime should not get in the way of OCaml's specified behaviour. More importantly, this problem also affects Pervasives.string_of_float as it's the caml_format_float primitive (byterun/floats.c) which is broken. float_of_string (string_of_float nan) should never raise an exception! This primitive is already a wrapper around printf so I fail to see the problem. I've just rapidly patched and tested byterun/floats.c in my 3.12.0 installation here as: CAMLprim value caml_format_float(value fmt, value arg) if (isfinite(d)) { It would obviously be quite reasonable to wrap the test in the relevant #ifdefs so that only the Windows ports use it (I expect #ifdef WIN32 would be enough). The above patch fixes both Pervasives.string_of_float and also the %f format specifier. Pervasives.float_of_string works anyway because C89 does specify "-inf", "inf" and "nan" as the string representations for strtod. |
Comment author: @protz You wrote:
While I can see why you may feel that way, the problem is manifold:
It is indeed desirable to have a way to manipulate floats correctly, and convert them back and forth "the right way", but again, I do not think this is the role of OCaml. Your modification is trivial to write in OCaml (I think), and by patching byterun.c, you're placing a maintenance burden on OCaml, which I don't think we want, sorry :). Cheers, jonathan |
Comment author: @dra27 Ultimately, it's your call! I build from sources anyway so can patch whatever I need to. I find the logic a little strange in the light of things like the thousand lines of C used to workaround the Win32 select function in the Unix module (although I appreciate the maintenance burden that's causing - see at least PR #5327-5239). Resolution would be clearer as "won't fix" rather than "not fixable", FWIW. |
Comment author: @alainfrisch Honestly, I think this should be fixed. The fix is much simpler than other workarounds implemented for Windows, it should should not be difficult to maintain, and several people have been bitten by the bug. |
Comment author: @protz Yes, I was thinking about it in the subway and while fixing Printf seems too big of a task, fixing string_of_float seems reasonable. |
Comment author: @alainfrisch I'm glad you reconsider (part) of the decision. Too big of a task? Doesn't the provided patch fix Printf? |
Comment author: @gasche When we found the bug in Batteries, I implemented a fix for our function using Printf internally (as string_of_float does) using classify_float: Just my two cents. The Printf patch looks rather reasonable and I wouldn't mind the issue being fixed once and for all, but you never know with C code and I definitely understand the cautiousness. |
Comment author: @dra27 Yes - the patch does fix both Printf.printf and Pervasives.string_of_float (technically I've only tested it in bytecode, but the file is shared between both). Printf.printf is only related to the C function by its design (it's definitely not a wrapper around C's one) - the actual function is largely implemented in OCaml, it uses that C stub to format float values hence the exposed C89/C99 issue. |
Comment author: @protz Yep, sorry I misread the code. As I said, after thinking about it, your patch is definitely short, so no objections for me :). |
Comment author: @xavierleroy This is one of these small issues where it is hard to tell what the right action is. C99 fully specifies the handling of infinities and NaNs in string->float and float->string conversions (strtod, scanf, printf, etc). C89 doesn't say anything about infinities and NaNs, both in the language and in the library functions. There is, however, a reasonable expectation that whatever printf generates, strtod can read back. From the PR, I understand that Microsoft's standard C library does not conform to C99 (12 years later...) and moreover that the "reasonable expectation" above doesn't hold. This is incredibly sloppy and low-quality. One more reason to use the Cygwin port of OCaml: at least it benefits from a quality, non-Microsoft C standard library. Yes, we've worked hard to emulate some useful Unix features under Win32, but that's normal given that Win32 is a different OS that isn't expected to conform to any Unix standard. In contrast, Microsoft's C implementation is expected to conform to the C standards, however, and it is not normal for us to work around Microsoft's failures in this department. Also, note that the proposed fix can also create new problems. Assume a pre-C99 C library that satisfies the "reasonable expectation" above. In the current situation, float_of_string can handle whatever is produced by string_of_float or by sprintf "%g". If we change the way sprintf prints infinities and NaNs, but leave float_of_string unchanged, we break this property. And, no, I'm not suggesting that float_of_string should also be changed to special-case infinities and NaNs, because I think this is going too far. So, I'm in favor of a "won't fix". One change that we should consider, though, is to ensure that the %F printf format prints infinities and NaNs in Caml syntax, like the toplevel does (file oprint.ml). |
Comment author: @protz (sorry, edited the wrong bug) |
Comment author: @dra27 Xavier - (I'm only commenting further because of the part on %F - if this is closed and done then a new PR may be needed for that instead) [There's an over-arching reason not to use the Cygwin port, sadly - namely its non-permissive licence! It would be nice to be able to GPL everything, but...] Fixing "%F" would involve coding this exact change for a specific branch of Printf but leaving the others broken which really does sound like cutting off your nose to spite your face. But: "%F" does not produce a valid caml lexeme on any platform for the special values (I can't tell from the docs whether that's by design) - oprint.ml's handling correctly produces nan, neg_infinity and infinity whereas printf.ml's "%F" on Linux produces nan, inf and -inf the latter two of which are syntax errors, not valid ocaml lexemes. Granted, though, if special handling for float special values is required for "%F" on all platforms then I just undermined my own argument against fixing "%F" only ;) The problem you propose with sane pre-C99 libraries is reasonbly solved by guarding the change with an appropriate #ifdef, which I had suggested (I don't know whether #ifdef WIN32 is too broad - it might cause Cygwin to include it). Indeed, in the perfect build-world, the configure script (which would handle the Windows ports too) would contain a test of the C compiler's C89/C99 handling of printf and strtod resulting in a flag in s.h being set and the appropriate code being brought into play... |
Comment author: @alainfrisch After some internal discussion, it seems we could reach a middle ground. Could you produce a patch with the proper #ifdefs so that it applies only to the MSVC and Mingw ports? If there is absolutely no chance to break the Cygwin version (and of course, the other Unix ports), I think we can get the patch through. |
Comment author: @dra27 Patch attached - it works by declaring a new variable in s.h for the native Windows ports. Cygwin generates s.h using configure (not s-nt.h) and I have tested that the alternate code is definitely not compiled when building for Cygwin. |
Comment author: @alainfrisch Did you try your patch with the MSVC port? I don't think isfinite and isnan exist for MSVC. One could use _finite and _isnan or call caml_classify_float (or duplicate its code). |
Comment author: @dra27 Whoops - see the attached patch appended "v2". I've made sure that the Cygwin port doesn't use the code (by building with syntax errors inserted inside the #ifdefs) and compiled and tested both the MinGW and MSVC ports with this new patch. |
Comment author: @alainfrisch David, thanks for your work! I've integrated your latest patch. |
Original bug ID: 4688
Reporter: @dra27
Assigned to: @alainfrisch
Status: closed (set by @xavierleroy on 2017-09-24T15:32:15Z)
Resolution: fixed
Priority: normal
Severity: minor
Version: 3.11.0+beta
Category: ~DO NOT USE (was: OCaml general)
Related to: #5443 #5739 #7069 #7186 #7218
Monitored by: @gasche @protz @hcarty
Bug description
Because of a lack of ANSI C99 (?) support in the MSVC runtime, using Printf.printf "%f" renders incorrect strings for the floating point special values neg_infinity, infinity and nan. Although this is technically speaking a bug in the MS libraries rather than OCaml, it would be helpful if the Windows Port of OCaml worked around this limitation.
The top level displays the values correctly because oprint.ml doesn't go via printf (possibly because this has been noticed before?).
Additional information
Both on Windows Vista:
a) MinGW Port
nan;;
Printf.printf "%f\n" nan;;
1.#QNAN0
b) MSVC64 Port
nan;;
Printf.printf "%f" nan;;
1.#SNAN0
File attachments
The text was updated successfully, but these errors were encountered: