The default bug view has changed. See this FAQ.

Windows 64 compilation error on change to nsIScriptGlobalObject.h

RESOLVED FIXED in mozilla1.8rc1

Status

()

Core
DOM
RESOLVED FIXED
12 years ago
12 years ago

People

(Reporter: Michael Moy, Assigned: bz)

Tracking

({fixed1.8})

Trunk
mozilla1.8rc1
x86
Windows XP
fixed1.8
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.8rc1 +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

12 years ago
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
(Reporter)

Updated

12 years ago
Blocks: 237202
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...
Assignee: nobody → general
Blocks: 305032
Status: UNCONFIRMED → NEW
Component: General → DOM
Ever confirmed: true
Product: Firefox → Core
QA Contact: general → ian
Version: unspecified → Trunk
Flags: blocking1.9a1?
(Reporter)

Comment 2

12 years ago
I'd be happy to code up a patch with a WIN64 ifdef to __int64 if all else fails.
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...
(Reporter)

Comment 4

12 years ago
(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.
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
> 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.  ;)
(Reporter)

Comment 7

12 years ago
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

(Reporter)

Comment 8

12 years ago
(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
bz, looks like a regression from bug 305032's patch -- the right thing is to
#include "jspubtd.h".  Can you fix?

/be
(Reporter)

Comment 10

12 years ago
I'll put in the #include and see how the build goes.
(Reporter)

Comment 11

12 years ago
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
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.
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

12 years ago
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?
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...
(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
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?
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
Flags: blocking1.8rc1?
(Reporter)

Comment 19

12 years ago
(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"
(Reporter)

Comment 20

12 years ago
(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.
Created attachment 199249 [details] [diff] [review]
Fix
Attachment #199249 - Flags: superreview?(brendan)
Attachment #199249 - Flags: review?(brendan)
(Reporter)

Comment 22

12 years ago
The patch applied and produced a usable Windows x86-64 build.
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
Attachment #199249 - Flags: superreview?(brendan)
Attachment #199249 - Flags: superreview+
Attachment #199249 - Flags: review?(brendan)
Attachment #199249 - Flags: review+
Attachment #199249 - Flags: approval1.8rc1?
Easy portability fix, usually we take these.

/be

Updated

12 years ago
Attachment #199249 - Flags: approval1.8rc1? → approval1.8rc1+

Updated

12 years ago
Flags: blocking1.8rc1? → blocking1.8rc1+

Updated

12 years ago
Assignee: general → bzbarsky
Created attachment 199340 [details] [diff] [review]
Updated to comments
Attachment #199249 - Attachment is obsolete: true
Fixed, trunk and branch.
Status: NEW → RESOLVED
Last Resolved: 12 years ago
Keywords: fixed1.8
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.8rc1
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.
Flags: blocking1.9a1?
You need to log in before you can comment on or make changes to this bug.