704313 - Add proper, overridable <stdint.h> support to mfbt

Status: RESOLVED FIXED

(Core :: MFBT, defect)

Description

[Jeff Walden [:Waldo]]

To fix bug 579517 basically as bug 579517 comment 10 suggests, we need to add <stdint.h> or its moral equivalent to mfbt.  This bug will do that.

I did a little noodling on patches to start doing this over the weekend.  The <SMVC2010 and non-MSVC paths seem to work and build, but I haven't tested the custom-override path yet.  I'll post patches when I have that working and tested.

Comment 1

[Jeff Walden [:Waldo]]

Attachment: 1 - Add an MS-compatible stdint.h reimplementation to mfbt without uses (those will come later)

If we're going to have a reimplementation for old MSVC to use, it needs to be in the tree.  This adds the stdint.h from msinttypes on Google Code:

https://code.google.com/p/msinttypes/source/browse/trunk/stdint.h
https://msinttypes.googlecode.com/svn/trunk/stdint.h

There's one other possible implementation[0] out there (two if you squint really hard[1]) -- see bug 662852 comment 19.  I picked msstdint over pstdint.h and cstdint.hpp for a few reasons.  First, msstdint is targeted specifically at our single problem compiler, which makes it simpler to understand.  Second, pstdint.h has a significant list of caveats in it, and this one has none.  (Whether or not they might ever apply, it's still a little worrisome.  And it's clear there's more complexity to understanding the file in any case.)  Third, that possibility seems to provide a bunch more undesired preprocessor symbols -- the PRINTF_* macros -- which we don't need, and really don't want in case someone thought they were available for use on non-Windows systems.

That's not to say msstdint is perfect, alas.  For example, the limit macros aren't all usable in preprocessor inequalities.  But I think this could be fixed pretty easily if necessary.

Gerv, we can add this file to the source tree, right?  It looks like a BSD license with a modification to #3 to say that we "may" promote products using the author's name.  (Someone else noticed this and filed a bug on it in their tracker, see <https://code.google.com/p/msinttypes/issues/detail?id=7>.  I see no reason not to believe this was a deliberate change by the author)  And is this the proper way to include attribution, by changing license.html like so?

Uses of this file will follow in another patch.  Given the licensing stuff, it seemed best to segregate it from the rest of the changes here.

0. http://www.azillionmonkeys.com/qed/pstdint.h
1. http://www.boost.org/doc/libs/1_37_0/boost/cstdint.hpp

Comment 2

[Jeff Walden [:Waldo]]

Attachment: 2 - Implement mozilla/StdInt.h, add it to mfbt/Types.h

This does the stdint.h song-and-dance routine to get the integer types defined: per a file defined by a macro if specified, then the MS-compatible stdint.h if the compiler needs it, else the standard location.

Luke wants to remove the jstypes.h include in Types.h.  I concur in this need.  Yet if we do that, we have to move all the MOZ_EXPORT_API macros and stuff into Types.h.  I am perfectly willing to do this, but I am opposed to then having to add gratuitous JS_EXPORT_API #defines in some js*.h header just so we can avoid using non-JS_* stuff in our code.  (And presumably have to add extra definitions for every macro that gets added to mfbt/* at any point in time forever into the future.)  Rather than figure out this whole argument now, I'd rather just make forward progress so that people can start affirmatively relying on stdint.h functionality.  We can have that argument in a different bug.

Comment 3

[Jeff Walden [:Waldo]]

Mike, we tried to start using stdint.h in bug 479258 awhile back, and things blew up at your end from it.  We're making another try at this now, hastened by continually running into problems from multiple incompatible definitions of "uint32"-like names, or linker errors from clashing definitions, in Mozilla code.

