312003 - Windows 64 compilation error on change to nsIScriptGlobalObject.h

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Michael Moy]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.2 x64; en-US; rv:1.8b5) Gecko/20051006 mmoy CE 1.5 Beta 2 K8M/64-X08
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.2 x64; en-US; rv:1.8b5) Gecko/20051006 mmoy CE 1.5 Beta 2 K8M/64-X08

Compiler error on addition of typedef long JSWord; from the patch in Bugzilla at
https://bugzilla.mozilla.org/show_bug.cgi?id=305032 (305032).

The change was in mozilla/ dom/ public/ nsIScriptGlobalObject.h (3.30)

I changed it to an __int64 to get the build to work (1.5 Beta 2). I tried a few
other things but they didn't work. Boris suggested that I file a new bug to make
sure that there isn't something else that can be used here.

Reproducible: Always

Comment 1

[Boris Zbarsky [:bzbarsky]]

Michael, you don't have to add cc's one at a time -- just add them all at once.  ;)

Brendan, do you know what's up here?  I traced the chain of typedefs for jsval
and ended up at |long|.  I don't want to include the relevant js header here
because that would mean REQUIRES changes in a bunch of places...

Comment 2

[Michael Moy]

I'd be happy to code up a patch with a WIN64 ifdef to __int64 if all else fails.

Comment 3

[Boris Zbarsky [:bzbarsky]]

I'm a little confused.  Is |long| not enough for a void* on Win64?  If so, how
does the JS engine ever compile and run?  Or any of the rest of Gecko, for that
matter (see PRWord, for example; we have people casting void* to PRWord and back
all the time).  If long _is_ big enough for a void*, then what is the compile
error?  You never actually said what that was...

Comment 4

[Michael Moy]

