599764 - Conflicting typedefs for uint64/JSUint64/PRUint64 leading to build failure

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Landry Breuil (:gaston)]

User-Agent:       Mozilla/5.0 (X11; U; OpenBSD i386; en-US; rv:1.9.1.11) Gecko/20100901 Firefox/3.5.11
Build Identifier: 

Builds of ffx4 betas fail since b4 (b5,b6,same issue) when linking xpcshell with -lxul, error message follows.

/usr/obj/mozilla-firefox-4.0b6/mozilla-central/dist/bin/libxul.so.22.0: undefined reference to `nsDOMWorkerSecurityManager::JSCheckAccess(JSContext*, JSObject*, long, JSAccessMode, unsigned long long*)'
/usr/obj/mozilla-firefox-4.0b6/mozilla-central/dist/bin/libxul.so.22.0: undefined reference to `mozilla::storage::convertJSValToVariant(JSContext*, unsigned long)'
/usr/obj/mozilla-firefox-4.0b6/mozilla-central/dist/bin/libxul.so.22.0: undefined reference to `nsDOMWorkerMessageEvent::SetJSVal(JSContext*, unsigned long long)'
/usr/obj/mozilla-firefox-4.0b6/mozilla-central/dist/bin/libxul.so.22.0: undefined reference to `nsContentUtils::WrapNative(JSContext*, JSObject*, nsISupports*, nsWrapperCache*, nsID const*, unsigned long*, nsIXPConnectJSObjectHolder**, int)'
/usr/obj/mozilla-firefox-4.0b6/mozilla-central/dist/bin/libxul.so.22.0: undefined reference to `JSValToNPVariant(_NPP*, JSContext*, unsigned long,_NPVariant*)'
/usr/obj/mozilla-firefox-4.0b6/mozilla-central/dist/bin/libxul.so.22.0: undefined reference to `nsDOMWorkerTimeout::Init(JSContext*, unsigned int, unsigned long long*, int)'
/usr/obj/mozilla-firefox-4.0b6/mozilla-central/dist/bin/libxul.so.22.0: undefined reference to `nsContentUtils::CreateStructuredClone(JSContext*, unsigned long, unsigned long*)'
collect2: ld returned 1 exit status

All those failures are due to conflicting types between JSUint64 and PRUint64.
They all involve a method having a jsval type (or jsval*) as one of the arguments.

Take for example:
undefined reference to `nsDOMWorkerMessageEvent::SetJSVal(JSContext*, unsigned long long)'
defined in:
./dom/src/threads/nsDOMWorkerEvents.cpp:nsDOMWorkerMessageEvent::SetJSVal(JSContext* aCx, jsval aData)

The compiled object for this file uses a wrong type for jsval:
$nm -g dom/src/threads/nsDOMWorkerEvents.o | grep SetJSVal | c++filt
00001d40 T nsDOMWorkerMessageEvent::SetJSVal(JSContext*, unsigned long)

Suddenly, jsval is an unsigned long, not an unsigned long long (as intended ?)

This is due to conflicts between PRUint64 and JSUint64, as shown when using c++ -E output on nsDOMWorkerEvent.cpp:

typedef __attribute__((aligned (8))) uint64 jsval;
typedef PRUint64 uint64;
typedef unsigned long PRUint64;

When doing the same on a 'good' file, say nsDOMWorker.cpp (calling SetJSVal) :
typedef __attribute__((aligned (8))) uint64 jsval;
typedef JSUint64 uint64;
typedef uint64_t JSUint64; 
typedef __uint64_t uint64_t;
typedef unsigned long long __uint64_t;

Somewhere in the include chain of all those failing-to-link files:
./dom/src/threads/nsDOMWorkerSecurityManager.cpp
./storage/src/mozStoragePrivateHelpers.cpp
./dom/src/threads/nsDOMWorkerEvents.cpp
./content/base/src/nsContentUtils.cpp
./dom/src/threads/nsDOMWorkerTimeout.cpp
./modules/plugin/base/src/nsJSNPRuntime.cpp
prtypes.h takes precedance over jstypes.h.

