74999 - JS engine uses XP_PC to mean XP_WIN or some _MSC_VER #if test

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Brendan Eich [:brendan]]

e.g., for working around NaN == 0 bugs in MSVC.  We need to make sure the right
macro is defined in standalone and Mozilla build cases.

This was pointed out in bug 53518 by [e-mail].

/be

Comment 1

[Brendan Eich [:brendan]]

beard, can you please reassign this?  Might be easy first blood for khanson?
I'll help in any way.

/be

Comment 2

[Patrick C. Beard]

Reassigning to khanson.

Comment 3

[hacker formerly known as [e-mail]]

Attachment: v1.0

Comment 4

[Brendan Eich [:brendan]]

Comment on attachment 119032 [details] [diff] [review]
v1.0

rs=[e-mail], and you don't need two levels of review for js/src
changes, but testing on both Windows and OS2 would be swell.

/be

Comment 5

[timeless]

Comment on attachment 119032 [details] [diff] [review]
v1.0

+++ js/src/jsdate.c	1 Apr 2003 04:26:02 -0000
@@ -148,7 +148,7 @@
-#ifdef XP_PC
+#if defined(XP_WIN) || defined(XP_OS2)
 /* Work around msvc double optimization bug by making these runtime values; if

this should probably be _MSC_VER

+++ js/src/jsdtoa.c	1 Apr 2003 04:26:02 -0000
-#ifdef XP_PC
+#if defined(XP_WIN) || defined(XP_OS2)
	     && !((word0(d) & Exp_mask) == Exp_mask && ((word0(d) & Frac_mask)
|| word1(d))) /* Visual C++ doesn't know how to compare against NaN */

unless visualage/metrowerks/borland have the same bug i'd argue this too is
_MSC_VER

+++ js/src/jsinterp.c	1 Apr 2003 04:26:03 -0000
these all appear to be msvc workarounds

+++ js/src/jsnum.c	1 Apr 2003 04:26:03 -0000
-#if defined XP_PC &&							      
\
+#if (defined XP_WIN || defined XP_OS2) &&				      
\
     !defined __MWERKS__ &&						      
\
     (defined _M_IX86 ||						      
\
     (defined __GNUC__ && !defined __MINGW32__ && !defined __EMX__))

this has irked me for a bit. i should probably find out why that last line
exists. it looks like it should be an _MSC_VER + visualage check...

+++ js/src/jsobj.c	1 Apr 2003 04:26:03 -0000
+#if defined(XP_WIN) && defined _MSC_VER && _MSC_VER <= 800
a bit of consistency would be nice, but isn't required, in some places the code
uses:
+#if defined XP_WIN && defined _MSC_VER && _MSC_VER <= 800
ok, there's no consistency in this stuff :)

+++ js/src/jsosdep.h	1 Apr 2003 04:26:03 -0000
how about this:

#if defined XP_OS2 || defined _WIN32
#define JS_HAVE_LONG_LONG
#elif defined XP_WIN
#undef JS_HAVE_LONG_LONG
#endif

+++ js/src/jsparse.c	1 Apr 2003 04:26:03 -0000
again, this looks like it should be _MSC_VER

Comment 6

[Mike Kaply [:mkaply]]

Javier, could you peruse these as well?

Comment 7

[Brendan Eich [:brendan]]

Thanks, timeless.

cls, can you attach a new patch that addresses all comments so far, and uses
-pu9 or better so more context is visible?  Thanks.

/be

Comment 8

[Mike Kaply [:mkaply]]

Do we need the XP_WIN check at all in the _MSC_VER cases?

Won't _MSC_VER only be set in the Windows build anyway?

Comment 9

[hacker formerly known as [e-mail]]