(In reply to comment #3)
> I'm a little confused.  Is |long| not enough for a void* on Win64?  If so, how
> does the JS engine ever compile and run?  Or any of the rest of Gecko, for that
> matter (see PRWord, for example; we have people casting void* to PRWord and back
> all the time).  If long _is_ big enough for a void*, then what is the compile
> error?  You never actually said what that was...

I think that a long is 32 bits and a pointer is 64 bits. Makoto Kato has a bunch
of patches that are waiting to get in that does the magic to get Firefox and
Thunderbird to build and run in Windows 64. I just build it and put it out there
and try to keep it buildable. And do a few other fun things too.

My guess is that it uses 32 bits in some places and 64 bits in a few others.

I can get you the error message but it will take a while as I have a Thunderbird
build running right now and I can only do one 64-bit build at a time. When the
current build is done, I can reset for a Firefox build to get the error message.

Comment 5

[Brendan Eich [:brendan]]

As Mikhail Teterin of FreeBSD has pointed out (in bugs where I misbehaved badly
in reaction to his idealism, I'm afraid), we should aspire to use <stdint.h> and
intptr_t.  But C99 uptake is still not what it should be, six years on.  Anyone
want to take a crack at using <stdint.h> on platforms that support it?

/be

Comment 6

[Boris Zbarsky [:bzbarsky]]

> Makoto Kato has a bunch of patches that are waiting to get in

Ah.  Do those patches change
http://lxr.mozilla.org/seamonkey/source/js/src/jstypes.h#390 ?  If so, to what?
 We should change this typedef to the same thing.

Using stdint.h when available is nice if we can get a nice HAVE_STDINT_H define
via configure.  ;)

Comment 7

[Michael Moy]

Error:

nsScriptSecurityManager.cpp
Building deps for nsScriptSecurityManager.cpp
/cygdrive/f/Mozilla/mozilla/build/cygwin-wrapper cl
-FonsScriptSecurityManager.obj -c  -DXPC_IDISPATCH_SUPPORT
 -DXPCOM_TRANSLATE_NSGM_ENTRY_POINT=1 -DMOZILLA_INTERNAL_API -D_IMPL_NS_GFX
-D_IMPL_NS_MSG_BASE -D_IMPL_NS_WID
GET  -DOSTYPE=\"WINNT5.2\" -DOSARCH=\"WINNT\" -D_WIN64 -D_AMD64_
-DBUILD_ID=0000000000  -I../../dist/include/x
pcom -I../../dist/include/string -I../../dist/include/pref
-I../../dist/include/js -I../../dist/include/dom -I
../../dist/include/xpconnect -I../../dist/include/necko -I../../dist/include/jar
-I../../dist/include/widget -
I../../dist/include/plugin -I../../dist/include/intl
-I../../dist/include/docshell -I../../dist/include/window
watcher -I../../dist/include/content -I../../dist/include/caps
-I../../dist/include -I../../dist/include/nspr
   -I../../dist/sdk/include -I/cygdrive/f/Mozilla/mozilla/caps/src/../include  
    -TP -nologo -GS- -W3 -Gy -
FdnsScriptSecurityManager.pdb  -DNDEBUG -DTRIMMED -O2 -GL -fp:fast -GS- -MD    
       -DX_DISPLAY_MISSING=1 -
DHAVE_64BIT_OS=1 -DMOZILLA_VERSION=\"1.8b5\" -DMOZILLA_VERSION_U=1.8b5
-DHAVE_SNPRINTF=1 -D_WINDOWS=1 -D_WIN32
=1 -DWIN32=1 -DXP_WIN=1 -DXP_WIN32=1 -DHW_THREADS=1 -DWINVER=0x400
-D_WIN32_WINNT=0x400 -DSTDC_HEADERS=1 -DWIN
32_LEAN_AND_MEAN=1 -DNO_X11=1 -D_AMD64_=1 -D_M_AMD64=1 -D_WIN64=1 -DWINVER=0x501
-DD_INO=d_ino -DMOZ_DEFAULT_T
OOLKIT=\"windows\" -DMOZ_PHOENIX=1 -DMOZ_BUILD_APP=browser -DMOZ_XUL_APP=1
-DMOZ_DISTRIBUTION_ID=\"org.mozilla
\" -DOJI=1 -DIBMBIDI=1 -DMOZ_VIEW_SOURCE=1 -DACCESSIBILITY=1 -DMOZ_XPINSTALL=1
-DMOZ_JSLOADER=1 -DMOZ_XTF=1 -D
MOZ_MATHML=1 -DMOZ_SVG=1 -DMOZ_SVG_RENDERER_CAIRO=1 -DMOZ_UPDATE_CHANNEL=default
-DMOZ_LOGGING=1 -DMOZ_USER_DI
R=\"Mozilla\" -DHAVE_UINT64_T=1 -DMOZ_XUL=1 -DMOZ_PROFILELOCKING=1
-DMOZ_DLL_SUFFIX=\".dll\" -DJS_THREADSAFE=1
 -DNS_PRINT_PREVIEW=1 -DNS_PRINTING=1 -DMOZILLA_LOCALE_VERSION=\"1.8b5\"
-DMOZILLA_REGION_VERSION=\"1.8b5\" -D
MOZILLA_SKIN_VERSION=\"1.8\"  -D_MOZILLA_CONFIG_H_ -DMOZILLA_CLIENT
/cygdrive/f/Mozilla/mozilla/caps/src/nsScr
iptSecurityManager.cpp
nsScriptSecurityManager.cpp
../../dist\include\dom\nsIScriptGlobalObject.h(53) : error C2371: 'jsval' :
redefinition; different basic types
        ../../dist\include\js\jspubtd.h(55) : see declaration of 'jsval'
make[4]: *** [nsScriptSecurityManager.obj] Error 2
make[4]: Leaving directory `/cygdrive/f/Mozilla/mozilla/caps/src'
make[3]: *** [libs] Error 2
make[3]: Leaving directory `/cygdrive/f/Mozilla/mozilla/caps'
make[2]: *** [tier_9] Error 2
make[2]: Leaving directory `/cygdrive/f/Mozilla/mozilla'
make[1]: *** [default] Error 2
make[1]: Leaving directory `/cygdrive/f/Mozilla/mozilla'
make: *** [build] Error 2

F:\Mozilla\mozilla>cd \mozilla

Which apparently is a redefinition of


typedef uint16    jschar;
typedef int32     jsint;
typedef uint32    jsuint;
typedef float64   jsdouble;
typedef jsword    jsval;
typedef jsword    jsid;
typedef int32     jsrefcount;   /* PRInt32 if JS_THREADSAFE, see jslock.h */

in jspubtd.h

Comment 8

[Michael Moy]

(In reply to comment #6)
> > Makoto Kato has a bunch of patches that are waiting to get in
> 
> Ah.  Do those patches change
> http://lxr.mozilla.org/seamonkey/source/js/src/jstypes.h#390 ?  If so, to what?
>  We should change this typedef to the same thing.
> 
> Using stdint.h when available is nice if we can get a nice HAVE_STDINT_H define
> via configure.  ;)

 /*
 ** A JSWord is an integer that is the same size as a void*
 */
+#if JS_BYTES_PER_WORD == 8 && JS_BYTES_PER_LONG != 8
+typedef JSInt64 JSWord;
+typedef JSUint64 JSUword;
+#else
 typedef long JSWord;
 typedef unsigned long JSUword;
+#endif

Comment 9

[Brendan Eich [:brendan]]

bz, looks like a regression from bug 305032's patch -- the right thing is to
#include "jspubtd.h".  Can you fix?

/be

Comment 10

[Michael Moy]

I'll put in the #include and see how the build goes.

Comment 11

[Michael Moy]

Just adding the #include failed as it appears that it couldn't find the
include file. Do I need to include a path to the file?

Building deps for nsBaseFilePicker.cpp
/cygdrive/f/Mozilla/mozilla/build/cygwin-wrapper cl -FonsBaseFilePicker.obj -c 
-D_IMPL_NS_WIDGET -DUSE_TLS_FO
R_TOOLKIT -DMOZILLA_INTERNAL_API -D_IMPL_NS_GFX -D_IMPL_NS_MSG_BASE
-D_IMPL_NS_WIDGET  -DOSTYPE=\"WINNT5.2\" -
DOSARCH=\"WINNT\" -D_WIN64 -D_AMD64_ -DBUILD_ID=0000000000
-I/cygdrive/f/Mozilla/mozilla/widget/src/xpwidgets/
../windows -I/cygdrive/f/Mozilla/mozilla/widget/src/xpwidgets 
-I../../../dist/include/xpcom -I../../../dist/i
nclude/string -I../../../dist/include/gfx -I../../../dist/include/layout
-I../../../dist/include/content -I../
../../dist/include/dom -I../../../dist/include/pref
-I../../../dist/include/locale -I../../../dist/include/nec
ko -I../../../dist/include/htmlparser -I../../../dist/include/uconv
-I../../../dist/include/unicharutil -I../.
./../dist/include/view -I../../../dist/include/docshell
-I../../../dist/include/view -I../../../dist/include/i
ntl -I../../../dist/include/widget -I../../../dist/include
-I../../../dist/include/nspr    -I../../../dist/sdk
/include       -TP -nologo -GS- -W3 -Gy -FdnsBaseFilePicker.pdb  -DNDEBUG
-DTRIMMED -O2 -GL -fp:fast -GS-  -MD
            -DX_DISPLAY_MISSING=1 -DHAVE_64BIT_OS=1 -DMOZILLA_VERSION=\"1.8b5\"
-DMOZILLA_VERSION_U=1.8b5 -DHA
VE_SNPRINTF=1 -D_WINDOWS=1 -D_WIN32=1 -DWIN32=1 -DXP_WIN=1 -DXP_WIN32=1
-DHW_THREADS=1 -DWINVER=0x400 -D_WIN32
_WINNT=0x400 -DSTDC_HEADERS=1 -DWIN32_LEAN_AND_MEAN=1 -DNO_X11=1 -D_AMD64_=1
-D_M_AMD64=1 -D_WIN64=1 -DWINVER=
0x501 -DD_INO=d_ino -DMOZ_DEFAULT_TOOLKIT=\"windows\" -DMOZ_PHOENIX=1
-DMOZ_BUILD_APP=browser -DMOZ_XUL_APP=1
-DMOZ_DISTRIBUTION_ID=\"org.mozilla\" -DOJI=1 -DIBMBIDI=1 -DMOZ_VIEW_SOURCE=1
-DACCESSIBILITY=1 -DMOZ_XPINSTAL
L=1 -DMOZ_JSLOADER=1 -DMOZ_XTF=1 -DMOZ_MATHML=1 -DMOZ_SVG=1
-DMOZ_SVG_RENDERER_CAIRO=1 -DMOZ_UPDATE_CHANNEL=de
fault -DMOZ_LOGGING=1 -DMOZ_USER_DIR=\"Mozilla\" -DHAVE_UINT64_T=1 -DMOZ_XUL=1
-DMOZ_PROFILELOCKING=1 -DMOZ_DL
L_SUFFIX=\".dll\" -DJS_THREADSAFE=1 -DNS_PRINT_PREVIEW=1 -DNS_PRINTING=1
-DMOZILLA_LOCALE_VERSION=\"1.8b5\" -D
MOZILLA_REGION_VERSION=\"1.8b5\" -DMOZILLA_SKIN_VERSION=\"1.8\" 
-D_MOZILLA_CONFIG_H_ -DMOZILLA_CLIENT /cygdri
ve/f/Mozilla/mozilla/widget/src/xpwidgets/nsBaseFilePicker.cpp
nsBaseFilePicker.cpp
../../../dist\include\dom\nsIScriptGlobalObject.h(44) : fatal error C1083:
Cannot open include file: 'jspubtd.
h': No such file or directory
make[5]: *** [nsBaseFilePicker.obj] Error 2
make[5]: Leaving directory `/cygdrive/f/Mozilla/mozilla/widget/src/xpwidgets'
make[4]: *** [libs] Error 2
make[4]: Leaving directory `/cygdrive/f/Mozilla/mozilla/widget/src'
make[3]: *** [libs] Error 2
make[3]: Leaving directory `/cygdrive/f/Mozilla/mozilla/widget'
make[2]: *** [tier_9] Error 2
make[2]: Leaving directory `/cygdrive/f/Mozilla/mozilla'
make[1]: *** [default] Error 2
make[1]: Leaving directory `/cygdrive/f/Mozilla/mozilla'
make: *** [build] Error 2

Comment 12

[Boris Zbarsky [:bzbarsky]]

Brendan, please see comment 1 paragraph 2 for why we are not doing that to start
with.

Given how much this file is included (via includes in other headers, which we so
love, etc) all over the codebase, it would take about forever to add all the
relevant REQUIRES lines.  And then I get to babysit the tree through its red for
all the non-Linux cases I missed.  I can do that sometime, I guess, but I won't
have the time in the near future.

Comment 13

[Brendan Eich [:brendan]]

bz: then use a void * (as other interfaces do) -- ripping off another modules
typedef violates modularity, and stuff.  Don't do it!

Michael: did you leave off the .h (not that we want jspubtd.h included, per bz's
cited comment 1)?  Diagnostic made it look that way, but that won't work for our
builds, IIRC.

/be

Comment 14

[Wan-Teh Chang]

I have an NSPR patch in bug 225859 that ports NSPR
to 64-bit Windows.  Who would be a good person to
review that patch?

Comment 15

[Boris Zbarsky [:bzbarsky]]

Sure, I can switch to a void* that window then casts to jsval*.  You want that
for 1.8?

For 1.9 we probably need to do something more language-neutral here anyway...

Comment 16

[Brendan Eich [:brendan]]

(In reply to comment #15)
> Sure, I can switch to a void* that window then casts to jsval*.  You want that
> for 1.8?

I meant void* for jsval, not jsval* -- unless the latter is all you need, in
which case it's even easier.

> For 1.9 we probably need to do something more language-neutral here anyway...

Yes.  nsIVariant comes to mind, costly though it is.  Cc'ing markh.

/be

Comment 17

[Boris Zbarsky [:bzbarsky]]

I think cost is really not something to worry about for window.arguments
attachment... ;)

As for jsval vs jsval*, the method is taking a jsval array and a length.  So I
do only need to pass in a jsval*...  Again, do you want me to make a patch for
1.8 branch?

Comment 18

[Brendan Eich [:brendan]]

We need to restore lost portability sooner or later to the branch.  I think the
patch will be zero-risk, but I'll wait to see it first!

/be

Comment 19

[Michael Moy]

(In reply to comment #13)
> bz: then use a void * (as other interfaces do) -- ripping off another modules
> typedef violates modularity, and stuff.  Don't do it!
> 
> Michael: did you leave off the .h (not that we want jspubtd.h included, per bz's
> cited comment 1)?  Diagnostic made it look that way, but that won't work for our
> builds, IIRC.
> 
> /be

I just copied the #includes already in the file:

#include "nsISupports.h"
#include "nsEvent.h"
#include "jspubtd.h"

Comment 20

[Michael Moy]

(In reply to comment #18)
> We need to restore lost portability sooner or later to the branch.  I think the
> patch will be zero-risk, but I'll wait to see it first!
> 
> /be

As an aside, I have a few other patches to fix breakages in the Windows 64-x86
branch build and I suppose that I should file bugzilla entries on those too. I
haven't done a Trunk build in a while and I'd guess that there are a few more
breakages there as well. The latest released Trunk is 8/31 from Makoto.

Boris: if you want me to test a patch, let me know as my machine is available
for a build tonight.

Comment 21

[Boris Zbarsky [:bzbarsky]]

Attachment: Fix

Comment 22

[Michael Moy]

The patch applied and produced a usable Windows x86-64 build.

Comment 23

[Brendan Eich [:brendan]]

Comment on attachment 199249 [details] [diff] [review]
Fix

>+   * @param aArgc the number of args
>+   * @param aArgv the pointer to the args.  This may be cast to jsval* and the
>+   *        args are found at
>+   *        *((jsval*)aArgv), ..., *(((jsval*)aArgv) + aArgc - 1)

Nit: ((jsval*)aArgv)[0] .. ((jsval*)aargv)[aArgc - 1]

r+sr=me with that picked.  Thanks,

/be

Comment 24

[Brendan Eich [:brendan]]

Easy portability fix, usually we take these.

/be

Comment 25

[Boris Zbarsky [:bzbarsky]]

Attachment: Updated to comments

Comment 26

[Boris Zbarsky [:bzbarsky]]

Fixed, trunk and branch.

Comment 27

[Mark Hammond [:markh] [:mhammond]]

FYI, my patch has changed this to an nsIArray.  To try and reduce performance
and conversion concerns, there is a new "argv holder" object that implements a
private interface to get back the raw jsvals, as well as nsIArray.  Thus,
existing callers of this interface do not require conversion of the jsvals -
just a QI and virtual call to get the original argv/argc back.

The factory function for this new object does expose a "jsval *argv" though -
I'll ensure that is updated accordingly.