On Wed, 11 Oct 2023 17:29:11 GMT, Andrew Haley <a...@openjdk.org> wrote:
>> A bug in GCC causes shared libraries linked with -ffast-math to disable >> denormal arithmetic. This breaks Java's floating-point semantics. >> >> The bug is https://gcc.gnu.org/bugzilla/show_bug.cgi?id=55522 >> >> One solution is to save and restore the floating-point control word around >> System.loadLibrary(). This isn't perfect, because some shared library might >> load another shared library at runtime, but it's a lot better than what we >> do now. >> >> However, this fix is not complete. `dlopen()` is called from many places in >> the JDK. I guess the best thing to do is find and wrap them all. I'd like to >> hear people's opinions. > > Andrew Haley has updated the pull request incrementally with one additional > commit since the last revision: > > Add TestDenormalDouble.java Overall, looks good. Primarily, I'm curious why did you decide to fix JNI on x86-64, but not on AArch64? FTR the fix covers shared library case on both architectures. > In addition, I dropped the proposed warning at safepoints if the handling of > denormal floating-point changes. It still makes sense to introduce an assert checking that FP operations behave according to the spec. The invariant should hold for any JavaThread on any thread state transition. But I'm fine with handling it separately. Some minor comments follow. src/hotspot/os/bsd/os_bsd.cpp line 977: > 975: > 976: void *os::Bsd::dlopen_helper(const char *filename, int mode) { > 977: #if defined(__GNUC__) What's the intention of limiting it to GCC on BSD? `os_linux.cpp` doesn't have it. Also, since 3rd party libraries are the root of the problem, does the information about toolchain JDK is built with help in any way? src/hotspot/os/bsd/os_bsd.cpp line 1001: > 999: static const volatile double thresh > 1000: = jdouble_cast(0x0000000000000003); // 0x0.0000000000003p-1022; > 1001: if (unity + thresh == unity || -unity - thresh == -unity) { The check is duplicated in 4 places (twice in os_bsd.cpp and twice in os_linux.cpp). Maybe extract it into some central shared location? Also, unity and thresh constants are duplicated in `stubGenerator_x86_64.cpp`. Why don't you place everything on `StubRoutines` instead? test/hotspot/jtreg/compiler/floatingpoint/libfast-math.c line 26: > 24: #include <assert.h> > 25: #include <fenv.h> > 26: #include "jni.h" Redundant includes? test/hotspot/jtreg/compiler/floatingpoint/libfast-math.c line 39: > 37: static void __attribute__((constructor)) set_flush_to_zero(void) { > 38: > 39: #if defined(__x86_64__) && defined(SSE) x86-64 implies SSE2, doesn't it? test/hotspot/jtreg/compiler/floatingpoint/libfast-math.c line 51: > 49: { __asm__ __volatile__ ("msr fpcr, %0" : : "r" (fpcr)); } > 50: /* Flush to zero, round to nearest, IEEE exceptions disabled. */ > 51: _FPU_SETCW (_FPU_FPCR_FZ); Macros make it a bit harder to decipher what happens there. At least, I'd suggest to change formatting around `_FPU_SETCW`. At first glance, it looks like it is part of the macro definition. ------------- PR Review: https://git.openjdk.org/jdk/pull/10661#pullrequestreview-1672027282 PR Review Comment: https://git.openjdk.org/jdk/pull/10661#discussion_r1355492626 PR Review Comment: https://git.openjdk.org/jdk/pull/10661#discussion_r1355478109 PR Review Comment: https://git.openjdk.org/jdk/pull/10661#discussion_r1355523173 PR Review Comment: https://git.openjdk.org/jdk/pull/10661#discussion_r1355438801 PR Review Comment: https://git.openjdk.org/jdk/pull/10661#discussion_r1355681073