*sigh* I'm really not interested in tracking down whether a particular
workaround is meant to be MSVC only.  Right now, anything inside a XP_PC ifdef
is being used by OS/2 as well.  This patch does not change that behavior but
your proposed changes will cause us to use a new code path for OS/2.  Given the
general delicacy of the js engine and the general brokeness of the VA optimizer,
I'd rather not be responible for those changes.  After a brief scan of bug
53518, it's still not clear whether or not OS/2 suffers from the NaN lossage.

Comment 10

[jhp (no longer active)]

Created bug 200144 to handle the removal of OS/2 defines from the MSVC paths.

Comment 11

[hacker formerly known as [e-mail]]

Attachment: v1.1

Umm, I wasn't trying to hijack this bug.  I can easily move these changes back
into bug 56767.

Comment 12

[Mike Kaply [:mkaply]]

Comment on attachment 119127 [details] [diff] [review]
v1.1

I agree with cls.

Let's just get rid of the XP_PC first.

Comment 13

[Brendan Eich [:brendan]]

Comment on attachment 119127 [details] [diff] [review]
v1.1

Didn't I already + this? Timeless is right, but this patch doesn't regress
things.  Further cleanup can happen in the follow-on bug.

/be

Comment 14

[hacker formerly known as [e-mail]]

The patch hass been checked in.

Comment 15

[Phil Schwartau]

Reopening - maybe I'm doing something wrong, but I'm now getting
an error on WinNT4.0 when I try to build the JS shell: 

[//d/JS_TRUNK/mozilla/js/src] make -f Makefile.ref

        etc.  etc.

jstypes.h(238) : fatal error C1189: #error :
"Must define one of XP_BEOS, XP_MAC, XP_OS2, XP_WIN or XP_UNIX"
make[1]: *** [WINNT4.0_DBG.OBJ/jsdhash.obj] Error 2
make: *** [all] Error 2


I will attach the full build output below -

Comment 16

[Phil Schwartau]

Attachment: Complete build output showing error

Comment 17

[hacker formerly known as [e-mail]]

Attachment: Fix jsref

Comment 18

[hacker formerly known as [e-mail]]

The jsref changes have been checed in.

Comment 19

[Phil Schwartau]

Sorry - have to reopen again.

With the latest patch, I can successfully build the JS shell on WinNT.
However, there is now an inconsistency on certain numerical operations.
For example, here are two successive sessions in the JS shell. Notice the
different results for the same expression:


js> -0 / Number.NEGATIVE_INFINITY
NaN

js> quit();

js> -0 / Number.NEGATIVE_INFINITY
0


This is causing two test regressions when I run the JS testsuite on WinNT:

Testcase ecma/Expressions/11.5.2.js failed
Failure messages were:
   0 / Number.NEGATIVE_INFINITY = 0 FAILED! expected: NaN
   -0 / Number.NEGATIVE_INFINITY = NaN FAILED! expected: 0

Testcase ecma/Expressions/11.5.3.js failed
Failure messages were:
   0 % -1 = 0 FAILED! expected: NaN
   -0 % 1 = 0 FAILED! expected: NaN


Something really strange is going on. I checked these two testcases,
and they do NOT expect NaN here! They are hard-coded to expect 0 or -0
in these sections.

So whatever is happening, it is somehow corrupting the test harness' 
reporting of these hard-coded literals as well as the expressions -

Comment 20

[Phil Schwartau]

FWIW, I have run those two testcases in today's trunk build of
the browser, which contains all the checkins above.

The two tests do not fail in the browser on WinNT. Whether this means
the bug I'm seeing pertains to the JS shell only, or is simply sporadic
and difficult to reproduce, I do not know -

Comment 21

[hacker formerly known as [e-mail]]

Have you tried running the jsshell tests again?  I missed a couple of ifdefs in
either my patch or the original checkin.  I fixed those last night so everything
should be in now.

Comment 22

[Phil Schwartau]

Chris: that did it!

No test regressions are observed on WinNT, Linux, or Mac OSX.
Marking FIXED. Thanks for fixing this bug!

Comment 23

[Phil Schwartau]

Marking Verified -