479258 - On MSVC, int8_t and other stdint types in jsstdint.h conflict with Xerces-C

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Mike Moening]

We use the xerces_c library all over that place in our code.
It appears that latest tracemonkey code now has typedef conflicts 
with the xerces_c project.

The header file jsstdint.h is creating conflicted with xerces pwin32.h 
header file.

These defines in jsstdint.h

---------------------------------------------
/* Microsoft Visual C/C++ has built-in __intN types.  */ #if 
defined(JS_HAVE___INTN)

typedef __int8 int8_t;
typedef __int16 int16_t;
typedef __int32 int32_t;
typedef __int64 int64_t;

typedef unsigned __int8 uint8_t;
typedef unsigned __int16 uint16_t;
typedef unsigned __int32 uint32_t;
typedef unsigned __int64 uint64_t;
----------------------------------

Produce errors like:
\Dev\Common\xerces-c-src_2_7_0\src\unicode/pwin32.h(101) : error C2371:
'int8_t' : redefinition; different basic types
        c:\dev\common\spidermonkey\js\src\jsstdint.h(73) : see declaration of 'int8_t'

Do you think it would be possible to give the types like (int8_t) 
slightly different names to fix this?
Maybe with a js in the name?

My hack around was to find & replace int8_t with int8_jt in the entire project.

Ideas?

Comment 1

[Benjamin Smedberg]

The whole point of that code was to provide definitions for the C99 standard int types, because the Microsoft compiler doesn't have a stdint.h header.

One possibility is, if you already have these types defined elsewhere, to just put an ifdef around this entire block, something like:

#ifndef HAVE_EXTERNAL_STDINT
... do all the stuff in jsstdint.h
#endif

But no, we want to use the standard integer names, not JS-specific names.

Comment 2

[Mike Moening]

> One possibility is, if you already have these types defined elsewhere, to just
> put an ifdef around this entire block, something like:

I can't because its not my code.  Its part of xerces-c.
Also the type definitions are diferent basic types.  i.e. not compatible.

Comment 3

[Jeff Walden [:Waldo]]

Hmm, I'd thought the jsstdint.h stuff was an internal hack only, nothing visible through jsapi.h (but maybe visible if you want to use friend APIs), but I guess jsapi.h -> jspubtd.h -> jstypes.h ->jsstdint.h says otherwise.  That could probably be changed without too much effort, but there'd be a disconnect between public definitions and actual uses.

Comment 4

[Mike Moening]

Xerxes:

typedef signed char int8_t;

Spidermonkey:
typedef __int8 int8_t;

Basic underlying types are not the same.

Comment 5

[Benjamin Smedberg]

Yeah, that's not the question. The actual type and size (8 bit signed integer) is still the same. So if you wrap jsstdint.h in a #ifndef SKIP_STDINT_STUFF guard and then you define SKIP_STDINT_STUFF somewhere in your build system, that should work.

Comment 6

[Mike Moening]

>So if you wrap jsstdint.h in a #ifndef SKIP_STDINT_STUFF
>guard and then you define SKIP_STDINT_STUFF somewhere in your build system,
>that should work.

Bad hack.  Better to use a namespace or hide spidermonkey internals from the outside. Don't you think?

Also not all typedefs are defined in xerces. Only some of them conflict.
I won't work.

Comment 7

[Benjamin Smedberg]

C99 has standard types. We are using them, and plan on using them more. Unfortunately MSVC doesn't have standard definitions of these types, so we have to hack them ourself. Here we have two packages who have hacked them differently... you'll need to teach one package or the other to stop doing that.

I really don't think we should stop using the standard types because xerces is also using the standard types.

Comment 8

[Mike Moening]

>>you'll need to teach one package or the other to stop doing that.
Does any public spidermonkey API use int8_t type?

If not, then hide this from those that include jsapi.h.

Simple fix.
Either way SM "broke the build" with 1.8 so far as I'm concerned.

