Closed Bug 579517 (stdint) Opened 14 years ago Closed 12 years ago

Use stdint types in gecko, replacing usage of NSPR types

Categories

(Core :: General, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla17

People

(Reporter: cjones, Assigned: ehsan.akhgari)

References

(Blocks 1 open bug)

Details

Attachments

(10 files, 2 obsolete files)

1.15 KB, text/plain
Details
4.62 KB, patch
benjamin
: review+
Details | Diff | Splinter Review
731 bytes, patch
benjamin
: review+
Details | Diff | Splinter Review
125.81 KB, patch
benjamin
: review+
Details | Diff | Splinter Review
1.00 KB, text/plain
mconley
: review+
Details
937 bytes, patch
mconley
: review+
Details | Diff | Splinter Review
1.01 KB, text/plain
benjamin
: review+
Details
15.18 KB, patch
benjamin
: review+
Details | Diff | Splinter Review
14.88 KB, patch
Details | Diff | Splinter Review
2.66 KB, patch
ehsan.akhgari
: review+
Details | Diff | Splinter Review
See bug 550610 for rationale.  It seems prudent to nick the build sorcery required for this from jsstdint.h.
To be clear, this would be "Replace NSPR types with stdint types"?
Yes, "Replace not-stdint types with stdint types".
Summary: Use stdint types in gecko → Use stdint types in gecko, replacing usage of NSPR types
I would like this to happen. My reasons:
 1) having fewer types means fewer template specializations, which means smaller executable code
 2) for templates that have to be specialized for each integer types, such as CheckedInt's internal helpers, this would reduce source code complexity
 3) for code that we share with other projects like WebKit, having custom PR types for no good reason is harmful, makes sharing code more difficult than necessary for no benefit. Again, CheckedInt is such an example.
(In reply to Benoit Jacob [:bjacob] from comment #3)
>  1) having fewer types means fewer template specializations, which means
> smaller executable code
>  2) for templates that have to be specialized for each integer types, such
> as CheckedInt's internal helpers, this would reduce source code complexity

Both of these are only true in cases when, say, uint32_t is typedef'd to something different from what PRUint32 is.  We saw that happen on windows for one type, but otherwise that hasn't been the case.

>  3) for code that we share with other projects like WebKit, having custom PR
> types for no good reason is harmful, makes sharing code more difficult than
> necessary for no benefit. Again, CheckedInt is such an example.

+1.  Similarly, developers new to Mozilla are quite likely to have stdint experience, and have a ~0% chance of NSPR experience.
(In reply to Chris Jones [:cjones] [:warhammer] from comment #4)
> (In reply to Benoit Jacob [:bjacob] from comment #3)
> >  1) having fewer types means fewer template specializations, which means
> > smaller executable code
> >  2) for templates that have to be specialized for each integer types, such
> > as CheckedInt's internal helpers, this would reduce source code complexity
> 
> Both of these are only true in cases when, say, uint32_t is typedef'd to
> something different from what PRUint32 is.  We saw that happen on windows
> for one type, but otherwise that hasn't been the case.

Well, currently PR types might or might not be equal to std types. Code like CheckedInt should not be written in a way that makes assumptions in this respect, as that would place an undue burden on anyone wanting to tweak PR types in the future. So from the point of view of CheckedInt, we can't make any assumptions at all and we have to pay a high price even if it's not strictly needed with the current state of PR types.
Here I was having only 2) in mind. You're entirely right about 1).
FWIW, I think we could actually use stdint.h after we switch trunk to MSVC 2010 and drop support for earlier versions of the MSVC (the latter being doable sometime early in 2012, I imagine).
A wrinkle here is compatibility with embedders, who are likely to have their own stdint hacks that aren't compatible with Mozilla's.  js/src/ dances and elaborate dance to not expose stdint types through "public" headers, while using them internally.  If we were to switch to stdint types in XPCOM headers, then we'd have this problem in gecko too.  Possibly worse.

