59855 - OS/2 - Random Javascript code doesn't work.

Status: VERIFIED FIXED

(SeaMonkey :: General, defect, P3)

Description

[[e-mail]]

The page: http://www.modenos.com has some javascript
code to display random images.

It was working with M16, but now it doesn't work
with the latest M18 nightly build.

Comment 1

[Phil Schwartau]

Browser, not engine --> Browser-General. 

[e-mail], could you be more specific about where the random 
images are supposed to be? I'm not sure I see any bug (on WinNT, anyway). 
Do they appear on the very first page you see at http://www.modenos.com, 
or do you have to click on the "Enter" text to go to the next page? 

Please give the exact steps to reproduce the bug - thanks.

Comment 2

[[e-mail]]

Sorry, i'll try to be more specific.

On this page: http://www.modenos.com/index.html
(which is the first one.) you'll see three images,
the top one (www.modenos.com), the middle one
(a page, but it actually changes randomly and it
could be any of TWO faces) and the bottom one
(Enter).

On my Mozilla the middle image is replaced with
a text string "undefined".

I've put up a captured image here:
http://www.michelinakis.gr/Dimitris/images/mozilla.png
(~23kb)

Comment 3

[Asa Dotzler [:asa]]

adding mkaply for OS/2 help (please).  

Comment 4

[Mike Kaply [:mkaply]]

Attachment: We don't have copysign on VACPP - make it do something more reasonable

Comment 5

[slaquinta]

SCREENED.  VERIFIED AS VALID.

Comment 6

[Mike McCabe]

I think best would be to make os/2 use the software-implemented copysign from
js/src/fdlibm.  I'm loath to make the copysign call a no-op as it's been there
for eons.  Using fd_copysign from fdlibm is safer.

Attachment to follow; Michael, what do you think?

Cleanest for the future would be to give os2/vacpp get its own section as with
the other OSes, but maybe on another round.

Comment 7

[Mike McCabe]

Attachment: Make os2 use fd_copysign from fdlibm.

Comment 8

[Mike Kaply [:mkaply]]

Is that code platform independent?

It seems to assume the location of the sign.

Can you be sure of the length of a double?

Comment 9

[Mike McCabe]

I believe it is xp; bit operations are parameterized with __HI or __LO to get
the appropriate part of the word/dword.  Best of course would be to try it out.

Comment 10

[Mike Kaply [:mkaply]]

Attachment: That extern statement didn't work for me - here is a new one.

Comment 11

[Mike Kaply [:mkaply]]

Attachment: We needed a makefile change to pull in fdlibm

Comment 12

[cls]

r=cls on attach 19190

Comment 13

[Mike McCabe]

Oops, I see what was wrong with my extern statement.  I copied the pattern from
the other OS sections, but the XP_OS2_VACPP code isn't with the other OS
sections, and precedes the __P define.

r=mccabe on your revised patch (it does the trick) but if you want to run with
it, I'll attach a patch that gives OS2 a proper fdlibm conditional linking
section.  It needs a better  #if defined() test against os2.  If you're
concerned about standards conformance, it might be worthwhile to run some of the
javascript testsuite to see if fdlibm could help with any of the other math
functions.  The testsuite uncovers problems with the math libraries of most
OSes, mostly for corner cases such as +- 0, NaN, infinity.  The behavior of
these functions is defined in the ECMA-262 spec., after IEEE.

Comment 14

[Mike McCabe]

Attachment: jslibmath.h with section for os2

Comment 15

[Mike Kaply [:mkaply]]

Attachment: OK, I'm going to try the Makefile.in again. I made may unnecessary changes.

Comment 16

[cls]

Not sure if I understand the $(FDLIBM_LIBRARY) rule changes.  I believe they
should be more like the ones in config/rules.mk in LOOP_OVER_DIRS.  There's no
reason to have a CONTINUE_ON_ERROR.

All platforms should probably have FDLIBM_LIBRARY set in SHARED_LIBRARY_LIBS but
I have a feeling that this may break the standalone JS build.  Brendan, any
thoughts?

Comment 17

[Mike Kaply [:mkaply]]

The change to the rules is because OS/2 doesn't handle the semicolon as a 
separator for commands. same as the REGCHROME problem.

Comment 18

[Mike Kaply [:mkaply]]

Mike mccabe:

Looks like fd_copysign is failing in some cases for us. It's copying extra stuff 
besides the sign.

I'm looking.

Comment 19

[Mike Kaply [:mkaply]]