But this time we think we have a working escape hatch: the user can specify a <stdint.h> of his own making to use, to be compatible with whatever his embedding requires.  Is there any chance you might be able to test out this strategy for us by applying patch 1, then patch 2, and trying to forge ahead with a MOZ_CUSTOM_STDINT_H environment variable containing the path of a <stdint.h> moral replacement?  I can't see a reason why it wouldn't theoretically work, shouldn't need anything more than the time to pull out the definitions the embedding uses (xerces-c, in that bug) into a custom header.  But of course reality trumps theory.  Mind testing this theory out for us?

Comment 4

[Mike Moening]

Sure thing.  I'm been off the bleeding edge for a while now..waiting for MT issues to sort themselves out a bit. Do you know if there is a way to control initial arena pool size for a runtime yet? I think that was the remaining issue for me and why I could not upgrade.  Imagine needing 250-500 runtimes...

Comment 5

[Jeff Walden [:Waldo]]

I don't know offhand, arena pools aren't my thing too much.  And probably best not to get this bug too off-track on that issue regardless.  Mind hopping onto #jsapi so we can figure it out sometime tomorrow or so?  I think I'm out for the day now, but I should be on tomorrow.

Comment 6

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 576049 [details] [diff] [review]
2 - Implement mozilla/StdInt.h, add it to mfbt/Types.h

I think I love you.

>--- a/mfbt/Types.h
>+++ b/mfbt/Types.h
>+/* Expose the standard integer types. */
>+#include "StdInt.h"

Not "mozilla/StdInt.h"? I guess it isn't necessary because they're in the same directory, but might be better to change for consistency with the other files here.

Comment 7

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Comment on attachment 576048 [details] [diff] [review]
1 - Add an MS-compatible stdint.h reimplementation to mfbt without uses (those will come later)

There's a subtle issue here ... we need to ensure that this stdint.h is compatible with the one shipped with VC10.  Binary extensions etc. might be built with any old VC, possibly <VC10.  If gecko is built with VC10 and uses its stdint.h, but binary code is built with another and uses another header, there's potential linkage issues.