In mfbt/, we punted this problem by using "almost" stdint types, aka protypes, e.g. uint32 instead of uint32_t.  These are safe for internal and public headers (at least as experience with js/src proves).  We'll need to decide which we want in the long run.  I lean towards having one set of types for internal/external, because that's not something I want to think about when writing code.
(In reply to Chris Jones [:cjones] [:warhammer] from comment #8)
We have talked about having:

  // jsstdint.h
  #ifdef JS_NON_STANDARD_INT_H
  # include JS_NON_STANDARD_INT_H
  #else
  # include <stdint.h>
  #endif

allowing embeddings to override stdint.h.  Of course, I might be missing a constraint in this system.

In general, I would reallly like this to be fixed (cf. bug 662852).  For some arcane reason, even uint32/64/etc have linker-based problems that periodically cause bustage!  Thus, right now in SM, the *only* truly safe thing to use in public headers is the universally-agreed-to-be-ugly JSUint32/JSUint64/etc typedefs (which we do inconsistently).  I would be very happy to use stdint.h in public JS headers and nuke the JSUint32 family from orbit.  It would certainly give a clear answer for what int types to use in mfbt/ and drop the dependence of mfbt/ on jstypes.h.
Alright.  Let's give it a shot.

We don't need to wait on dropping support for older MSVC if we do something like
  #if defined(NON_STANDARD_INT_H)
  # include NON_STANDARD_INT_H
  #elif !(HAVE_STDINT_H)
  # include [what's now jsstdint.h]
  #else
  # include <stdint.h>
  #endif
Depends on: 704313
(In reply to Chris Jones [:cjones] [:warhammer] from comment #4)
> Both of these are only true in cases when, say, uint32_t is typedef'd to
> something different from what PRUint32 is.  We saw that happen on windows
> for one type, but otherwise that hasn't been the case.

What types had such a mismatch between the NSPR PR* type and the analogous *_t type? Necko and PSM interface directly with NSPR and NSS, so this would be helpful to know. Also, I would like to change NSPR so that all the NSPR PR* types are typedef'd to the *_t types whenever stdint.h is available, to make NSPR's build system simpler and its IDE support (VS2010+ in particular) better.
We have a separate import of NSPR through ipc/chromium.  IIRC, one version had |typedef long int32|, and the other had |typedef int int32|, which caused linkage problems depending on which NSPR header the decl/defn saw first.

The change you propose to NSPR sounds good, but I'm not sure if it's worth the effort.  Doesn't sound like too much work I guess.
(In reply to Brian Smith (:bsmith) from comment #11)
> (In reply to Chris Jones [:cjones] [:warhammer] from comment #4)
> > Both of these are only true in cases when, say, uint32_t is typedef'd to
> > something different from what PRUint32 is.  We saw that happen on windows
> > for one type, but otherwise that hasn't been the case.
> 
> What types had such a mismatch between the NSPR PR* type and the analogous
> *_t type? 

See bug #634793 and all the bugs blocked by it. I've had enough build failures because of PRInt64 != int64_t (and its unsigned variants)..

Necko and PSM interface directly with NSPR and NSS, so this would
> be helpful to know. Also, I would like to change NSPR so that all the NSPR
> PR* types are typedef'd to the *_t types whenever stdint.h is available, to
> make NSPR's build system simpler and its IDE support (VS2010+ in particular)
> better.
(In reply to Brian Smith (:bsmith) from comment #11)
> What types had such a mismatch between the NSPR PR* type and the analogous
> *_t type?

On Windows {u,}int32_t doesn't align with PR{Ui,I}nt32 -- one's {unsigned, }long, the other's {unsigned, }int.

I have a vague recollection of having to Schlemiel-the-painter my way through some mismatch on Linux as well.  I think it was {u,}int64_t and PR{Ui,I}nt64 not agreeing, although I don't remember with absolute certainty.  But that'd match with the 64-bit-type changes in this patch from bug 708735, certainly (changes I know I made out of necessity):

https://hg.mozilla.org/mozilla-central/rev/d6d732ef5650
Assignee: nobody → ehsan
The bulk of what needs to land here is auto-generated by this script, which I created based on the one I used in bug 690892.  I can also attach a sample patch generated by it but it's huge (~21MB) and I don't think there is much point in reviewing the patch itself.
Attachment #650574 - Flags: review?(benjamin)
This patch makes the IDL parser generate and consume stdint types.  Note that the stdint types will no longer be considered built-in types, and they're resolved using the typedefs in http://mxr.mozilla.org/mozilla-central/source/xpcom/base/nsrootidl.idl which gets rewritten by the script in Part 1.
Attachment #650575 - Flags: review?(benjamin)
We no longer need NSPR types in the IPDL parser to be considered as built-in types, since the IPDL files are rewritten by the script in Part 1.
I tried to be conservative in the auto-conversion script, and that left out a number of mentions of the NSPR types in the code base which I converted over manually after inspection.  This patch should be fairly uninteresting but I'm asking for review on it in case I have made a mistake somewhere.
Attachment #650580 - Flags: review?(benjamin)
Attachment #650576 - Flags: review?(benjamin)
This patch adds #includes for StandardInteger.h where it was missing before (it is #included in nscore.h so it's available in most places in the code base).  Wherever possible, I tried to remove the prtypes.h #include, but we're still using some things from that header so removing it entirely is not possible yet.
Attachment #650586 - Flags: review?(benjamin)
Attachment #650574 - Attachment mime type: application/x-sh → text/plain
Attachment #650572 - Attachment mime type: application/x-sh → text/plain
Comment on attachment 650575 [details] [diff] [review]
Part 2: Make the IDL parser aware of stdint types

Bug 780203 ruined my work here.  I'll give this another shot and see what I need to do here.
Attachment #650575 - Attachment is obsolete: true
Attachment #650575 - Flags: review?(benjamin)
Comment on attachment 650575 [details] [diff] [review]
Part 2: Make the IDL parser aware of stdint types

Actually this is still good!
Attachment #650575 - Attachment is obsolete: false
Attachment #650575 - Flags: review?(benjamin)
Alias: stdint
This script moves comm-central over to stdint types as well.  Let there be a race on who reviews this sooner!
Attachment #650886 - Flags: review?(mbanner)
Attachment #650886 - Flags: review?(irving)
Attachment #650919 - Flags: review?(mbanner)
Attachment #650919 - Flags: review?(irving)
> convert PRIntn int32_t
> convert PRUintn uint32_t

AFAICT, this should be "convert PRintn int" and "convert PRUintn unsigned int". Further, if we want to be really paranoid, we should add a static_assert that (unsigned) int has at least the expected range of 32-bit (un)signed integers.

> convert PROffset32 int32_t

I suggest that we do not do the conversion of PROffset32 to int32_t. PROffset32 represents a 32-bit file offset, and it makes it very obvious that the code is not correct if given large files. converting usages of this type to int32_t makes this potential footgun much harder to recognize.

> convert PROffset64 int64_t

The C++0x type for PROffset64 is std::streamoff. Another common name for this type is off_t. I understand that one potential problem with these types is that they are not required to be 64-bits. IMO, that can probably be solved simply by a static_assert that streamoff or off_t has a signed 64-bit range. But, regardless, I think it would be nice to keep some indication of the intended usage of the type as a file offset in the name of the replacement type.
(In reply to comment #26)
> > convert PRIntn int32_t
> > convert PRUintn uint32_t
> 
> AFAICT, this should be "convert PRintn int" and "convert PRUintn unsigned int".

OK, I'll make this change.

> Further, if we want to be really paranoid, we should add a static_assert that
> (unsigned) int has at least the expected range of 32-bit (un)signed integers.

That's outside of the scope of this bug, I think.  I would be heavily surprised if we're not terribly broken on those hypothetical platforms already!  ;-)

> > convert PROffset32 int32_t
> 
> I suggest that we do not do the conversion of PROffset32 to int32_t. PROffset32
> represents a 32-bit file offset, and it makes it very obvious that the code is
> not correct if given large files. converting usages of this type to int32_t
> makes this potential footgun much harder to recognize.

PROffset32 is only used in two places outside of NSPR and NSS: bsdiff.c where it's used as file offsets among other things, and nsISupportsImpl.h where it is not used as a file offset.  It's very easy to file a bug for the first case (although I'm not sure exactly what I would put in that bug, since we don't use bsdiff for large files), so I will still go ahead and convert PROffset32.

> > convert PROffset64 int64_t
> 
> The C++0x type for PROffset64 is std::streamoff. Another common name for this
> type is off_t. I understand that one potential problem with these types is that
> they are not required to be 64-bits. IMO, that can probably be solved simply by
> a static_assert that streamoff or off_t has a signed 64-bit range. But,
> regardless, I think it would be nice to keep some indication of the intended
> usage of the type as a file offset in the name of the replacement type.

I believe streamoff is only supposed to be used for <iostream> APIs, and StandardInteger.h doesn't support off_t.  And to put things in perspective, PROffset64 is only used three times, all of them being used for the return value of PR_Seek64, so I think I will stick to int64_t for now.
Given that you're leaving PRUptrdiff alone already, I'd just leave the PROffsets alone too; those five places don't really bother use, do they?
(In reply to comment #28)
> Given that you're leaving PRUptrdiff alone already, I'd just leave the
> PROffsets alone too; those five places don't really bother use, do they?

The reason that I am leaving PRUptrdiff out is that there is no stdint type corresponding to an unsigned offset (whatever that means!), it's not a matter of it bothering me!  :-)  I'll wait to see what bsmedberg thinks, I can easily leave them out if needed.
(In reply to Ehsan Akhgari [:ehsan] from comment #27)
> (In reply to comment #26)
> > > convert PRIntn int32_t
> > > convert PRUintn uint32_t
> > 
> > AFAICT, this should be "convert PRintn int" and "convert PRUintn unsigned int".
> 
> OK, I'll make this change.

Please don't. See the dev-platform thread as to why and discuss there if there is to discuss.
(In reply to Ehsan Akhgari [:ehsan] from comment #27)
> PROffset32 is only used in two places outside of NSPR and NSS: bsdiff.c
> where it's used as file offsets among other things, and nsISupportsImpl.h
> where it is not used as a file offset.  It's very easy to file a bug for the
> first case (although I'm not sure exactly what I would put in that bug,
> since we don't use bsdiff for large files), so I will still go ahead and
> convert PROffset32.

Bug 399549 is about upgrading to bsdiff 4.3 which uses off_t in its upstream source code, so no need for a new bug.

> I believe streamoff is only supposed to be used for <iostream> APIs, and
> StandardInteger.h doesn't support off_t.  And to put things in perspective,
> PROffset64 is only used three times, all of them being used for the return
> value of PR_Seek64, so I think I will stick to int64_t for now.

We use off_t in multiple places and we also support ctypes.off_t (bug 757462), so we should make sure off_t works in C++ in a reasonable way and use it, by making sure that off_t is always defined as a 64-bit int in all of our builds. (The patch for ctypes.off_t indicates that is not the case, as it supports 32-bit off_t, which IMO is a bug on its own.) Still, it's no reason to hold up this bug if others don't agree.
(In reply to comment #30)
> (In reply to Ehsan Akhgari [:ehsan] from comment #27)
> > (In reply to comment #26)
> > > > convert PRIntn int32_t
> > > > convert PRUintn uint32_t
> > > 
> > > AFAICT, this should be "convert PRintn int" and "convert PRUintn unsigned int".
> > 
> > OK, I'll make this change.
> 
> Please don't. See the dev-platform thread as to why and discuss there if there
> is to discuss.

I didn't really understand the arguments being made there.  Can you please explain it again?
Sorry, I was confused, I thought we were talking convert PRInt32 int. Forget what I said.
Comment on attachment 650886 [details]
Part 6: c-c conversion script

Stealing review...
Attachment #650886 - Flags: review?(mconley)
Attachment #650886 - Flags: review?(mbanner)
Attachment #650886 - Flags: review?(irving)
Comment on attachment 650919 [details] [diff] [review]
Part 7: Required includes for comm-central

Stealing review...
Attachment #650919 - Flags: review?(mconley)
Attachment #650919 - Flags: review?(mbanner)
Attachment #650919 - Flags: review?(irving)
Ehsan:

The code is fine, but I have two questions:

1) Did you ever get a resolution on how to deal with PRUPtrdiff?

2) Part 4 manually converts to stdint types where the script didn't do the job. Do you think we're going to run into the same issue in comm-central? How did you detect these - just by landing all of these patches and seeing what didn't build?

If so, we might want to do the same for comm-central.

Let me know your position on these two questions, and I'll complete my r? accordingly.

Thanks for looking out for c-c,

-Mike
(In reply to comment #36)
> Ehsan:
> 
> The code is fine, but I have two questions:
> 
> 1) Did you ever get a resolution on how to deal with PRUPtrdiff?

No, and I'm ignoring that for now.

> 2) Part 4 manually converts to stdint types where the script didn't do the job.
> Do you think we're going to run into the same issue in comm-central? How did
> you detect these - just by landing all of these patches and seeing what didn't
> build?

I detected those occurrences by just grepping the sources.  The script only modifies C/C++/Objective-C files automatically, and ignores all the rest.  Whether or not we want to detect the possible remaining usages is a matter of cleanliness, but I'm not planning to do that in this bug.

> If so, we might want to do the same for comm-central.

I will be happy to help someone else do that!  :-)
(In reply to Ehsan Akhgari [:ehsan] from comment #37)
> I detected those occurrences by just grepping the sources.  The script only
> modifies C/C++/Objective-C files automatically, and ignores all the rest. 
> Whether or not we want to detect the possible remaining usages is a matter
> of cleanliness, but I'm not planning to do that in this bug.
> 
> > If so, we might want to do the same for comm-central.
> 
> I will be happy to help someone else do that!  :-)

Ok, I'm running your conversation script locally, and I'll grep around to see if I get any hits.
Comment on attachment 650886 [details]
Part 6: c-c conversion script

Does the job.
Attachment #650886 - Flags: review?(mconley) → review+
Comment on attachment 650919 [details] [diff] [review]
Part 7: Required includes for comm-central

Makes sense from inspection - I haven't tried building any of this, though.
Attachment #650919 - Flags: review?(mconley) → review+
Modified the script to convert PRIntn and PRUintn to int and unsigned, respectively.
Attachment #650574 - Attachment is obsolete: true
Attachment #650574 - Flags: review?(benjamin)
Attachment #653946 - Flags: review?(benjamin)
Attachment #650586 - Attachment is obsolete: true
Attachment #650586 - Flags: review?(benjamin)
Attachment #654090 - Flags: review?(benjamin)
The second version of part 5 seems to give us a green run on the try server.
Attachment #650575 - Flags: review?(benjamin) → review+
Attachment #650576 - Flags: review?(benjamin) → review+
Attachment #650580 - Flags: review?(benjamin) → review+
Attachment #653946 - Flags: review?(benjamin) → review+
Attachment #654090 - Flags: review?(benjamin) → review+
Attachment #653946 - Attachment is patch: false
Removed some of the NSPR types that crept in since yesterday:

https://hg.mozilla.org/integration/mozilla-inbound/rev/ef0db3592a2e
Depends on: 785066
Depends on: 785229
Blocks: 785738
No longer blocks: 785738
Depends on: 785738
Removed some more NSPR types that crept in... https://hg.mozilla.org/integration/mozilla-inbound/rev/d6aeeb24e99e
(In reply to Vicamo Yang [:vicamo][:vyang] from comment #51)
> https://hg.mozilla.org/integration/mozilla-inbound/rev/f0c8020ccf94

Thanks for doing this.  :-)
Everyone is continuing to happily use NSPR types... :(
https://hg.mozilla.org/integration/mozilla-inbound/rev/130e416aefd2
Is there any way to prevent further creeping in?
(e.g. defining poison pills in mfbt)
(In reply to comment #56)
> Is there any way to prevent further creeping in?
> (e.g. defining poison pills in mfbt)

We could do things like that, but they won't be effective for all cases.
(In reply to Ehsan Akhgari [:ehsan] from comment #57)
> (In reply to comment #56)
> > Is there any way to prevent further creeping in?
> > (e.g. defining poison pills in mfbt)
> 
> We could do things like that, but they won't be effective for all cases.

So this patch starts to remove the include prtypes.h from nscore.h. It isn't a complete patch, but gives the basic idea.

I think given that nscore.h (and hence prtypes.h) gets included virtually everywhere, it'd give a much better chance of developers hitting the case where PRInt32 etc isn't defined, and then getting used to using uint32_t etc. If folks then have to start including prtypes.h, that's an additional red flag to reviewers.

Yes, we'd still need to include it in various places, and I haven't done any analysis on the amount of places that it would be not included with a complete patch, but I think it'd be a start to helping reducing the familiarity with the old types.
(In reply to Mark Banner (:standard8) from comment #59)
> Created attachment 656791 [details] [diff] [review]
> Possible way of reducing recurrances
> 
> (In reply to Ehsan Akhgari [:ehsan] from comment #57)
> > (In reply to comment #56)
> > > Is there any way to prevent further creeping in?
> > > (e.g. defining poison pills in mfbt)
> > 
> > We could do things like that, but they won't be effective for all cases.
> 
> So this patch starts to remove the include prtypes.h from nscore.h. It isn't
> a complete patch, but gives the basic idea.
> 
> I think given that nscore.h (and hence prtypes.h) gets included virtually
> everywhere, it'd give a much better chance of developers hitting the case
> where PRInt32 etc isn't defined, and then getting used to using uint32_t
> etc. If folks then have to start including prtypes.h, that's an additional
> red flag to reviewers.

This is a great idea!  I was approaching the problem the other way around (trying to reduce the stuff from prtypes.h that we use around the code base and then try to remove the prtypes.h includes), but this makes much more sense!

> Yes, we'd still need to include it in various places, and I haven't done any
> analysis on the amount of places that it would be not included with a
> complete patch, but I think it'd be a start to helping reducing the
> familiarity with the old types.

Yeah, I agree.  Can you please make this patch compile, and file another bug and attach it there?  I would be happy to review it.
Depends on: 788012
Blocks: 788014
(In reply to Ehsan Akhgari [:ehsan] from comment #60)
> Yeah, I agree.  Can you please make this patch compile, and file another bug
> and attach it there?  I would be happy to review it.

Filed bug 788014, but I won't have time to work on it for the next few weeks.
Blocks: 792688
Backed that out in https://hg.mozilla.org/integration/mozilla-inbound/rev/1424662e3136 to back out one of the things it was cleaning up after.
Blocks: 794186
Depends on: 815359
Blocks: 828003
Remove a couple PRBools from dom/ and xpcom/.

https://tbpl.mozilla.org/?tree=Try&rev=982ac5145786
Attachment #733194 - Flags: review?(ehsan)
Comment on attachment 733194 [details] [diff] [review]
remove-some-PRBools.patch

Review of attachment 733194 [details] [diff] [review]:
-----------------------------------------------------------------

Technically this is not the PRBool bug, but whatever.  ;-)

Thanks!
Attachment #733194 - Flags: review?(ehsan) → review+
Nit on the cset in comment 96:
>-  PRIntn openFlags;
>+  int openFlags;
[...skipping a bunch of lines...]
>   rv = path->OpenNSPRFileDesc(openFlags, 0644, &mFileDesc);

It looks like the type of that OpenNSPRFileDesc parameter is technically int32_t (not int), based on the function signatures at the top of this MXR search:
 http://mxr.mozilla.org/mozilla-central/ident?i=OpenNSPRFileDesc
Flags: needinfo?(ehsan)
(In reply to Daniel Holbert [:dholbert] from comment #97)
> Nit on the cset in comment 96:
> >-  PRIntn openFlags;
> >+  int openFlags;
> [...skipping a bunch of lines...]
> >   rv = path->OpenNSPRFileDesc(openFlags, 0644, &mFileDesc);
> 
> It looks like the type of that OpenNSPRFileDesc parameter is technically
> int32_t (not int), based on the function signatures at the top of this MXR
> search:
>  http://mxr.mozilla.org/mozilla-central/ident?i=OpenNSPRFileDesc

Yes.  Blame whoever wrote the code using PRIntn.  :-)

Fixed in https://hg.mozilla.org/integration/mozilla-inbound/rev/0beea724bb11
Flags: needinfo?(ehsan)
Pushed by frgrahl@gmx.net:
https://hg.mozilla.org/comm-central/rev/8155c0ef40e6
Part 1: Automated conversion of NSPR numeric types to stdint types in Gecko; r=bsmedberg
You need to log in before you can comment on or make changes to this bug.