708735 - Use <stdint.h> types in JSAPI, remove {u,}int* and JS* integer types

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jeff Walden [:Waldo]]

Part of the goal of bug 704313 was to use <stdint.h> types in the JSAPI.  We now use those types as the underlying definitions for uint8/16/32/64 and so on, which are currently just typedef aliases.  But to really avoid the problems that motivated that bug, we need to not define uint8/16/32/64 so we don't conflict with anyone.  And since the JSUint8/16/32/64 and so on types are no longer necessary, as we're straightforwardly depending on <stdint.h> types, we should remove those too.

Comment 1

[Luke Wagner [:luke]]

Totally agreed.  IIUC, then we will finally have a somewhat simple story:
 1. stdint types for headers in js/public
 2. stdint-minus-_t types for everything in js/src

In particular js/public (and its transitive includes) should not include anything that #defines the stdint-minus-_t types to avoid unwanted collisions in the embedding.  (The stdint-minus-_t types will still leak out due to INSTALLED_HEADERS, but once the stop-exposing-our-privates effort concludes (currently blocked on bug 677079, I think), then they should not be in any INSTALLED_HEADERS.)

I'm mostly saying this to double-check that this is indeed the plan?

Comment 2

[Jeff Walden [:Waldo]]

That sounds more or less right.

Comment 3

[Jeff Walden [:Waldo]]

Attachment: Patch

This:

1) takes a scythe to jsapi.h, jspubtd.h, jsxdrapi.h, jspubtd.h, and every other public header included by jsapi.h, changing every fixed-size type to the <stdint.h> version,
2) adds js/public/LegacyIntTypes.h with typedefs for the old names (including the JS* names, since this header is intended for embedder use), which SpiderMonkey-internal files still use sometimes and which I expect embedders will use temporarily until they can adapt to the <stdint.h> names,
3) and fixes up all gazillion one-off places where removing the JSAPI typedefs means other, incompatible, typedefs get used -- so the JSAPI uses uint32_t=unsigned int, say, which means NSPR's uint32=unsigned long shines through to the user and you either get a bad-pointer cast or a linker error.

This is undoubtedly a bit ugly.  I mostly sprinkled LegacyIntTypes.h includes where they were needed until things compiled.  So there's probably some #include bootlegging.  I propose these be fixed as we encounter them, because it is as yet unclear to me what "right" style would be for making this work.

I also feel obligated to state for the record that I still don't think we should mix types this way.  Much of the benefit of standard types is that there's no thought required in deciding what to use.  Having a second typedef does away with that.  And it means we'll have to think about which typedef to use in what circumstances.  It's perhaps easy enough to state a rule that friend APIs use uint32_t, everything else uses uint32.  Making sure we get that right in practice will be tricky; it's a subtle thing for review to catch, and I'm not confident we'll do it well (and will only find out when Windows blows up) for awhile.

Also, pretending we can use the non-<stdint.h> typedefs in "internal" headers that really aren't is a good way to court the same compile errors that motivated all the changes to random files in this patch.  It's particularly worth noting that we can't restrict the <stdint.h> types to just public headers.  They have to be used for all exposed API, which also means friend declarations -- spread across many non-public files.  They also have to be used for certain internal headers included by jsapi-test/tests.h -- this is necessary so that jsapi-tests/testIntTypesABI.cpp can be written (that test actually was practically useful in writing this patch).  (Trimming tests.h's didn't look especially easy, although I might be mistaken about that.)  Preserving *this* requirement will be even harder than preserving the friend-APIs-use-<stdint.h>-types requirement.

It's also worth noting the in-engine/out-of-engine boundary is sometimes a bit blurry on this point.  Is js/src/perf in-engine?  What about js/src/ctypes?

But anyway.  This is an improvement on the status quo in terms of getting Mozilla to One Standard Definition for the fixed-size types, and in the long run type incompatibilities will disappear.  It's still fewer incompatibilities, even if it's not fewest.

This patch builds locally for me, and it builds with VC9 and VC10 on Windows.  Try should report on its bill of health by morning.

Comment 4

[Luke Wagner [:luke]]