Failure is probably hidden on other platforms because of a different typedef for __uint64_t ?

suggested fixes :
-redefining jsval typedef to something fixed ?
-shuffling includes order in the failing files so that the correct type is used ?
-make JSUint64 and PRUint64 the same typedef ?

Reproducible: Always

Steps to Reproduce:
1.try to build firefox 4 beta
2.
3.
Actual Results:  
failure :(

Expected Results:  
success \o/

Comment 1

[Boris Zbarsky [:bzbarsky]]

Doesn't seem like a nanojit issue.  But does seem like a jsapi problem....  Why are we typedefing jsval to something that we don't necessarily control the definition of (uint64, to be exact)?

Comment 2

[Luke Wagner [:luke]]

I started with uint64_t et al, but was told to drop the _t because it was the same thing but shorter.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Well, clearly whoever told you that was wrong in the presence of NSPR stuff.  :(

Comment 4

[Landry Breuil (:gaston)]

Attachment: typedef jsval using uint64_t instead of uint64

The attached patch fixes the build error. No idea if it's the best fix though, or if JSuint64 should be used instead.

Comment 5

[Luke Wagner [:luke]]

Brendan: is this right?

Comment 6

[Brendan Eich [:brendan]]

(In reply to comment #5)
> Brendan: is this right?

That can't matter because if uint64 is not uint64_t there's trouble.

How are the two typedefs ever different?

The JSUint64 etc. NSPR names are old and we should get rid of them. They resulted from incomplete forking of NSPR in 1998.

/be

Comment 7

[Boris Zbarsky [:bzbarsky]]

> That can't matter because if uint64 is not uint64_t there's trouble.

NSPR never defines or really uses uint64_t.

It does typedef uint64 to PRUint64, except on beos.

And PRUint64 is defined as follows (PRInt64 stuff elided):

342 /*
343  * On 64-bit Mac OS X, uint64 needs to be defined as unsigned long long to
344  * match uint64_t, otherwise our uint64 typedef conflicts with the uint64
345  * typedef in cssmconfig.h, which CoreServices.h includes indirectly.
346  */
347 #if PR_BYTES_PER_LONG == 8 && !defined(__APPLE__)
349 typedef unsigned long PRUint64;
350 #elif defined(WIN32) && !defined(__GNUC__)
352 typedef unsigned __int64 PRUint64;
353 #else
355 typedef unsigned long long PRUint64;
356 #endif /* PR_BYTES_PER_LONG == 8 */

Sounds like we should perhaps change that first condition?  Or just always use uint64_t if available?

Comment 8

[Boris Zbarsky [:bzbarsky]]

Or, again, fix this on the js side.... while possibly spinning off a separate bug on NSPR.

Comment 9

[Brendan Eich [:brendan]]

I thought with jsinttypes.h we finally had consistency and a single source of truth, which came from C99 if supported, else from our own workalike. Cc'ing Jim.

/be

Comment 10

[Brendan Eich [:brendan]]

(In reply to comment #8)
> Or, again, fix this on the js side.... while possibly spinning off a separate
> bug on NSPR.

Yes, this should be fixed on the JS side. I didn't realize this was an NSPR vs. JS issue.

We long ago forked NSPR stuph including the PRUint32 ugly names as JSUint32 etc., in order to stand alone. If we aren't able to stand alone any longer, but we want stdint/inttypes.h C99 goodness to be the source of truth, then it's high time to stop relying on the NSPR int typedefs.

We could still have problems in interactions between NSPR or XPCOM code based on it and JS, though -- which must be what is going on here, duh. But two bugs, one on JS we can act on quickly, another on NSPR, seems right.

/be

Comment 11

[Landry Breuil (:gaston)]

Can i get a followup on that issue ? I stumbled upon it again when porting thunderbird 3.3a2, and my 'fix' is still needed for ffx4b11.

Comment 12

[Jim Blandy :jimb]

(In reply to comment #0)
> This is due to conflicts between PRUint64 and JSUint64, as shown when using c++
> -E output on nsDOMWorkerEvent.cpp:
> 
> typedef __attribute__((aligned (8))) uint64 jsval;
> typedef PRUint64 uint64;
> typedef unsigned long PRUint64;
> 
> When doing the same on a 'good' file, say nsDOMWorker.cpp (calling SetJSVal) :
> typedef __attribute__((aligned (8))) uint64 jsval;
> typedef JSUint64 uint64;
> typedef uint64_t JSUint64; 
> typedef __uint64_t uint64_t;
> typedef unsigned long long __uint64_t;

This is really the key here. Our header files shouldn't be leaning on NSPR when <stdint.h>, and thus uint64_t, is available.

(In reply to comment #6)
> (In reply to comment #5)
> > Brendan: is this right?
> 
> That can't matter because if uint64 is not uint64_t there's trouble.
> 
> How are the two typedefs ever different?

The underlying absurdity is that, on x86_64, 'long' and 'long long' are treated differently when they appear in mangled C++ symbol names ('l' vs. 'x') --- even though they are both 64 bits long. So compile-time selection of the underlying type can get you definitions that are correct for execution but not for linking. :(

Comment 13

[Jim Blandy :jimb]

The fix is to figure out why the non-<stdint.h> uint64 definition is being used for jsval, and change that. If <stdint.h> is present, we should always be using it for jsval and the other jsapi types.

Comment 14

[Landry Breuil (:gaston)]

Comment on attachment 478732 [details] [diff] [review]
typedef jsval using uint64_t instead of uint64

In the meantime, can this get commited ? Or you really want to find a better 'global way' to pull types from stdint.h when available ?

Comment 15

[Luke Wagner [:luke]]

Comment on attachment 478732 [details] [diff] [review]
typedef jsval using uint64_t instead of uint64

(In reply to comment #13)
> The fix is to figure out why the non-<stdint.h> uint64 definition is being used
> for jsval, and change that. If <stdint.h> is present, we should always be using
> it for jsval and the other jsapi types.

jsval uses uint64/uint32/etc because these types are used exclusively in the jsapi.h; there are no uses of the _t stdint types.h.

I have been operating under the impression (based on a Waldo v. Brendan discussion somewhere on this same issue) that the SM style is to use non-_t types in preference to the _t types.  grep uint64_t -c shows that this style is mostly followed, except in jsclone.  I would be much happier if we could just kill the non-standard non-_t types altogether and *only* use the _t types.  Is there any reason we can't do that?  If not, then it seems like the fix is to make uint64 not conflict with NSPR, if at all possible.

Either way, I don't think this patch is the one that should land.

Comment 16

[Brendan Eich [:brendan]]

We have the shorter names, they're in the API. Brevity + API-mixing inside the engine trump standards. Better to have fewer typename aliases than more.

BTW, we should get rid of jsint and jsuint inside the engine and in the API (since they are POD and equivalent to int32 and uint32, IINM).

/be

Comment 17

[Jim Blandy :jimb]

(In reply to comment #15)
> jsval uses uint64/uint32/etc because these types are used exclusively in the
> jsapi.h; there are no uses of the _t stdint types.h.

That's not what I meant --- certainly SpiderMonkey should be using its traditional uint64, uint32, etcetera. I have no beef with comment 16; we had that discussion, and the decision was made.

The issue here, I thought, is that the *definition* of uint64 comes from different places depending on who's including jsapi.h, so that jsval within SpiderMonkey cites uint64 with one definition, and jsval outside SpiderMonkey cites uint64 with a different definition.

This would be fine, since the two types are both 64-bit unsigned types; the code would run perfectly. But C++ distinguishes in mangled names between integer types that have the same signedness and bit length.

Comment 18

[Luke Wagner [:luke]]

(In reply to comment #17)
> That's not what I meant

Oops, sorry, I see that now.

Comment 19

[Landry Breuil (:gaston)]

Fwiw, that patch is still needed to build ffx 5.0b3 (and mozilla-aurora, and mozilla-central, etc...) on OpenBSD/amd64 and probably others. Can something be done to fix that properly ?

Comment 20

[Jim Blandy :jimb]

So, this is the definition that seems to be causing us misery:

typedef PRUint64 uint64;

I believe this appears at nsprpub/pr/include/obsolete/protypes.h:100; Landry, can you verify that that is indeed the definition of uint64 that's taking effect on the affected platforms?

The comment atop that file says:

/*
 * This header typedefs the old 'native' types to the new PR<type>s.
 * These definitions are scheduled to be eliminated at the earliest
 * possible time. The NSPR API is implemented and documented using
 * the new definitions.
 */

And SM is carefully stepping aside if that header is included, in js/src/jsotypes.h:

/*
 * Note that we test for PROTYPES_H, not JSOTYPES_H.  This is to avoid
 * double-definitions of scalar types such as uint32, if NSPR's
 * protypes.h is also included.
 */
#ifndef PROTYPES_H
#define PROTYPES_H

So... how is this "obsolete" header file making its way into the affected compilation units? Can we delete its #inclusion, and perhaps use the PR types directly instead?

Comment 21

[Jim Blandy :jimb]

Okay, so what's going on here is that there's a #inclusion race.

Both js/src/jsotypes.h and nsprpub/pr/include/obsolete/protypes.h say:

#ifndef PROTYPES_H
#define PROTYPES_H

(which is charming, but leave that aside) which means that whichever one gets #included first gets its definition to win.

The universal SpiderMonkey header, jsapi.h, #includes jspubtd.h -> jstypes.h -> jsotypes.h, so jsotypes.h is part of SpiderMonkey's public API. protypes.h is #included from prtypes.h, which is #included everywhere. Ideally we'd #defined NO_NSPR_10_SUPPORT and stop using headers from directories marked "obsolete", but that may be more work and churn than we want to deal with now, and the benefit is unclear.

But it suggests a quick fix: just make sure that, in the broken files, jsapi.h gets #included before prtypes.h.

Comment 22

[Jim Blandy :jimb]

Landry, do you want to pursue this? I don't have a system on which this is broken handy to test against.

Comment 23

[Landry Breuil (:gaston)]

Quoting myself from comment 1:

> Somewhere in the include chain of all those failing-to-link files:
> ./dom/src/threads/nsDOMWorkerSecurityManager.cpp
> ./storage/src/mozStoragePrivateHelpers.cpp
> ./dom/src/threads/nsDOMWorkerEvents.cpp
> ./content/base/src/nsContentUtils.cpp
> ./dom/src/threads/nsDOMWorkerTimeout.cpp
> ./modules/plugin/base/src/nsJSNPRuntime.cpp
> prtypes.h takes precedance over jstypes.h.

If i get it right, you want me to include jsapi.h on top of each of those files ? Will do.

But wouldn't it be "cleaner" to just stop using obsolete headers, as you said ?

Comment 24

[Jim Blandy :jimb]

(In reply to comment #23)
> But wouldn't it be "cleaner" to just stop using obsolete headers, as you
> said ?

I'm willing to bet that #defining NO_NSPR_10_SUPPORT by default, across the tree, would be a major project. I don't know how much code there is out there that uses bits and pieces of the NSPR 1.0 interface.

Comment 25

[Jim Blandy :jimb]

The key point being that a modern, essential NSPR header file, prtypes.h, conditionally includes the obsolete header protypes.h, with the effect that the entire source tree expects that protypes.h is in force. Removing something so widely used is, in my experience, a major project.

On the other hand...

protypes.h doesn't have any effect if PROTYPES_H is #defined. And jsapi.h has been #defining PROTYPES_H itself. And the limited extent of the problem you ran into --- only a few files --- suggests that almost everywhere in the code base is seeing jsapi.h before prtypes.h. This is plausible. We could test this hypothesis by putting a blatant error in protypes.h and seeing how much things still build.

If true, then removing the #inclusion of protypes.h from prtypes.h might not really have much effect at all. We'd have less obsolete code participating in the build. That would be the best outcome, and not much work.

Comment 26

[Landry Breuil (:gaston)]

Attachment: Ensure jsapi.h is included first

Here's a patch trying to include jsapi.h first (well, before prtypes.h)

Comment 27

[Landry Breuil (:gaston)]

With the latter patch (which might be wrong, to me it'd make more sense to #include "jsapi.h" at the top of each corresponding headers for all offended cpp file... otherwise another include in the header could pull prtypes.h first. Ugh.), i'm still getting the corresponding failure when linking TestINIParser with libxul (i suppose it's the first binary that gets linked against it..) :

../../dist/bin/libxul.so.1.0: undefined reference to `nsDOMWorkerTimeout::Init(JSContext*, unsigned int, unsigned long*, int)'
../../dist/bin/libxul.so.1.0: undefined reference to `nsXPConnect::Base64Encode(JSContext*, unsigned long, unsigned long*)'
../../dist/bin/libxul.so.1.0: undefined reference to `non-virtual thunk to nsDOMFile::Initialize(nsISupports*, JSContext*, JSObject*, unsig
ned int, unsigned long long*)'
../../dist/bin/libxul.so.1.0: undefined reference to `mozilla::storage::convertJSValToVariant(JSContext*, unsigned long)'
../../dist/bin/libxul.so.1.0: undefined reference to `nsXPConnect::Base64Decode(JSContext*, unsigned long, unsigned long*)'
../../dist/bin/libxul.so.1.0: undefined reference to `nsContentUtils::WrapNative(JSContext*, JSObject*, nsISupports*, nsWrapperCache*, nsID const*, unsigned long*, nsIXPConnectJSObjectHolder**, int)'
../../dist/bin/libxul.so.1.0: undefined reference to `JSValToNPVariant(_NPP*, JSContext*, unsigned long, _NPVariant*)'
../../dist/bin/libxul.so.1.0: undefined reference to `nsDOMFile::Initialize(nsISupports*, JSContext*, JSObject*, unsigned int, unsigned long long*)'

To be honest, i don't think shuffling include order is the way to go.

Should we open a bug against nspr asking for #include "protypes.h" to be removed from prtypes.h ? This starts to be beyond my understanding of the codebase...

Comment 28

[Landry Breuil (:gaston)]

Oh, and fwiw commenting out the inclusion of obsolete/protypes.h in prtypes.h breaks early in the build, as xpidl_typelib.c relies on uint16 being defined :

/home/landry/src/mozilla-central/xpcom/typelib/xpidl/xpidl_typelib.c:50: error: expected specifier-qualifier-list before 'uint16'
/home/landry/src/mozilla-central/xpcom/typelib/xpidl/xpidl_typelib.c: In function 'add_interface_maybe':
/home/landry/src/mozilla-central/xpcom/typelib/xpidl/xpidl_typelib.c:144: error: 'struct priv_data' has no member named 'interface_map'
/home/landry/src/mozilla-central/xpcom/typelib/xpidl/xpidl_typelib.c:148: error: 'struct priv_data' has no member named 'interface_map'
/home/landry/src/mozilla-central/xpcom/typelib/xpidl/xpidl_typelib.c:162: error: 'struct priv_data' has no member named 'interface_map'
/home/landry/src/mozilla-central/xpcom/typelib/xpidl/xpidl_typelib.c:164: error: 'struct priv_data' has no member named 'ifaces'

etc etc

Comment 29

[Landry Breuil (:gaston)]

Another try... don't 'typedef PRUint64 uint64' in protypes.h on OpenBSD, which also fails :


../../../dist/include/mozilla/Util.h:148: error: 'uint64' does not name a type
../../../dist/include/mozilla/Util.h:160: error: 'uint64' does not name a type
In file included from /home/landry/src/mozilla-central/js/src/jsval.h:48,
                 from /home/landry/src/mozilla-central/js/src/jspubtd.h:47,
                 from /home/landry/src/mozilla-central/js/src/jsapi.h:49,
                 from /home/landry/src/mozilla-central/js/src/shell/jsworkers.cpp:47:
/home/landry/src/mozilla-central/js/src/jsutil.h: In static member function 'static T* js::OffTheBooks::array_new(size_t)':
/home/landry/src/mozilla-central/js/src/jsutil.h:474: error: 'uint64' was not declared in this scope
/home/landry/src/mozilla-central/js/src/jsutil.h:474: error: expected `;' before 'numBytes64'
/home/landry/src/mozilla-central/js/src/jsutil.h:474: error: 'numBytes64' was not declared in this scope
In file included from /home/landry/src/mozilla-central/js/src/jspubtd.h:47,
                 from /home/landry/src/mozilla-central/js/src/jsapi.h:49,
                 from /home/landry/src/mozilla-central/js/src/shell/jsworkers.cpp:47:
/home/landry/src/mozilla-central/js/src/jsval.h: At global scope:
/home/landry/src/mozilla-central/js/src/jsval.h:154: error: 'uint64' was not declared in this scope

Sorry, i'm trying random things, as all that mess seems intermixed... and i don't have more ideas. So far the only patch that works for me is https://bugzilla.mozilla.org/attachment.cgi?id=478732

Comment 30

[Jim Blandy :jimb]

Attachment: Define jsval in terms of a type we control, not a type defined, depending on circumstances, by either NSPR or our own headers.

Does this patch work for you?

If it does, I'm disappointed in myself: this is the obvious change, suggested in the earliest comments in the bug.

Comment 31

[Jim Blandy :jimb]

Comment on attachment 537836 [details] [diff] [review]
Define jsval in terms of a type we control, not a type defined, depending on circumstances, by either NSPR or our own headers.

Try server says it's all a-okay. Thanks for your patience. [muttered obscenities]

Comment 32

[Luke Wagner [:luke]]

I see there has been a windy road back to the same place, but can you explain why only jsval's type changes, and not every other use of uint64/uint32 in jsapi.h?  My naive expectation would be that either we'd use uintX everywhere in jsapi.h, or JSUintX, but not a mix.  Said differently, lets say I have to add a new decl to jsapi.h, how do I know whether to pick uint64 (like JS_ReadStructuredClone) or JSUint64 (like jsval would be)?  Is the weirdness isolated to uint64 and not uint32?

Comment 33

[Jim Blandy :jimb]

(In reply to comment #32)
> I see there has been a windy road back to the same place, but can you
> explain why only jsval's type changes, and not every other use of
> uint64/uint32 in jsapi.h?

Perhaps we should be doing so. I don't see why we wouldn't get the same sorts of linking errors.

Comment 34

[Jim Blandy :jimb]

(In reply to comment #33)
> Perhaps we should be doing so. I don't see why we wouldn't get the same
> sorts of linking errors.

However, changing the types of all the jsapi.h functions is a much larger change, and while it seems to be an improvement, it doesn't actually fix any problems we know of --- unlike the jsval change, which fixes a build.

Can we put this patch in, given that I've filed the follow-up bug 662852?

Comment 35

[Luke Wagner [:luke]]

Comment on attachment 537836 [details] [diff] [review]
Define jsval in terms of a type we control, not a type defined, depending on circumstances, by either NSPR or our own headers.

Bug 662852 looks great.  Landing this now seems fine; I just wanted to make sure I understood whether this was a partial fix or whether there was really something special about jsvals.

Comment 36

[Jim Blandy :jimb]

http://hg.mozilla.org/tracemonkey/rev/ed6d1f5ffed8

Comment 37

[Jim Blandy :jimb]

Landry, haven't heard from you whether this patch fixes your problem. If it doesn't, please re-open.

Comment 38

[Jim Blandy :jimb]

Oh, wait, resolved/fixed means "in mozilla-central". Sorry for the noise.

Comment 39

[Landry Breuil (:gaston)]

Sorry for the delay, life's been pretty busy. Yes, your JSUint64 fix works for me on OpenBSD/amd64, i'll just have to check other archs to be sure. Thanks a lot !

Comment 40

[Chris Leary [:cdleary] (not checking bugmail)]

cdleary-bot mozilla-central merge info:
http://hg.mozilla.org/mozilla-central/rev/ed6d1f5ffed8