Closed Bug 465640 Opened 11 years ago Closed 11 years ago

Use autoconf to declare stdint types on platforms that don't have stdint.h

Categories

(Firefox Build System :: General, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: jorendorff, Assigned: jimb)

References

Details

(Keywords: fixed1.9.1)

Attachments

(1 file, 4 obsolete files)

nanojit uses stdint.h, which still isn't available on all platforms.

We don't need all the types defined in C99.  I'm thinking of {u,}int{8,16,32,64,ptr}_t (and whatever else nanojit uses, if anything).
Blocks: 465641
At configure time, check for <stdint.h>.  If we don't have it, observe
the sizes of various integer types.  New header js/src/jsstdint.h
includes <stdint.h> if it's available, or uses the sizes from
configure.in to define the {,u}int{8,16,32,64,ptr}_t types itself
otherwise.

Note that since these types are used in the public JavaScript API, the
configure script needs to place the definitions it finds in
js-config.h, the installed configure-generated header, so it can be
used by jsapi.h and that gang.
Jason, the attached patch should give you definitions for {,u}int{8,16,32,64,ptr}_t on all platforms.  Let me know if it's good enough for what you need.

Oh, I think it'll make 'make check' fail because I didn't add js/src/build/autoconf/mozsizeof.m4 to build/autoconf as well.  I'll have to revise that.  But it shouldn't interfere with converting js/src to use the new types.
Comment 1 makes the point that API users have been given public typedefs such as jsint, uint32, etc., for ever, via jsapi.h. We should not break API compat there, even if we make our internal code use the stdint names.

/be
(In reply to comment #3)

Agreed.  This patch doesn't (and shouldn't) delete any types or redefine any existing types.
Assignee: general → nobody
Component: JavaScript Engine → Build Config
QA Contact: general → build-config
Assignee: nobody → jorendorff
Is this patch ready for review?
(In reply to comment #2)
> Jason, the attached patch should give you definitions for
> {,u}int{8,16,32,64,ptr}_t on all platforms.  Let me know if it's good enough
> for what you need.

Yes, it's good enough!
Jim, please feel free to request review.
Assignee: jorendorff → jim
[Revised as suggested in comment 2.]

At configure time, check for <stdint.h>.  If we don't have it, observe
the sizes of various integer types.  New header js/src/jsstdint.h
includes <stdint.h> if it's available, or uses the sizes from
configure.in to define the {,u}int{8,16,32,64,ptr}_t types itself
otherwise.

Note that since these types are used in the public JavaScript API, the
configure script needs to place the definitions it finds in
js-config.h, the installed configure-generated header, so it can be
used by jsapi.h and that gang.
Attachment #349057 - Attachment is obsolete: true
Attachment #353247 - Flags: review?(crowder)
Comment on attachment 353247 [details] [diff] [review]
Bug 465640: Use autoconf to declare stdint types on platforms that don't have stdint.h

This looks good to me; though, I've not tried building with it on WinCE yet.  Also, ted or bsmedberg should probably look more closely as the AC/M4 stuff.
Attachment #353247 - Flags: review?(crowder) → review+
Comment on attachment 353247 [details] [diff] [review]
Bug 465640: Use autoconf to declare stdint types on platforms that don't have stdint.h

Hi, Ted.  How does the Autoconf stuff look?
Attachment #353247 - Flags: review+ → review?(ted.mielczarek)
Jim, if you use the "Add'l Review" field (on the attachment details page) to request another review, it won't erase the previous r+.
Attachment #353247 - Flags: review+
Ah, okay --- thanks!
Comment on attachment 353247 [details] [diff] [review]
Bug 465640: Use autoconf to declare stdint types on platforms that don't have stdint.h

+dnl The Initial Developer of the Original Code is
+dnl Jim Blandy
+dnl Portions created by the Initial Developer are Copyright (C) 2008
+dnl the Initial Developer. All Rights Reserved.
+dnl
+dnl Contributor(s):

I believe standard practice is to list "The Mozilla Foundation" as original author (since they hold copyright on our work), and yourself under contributors.

Overall this looks good, although I will admit that I'm not an m4 expert. (is anyone?) I do wonder if you could tie this in using the configure_static_assert macros (shorthand for the ternary-operator-in-array-size-typedef trick you're using here), but it may not be worth the effort, and there's already a configure test at that point to ensure that the trick works (we error out if it doesn't).

My only qualm is that it feels like the #error conditions in jsstdint.h ought to be happening in configure. If your system isn't going to compile our code, it ought to error in configure, not in a random header file. Do you think you could rework it to have those conditions error out in configure instead?
IRC conversation: configure should fail, and the #error directives should stay in jsstdint.h as sanity checks.
The CONFIGURE_STATIC_ASSERT stuff is headed in the right direction, in that there's no reason to be writing out the array magic each time, and in trying to ensure some decent error messages result from it.

But the way it's done in configure.in makes it hard to use reliably from different .m4 files.  The setup should be pulled out into its own M4 macro, and AC_REQUIRE/AC_PROVIDE used to make sure everything gets run.

Or, alternatively, the whole thing might be able to be converted into an M4 macro that expands to the C code.  There's a slim chance it might even be able to generate configure.in line numbers, if I remember right.