Comment 9

[Brendan Eich [:brendan]]

MikeM has a point, the jsapi.h header and its includes are old and they have not until recently added stdint.h typedef names. If we can avoid them doing so, we should. It's hard to use the stdint types and then erase them, though. Jim, any ideas?

/be

Comment 10

[Jim Blandy :jimb]

I'm with Mike, and with Ben.  I think it was a dumb mistake on my part to have jsapi.h introduce definitions for the standard integer types into client code.  But we definitely should feel free to use such types within SpiderMonkey; there's no reason to let Microsoft's silly decision prevent us from using modern C facilities.

I think it should be fine for "jsapi.h" to include whatever standard system headers it needs, because it's impractical not to, and it's reasonable to expect client code not to introduce their own definitions that conflict with such headers.  So on systems that actually have <stdint.h>, it should be okay for jsapi.h to #include that.

But on systems that lack <stdint.h>, I think we'll run into the xerces problem over and over again, because everyone else has been producing their own definitions of the standard integer types, just as we have.  So I think on such systems we should not introduce our own definitions for the standard types.

Fixing this would be a little easier if jsapi.h didn't end up #including most of the other SpiderMonkey header files, because that means we have to follow public hygiene rules in more header files.  But that just means more typing.

Here's what I'd suggest:

For each standard integer type intFOO_t, jstypes.h should define js_intFOO_t.  Public headers (effectively, almost all) should use only the js_-prefixed types.  Then, jsstdint.h should become an uninstalled header #included explicitly by SpiderMonkey .cpp files that defines the standard integer type names if jsapi.h hasn't done so already.

Comment 11

[Wesley W. Garland]

Another possible solution -- stomp the typedef with a macro under VC. It's only the 8-bit type that is a problem here, due to char/__int8 duality.

#if defined(JS_HAVE___INTN)
# define int8_t __int8
# define uint8_t unsigned __int8
#endif

in jsstdint.h and

#if defined(JS_HAVE___INTN)
# undef int8_t
# undef uint8_t
#endif

at bottom of jsapi.h

Con: embeddings needing to declare int8_t or uint8_t local variables will have
to supply their own types.  Also may stomp on other third party libs using the same trick.

Comment 12

[Brendan Eich [:brendan]]

