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

Jump to bottom

test failures for alternative implementation of Float.round #7917

Closed
vicuna opened this issue Feb 11, 2019 · 4 comments
Closed

test failures for alternative implementation of Float.round #7917

vicuna opened this issue Feb 11, 2019 · 4 comments
Labels
configure-related stdlib

Comments

@vicuna
Copy link

vicuna commented Feb 11, 2019

Original bug ID: 7917
Reporter: aha
Status: new
Resolution: open
Priority: normal
Severity: minor
Category: standard library
Monitored by: @nojb

Bug description

First failing test is
https://github.com/ocaml/ocaml/blob/4.08.0%2Bbeta1/testsuite/tests/lib-float/test.ml#L27

Steps to reproduce

I can only reproduce it with gcc version 7.x (and newer) and 32-bit targets, e.g. mingw:

i686-w64-mingw32-gcc --version # i686-w64-mingw32-gcc (GCC) 7.4.0
./configure --build=x86_64-unknown-cygwin --host=i686-w64-mingw32
make world.opt
cd testsuite
make one DIR=tests/lib-float

(The Appveyor CI still uses gcc 6.4.0-1 ( https://ci.appveyor.com/project/avsm/ocaml/builds/22228711/job/v4rdbiu16yiwk2i6?fullLog=true#L26 ) instead of the current 7.4.0-1.)

It's not windows specific. The same happens under 32-bit linux, if the alternative implementation is chosen manually:

gcc --version # gcc (Ubuntu 7.3.0-27ubuntu1~18.04) 7.3.0
./configure
sed -i 's|#define HAS_C99_FLOAT_OPS 1||g' runtime/caml/s.h
make world.opt
cd testsuite
make one DIR=tests/lib-float
@vicuna vicuna added the stdlib label Mar 14, 2019
@xavierleroy
Copy link
Contributor

Sorry if this bug was ignored for so long. I was able to reproduce it with x86 in 32-bit mode under Linux, as described.

This is an "excess precision" issue with GCC, similar to the infamous GCC bug 323. Consider

ocaml/runtime/floats.c

Line 496 in 1d76e2c

return floor(f + pred_one_half.d);

Recent enough versions of GCC implement floor with nice inline x87 code. Hence, f + pred_one_half.d is computed on the x87 stack in extended precision, not rounded to double precision (that's the bug), then passed to floor. The lack of rounding the sum to double precision causes the wrong result for caml_round(0.5). The same lack of rounding is a GCC bug w.r.t. the ISO C standard, of the bug 323 kind.

Passing -fexcess-precision=standard to GCC makes the bug go away. Now I need to check with other developers if it's the best workaround and how to implement it cleanly.

@xavierleroy xavierleroy mentioned this issue Apr 5, 2020
Merged
xavierleroy added a commit to xavierleroy/ocaml that referenced this issue Apr 6, 2020
@xavierleroy
configure: add GCC option -fexcess-precision=standard
fa61b58 
This option forces GCC to follow the ISO C standards concerning
rounding of intermediate FP results.  It avoids some FP issues
with the x86 32 bits ports of OCaml, which can run into
excess precision problems due to the x87 FP unit.

Closes: ocaml#7917
xavierleroy added a commit to xavierleroy/ocaml that referenced this issue Apr 6, 2020
@xavierleroy
configure: add GCC option -fexcess-precision=standard
6a36c0b 
This option forces GCC to follow the ISO C standards concerning
rounding of intermediate FP results.  It avoids some FP issues
with the x86 32 bits ports of OCaml, which can run into
excess precision problems due to the x87 FP unit.

Closes: ocaml#7917
@xavierleroy xavierleroy mentioned this issue Apr 6, 2020
Merged
xavierleroy added a commit to xavierleroy/ocaml that referenced this issue Apr 8, 2020
@xavierleroy
configure: add GCC option -fexcess-precision=standard
11f16d2 
This option forces GCC to follow the ISO C standards concerning
rounding of intermediate FP results.  It avoids some FP issues
with the x86 32 bits ports of OCaml, which can run into
excess precision problems due to the x87 FP unit.

Closes: ocaml#7917
@rwmjones
Copy link
Contributor

rwmjones commented Apr 22, 2020 •
edited

This breaks compiling C++ files, eg if they use :standard flags in the dune file. The reason is that although GCC supports -fexcess-precision=standard for C files, it doesn't support it for C++ files.

See:
https://lists.fedoraproject.org/archives/list/devel@lists.fedoraproject.org/thread/M2VM5DNB7HSAJIFDGT4WAIZDA5JPE5KM/

@fweimer
Copy link

fweimer commented Apr 22, 2020

And the flag isn't need on modern distributions which build there x86 (32-bit) variants with -msse2 -mfpmath=sse (no excess precision there due to SSE2).

@xavierleroy xavierleroy mentioned this issue Apr 24, 2020
Merged
@xavierleroy
Copy link
Contributor

Thanks for the report. I didn't know that some people would compile C++ files with ocamlc -c or ocamlopt -c. See #9496 for a proposed fix.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
configure-related stdlib
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
4 participants
@rwmjones @fweimer @xavierleroy @vicuna