But I don't think this is worth doing right now.
[Revised to error out at configure time if we can't find the types we expect.]

At configure time, check for <stdint.h>.  If we don't have it, observe
the sizes of various integer types.  New header js/src/jsstdint.h
includes <stdint.h> if it's available, or uses the sizes from
configure.in to define the {,u}int{8,16,32,64,ptr}_t types itself
otherwise.

Note that since these types are used in the public JavaScript API, the
configure script needs to place the definitions it finds in
js-config.h, the installed configure-generated header, so it can be
used by jsapi.h and that gang.
Attachment #353247 - Attachment is obsolete: true
Attachment #353891 - Flags: review?(ted.mielczarek)
Attachment #353247 - Flags: review?(ted.mielczarek)
Attachment #353891 - Flags: review?(ted.mielczarek) → review+
Comment on attachment 353891 [details] [diff] [review]
Bug 465640: Use autoconf to declare stdint types on platforms that don't have stdint.h

Looks good, thanks!
Blocks: 470791
Comment on attachment 353891 [details] [diff] [review]
Bug 465640: Use autoconf to declare stdint types on platforms that don't have stdint.h

Sorry, guys.  This is totally borked on Windows.  Suggest we obsolete that platform.
Attachment #353891 - Attachment is obsolete: true
[Ben --- note especially the way the patch deals with Windows, hard-coding answers to questions configure is expected to answer.  If we can't actually run compilation tests in configure, I'm not sure what else would be better.  Comments welcome.]

At configure time, check for <stdint.h>.  If we don't have it, observe
the sizes of various integer types.  On Windows, where we can't run
compilation tests in configure, hard-code definitions suggesting the
use of the built-in __intN types and <stddef.h> for the pointer-sized
types.

Note that since these types are used in the public JavaScript API, the
configure script needs to place the definitions it finds in
js-config.h, the installed configure-generated header, so it can be
used by jsapi.h and that gang.

New header js/src/jsstdint.h does what it takes to get the right
definitions.  It includes <stdint.h> if it's available, uses the sizes
from configure.in to define the {,u}int{8,16,32,64,ptr}_t types
itself, or uses the __intN types and the <stddef.h> header.

Remove now-extraneous and possibly conflicting definitions of intN_t
types from js/src/nanojit/avmplus.h.
Attachment #355830 - Flags: review?(benjamin)
Now that we have these standard names, can we get rid of jscpucfg? See bug 269538, bug 471059, bug 470071. I *think* the only thing we don't have is stack-grown-direction... and perhaps we can just hard-code the OS/CPUs that have upwards growth?
I think getting rid of jscpucfg is covered by bug 467453.
(In reply to comment #20)
> Now that we have these standard names, can we get rid of jscpucfg? See bug
> 269538, bug 471059, bug 470071. I *think* the only thing we don't have is
> stack-grown-direction... and perhaps we can just hard-code the OS/CPUs that
> have upwards growth?

Downward. But see bug 192414 on upward-stack-growth oddball architectures.

/be
Ben would like the axes rotated for the tests, so configure produces things like

-DJS_INT32_TYPE=int

Something like MOZ_TYPE_SIZE(JS_, 32, [short, int, long int])
but not quite.  "If that's not too hard, I think it's preferable"
Attachment #355830 - Flags: review?(benjamin)
Comment on attachment 355830 [details] [diff] [review]
Bug 465640: Use autoconf to declare stdint types on platforms that don't have stdint.h

Speaking with jimb briefly, I'd *like* to see this try along the other axis, e.g.

MOZ_TYPE_SIZE(JS_, unsigned, 32, ["int", "short", "long"]);
[Revised per Ben's suggestions.]

At configure time, check for <stdint.h>.  If we don't have it, find
integer types of various sizes.  On Windows, where we can't run
compilation tests in configure, hard-code definitions suggesting the
use of the built-in __intN types for the exact-size types, and
<stddef.h> for the pointer-sized types.

Use namespace-clean names for the preprocessor macros we define.
Since these types are used in the public JavaScript API, the configure
script needs to place the definitions it finds in js-config.h, the
installed configure-generated header, so it can be used by jsapi.h and
that gang.

New header js/src/jsstdint.h does what it takes to get definitions for
the exact-size and pointer-size integral types.  It includes
<stdint.h> when available, uses the types found by configure.in to
define the {,u}int{8,16,32,64,ptr}_t types itself, or uses the __intN
types and the <stddef.h> header.

Remove now-unnecessary and possibly conflicting definitions of intN_t
types from js/src/nanojit/avmplus.h.
Attachment #355830 - Attachment is obsolete: true
Attachment #356204 - Flags: review?(benjamin)
Comment on attachment 356204 [details] [diff] [review]
Bug 465640: Use autoconf to declare stdint types on platforms that don't have stdint.h

woot
Attachment #356204 - Flags: review?(benjamin) → review+
Landed in M-C: http://hg.mozilla.org/mozilla-central/rev/557bc4bcdc30
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Just out of curiosity: Are there plans to use stdint types throughout the whole mozilla tree?
Flags: in-testsuite-
Flags: in-litmus-
I think it depends on how well this turns out, and how enthusiastic the maintainers of that code are.  We're not going to mess with NSPR or NSS, certainly.  js/src/nanojit is already using the <stdint.h> types, and js/src also seems to be very deliberate about value sizes, so it's not a hard call there.
More standard-looking code, all else equal, would be good. Some object to the _t suffix (I'm an old kernel hacker, so used to it). But really, changing the window dressing is low priority. It risks messing with indentation style; it requires reviewer bandwidth. It's not automatable.

Mozilla has never imposed a single, detailed code style on all original code, and we won't start. Using standard types is generally winning, although typedefs have their place too. I'm just happy we have stdint.h support in 2009 -- we thought it would have to wait for the next decade.

/be
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/6edbb7b663b3
Pushed to 1.9.1 as a part of the sequence of patches for bug 470071
Keywords: fixed1.9.1
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.