343998 - JS1.7 landing broke windows ce.

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Doug Turner (:dougt)]

js3250.lib(jsmath.obj) : error LNK2019: unresolved external symbol copysign referenced in function math_atan2

copysign isn't around.  using the same workaround for the bustage in VC6 and VC7 seams to work fine.

patch coming up.

Comment 1

[Doug Turner (:dougt)]

Attachment: Patch v.1

This patch uses the existing |js_copysign| function impl. in js/.  

Alternatively, I could implement copysign in the windows ce shunt layer.

We could simply drop off "&& !defined WINCE" from the #elif, but I think it is more clear being verbose.

Comment 2

[Brendan Eich [:brendan]]

Comment on attachment 228579 [details] [diff] [review]
Patch v.1

What defines js_copysign for WINCE?  Isn't there a libm or libc copysign or _copysign or whatever that we can use?

/be

Comment 3

[Doug Turner (:dougt)]

Attachment: patch v.2

on the devices i have tested against _copysign is available.  I verified that _copysign for WINCE doesn't have the same problems outlined in bug 329383.

Also, i have verified that with the latest sdk, it is perfectly fine to use the builtin tan and exp functions.

Comment 4

[Brendan Eich [:brendan]]

Comment on attachment 228700 [details] [diff] [review]
patch v.2

>Index: jslibmath.h
>===================================================================
>RCS file: /cvsroot/mozilla/js/src/jslibmath.h,v
>retrieving revision 3.17.6.1
>diff -u -1 -0 -r3.17.6.1 jslibmath.h
>--- jslibmath.h	7 Jul 2006 02:12:02 -0000	3.17.6.1
>+++ jslibmath.h	10 Jul 2006 18:52:01 -0000
>@@ -70,21 +70,23 @@
> 
> #define fd_acos acos
> #define fd_asin asin
> #define fd_atan atan
> #define fd_atan2 atan2
> #define fd_ceil ceil
> 
> /* The right copysign function is not always named the same thing. */
> #if __GNUC__ >= 4 || (__GNUC__ == 3 && __GNUC_MINOR__ >= 4)
> #define fd_copysign __builtin_copysign
>-#elif defined _WIN32 && !defined WINCE
>+#elif defined WINCE
>+#define fd_copysign _copysign
>+#elif defined _WIN32
> #if _MSC_VER < 1400
> /* Try to work around apparent _copysign bustage in VC6 and VC7. */
> #define fd_copysign js_copysign
> extern double js_copysign(double, double);
> #else
> #define fd_copysign _copysign
> #endif
> #else
> #define fd_copysign copysign
> #endif

This is all within #if !JS_USE_FDLIBM_MATH (just noting).

It looks ok, just wondering why you can't share the #define fd_copysign _copysign code now, by changing from

#elif defined _WIN32 && !defined WINCE

to just testing for "Windows (32 or CE)":

#elif defined _WIN32

IOW, fall into the common code, including the _MSC_VER < 1400 test.

(Is _WIN32 the right macro?)

>@@ -106,21 +108,21 @@
>  * Use math routines in fdlibm.
>  */
> 
> #undef __P
> #ifdef __STDC__
> #define __P(p)  p
> #else
> #define __P(p)  ()
> #endif
> 
>-#if (defined _WIN32 && !defined WINCE) || defined SUNOS4
>+#if defined _WIN32 || defined SUNOS4

This is in the big #else for that #if !JS_USE_FDLIBM_MATH -- the major comment introducing the guts of this #else says "Use math routines in fdlibm".  Did you test compiling with JS_USE_FDLIBM_MATH defined too?

> #define fd_acos acos
> #define fd_asin asin
> #define fd_atan atan
> #define fd_cos cos
> #define fd_sin sin
> #define fd_tan tan
> #define fd_exp exp
> #define fd_log log
> #define fd_sqrt sqrt

Reason I ask is because the next three significant lines are:

extern double fd_atan2 __P((double, double));
extern double fd_copysign __P((double, double));
extern double fd_pow __P((double, double));

which means JS_USE_FDLIBM_MATH defined as 1 implies most transcendentals are *not* from fdlibm, but atan2, pow, and the underlying copysign are.  That's not quite right if we can use _copysign from the OS/SDK.  The hard cases are atan2 and pow, where signed zeroes and other bugs exist.  It'd be good to test these too.

/be

Comment 5

[Doug Turner (:dougt)]

I did not want to depend on the _MSC_VER macro here.  For the MS compiler with the ppc/sp sdk, this isn't a problem at all (the macro is defined).  I am worried about using the Intel WINCE arm compiler (which probably does not use this macro).

To get the build off the floor (tbox has been burning for a few days), lets just ignore this change for now:

-#if (defined _WIN32 && !defined WINCE) || defined SUNOS4
+#if defined _WIN32 || defined SUNOS4

If in agreement, i will open up a new bug to test and follow up.

Comment 6

[Brendan Eich [:brendan]]

Yeah, that sounds good -- it means JS_USE_FDLIBM_MATH is defined for Windows builds, or at least for WINCE builds -- can you confirm?

My point was to test both forks of the #if !JS_USE_FDLIBM_MATH river, and to try to unify with existing WIN32 ifdefs.  If you can do that in this bug, I'm good with not filing a followup.

/be

Comment 7

[Doug Turner (:dougt)]

Attachment: fixes wince build bustage only.

this simply defines fd_copysign to the right thing on windows ce as discussed above.

Comment 8

[Doug Turner (:dougt)]

pretty sure that |JS_USE_FDLIBM_MATH| is never defined on windows (32 or ce)

Comment 9

[Brendan Eich [:brendan]]

Comment on attachment 228720 [details] [diff] [review]
fixes wince build bustage only.

r=me, thanks.

/be

Comment 10

[Mike Schroepfer]

Comment on attachment 228720 [details] [diff] [review]
fixes wince build bustage only.

a=schrep since b1 is out the door.  Trivial WinCE fix

Comment 11

[Doug Turner (:dougt)]

patch landed on 1.8 branch (not sure what keyword to add for something post ff2a1).

Checking in jslibmath.h;
/cvsroot/mozilla/js/src/jslibmath.h,v  <--  jslibmath.h
new revision: 3.17.6.2; previous revision: 3.17.6.1
done


This patch should also land on trunk.  brendan, I can't check this into mozilla/js.  Can you do the honor or point me to someone with the right stuff.

Comment 12

[:Gavin Sharp [email: [e-mail]]]

I checked this in for Doug:
mozilla/js/src/jslibmath.h 	3.22