(In reply to Jeff Walden (remove +bmo to email) from comment #3)
Wow, that is a lot remaining sadness, all caused by the uint32 family.

Active SM hackers: does anyone violently object (in the face of the issues raised in comment 3) to just removing the uint32 (and, of course, JSUint32) families of typedefs?  It sounds like the platform-specific type-conflict weirdness will only continue until there is only <stdint.h>.

I, for one, wouldn't mind <stdint.h>.  It's just two extra chars; it's just in the decls; it matches size_t and ptrdiff_t; and it's what I expected (before facing the bewildering reality) when I first started hacking on SM.  I think the last point is a particular bonus.

Comment 5

[Luke Wagner [:luke]]

Forgot some obvious CCs.

Comment 6

[David Mandelin [:dmandelin]]

Comment on attachment 580322 [details] [diff] [review]
Patch

Waiting for more info on approach before reviewing.

Comment 7

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

(In reply to Luke Wagner [:luke] from comment #4)
> to just removing the uint32 (and, of course, JSUint32)
> families of typedefs?  It sounds like the platform-specific type-conflict
> weirdness will only continue until there is only <stdint.h>.

+1 -- IMO it became a C99 standard for good reason.

Comment 8

[Nicholas Nethercote [inactive]]

Two extra chars per typename is a small price to pay for avoiding all manner of subtle problems, IMO.

Comment 9

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

Comment on attachment 580322 [details] [diff] [review]
Patch

>--- a/js/src/jsxdrapi.h
>+++ b/js/src/jsxdrapi.h
> typedef struct JSXDROps {
>-    JSBool      (*get32)(JSXDRState *, uint32 *);
>-    JSBool      (*set32)(JSXDRState *, uint32 *);
>-    JSBool      (*getbytes)(JSXDRState *, char *, uint32);
>-    JSBool      (*setbytes)(JSXDRState *, char *, uint32);
>-    void *      (*raw)(JSXDRState *, uint32);
>-    JSBool      (*seek)(JSXDRState *, int32, JSXDRWhence);
>-    uint32      (*tell)(JSXDRState *);
>+    JSBool      (*get32)(JSXDRState *, uint32_t *);
>+    JSBool      (*set32)(JSXDRState *, uint32_t *);
>+    JSBool      (*getbytes)(JSXDRState *, char *, uint32_t);
>+    JSBool      (*setbytes)(JSXDRState *, char *, uint32_t);
>+    void *      (*raw)(JSXDRState *, uint32_t);
>+    JSBool      (*seek)(JSXDRState *, int32_t, JSXDRWhence);
>+    uint32_t      (*tell)(JSXDRState *);

Indentation is off here.

Comment 10

[Luke Wagner [:luke]]

Sounds like consensus to me.  Waldo, perhaps you'd be interested in writing the revised patch?

Comment 11

[Jeff Walden [:Waldo]]

Sure, now working on the patch to just use _t names everywhere.

Incidentally, in the process of doing that, I've fixed up yet more JS_FRIEND_API stuff declared with the uint32-style names.  So that problem of wrongly-linked stuff would be with us even after the patch posted here....  Those weren't detected, it appears, because nobody took the address of them, and/or I didn't notice them when manually fixing up friend declarations I noticed to be using the short types.

Comment 12

[Jeff Walden [:Waldo]]

...and it seems I didn't even properly fix up all the public API points, either, judging by jsapi.h changes a global s&r is finding!  Fun times.

Comment 13

[Jeff Walden [:Waldo]]

Attachment: Change the engine to use the <stdint.h> types only, too

This is an extra patch which, applied atop the previous one (or against it rebased not too heavily, don't remember if I had to do that), makes the shell and browser both buildable on Linux64 debug with Clang -- posting mostly for archival purposes.  It's a combination of a bunch of find/perling with various patterns to avoid disturbing existing patterns (like just-so aligned field names) too much, plus a few manual changes in places.

My tree's based on m-i, so these patches are several days out of date now.  I see no real reason to rush this, so I'm going to leave this all in a holding pattern until the current m-c/mi-i mess gets sorted out, I've tested other platforms (looking at you, Windows) and they can be landed, and not bother with getting a review of some sort yet.  (And if the delay extends long enough, I'll probably wait til after the next merge as well, but no need to get too far ahead of myself here.)  Plus, I'm pretty sure the review will want to be of a unified patch, since a lot of lines touched in the first get touched in the second as well.

But anyway: unless you're morbidly curious, nothing to see here, move along...

Comment 14

[Jeff Walden [:Waldo]]

Attachment: Combined patch, works on Windows assuming some rebasing since last night didn't break it

I sent mail to .platform about this change, warning Gecko developers of it and explaining how they can update the patches they're working on to cope:

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

This built on Windows with vc9/10 last night.  It probably builds on Linux as well, since the changes in this versus the previous patches are only additive, but I'll make sure to test before landing.  I think it's ready to go now.

Comment 15

[Luke Wagner [:luke]]

Comment on attachment 582022 [details] [diff] [review]
Combined patch, works on Windows assuming some rebasing since last night didn't break it

Righteous.

>diff --git a/js/xpconnect/wrappers/CrossOriginWrapper.h b/js/xpconnect/wrappers/CrossOriginWrapper.h
>+#include "mozilla/Attributes.h"

I'd prefer if we tried to keep this patch single-minded and not add any of these MOZ_OVERRIDEs.

Comment 16

[Jeff Walden [:Waldo]]

I added those because if they'd been there originally, my need to change their signatures would have been evident at compile time, not link time.  (I think uint32 was getting inherited from a non-JS header, and uses would compile against that non-JS uint32, then things would only die at link time when trying to match up call sites against the JS engine's uint32_t signatures.)  Is that reasonable?

Comment 17

[Jeff Walden [:Waldo]]

Hm, although, looking now, I can't see the arguments that were of those types.  Still, I definitely remember that change happening because of some sort of overriding issue.  But I can remove it from the patch here, I guess.

Comment 18

[Jeff Walden [:Waldo]]

https://hg.mozilla.org/mozilla-central/rev/d6d732ef5650
https://hg.mozilla.org/mozilla-central/rev/f879c54dffae
https://hg.mozilla.org/mozilla-central/rev/80ac06ad280d
https://hg.mozilla.org/mozilla-central/rev/3bf9653c4e46
https://hg.mozilla.org/mozilla-central/rev/7cc472108eb4

Comment 19

[Marco Bonardo [:mak]]

this is likely a follow-up:
https://hg.mozilla.org/mozilla-central/rev/cbe10cc8c219