1303085 - Include and build nearbyint in fdlibm

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: fdlibm.patch

(motivation: nearbyint doesn't behave consistently across linux/windows, see also https://twitter.com/bnjbvr/status/776449182840164352, causing a test failure in bug 1248555)

Here's a patch that adds (and makes use of) fdlibm's implementation of nearbyint, and it doesn't somehow build for me on my machine:

Executing: /usr/bin/g++ -std=gnu++11 -Wall -Wc++11-compat -Wempty-body -Wignored-qualifiers -Woverloaded-virtual -Wpointer-arith -Wsign-compare -Wtype-limits -Wunreachable-code -Wwrite-strings -Wno-invalid-offsetof -Wno-error=maybe-uninitialized -Wno-error=deprecated-declarations -Wno-error=array-bounds -fno-rtti -fno-exceptions -fno-math-errno -pthread -pipe -g -fno-omit-frame-pointer -fPIC -shared -Wl,-z,defs -Wl,-h,libmozjs-51a1.so -o libmozjs-51a1.so /home/ben/code/moz/builds/d64/js/src/tmpQP2gFc.list -lpthread -Wl,-z,noexecstack -Wl,-z,text -Wl,--build-id -B ../../build/unix/gold -Wl,-version-script,symverscript -Wl,-rpath-link,/home/ben/code/moz/builds/d64/dist/bin -Wl,-rpath-link,/usr/local/lib -lm -ldl -lm -lz -ldl
/home/ben/code/moz/builds/d64/js/src/tmpQP2gFc.list:
    INPUT("RegExp.o")
    INPUT("Parser.o")
    INPUT("StoreBuffer.o")
    INPUT("ExecutableAllocatorPosix.o")
    INPUT("Disassembler-x86-shared.o")
    INPUT("jsarray.o")
    INPUT("jsatom.o")
    INPUT("jsdtoa.o")
    INPUT("jsmath.o")
    INPUT("jsutil.o")
    INPUT("pm_linux.o")
    INPUT("ConditionVariable.o")
    INPUT("Mutex.o")
    INPUT("Thread.o")
    INPUT("Initialization.o")
    INPUT("TraceLogging.o")
    INPUT("TraceLoggingGraph.o")
    INPUT("TraceLoggingTypes.o")
    INPUT("Unified_cpp_js_src0.o")
    INPUT("Unified_cpp_js_src1.o")
    INPUT("Unified_cpp_js_src10.o")
    INPUT("Unified_cpp_js_src11.o")
    INPUT("Unified_cpp_js_src12.o")
    INPUT("Unified_cpp_js_src13.o")
    INPUT("Unified_cpp_js_src14.o")
    INPUT("Unified_cpp_js_src15.o")
    INPUT("Unified_cpp_js_src16.o")
    INPUT("Unified_cpp_js_src17.o")
    INPUT("Unified_cpp_js_src18.o")
    INPUT("Unified_cpp_js_src19.o")
    INPUT("Unified_cpp_js_src2.o")
    INPUT("Unified_cpp_js_src20.o")
    INPUT("Unified_cpp_js_src21.o")
    INPUT("Unified_cpp_js_src22.o")
    INPUT("Unified_cpp_js_src23.o")
    INPUT("Unified_cpp_js_src24.o")
    INPUT("Unified_cpp_js_src25.o")
    INPUT("Unified_cpp_js_src26.o")
    INPUT("Unified_cpp_js_src27.o")
    INPUT("Unified_cpp_js_src28.o")
    INPUT("Unified_cpp_js_src29.o")
    INPUT("Unified_cpp_js_src3.o")
    INPUT("Unified_cpp_js_src30.o")
    INPUT("Unified_cpp_js_src31.o")
    INPUT("Unified_cpp_js_src32.o")
    INPUT("Unified_cpp_js_src33.o")
    INPUT("Unified_cpp_js_src34.o")
    INPUT("Unified_cpp_js_src35.o")
    INPUT("Unified_cpp_js_src36.o")
    INPUT("Unified_cpp_js_src37.o")
    INPUT("Unified_cpp_js_src38.o")
    INPUT("Unified_cpp_js_src39.o")
    INPUT("Unified_cpp_js_src4.o")
    INPUT("Unified_cpp_js_src5.o")
    INPUT("Unified_cpp_js_src6.o")
    INPUT("Unified_cpp_js_src7.o")
    INPUT("Unified_cpp_js_src8.o")
    INPUT("Unified_cpp_js_src9.o")
    INPUT("../../modules/fdlibm/src/e_acos.o")
    INPUT("../../modules/fdlibm/src/e_acosh.o")
    INPUT("../../modules/fdlibm/src/e_asin.o")
    INPUT("../../modules/fdlibm/src/e_atan2.o")
    INPUT("../../modules/fdlibm/src/e_atanh.o")
    INPUT("../../modules/fdlibm/src/e_cosh.o")
    INPUT("../../modules/fdlibm/src/e_exp.o")
    INPUT("../../modules/fdlibm/src/e_hypot.o")
    INPUT("../../modules/fdlibm/src/e_log.o")
    INPUT("../../modules/fdlibm/src/e_log10.o")
    INPUT("../../modules/fdlibm/src/e_log2.o")
    INPUT("../../modules/fdlibm/src/e_pow.o")
    INPUT("../../modules/fdlibm/src/e_sinh.o")
    INPUT("../../modules/fdlibm/src/e_sqrt.o")
    INPUT("../../modules/fdlibm/src/k_exp.o")
    INPUT("../../modules/fdlibm/src/s_asinh.o")
    INPUT("../../modules/fdlibm/src/s_atan.o")
    INPUT("../../modules/fdlibm/src/s_cbrt.o")
    INPUT("../../modules/fdlibm/src/s_ceil.o")
    INPUT("../../modules/fdlibm/src/s_ceilf.o")
    INPUT("../../modules/fdlibm/src/s_copysign.o")
    INPUT("../../modules/fdlibm/src/s_expm1.o")
    INPUT("../../modules/fdlibm/src/s_fabs.o")
    INPUT("../../modules/fdlibm/src/s_floor.o")
    INPUT("../../modules/fdlibm/src/s_floorf.o")
    INPUT("../../modules/fdlibm/src/s_log1p.o")
    INPUT("../../modules/fdlibm/src/s_nearbyint.o")
    INPUT("../../modules/fdlibm/src/s_rint.o")
    INPUT("../../modules/fdlibm/src/s_rintf.o")
    INPUT("../../modules/fdlibm/src/s_scalbn.o")
    INPUT("../../modules/fdlibm/src/s_tanh.o")
    INPUT("../../modules/fdlibm/src/s_trunc.o")
    INPUT("../../modules/fdlibm/src/s_truncf.o")

../../build/unix/gold/ld: error: /home/ben/code/moz/builds/d64/js/src/../../modules/fdlibm/src/s_nearbyint.o: requires dynamic R_X86_64_PC32 reloc against 'fegetenv' which may overflow at runtime; recompile with -fPIC
../../build/unix/gold/ld: error: read-only segment has dynamic relocations
../../build/unix/gold/ld: error: hidden symbol 'fegetenv' is not defined locally
../../build/unix/gold/ld: error: hidden symbol 'fesetenv' is not defined locally
../../build/unix/gold/ld: error: hidden symbol 'fegetenv' is not defined locally
../../build/unix/gold/ld: error: hidden symbol 'fesetenv' is not defined locally



However, a toy C program shows that fegetenv/fesetenv are resolved when linking with -lm. I am struggling to understand what's going on: there seems that a list of symbol files generated for fdlibm are stored in a temporary descriptor fdlibm.a.desc, and this is used later for linking; although linking with -lm doesn't work.

Can a build peer help me, please? The patch by applied on top of m-i 13e09cb5ffe9 and should at least build under linux (it's just a WIP for linux; guards might be added later if needed).

Comment 1

[Nathan Froyd [:froydnj]]

You need to add fenv.h to config/system-headers so that it's declared with the appropriate visibility.

(Longer explanation: We included <fenv.h> with hidden visibility, so all functions declared therein ought to be resolved within the final linked object.  But fegetenv doesn't exist in the shell; it exists in libm.  So we have a symbol that now requires a different sort of call than what the assembly said--a call to some shared library, rather than a local, direct call--and the linker complains.)

Comment 2

[Benjamin Bouvier [:bbouvier] (inactive)]

(In reply to Nathan Froyd [:froydnj] from comment #1)
> You need to add fenv.h to config/system-headers so that it's declared with
> the appropriate visibility.
> 
> (Longer explanation: We included <fenv.h> with hidden visibility, so all
> functions declared therein ought to be resolved within the final linked
> object.  But fegetenv doesn't exist in the shell; it exists in libm.  So we
> have a symbol that now requires a different sort of call than what the
> assembly said--a call to some shared library, rather than a local, direct
> call--and the linker complains.)

Thanks, you're my hero!

Comment 3

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: 1.add-fenv.h.patch

Mike, I've chosen you as a frequent reviewer for this file, but feel free to bounce to somebody else if needed.

Comment 4

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: 2.add-nearbyint.patch

Arai, chosing you because you already reviewed functions I've added to this file in the past.

Comment 5

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: 3.meta.patch

And the traditional meta-patches, which are an horror to maintain but so great to have the next time you update fdlibm or add a new function.

Comment 6

[Tooru Fujisawa [:arai]]

Comment on attachment 8791729 [details] [diff] [review]
2.add-nearbyint.patch

Review of attachment 8791729 [details] [diff] [review]:
-----------------------------------------------------------------

nice!
r+, with the comment addressed :)

::: modules/fdlibm/src/s_nearbyint.cpp
@@ +27,5 @@
> +//#include <sys/cdefs.h>
> +//__FBSDID("$FreeBSD$");
> +
> +#include <fenv.h>
> +#include <math_private.h>

this should be

#include "math_private.h"

Comment 7

[Tooru Fujisawa [:arai]]

Comment on attachment 8791732 [details] [diff] [review]
3.meta.patch

Review of attachment 8791732 [details] [diff] [review]:
-----------------------------------------------------------------

the same thing as part 2, in 2 patches.

::: modules/fdlibm/patches/04_include_fdlibm_h_from_math_private_h.patch
@@ +578,5 @@
> + __FBSDID("$FreeBSD$");
> + 
> + #include <fenv.h>
> +-#include <math.h>
> ++#include <math_private.h>

this should be

+#include "math_private.h"

::: modules/fdlibm/patches/09_comment_out_rcsid_variable.patch
@@ +687,5 @@
> ++//#include <sys/cdefs.h>
> ++//__FBSDID("$FreeBSD$");
> + 
> + #include <fenv.h>
> + #include <math_private.h>

this should be

 #include "math_private.h"

(from previous change)

Comment 8

[Benjamin Bouvier [:bbouvier] (inactive)]

Thanks for the reviews!

Now the whac-a-mole game continues, and it's win7 / winxp which won't pass the tests... Investigating.

Comment 9

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: 4. miscompilation.patch

So I've spent the day investigating, and it's actually:

- test failing with --enable-optimize
- not failing with --disable-optimize --enable-debug

So I blame the compiler (posting to try to double check this).

Comment 10

[Tooru Fujisawa [:arai]]

I'll try some experiment this weekend

Comment 11

[Tooru Fujisawa [:arai]]

Attachment: test code and result

surely it's not working.

Comment 12

[Tooru Fujisawa [:arai]]

looks like STRICT_ASSIGN isn't working as expected.

VisualStudio is using this one:

> #define STRICT_ASSIGN(type, lval, rval) ((lval) = (rval))

but if I changed it to use this one, it works:

> #define STRICT_ASSIGN(type, lval, rval) do {    \
>         volatile type __lval;                   \
>                                                 \
>         if (sizeof(type) >= sizeof(long double))        \
>                 (lval) = (rval);                \
>         else {                                  \
>                 __lval = (rval);                \
>                 (lval) = __lval;                \
>         }                                       \
> } while (0)

also, if I add `global = w;` after `STRICT_ASSIGN(float,w,TWO23[sx]+x);`, it also works.
maybe we're hitting undefined behavior?

Comment 13

[Tooru Fujisawa [:arai]]

anyway, it would be better changing VisualStudio to use safer STRICT_ASSIGN.

Comment 14

[Tooru Fujisawa [:arai]]

Comment on attachment 8792036 [details] [diff] [review]
4. miscompilation.patch

Review of attachment 8792036 [details] [diff] [review]:
-----------------------------------------------------------------

clearing r?, as this should be fixed in STRICT_ASSIGN macro definition.

Comment 15

[Tooru Fujisawa [:arai]]

Here's try run with this bug's patches and bug 1303463 patch.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=2950d3c9fdb6

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9104d282eb56
Add fenv.h to the list of system-headers for rint/rintf; r=glandium
https://hg.mozilla.org/integration/mozilla-inbound/rev/420aea33cee6
Add nearbyint/nearbyintf to fdlibm; r=arai
https://hg.mozilla.org/integration/mozilla-inbound/rev/922941564e6a
updates meta-patches after adding rint/rintf/nearbyint/nearbyintf; r=arai

Comment 17

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/9104d282eb56
https://hg.mozilla.org/mozilla-central/rev/420aea33cee6
https://hg.mozilla.org/mozilla-central/rev/922941564e6a