Looks like LITTLE_ENDIAN wasn't defined for OS/2.

Attaching another diff.

Comment 20

[Mike Kaply [:mkaply]]

Attachment: Change fdlibm header to define LITTLE_ENDIAN for OS/2

Comment 21

[timeless]

mkaply: w/o looking at that file itself, your patch would indicate we're using 
really crummy logic for the #if/#ifdefs to #define __LITTLE_ENDIAN

Shouldn't we have:
#ifdef __i386__
#define __LITTLE_ENDIAN
#endif
? and maybe others.

Comment 22

[Mike Kaply [:mkaply]]

mccabe:

everything works for me, so I guess I r= your changes to jslibmath.h.

Do you think we should fix the LITTLE_ENDIAN stuff in fdlibm.h?

I'd really like to get these changes in.

Thanks

Comment 23

[Brendan Eich [:brendan]]

timeless makes a good point.  mccabe, can you unify #defines?

Setting js1.5 and mozilla0.9 milestone keywords.

/be

Comment 24

[Mike Kaply [:mkaply]]

I'd really like these changes to go in.

What can I do to help speed this along?

Thanks

Comment 25

[Mike Kaply [:mkaply]]

OK, fdlibm.h shouldn't have its own check for endianness - NSPR and JS already 
define IS_LITTLE_ENDIAN if necessary.

I'm attaching a diff that cleans up fdlibm.h.

Comment 26

[Mike Kaply [:mkaply]]

Attachment: Use system level define IS_LITTLE_ENDIAN for checking endianness

Comment 27

[Brendan Eich [:brendan]]

IS_LITTLE_ENDIAN, if defined rather than IS_BIG_ENDIAN, comes from jscpucfg.h,
which is included by jstypes.h (which a lot of files include -- it's akin to
prtypes.h).  How does fdlibm.h include or otherwise acquire jscpucfg.h's
definition of IS_LITTLE_ENDIAN?

/be

Comment 28

[Mike Kaply [:mkaply]]

A very good point :)

I'm looking at the best way to include a header that defines IS_LITTLE_ENDIAN.

I'll get this if it kills me.

Comment 29

[Mike Kaply [:mkaply]]

Attachment: include jstypes.h in fdlibm - can't use jscpucfg because jsautocfg is used for UNIX

Comment 30

[Brendan Eich [:brendan]]

Looks good to me, daring sr=[e-mail] before any r=, cc'ing mang, who
helped integrate fdlibm.

/be

Comment 31

[Mike McCabe]

So what's the state of things?  Does the last patch (using jstypes.h
IS_LITTLE_ENDIAN) fix the whole suite of problems?  Or do we still need an added
OS/2 twiddle section in jslibmath.h?  I'm looking to r=, but I'm confused :)

BTW, mac-savvy heads assure me that using <> for non-system header files will
bust on the mac, so we probably want "".

Comment 32

[Mike Kaply [:mkaply]]

We definitely need the OS/2 stuff in jslibmath.h.

Once I made those changes, though, it still wasn't working correctly and I 
discovered that fdlibm.h did only defined LITTLE_ENDIAN for Windows.

So, I thought the best course of action was to include jstypes.h which already 
defined LITTLE_ENDIAN properly, rather than having another #define in in fdlibm.

So, I need r= on fdlibm.h which brendan already sr=, and then I'll check the 
whole bunch in based on the previous r= on the changes to jslibmath.h and the 
Makefile

Comment 33

[Michael Ang]

Yeah, we need the Makefile.in patch to build fdlibm, jslibmath.h patch so OS/2
uses fdlibm, and the IS_LITTLE_ENDIAN patch against fdlibm.h to fix fdlibm.

However, the fdlibm.h patch currently breaks fdlibm/Makefile.ref since the
include paths in the makefile need to be changed.  I'll have a look at this and
submit a patch.

r=mang on jslibmath.h (19212)
r=mang on Makefile.in (19190)

I'd like to hold off on 20342 until I have a corresponding patch to
fdlibm/Makefile.ref.  Also note mccabe's comment about using <>.  Someone should
also check if the Windows standalone makefile (js.mak) and Mac project (js.mdp)
still work with the patched fdlibm.h.  fdlibm currently has no external
dependencies at all, but adding a dependency on jstypes.h seems worthwhile
instead of duplicating the endianness logic.

(Aside: I had to munge the end of line sequences to get mkaply's patches to
apply.)

Comment 34

[Michael Ang]