Can we use the stdint.h shipped with VC10 verbatim?  (I don't have a VC10 install around to check its license.)  If not, we need to be exceedingly careful that the typedefs match up.

Comment 8

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Comment on attachment 576049 [details] [diff] [review]
2 - Implement mozilla/StdInt.h, add it to mfbt/Types.h

>diff --git a/js/src/jsstdint.h b/js/src/jsstdint.h

>+#include "mozilla/StdInt.h"

\o/

>diff --git a/mfbt/StdInt.h b/mfbt/StdInt.h

>+#if defined(MOZ_CUSTOM_STDINT_H)
>+#  include MOZ_CUSTOM_STDINT_H
>+#elif defined(_MSC_VER) && _MSC_VER < 1600
>+#  include "MSStdInt.h"

Needs to be included through mozilla/MSStdInt.h.

>diff --git a/mfbt/Types.h b/mfbt/Types.h
>--- a/mfbt/Types.h
>+++ b/mfbt/Types.h
>@@ -45,6 +45,9 @@
> #ifndef mozilla_Types_h_
> #define mozilla_Types_h_
> 
>+/* Expose the standard integer types. */
>+#include "StdInt.h"
>+

As Ms2ger notes, this needs to be included through mozilla/, since
mozilla/Types.h can be used anywhere in the codebase.

> /* 
>  * mfbt is logically "lower level" than js/src, but needs basic
>  * definitions of numerical types and macros for compiler/linker

The comment below here starting with 

/*
 * The numerical types provided by jstypes.h that are allowed within
 * mfbt code are
 *

needs to be updated :).

Now is probably a good time to announce this change to
.platform/.js-engine to give embedders some lead time.

r=me with those changes. \o/ \o/

Comment 9

[Gervase Markham [:gerv]]

Comment on attachment 576048 [details] [diff] [review]
1 - Add an MS-compatible stdint.h reimplementation to mfbt without uses (those will come later)

No problem with this licence; please file a bug (CCing me) for you to do the work of adding the text to about:licence in exactly the same way as all the other BSD boilerplates there (in alphabetical order).

Gerv

Comment 10

[Jeff Walden [:Waldo]]

Gerv, the patch with the r? added the license text to that file.  :-)  Re-review for that specifically?

Comment 11

[Jeff Walden [:Waldo]]

[2011-11-22 09:24:31] <Waldo> gerv: ping, bug 704313 comment 10?
[2011-11-22 09:25:50] <gerv> Waldo: ah, sorry, didn't scroll down far enough!
[2011-11-22 09:25:51] <gerv> That's fine.
[2011-11-22 09:25:54] <Waldo> :-)
[2011-11-22 09:26:12] <gerv> r=gerv again.

(with a couple intervening lines omitted)

Comment 12

[Jeff Walden [:Waldo]]

(In reply to Chris Jones [:cjones] [:warhammer] from comment #7)
> There's a subtle issue here ... we need to ensure that this stdint.h is
> compatible with the one shipped with VC10.  Binary extensions etc. might be
> built with any old VC, possibly <VC10.  If gecko is built with VC10 and uses
> its stdint.h, but binary code is built with another and uses another header,
> there's potential linkage issues.

So you're interested in the compatibility of every little bit of the header?  Okay, here goes.

Comparing those particular typedefs first, they facially different, sometimes: this one uses, e.g. "signed __int8" for "int8_t", while the VC10 one uses "signed char" for "int8_t".  But according to MSDN, __int8 is synonymous with char, __int16 with short, __int32 with int, __int64 with long long.  So those are all compatible.

http://msdn.microsoft.com/en-us/library/s3f49ktz%28v=vs.80%29.aspx

Examining further in MS's stdint.h, the {u,}int_leastN_t types are identical to the {u,}intN_t types.  The {u,}int_fastN_t types are equivalent *except* for N = 16.  There they're equivalent to {u,}int32_t, presumably because 16-bit integers are deemed slow, while 8-bit (bytes), 32-bit (words), and 64-bit (dwords) are not.  msstdint is consistent with all this, including the 16-bit exceptional case.

The limit macros are compatible with the quite interesting exception of UINT_FAST16_MAX and INT_FAST16_{MIN,MAX}.  It turns out MSVC2010's stdint.h is buggy here: it defines these to the {U,}INT16_{MIN,MAX} values rather than the {U,}INT32_{MIN,MAX} values!  I checked with gps, and MSVC2011 fixes these to properly be the 32-bit limits.  msstdint.h uses the correct values for {U,}INT_FAST16_{MIN,MAX}; it's not compatible with MSVC2010 (but will be with subsequent versions), but I think this is what we want to do.

In other differences, msstdint only defines the limit macros if __STC_LIMIT_MACROS is defined, but the MSVC stdint.h defines them unconditionally.  I think we want to be compatible with stdint.h on other platforms here, so I think this difference is proper.  msstdint also only defines the constant macros (INT8_C() and the like) if __STDC_CONSTANT_MACROS is defined, while MSVC defines them unconditionally.  I don't think anyone uses the constant macros, but if they do, I think again we want to be compatible with other-platform stdint.h.

The constant macros in msstdint use ##i8, ##ui8, and the like.  MSVC2010 doesn't use these prefixes, doing things a little differently.  This bug report is somewhat interesting on the matter:

https://connect.microsoft.com/VisualStudio/feedback/details/685726/int64-c-and-uint64-c-should-be-defined-in-more-cross-platform-way

Honestly, my head is starting to spin reading all of this, and trying to understand the hyper-nuances of it all.  I don't think it's likely to ever matter; aside from sizeof(UINT16_C(2357)) or something, the difference seems unlikely to be visible unless someone's trying to be super-nitpicky.  And if any difference ever does matter, I think we should fix it then, and not rathole on this any longer now.

There are also seemingly minor differences in exact headers included -- yvals.h, crtdefs.h, and so on from the MS SDKs.  I think we should worry about these problems if and only if someone complains.

More generally, I think we shouldn't worry about incompatibilities in extreme edge cases until someone comes along and says something's wrong.  There's a huge amount of complexity to all this, but in practice the way we use things is not going to depend on this complexity.  My spending an excess of time now on every little edge case (more than I have just now) doesn't seem productive.

> Can we use the stdint.h shipped with VC10 verbatim?  (I don't have a VC10
> install around to check its license.)  If not, we need to be exceedingly
> careful that the typedefs match up.

I don't think so, although I can't actually find actual text of the MSVC license to verify this.  But a couple things of note.

First, remember that the MSVC2010 stdint.h includes other headers which doubtless vary across MSVC versions, so its effects would probably vary if used with 2005/2008/etc.

Second, note that MSVC2011's stdint.h has bugfixes in it, so if we used 2010's, or 2011's, or whatever, we'd lose out on any bugfixes over time.

In sum, I don't think it's really possible, or practical, to exactly replicate the semantics of the one in (which particular version of?) MSVC.  We should just use this, and if we actually ever do encounter problems, we should address them then.

Comment 13

[Jeff Walden [:Waldo]]

Comment on attachment 576048 [details] [diff] [review]
1 - Add an MS-compatible stdint.h reimplementation to mfbt without uses (those will come later)

See comment 12?

Comment 14

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Comment on attachment 576048 [details] [diff] [review]
1 - Add an MS-compatible stdint.h reimplementation to mfbt without uses (those will come later)

If we can't use the VC headers, ok.  I trust MS a lot more than us to maintain backwards compat.

I obviously care more about the common cases than the extreme edges (no one uses uint_least8_t, e.g.).  But I'm confident that due diligence has been done for those.  Thanks.

Comment 15

[Jeff Walden [:Waldo]]

Attachment: 1 - MS-compatible stdint.h (ready for checkin)

Comment 16

[Jeff Walden [:Waldo]]

Attachment: 2 - Implement mozilla/StdInt.h (ready for checkin)

Comment 17

[Jeff Walden [:Waldo]]

I've just uploaded the final, ready-for-checkin patches as the previous two attachments.  They address review comments here, plus they address one or two little things I noticed when doing a last sanity-check of |hg o -p|.  They're ready to go when it's time to push them.

Mike, with the patches reviewed and completed, I'm just waiting for confirmation from you that our approach -- in which you define an environment variable MOZ_CUSTOM_STDINT_H to the path to a stdint.h file compatible with your embedding -- actually does work.  You should be able to take current mozilla-central and apply attachment 576367 [details] [diff] [review], then attachment 576368 [details] [diff] [review], and test against that.

Comment 18

[Jeff Walden [:Waldo]]

I posted about this work in m.d.platform and m.d.t.js-engine, also asking for embedder help to test it out:

http://groups.google.com/group/mozilla.dev.platform/browse_thread/thread/aba1d224fd6442d0

Once I get some feedback that MOZ_CUSTOM_STDINT_H works, in particular, I'll land these patches.  Until then, it seems best to be safe, given that embedders may well be affected by this.

Comment 19

[Jeff Walden [:Waldo]]

Mike, when are you going to have time to test this out?  We'd really like to be able to land this and know any third-party breakage really can be worked around.  But if worse comes to worst we'd likely land and just deal with ensuing bustage after the fact.

Oh, and in case it wasn't clear, this bit in comment 12 should not have been interpreted as having an undertone of annoyance (if indeed anyone interpreted it that way, I have no idea):

(In reply to Jeff Walden (remove +bmo to email) from comment #12)
> So you're interested in the compatibility of every little bit of the header?
> Okay, here goes.

More like "I'm telling you that I'm going to run through everything to cover all the bases".  Although its intent is a bit ambiguous.  :-)

Comment 20

[Jeff Walden [:Waldo]]

Attachment: 2 - Implement mozilla/StdInt.h (slightly unbitrotted)

Comment 21

[Jeff Walden [:Waldo]]

Attachment: Add a don't-define-integer-types frob to libvpx

I was planning on pushing the patches here this morning, but a try-push I did last night turned up Windows build failures.  Apparently libvpx also defines stdint.h types, with conflicts.  That needs fixing before the patches here can land.  (Did a recent change break this in the last two weeks?  I assume so, but I can't actually find the regressing changeset.  Without such a change, tho, I can't explain why these patches built just fine for me previously.  *shrug*  Not really worth figuring out this mystery, seems to me...)

I did some pinging of libvpx people, and one of them seemed cool with the idea of doing a pixman-style VPX_DONT_DEFINE_STDINT_TYPES check around defining the stdint types.  (I've verified that with appropriate changes to the existing patches here, this will work for us.)  There's drudgery involved in getting that change pushed upstream, plus I'm not sure how often we actually sync up with upstream anyway, so we should do this in advance of upstream picking it up.  We want stdint.h sooner than the normal upstream->us updates would allow, I'm sure.

This patch adds an ifdef guard for VPX_DONT_DEFINE_STDINT_TYPES to vpx_integer.h.  It also adds a patch to do this work, and it makes update.sh, used to update the in-tree libvpx, apply the patch.  I expect that'll go away with the next merge -- it can't possibly take that long for the change to be accepted upstream, with whatever naming bikeshedding they want :-) -- but it's probably cleanest to add it all the same.

Comment 22

[Jeff Walden [:Waldo]]

Attachment: 2 - Implement mozilla/StdInt.h (with vpx-induced adjustment)

The changes to content/media/webm/nsWebMReader.h are the only interesting bits here -- rest should be the same as in the slightly-unbitrotted past version.

Comment 23

[Jeff Walden [:Waldo]]

\o/ \o/ \o/

https://hg.mozilla.org/integration/mozilla-inbound/rev/be47924d7a08
https://hg.mozilla.org/integration/mozilla-inbound/rev/76190177b149
https://hg.mozilla.org/integration/mozilla-inbound/rev/987a82806246

I have a blog post queued up about this, will probably push it live in a day or so -- ditto for a newsgroup post.

Comment 24

[Luke Wagner [:luke]]

Nice work!  IIUC, with this in place, we can now (1) nuke the JSInt32 family and remove jsinttypes.h and (2) use stdint.h types everywhere in JS public headers.  Is that right?  If so, is there a bug?

Comment 25

[Jeff Walden [:Waldo]]

This is right, and I've now filed bug 708375 for that matter.

Comment 26

[Jeff Walden [:Waldo]]

Er, bug 708735.

Comment 27

[Jeff Walden [:Waldo]]

Newsgroup posts:

http://groups.google.com/group/mozilla.dev.platform/browse_thread/thread/61624f1ee9fe12b8
http://groups.google.com/group/mozilla.dev.tech.js-engine/browse_thread/thread/007f5163bd1ca02b

And a blog post:

http://whereswalden.com/2011/12/08/party-like-its-1999-stdint-h-comes-to-mozilla/

As comment 24 and comment 25 make clear, I plan to change the JS engine so it uses these new types directly, rather than continuing to use uint32-style names.  I am not planning on changing NSPR types or whatever in Gecko more broadly.  That's bug 579517, and anyone who wants to take that on is more than welcome to do so.

Comment 28

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/be47924d7a08
https://hg.mozilla.org/mozilla-central/rev/76190177b149
https://hg.mozilla.org/mozilla-central/rev/987a82806246

Comment 29

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Waldo, what needs to be documented here?

Comment 30

[Jeff Walden [:Waldo]]

It needs to be added to the SpiderMonkey release notes for the corresponding SpiderMonkey release.  I filed bug 749982 for the release to follow 1.8.7 (placeholder-ly numbered 1.8.8 for now), and I documented the type changes here:

https://developer.mozilla.org/en/SpiderMonkey/1.8.8