(In reply to comment #10)
> I'm with Mike, and with Ben.  I think it was a dumb mistake on my part to have
> jsapi.h introduce definitions for the standard integer types into client code. 
> But we definitely should feel free to use such types within SpiderMonkey;
> there's no reason to let Microsoft's silly decision prevent us from using
> modern C facilities.

For brevity (_t adds noise-char length) and consistency with the API types, I've been advocating against mixing. OTOH we are injecting bool without taking the time to sweep clean all the non-API uses of JSBool. We should do that sweep after 3.1, though (MSVC likes to warn about mixing).

I could see a world where jsapi.h was kinda old and frozen and poor-man's namespaced to js<typename> and JS<APIorMacroName>, we had better C++ APIs over time, and stdint everywhere except jsapi.h. But that's not a nearby planet and I'd hate to make worlds collide right now. So I'd rather we start injectin bool, but keep using uint8, int32, etc. for now.

Does this sound ok?
 
> Fixing this would be a little easier if jsapi.h didn't end up #including most
> of the other SpiderMonkey header files,

It doesn't. Apart from stddef, stdio, and js-config.h and its includes, jsapi.h includes:

  jspubtd.h
    jstypes.h
      jscompat.h
    jsproto.tbl
      jsversion.h
  jsutil.h

There are over fifty .h files total in my js/src, so this is pretty good (and it's intentional -- we don't just nest #includes in jsapi.h or any header for no good reason, ya know :-P).

> because that means we have to follow
> public hygiene rules in more header files.  But that just means more typing.

Shouldn't be too bad based on above analysis.

> Here's what I'd suggest:
> 
> For each standard integer type intFOO_t, jstypes.h should define js_intFOO_t. 
> Public headers (effectively, almost all) should use only the js_-prefixed
> types.

Why inject new (and non-conforming -- no _ in typedef names per style guide, ZOMG :->) type names into public headers? We have uint32, int8, etc. already in the public APIs. We just need to keep these working without dragging in stdint.h where it hurts (Windows, boo hiss).

/be

Comment 13

[Jason Orendorff [:jorendorff]]

Let's avoid adding more integer typedefs.  SM already has several:
https://developer.mozilla.org/en/jsint

We introduced jsstdint.h so we could use the standard ones.  stdint.h might reduce porting effort, and it could make our code and API docs more readable.  (Not to brendan-- but to anyone who gets more information out of int32_t than jsint.)

These reasons still matter.  It is still worth using stdint.h if we can somehow fix this bug.

Comment 14

[Brendan Eich [:brendan]]

(In reply to comment #13)
> We introduced jsstdint.h so we could use the standard ones.  stdint.h might
> reduce porting effort, and it could make our code and API docs more readable. 
> (Not to brendan-- but to anyone who gets more information out of int32_t than
> jsint.)

First, let's not personalize this. I'm not the issue, the plain fact that many types (not jsint chiefly or alone) have been part of the JS API, and the stdint ones have not, makes it questionable to argue about readability. Standards are great, including that there are so many of them. If we could do-over and stdint were around in 1995, we would have used it (old kernel hackers can handle _t). But we don't have that option, and this is objective, not subjective.

Second, the trade-off is between int32 and int32_t, no jsint and int32_t. The jsint type is perhaps an unnecessary extra type, but it seemed worth abstracting from the 31-bit int implementation of jsval ints. Some day we could have a 64-bit jsint, in other words. So stdint does not help and could hinder in the case of this particular typedef (not so with its fraternal twin jsuint, which really should be uint32{,_t} to match ECMA-262, since it's used for Array indexes and lengths -- but perhaps that bad rep could change too [we can hope]).

> These reasons still matter.  It is still worth using stdint.h if we can somehow
> fix this bug.

Definitely. I don't think that's at issue. What we need is jsapi.h without extra baggage for those who can't take it, and automagic or manual opt into the modern C99 world, circa 2009, for those who can take it and/or want it.

/be

Comment 15

[Jim Blandy :jimb]

I'm not particular about what names we use in jsapi.h.  But in the long run, I don't think we should be defining int32 and uint32 anyway.  We should be migrating the API away from using those.

Comment 16

[Brendan Eich [:brendan]]

The API migrates slowly, with compatibility across years, into the decade range (although a lot of bugs under the bridge, and some intended and unintended breaks in compatibility).

Since the feedback loops with embedders, where they exist, have long time delay, we need to move slowly still, or we will tend more to lose embedders who can't catch up and have no fallback but sticking with an ancient SpiderMonkey. This suggests leaving jsapi.h alone as much as we can, and inventing a new API.

I'm very much in favor of a new API, provided it conserves what works in the current API and does not reinvent any round-enough wheels. It should use C++ too -- auto RAII helpers are a big win. It could use some wiki'ed design docs. Who will lead this charge?

Back to this bug. Can we do something quick and dirty like what Wes suggested in comment 11, and see how it flies with MikeM?

/be

Comment 17

[Jim Blandy :jimb]

Attachment: Bug 479258: Don't define <stdint.h> types in public headers.

On systems that don't have <stdint.h> (i.e., Microsoft, which is
tragically underfunded and cannot spare the resources necessary to
provide and support this header), SpiderMonkey header files should not
introduce definitions for these types, as doing so may conflict with
client code's attempts to provide its own definitions for these types.

Instead, have jstypes.h define JS{Int,Uint}{8,16,32,64,Ptr} types
based on configure's results, and make jsstdint.h into an uninstalled
header for use within SpiderMonkey that does whatever is necessary to
get definitions for the <stdint.h> types.

The changes to make the appropriate SpiderMonkey .cpp files #include
"jsstdint.h" explicitly are in a separate patch, for ease of review.

Comment 18

[Jim Blandy :jimb]

Attachment: Bug 479258: Include "jsstdint.h" for <stdint.h> type use within SpiderMonkey.

Comment 19

[Jim Blandy :jimb]

Just attaching the patches for now; if the Try server likes these, I'll r? them.

Comment 20

[Jim Blandy :jimb]

Comment on attachment 363763 [details] [diff] [review]
Bug 479258: Don't define <stdint.h> types in public headers.

Brendan, does this approach work for you?

Comment 21

[Brendan Eich [:brendan]]

Comment on attachment 363764 [details] [diff] [review]
Bug 479258: Include "jsstdint.h" for <stdint.h> type use within SpiderMonkey.

Does this fix the bug as reported by MikeM? I don't see a removal of the #include "jsstdint.h" in jstypes.h in this patch.

If the include of jsstdint.h by jstypes.h were removed, then it seems the order of includes in the patch would be wrong: you'd need jsstdint.h before jstypes.h, right?

Could we keep including jsstdint.h in jstypes.h but guard it with an ifdef whose macro is defined only by the engine, and by willing and able embedders? Sorry if this was already proposed and shot down for good reason. Hate to see too many distributed, order-dependent includes when the mightly C pre-processor stands ready to help with a macro and an ifdef'd nested include.

/be

Comment 22

[Brendan Eich [:brendan]]

Comment on attachment 363763 [details] [diff] [review]
Bug 479258: Don't define <stdint.h> types in public headers.

Haha, I reviewed the wrong patch. Knew there was one out there using ifdef'ed include.

But why does jsstdint.h include jstypes.h? The latter is a big boat-ful of other stuff than int typedefs. I was hoping for the other way 'round, with ifdef screening of the stddint names from unready embedders.

Other than that, this works for me if MikeM and Ted like it.

/be

Comment 23

[Jim Blandy :jimb]

(In reply to comment #22)
> But why does jsstdint.h include jstypes.h? The latter is a big boat-ful of
> other stuff than int typedefs.

For intptr_t and uintptr_t.  Let me see if I can rework that.

> I was hoping for the other way 'round, with
> ifdef screening of the stddint names from unready embedders.

Given the lay of the land outside our control (Microsoft won't provide <stdint.h>; embedders will), I don't think we'll ever be able to use <stdint.h> types in our headers --- thus, as long as jstypes.h is a public header, it just shouldn't mention the <stdint.h> types at all.  They're not part of our interface.

Comment 24

[Jim Blandy :jimb]

(In reply to comment #23)
> (In reply to comment #22)
> > But why does jsstdint.h include jstypes.h? The latter is a big boat-ful of
> > other stuff than int typedefs.
> 
> For intptr_t and uintptr_t.  Let me see if I can rework that.

Oh --- duh: jsstdint.h #includes jstypes.h because it needs JS{Int,Uint}N (and intptr_t and uintptr_t).  If a boat-free jsstdint.h is desireable, I could pull out the stuff the patch adds to jstypes.h into its own header, and then have both jstypes.h and jsstdint.h #include it from there.  Would that be better?

Comment 25

[(not currently active) Ted Mielczarek]

Do you still want my review here, or are you going to prepare a new patch?

Comment 26

[Jim Blandy :jimb]

Depends on what Brendan says.  I've cancelled your review requests; I'll put them up again when we've closed.  (Although if you want to look over the patch anyway, that'd be great, too!)

Comment 27

[Brendan Eich [:brendan]]

(In reply to comment #24)
> (In reply to comment #23)
> > (In reply to comment #22)
> > > But why does jsstdint.h include jstypes.h? The latter is a big boat-ful of
> > > other stuff than int typedefs.
> > 
> > For intptr_t and uintptr_t.  Let me see if I can rework that.
> 
> Oh --- duh: jsstdint.h #includes jstypes.h because it needs JS{Int,Uint}N (and
> intptr_t and uintptr_t).  If a boat-free jsstdint.h is desireable, I could pull
> out the stuff the patch adds to jstypes.h into its own header, and then have
> both jstypes.h and jsstdint.h #include it from there.  Would that be better?

I guess it would -- jsinttypes.h? jsnonstdint.h? jsinttd.h? Yay, naming. At least we don't have the 8.3 procrustean bed any longer.

I agreed already about not injecting stdint.h type names into jsapi.h clients' namespaces, so this must be the only door left ;-).

/be

Comment 28

[Jim Blandy :jimb]

Attachment: Bug 479258: Don't define <stdint.h> types in public headers.

Revised as requested.  New installed header jsinttypes.h, used by installed header jstypes.h and by private header jsstdint.h.

Liked by try server.

Comment 29

[Brendan Eich [:brendan]]

Comment on attachment 365746 [details] [diff] [review]
Bug 479258: Don't define <stdint.h> types in public headers.

>+++ b/js/src/jsinttypes.h
>+/*
>+** File: jsinttypes.h

Nit: this comment style comes from NSPR, forked into SpiderMonkey in 1998. You could stop it here and use Harbison&Steele JimB style ;-). Or prevailing K&R/BSD-Unix stack-of-single-stars style. I don't see the need to copy the NSPR style, though.

>+**/

Uber-nit: NSPR style would have

>+************************************************************************/

here.

>+**  Private SpiderMonkey files should include <jsstdint.h> and use the
>+**  C standard type names.

The current code uses jsotypes.h names, and it'll be hard to stop this train. Advert to the issue? If you think we are better off on the long run with _t all around, we should plan that flag day. But not inject _t randomly when patching bugs.

I'm still thinking the uint32 vs. uint32_t battle will be won by the shorter name, standardosity be damned. I could be wrong. We should have a wider discussion, but for this patch the comment should describe more the prescribe.

r=me, testing by embedders who care is key. Thanks,

/be

Comment 30

[Jim Blandy :jimb]

(In reply to comment #29)
> The current code uses jsotypes.h names, and it'll be hard to stop this train.
> Advert to the issue? If you think we are better off on the long run with _t all
> around, we should plan that flag day. But not inject _t randomly when patching
> bugs.
> 
> I'm still thinking the uint32 vs. uint32_t battle will be won by the shorter
> name, standardosity be damned. I could be wrong. We should have a wider
> discussion, but for this patch the comment should describe more the prescribe.

We have a standing offer from Jason to convert the sources; I'm going to review.  So the question is, is this change what we want?  I think it'd be an improvement.

Comment 31

[Jim Blandy :jimb]

Attachment: Bug 479258: Don't define <stdint.h> types in public headers.

Revised per Brendan's comments: comment syntax, normative language.

Comment 32

[Jim Blandy :jimb]

Attachment: Bug 479258: Include "jsstdint.h" for <stdint.h> type use within SpiderMonkey.

Refreshed.

Comment 33

[Brendan Eich [:brendan]]

(In reply to comment #30)
> (In reply to comment #29)
> > I'm still thinking the uint32 vs. uint32_t battle will be won by the shorter
> > name, standardosity be damned. I could be wrong. We should have a wider
> > discussion, but for this patch the comment should describe more the prescribe.
> 
> We have a standing offer from Jason to convert the sources; I'm going to
> review.  So the question is, is this change what we want?  I think it'd be an
> improvement.

At this point frankly I am ambivalent. We have much bigger fish to fry, although we do many things including long-term cleanups, so I don't want to starve out this work if it is considered good by a larger community (not just you and Jason :-P).

It still bugs me that uint32 is as clear as, if not clearer than, uint32_t for a type whose name strongly connotes what it means. And the JS API will use the {u,}int{8,16,32,64} type names forever.

If we get to a better world of new APIs informed by our jsapi.h experiences, settled wisdom, fresh thinking, etc., *and* these new APIs over time combine to make jsapi.h vestigial, then we could aim for stdint as part of a "we use standard C++" story to lower barriers for new contributors and market our open source to developers.

But again, it seems to me that the contest is not crucial and not decisively won by stdint right now. In the particular case the _t adds visual noise when reading code and keystrokes when writing code. The long-term best case savings would be a global reduction in typename redundancy, and (secondary, weak IMHO) familiarity at first glance if uint32 would need a second glance.

There's also the fact that nanojit uses stdint, but we generally don't try to look like nanojit code! :-P

One bigger picture is the rest of Mozilla code, which uses PRInt32, etc. Ugly by comparison, int32_t clearly wins. Probably the best way to get agreement in the longer term is to use stdint names, since NSPR moved away from the uint32, etc. names long ago to avoid conflicts with certain OS libraries.

So while I am ambivalent, I'm not opposed when the time is right to the flag day. But please build consensus among a larger group, using m.d.t.js-engine. We should have a talk with top Mozilla hackers too, not that we must move in lockstep -- just to avoid missing opportunities for coordination.

/be

Comment 34

[Jim Blandy :jimb]

It was a while ago, but I don't think Jason and I just rushed off to do this by ourselves; the idea was born on #jsapi.  I seem to recall bsmedberg being in favor, perhaps ted?, and no objections.  Not to claim this meets your threshold; just clarifying the record that there was discussion and enthusiasm amongst the people on the channel at the time.

Comment 35

[Brendan Eich [:brendan]]

Yeah, the usual suspects :-P. Kidding, I will go with the flow but it has to include igor, mrbkap, gal, dvander, waldo, sayrer, shaver, and a few others I'm forgetting who may be listed in the despot-generated http://www.mozilla.org/owners.html page.

I do think stdint is a big improvement for the entire codebase considered as a whole. PRInt32 always gave me virtual newsprint stains ;-).

/be

Comment 36

[Jim Blandy :jimb]

I'll bring the question up on m.d.t.js-engine after the release.

Comment 37

[Mike Moening]

Jim,

I appears you fixed the type errors I was seeing in this bug!
I hereby make a motion to approve this patch for checkin...
Do I have second?

Comment 38

[Jim Blandy :jimb]

(In reply to comment #37)
> I appears you fixed the type errors I was seeing in this bug!

Awesome --- thanks for trying it out!

Comment 39

[Brendan Eich [:brendan]]

This got r+ from me, can try to get it in -- marking wanted.

/be

Comment 40

[Jim Blandy :jimb]

Landed in M-C: http://hg.mozilla.org/mozilla-central/rev/5917a57686c3

Comment 41

[Mike Moening]

Hey Jim,

I'm now having this bug bite me again.  Did the patch get regressed somehow?
Find me via email or IRC.

Comment 42

[Mike Moening]

Jim,

Here's the include dependency chain that caused the error.
I thought those types were not supposed to be in public headers. ?

spiderMonkey\js\src\jsapi.h
  spidermonkey\js\src\js-config.h
  spidermonkey\js\src\jspubtd.h
    spidermonkey\js\src\jstypes.h
       spidermonkey\js\src\jsstdint.h
        spidermonkey\js\src\js-config.h

spidermonkey\js\src\jsstdint.h(73) : error C2371: 'int8_t' : redefinition; different basic types
xerces-c-src_2_7_0\src\unicode/pwin32.h(101) : see declaration of 'int8_t'

Comment 43

[Mike Moening]

Please ignore those last two comments.
After pulling down tip again just now your patch is back in again.
All is well.
Sorry for the confusion!

Comment 44

[Jim Blandy :jimb]

(In reply to comment #43)
> Please ignore those last two comments.
> After pulling down tip again just now your patch is back in again.
> All is well.
> Sorry for the confusion!

*whew*