Since jstypes.h depends on a generated header (jsautocfg.h) the problem for the
makefile is to build that header before descending into the fdlibm directory. 
This is only slightly ugly on Unix, but those makefiles are plenty ugly
already.  Doing a similar trick on Windows and Mac will probably be more
difficult.

At this point I'm tempted to say "worse is better" and that we should keep
fdlibm a standalone library without dependencies.  So I propose we add a define
for OS2 to fdlibm.h instead of including jstypes.h.

Comment 35

[Michael Ang]

Actually, it's not that bad, since Mac and Windows use a static header, not the
generated one.  So the makefile/project for each platform only needs to add the
relevant dirs to the include path.

Comment 36

[Michael Ang]

Attachment: Makefile.ref - descend into fdlibm after generating headers

Comment 37

[Michael Ang]

Attachment: fdlibm/Makefile.ref - include JS headers

Comment 38

[Michael Ang]

The two patches I just attached make it possible to build JSRef on Unix with the
right set of the other patches.  The standalone build files for Windows and Mac
will have to be updated by someone with access to those machines.  (Apparently
camelot, the old web-accessible Mac project file editor, is off-line.)

Comment 39

[Mike McCabe]

Neat!  fdlibm is a real dependency instead of a PREDIR.  I wonder if the same
thing would work with editline...

I think you're OK with the windows standalone build, as it's done with
Makefile.ref (and gnu tools).  Brendan, is this true?  Is js.mak being used?  

I'm attaching a combined diff; this piecemeal stuff is confusing!  It includes
attachments 20661, 20342, 20310, 19212, 20662... I wasn't quite sure which
change was appropriate for js/src/Makefile.in.  Changed <> to "".  I think
exports to dist/include mean that we're ok for fdlibm/Makefile.in; I'm not sure
if this applies for fdlibm/makefile.win or the mac build.

Comment 40

[Mike McCabe]

Attachment: Rolled up version of previous diffs

Comment 41

[Brendan Eich [:brendan]]

mccabe: Windoze standalone still uses js.mak -- we need to test that out with
this patch.  My Win98 machine is monitor-less right now, who can help?

/be

Comment 42

[Mike Kaply [:mkaply]]

Attachment: More rollup - makefile cleanup on OS/2

Comment 43

[Mike Kaply [:mkaply]]

OK, I built this standalone on Windows (js.mak) and it builds OK.

I had to update the Makefile a little to get it building right on OS/2 (I 
actually removed some stuff)

I think this is good to go.

Comment 44

[Mike Kaply [:mkaply]]

Attachment: Last diff was missing jstypes include

Comment 45

[Brendan Eich [:brendan]]

One thing, maybe my confusion: why do you need this ifdef?  The Makefile.ref
does not have it around its FDLIBM rule.

+ifeq ($(OS_ARCH),OS2)
$(FDLIBM_LIBRARY):
+
$(MAKE) -C $(@D) $(@F)
+else
+$(FDLIBM_LIBRARY):
@$(CONTINUE_ON_ERROR) \
$(MAKE) -C $(@D) $(@F); \
$(EXIT_ON_ERROR)
+endif

/be

Comment 46

[Mike Kaply [:mkaply]]

The way those lines were written is totally for a unix shell.

But, I just talked to Chris Seawood, and he indicated that the two *_ON_ERROR 
lines should probably just be removed, then we wouldn't have to have a special 
OS/2 path. Essentially, errors building fdlibm are being ignored.

This file is one of the older makefiles that needs to be cleaned up at some 
point.

Comment 47

[Brendan Eich [:brendan]]

Can you remove the CONTINUE_ON_ERROR jazz from Makefile.ref, too -- or doesn't
OS2 use that makefile for standalone JS?

sr=[e-mail] with the CONTINUE_ON_ERROR junk nuked.

/be

Comment 48

[Mike Kaply [:mkaply]]

Finally checked in.

Comment 49

[Phil Schwartau]

Reopening bug - on WinNT, I can no longer build the standalone JS shell 
from scratch via:

            
                      make -f Makefile.ref
 

"From scratch" means: make a NEW root directory, pull mozilla/js/src
into this brand-new directory, and try to build.


Reason: Makefile.ref no longer builds js/src/fdlibm/(OBJ_DIR)/fdlibm.lib
So the build comes to a halt at a line where this is needed.


You don't encounter this problem in an EXISTING js/src directory, 
because js/src/fdlibm/(OBJ_DIR)/fdlibm.lib was built there sometime
in the past, and does not get deleted by: 


                      make -f Makefile.ref clean


Therefore you don't run into the error when you re-build...

Comment 50

[Mike Kaply [:mkaply]]

If I pull just mozilla/js/src, then go back to 3.21 of Makefile.ref, I get an 
error in config.mk in js/src when running

nmake -f Makefile.ref

Is it supposed to be gmake?

Comment 51

[Phil Schwartau]

That's right. On WinNT, I am also not able to use nmake; I have to use GNU make. 


[/mozilla/js/src] gmake --version
GNU Make version 3.74, by Richard Stallman and Roland McGrath.
Copyright (C) 1988, 89, 90, 91, 92, 93, 94, 95 Free Software Foundation,Inc.
This is free software; see the source for copying conditions.
There is NO warranty; not even for MERCHANTABILITY or FITNESS FOR A
PARTICULAR PURPOSE.

This version was ported to Win32 by Netscape Communications Corp.
Default shell: shmsdos.  Build number: 19980324.


[/mozilla/js/src] make --version
GNU Make version 3.75, by Richard Stallman and Roland McGrath.
Copyright (C) 1988, 89, 90, 91, 92, 93, 94, 95, 96
        Free Software Foundation, Inc.
This is free software; see the source for copying conditions.
There is NO warranty; not even for MERCHANTABILITY or FITNESS FOR A
PARTICULAR PURPOSE.

Report bugs to <[e-mail]>.

Comment 52

[Mike Kaply [:mkaply]]

Attachment: Hopefully last diff - fix Makefile.ref, fix makefile.win so clobber build can handle jstypes.h change in fdlibm.h

Comment 53

[Daniel (Leaf) Nunes]

and this last change builds with nmake?

Comment 54

[Mike Kaply [:mkaply]]

Attachment: If it's not one thing, it's another - js.mak so jstypes.h can be included - I think this might be it.

Comment 55

[Mike Kaply [:mkaply]]

Attachment: Current diff for js makefiles

Comment 56

[Phil Schwartau]

Mike has sent me a zip file of the affected files: 

                    js/src/Makefile.ref
                    js/src/js.mak
                    js/src/makefile.win
                    js/src/fdlibm/fdlibm.h


By using these, I was able to successfully build the debug and 
optimized JS shells on WinNT. 

Comment 57

[Mike Kaply [:mkaply]]

OK, I'M GIVING UP.

I don't think you can ever include jstypes.h in fdlibm.h There are too many 
build dependencies between things.

All I was trying to do was not have a second ENDIAN check in fdlibm.h, but it is 
simply not worth the effort.

The current build changes should/could go in - they make things a little 
cleaner, but the fdlibm.h include should not.

I want to add:

#ifdef XP_OS2
#define _LITTLE_ENDIAN
#endif

to fdlibm.h and then this stupid bug will finally be gone.

Comment 58

[Phil Schwartau]

Mike's latest idea works like a charm on both WinNT and Linux. 
That is, we retain the changes indicated in attachment 26834 [details] [diff] [review] for:

                    js/src/Makefile.ref
                    js/src/js.mak
                    js/src/makefile.win


We do NOT use the changes in attachment 26834 [details] [diff] [review] for:

                    js/src/fdlibm/fdlibm.h

Instead, this file will have only the change Mike indicated in his last comment: 

                    #ifdef XP_OS2
                    #define _LITTLE_ENDIAN
                    #endif



With this setup, I am able to successfully build the debug and optimized 
JS shells on BOTH WinNT and Linux...

Comment 59

[Mike Kaply [:mkaply]]

Brendan sent me a note with the sr, pschwartau was the r

All fixes checked in except the actual include of jstypes.h in fdlibm.h because 
that doesn't work with Linux Makefile.ref.

I'm marking this bug fixed. 

At some point, we should probably cleanup the fdlibm/js relationship, but for 
now I just want this one gone.

Comment 60

[Phil Schwartau]

[e-mail], or anyone with OS/2: 

Can you verify that this bug is fixed? If so, please mark the bug 
"Verified", otherwise, please reopen it - thanks.

Comment 61

[Mike Kaply [:mkaply]]

sehh's email is no longer valid

I'm going to mark this one verified - we have tested his testcase.

Comment 62

[[e-mail]]

My email is fine, i just couldn't reply because
i was in Sweden for some time.

Latest nightly build is still broken with this, but
i'm not sure if Mike has fixed it in an unpublished
nightly build.