796941 - (dieprtypesdie) Tracking bug for removal of the prtypes.h inclusions in our tree

Status: NEW

(Core :: General, defect)

Description

[(no longer active)]

prtypes.h is quite poisonous, it lets people type things like PRInt32, PR_BEGIN_MACRO, PR_END_EXTERN_C, etc.  I've been on a secret war against prtypes.h first by working on bug 579517 and then by filing many mentored bugs to remove other things in prtypes.h that our code depends on.  So I figured it's time to track this undercover project.

Comment 1

[(no longer active)]

Oh, forgot to say that the only thing in prtypes.h that we probably need to depend on (unless Waldo finds a way out of that!) is PRUnichar, but once we get to a point where that is our only dependency, we can just put that in our header.

Comment 2

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

There is an increasingly large number of Gecko files that include NSS files. And, Necko tends to include NSPR files. Because the NSPR and NSS headers all use prtypes, this means that these Gecko files will continue to have the "problems" caused by prtypes.h, unless we change the NSPR and NSS headers.

If we are willing to contribute our StandardInteger.h implementation to NSPR, then potentially we could have all the NSPR and NSS headers modified to use the standard integer types. I am not sure that Wan-Teh and Bob would accept such a change. It would be good if somebody could describe, succinctly, the benefit of converting from prtypes to ISO C/C++ types in the first place, so that I can approach them about the issue. But, I don't even want to bother with the issue if we're not willing to contribute StandardInteger.h to NSPR in the first place.

Comment 3

[(no longer active)]

(In reply to comment #2)
> There is an increasingly large number of Gecko files that include NSS files.
> And, Necko tends to include NSPR files. Because the NSPR and NSS headers all
> use prtypes, this means that these Gecko files will continue to have the
> "problems" caused by prtypes.h, unless we change the NSPR and NSS headers.
> 
> If we are willing to contribute our StandardInteger.h implementation to NSPR,
> then potentially we could have all the NSPR and NSS headers modified to use the
> standard integer types. I am not sure that Wan-Teh and Bob would accept such a
> change. It would be good if somebody could describe, succinctly, the benefit of
> converting from prtypes to ISO C/C++ types in the first place, so that I can
> approach them about the issue. But, I don't even want to bother with the issue
> if we're not willing to contribute StandardInteger.h to NSPR in the first
> place.

The benefit we're looking for is mostly code cleanliness, and also compatibility with new code that we import into mozilla-central.  Most C++ programmers have heard of stdint types these days, and we have been looking into ways for lowering the barrier to entry for Mozilla development to newcomers to the mozilla code base.  Using things like NSPR boolean, integral and character types hurts us in that regard.

I would be more than willing to upstream StandardInteger.h into NSPR, and I would even be willing to write the patch if there is any interest on the NSPR side.  But in my experience, NSPR mostly resists these types of changes.  Do you have an indication that they might be interested in something like StandardInteger.h?  Also, would adding StandardInteger.h to NSPR have any benefit without us being able to use stdint types in their headers?  I'm quite certain that the response to a request for such a change to be no from the NSPR folks.  :(

Comment 4

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

The point of contributing StandardInteger.h to NSPR would be to replace the usage of PRTypes everywhere in NSPR and NSS.

I will ask people how they feel about making this change. I am not as pessimistic as others are about it.

Is it the case that every ANSI C/C++ standard ***_t type name is the same length as the corresponding NSPR PR*** type? In other words, would this patch affect the indention or line length of the source code in any way? What is the exact correspondence between NSPR and ISO C/C++ types? 

One possibility would be that we create a new NSPR header, that contains things like this:

#define PRUint8 uint8_t
#define PRUint16 uint16_t

and then another one that does

#undef PRUint8
#undef PRUint16

and then, in every NSPR (and/or NSS) header, we include the former header at the start, and include the latter header at the end. This way, the PR*** names never pollute the namespace of applications that use NSPR, and NSPR and NSS can avoid mass find/replace like we did in mozilla-central.

But, that sounds like more work than just doing the mass find/replace.

One thing to keep in mind is that Google has some private patches to NSS and NSPR that would break with this change. So, giving them a reliable script that rewrites their custom patches would go a long way to making it easier on them, as would helping them get their custom patches checked into the NSPR and NSS trunks.

Comment 5

[(no longer active)]

(In reply to comment #4)
> The point of contributing StandardInteger.h to NSPR would be to replace the
> usage of PRTypes everywhere in NSPR and NSS.
> 
> I will ask people how they feel about making this change. I am not as
> pessimistic as others are about it.
> 
> Is it the case that every ANSI C/C++ standard ***_t type name is the same
> length as the corresponding NSPR PR*** type? In other words, would this patch
> affect the indention or line length of the source code in any way? What is the
> exact correspondence between NSPR and ISO C/C++ types? 

Well, in bug 634793 comment 3, wtc has said that he doesn't believe that PRUint64 and uint64_t should be considered the same type, and I can sort of extrapolate on that to say that he probably doesn't agree that the rest of the corresponding types should be the same.

About the question of textual size, PR{I,Ui}nt{\d\d} is the same textyal size as {,u}int{\d\d}_t.  This property doesn't hold on all of the types that we replaced though (see attachment 653946 [details] for a full list).

> One possibility would be that we create a new NSPR header, that contains things
> like this:
> 
> #define PRUint8 uint8_t
> #define PRUint16 uint16_t
> 
> and then another one that does
> 
> #undef PRUint8
> #undef PRUint16
> 
> and then, in every NSPR (and/or NSS) header, we include the former header at
> the start, and include the latter header at the end. This way, the PR*** names
> never pollute the namespace of applications that use NSPR, and NSPR and NSS can
> avoid mass find/replace like we did in mozilla-central.
> 
> But, that sounds like more work than just doing the mass find/replace.

I think NSPR should keep the callers using the NSPR types working.  But really, the work involved in this is not that complicated, it's mostly a matter of whether they want to do this.  (I'm willing to help if the answer is yes!)

> One thing to keep in mind is that Google has some private patches to NSS and
> NSPR that would break with this change. So, giving them a reliable script that
> rewrites their custom patches would go a long way to making it easier on them,
> as would helping them get their custom patches checked into the NSPR and NSS
> trunks.

Sure, that's fair.  We provided our own developer with such a script, and I can happily do the same for those folks.

Comment 6

[Joshua Cranmer [:jcranmer]]

(In reply to Ehsan Akhgari [:ehsan] from comment #5)
> Well, in bug 634793 comment 3, wtc has said that he doesn't believe that
> PRUint64 and uint64_t should be considered the same type, and I can sort of
> extrapolate on that to say that he probably doesn't agree that the rest of
> the corresponding types should be the same.

Would it be possible to convince him to add a configure-time option (or similar) to typedef NSPR types to the equivalent C99 type if available? The complaints I see are mostly that changing the underlying types could break backwards compatibility in typedef, so if it's a configure-time (or even an include-time) option, that should provide all the necessary things for backwards compatibility...

Comment 7

[Mike Hommey [:glandium]]

(In reply to Joshua Cranmer [:jcranmer] from comment #6)
> Would it be possible to convince him to add a configure-time option (or
> similar) to typedef NSPR types to the equivalent C99 type if available? The
> complaints I see are mostly that changing the underlying types could break
> backwards compatibility in typedef, so if it's a configure-time (or even an
> include-time) option, that should provide all the necessary things for
> backwards compatibility...

On Linux systems, and especially fedora, the nspr that comes with Firefox may be used by some of the system libraries Firefox itself loads, because these system libraries normally use a system nspr. As a consequence, the nspr that comes with Firefox *needs* to be binary compatible with the system one.

Comment 8

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

(In reply to Mike Hommey [:glandium] from comment #7)
> On Linux systems, and especially fedora, the nspr that comes with Firefox
> may be used by some of the system libraries Firefox itself loads, because
> these system libraries normally use a system nspr. As a consequence, the
> nspr that comes with Firefox *needs* to be binary compatible with the system
> one.

On Linux, is there any case where #defining or typedefing the NSPR types to their standard C/C++ counterparts changes the ABI?

My understanding is that the compatibility issues are purely at the source-code compatibility level, not at the ABI level. I think it is reasonable to break source-code compatibility with some small amount of code, that is trivial to fix, as long as we're not breaking binary compatibility in any way.

Comment 9

[Joshua Cranmer [:jcranmer]]

(In reply to Mike Hommey [:glandium] from comment #7)
> On Linux systems, and especially fedora, the nspr that comes with Firefox
> may be used by some of the system libraries Firefox itself loads, because
> these system libraries normally use a system nspr. As a consequence, the
> nspr that comes with Firefox *needs* to be binary compatible with the system
> one.

So long as sizeof(PRInt64) == sizeof(int64_t) (and alignof(PRInt64) == alignof(int64_t), outside of C++ name mangling requirements (which NSPR doesn't worry about since it's all in C), what binary compatibility concerns exist?

Comment 10

[(no longer active)]

(In reply to comment #9)
> (In reply to Mike Hommey [:glandium] from comment #7)
> > On Linux systems, and especially fedora, the nspr that comes with Firefox
> > may be used by some of the system libraries Firefox itself loads, because
> > these system libraries normally use a system nspr. As a consequence, the
> > nspr that comes with Firefox *needs* to be binary compatible with the system
> > one.
> 
> So long as sizeof(PRInt64) == sizeof(int64_t) (and alignof(PRInt64) ==
> alignof(int64_t), outside of C++ name mangling requirements (which NSPR doesn't
> worry about since it's all in C), what binary compatibility concerns exist?

Just to make it clear, there is no binary compatibility issue here.  It's just being pedantic for the sake of it.

Comment 11

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

I believe the issue is that, if NSPR changes the type of PRUint32 to exactly match uint32_t (etc.), then there may be some existing code on some existing platforms that will break at build time, for the same reason our code sometimes fails to compile if NSPR *doesn't* make that change. So, it isn't just about pedantry.

Now, I think it is fair to say that the source-code compatibility concerns should swing the other way (in favor of code that wants to assume PRUint32 == uint32_t, instead of in favor of code the relies on the NSPR types not changing).

Comment 12

[Mike Hommey [:glandium]]

(In reply to Brian Smith (:bsmith) from comment #8)
> On Linux, is there any case where #defining or typedefing the NSPR types to
> their standard C/C++ counterparts changes the ABI?

Actually, there might, on s390, depending how some types are redefined. But it's also too late for me to think straight, so don't take my word on it.

Comment 13

[(no longer active)]

(In reply to comment #11)
> I believe the issue is that, if NSPR changes the type of PRUint32 to exactly
> match uint32_t (etc.), then there may be some existing code on some existing
> platforms that will break at build time, for the same reason our code sometimes
> fails to compile if NSPR *doesn't* make that change. So, it isn't just about
> pedantry.

To the best of our knowledge so far, out of all of the platforms that Firefox is ported to now (which means more than just our tier-1 platforms) the only case where these do not map to the underlying same type is that PRUint64!=uint64_t on OpenBSD.  So, NSPR integral types and stdint types are already source- and binary-compatible everywhere but OpenBSD, where they're only non-source-compatible on 64-bit unsigned integers, and wtc says that he believes that people should not assume the compatibility even though in almost all cases they _are_ compatible.  Which is why I think this is merely pedantry.

> Now, I think it is fair to say that the source-code compatibility concerns
> should swing the other way (in favor of code that wants to assume PRUint32 ==
> uint32_t, instead of in favor of code the relies on the NSPR types not
> changing).

Agreed.

Comment 14

[Jeff Walden [:Waldo]]

(In reply to Ehsan Akhgari [:ehsan] from comment #13)
> To the best of our knowledge so far, out of all of the platforms that
> Firefox is ported to now (which means more than just our tier-1 platforms)
> the only case where these do not map to the underlying same type is that
> PRUint64!=uint64_t on OpenBSD.

I don't believe that's true; I had to switch a good number of variables of PR* type to <stdint.h> type when fixing bug 708735, because the underlying types were different.  (This was in places where Gecko would use a PR* value, then pass the address of it into a JSAPI method, for the most part.)  I definitely remember changing PRUint32 to uint32_t in places to be able to compile on my Linux64 system.  And I remember laboriously doing the same on Windows (this was a bigger sticking point because I had to rdesktop into a Windows box to fix them) for PRUint64 and uint64_t.

https://hg.mozilla.org/mozilla-central/rev/d6d732ef5650

This changeset is replete with such changes, just search for PRUint32 or PRUint64.  I wasn't making any more changes than necessary to not turn the tree red, either -- if a type changed, it changed because it broke compilation somewhere.

Comment 15

[(no longer active)]

(In reply to comment #14)
> (In reply to Ehsan Akhgari [:ehsan] from comment #13)
> > To the best of our knowledge so far, out of all of the platforms that
> > Firefox is ported to now (which means more than just our tier-1 platforms)
> > the only case where these do not map to the underlying same type is that
> > PRUint64!=uint64_t on OpenBSD.
> 
> I don't believe that's true; I had to switch a good number of variables of PR*
> type to <stdint.h> type when fixing bug 708735, because the underlying types
> were different.  (This was in places where Gecko would use a PR* value, then
> pass the address of it into a JSAPI method, for the most part.)  I definitely
> remember changing PRUint32 to uint32_t in places to be able to compile on my
> Linux64 system.  And I remember laboriously doing the same on Windows (this was
> a bigger sticking point because I had to rdesktop into a Windows box to fix
> them) for PRUint64 and uint64_t.
> 
> https://hg.mozilla.org/mozilla-central/rev/d6d732ef5650
> 
> This changeset is replete with such changes, just search for PRUint32 or
> PRUint64.  I wasn't making any more changes than necessary to not turn the tree
> red, either -- if a type changed, it changed because it broke compilation
> somewhere.

Hmm, I wonder why I did not run into this problem in bug 579517?  Could that be because you were switching JS API interfaces from protypes?

Comment 16

[Jeff Walden [:Waldo]]

No idea why you didn't run into it.  Maybe we don't use any NSPR functions taking a pointer-to-PR{Ui,I}nt{8,16,32,64} argument?  JS uses them everywhere (because we signal fallibility with a bool return), which was the cause of every issue I remember hitting.  A far-fetched guess, but it's the only one I can think of offhand.

(As for what JSAPI used, it wasn't PR* types, and it wasn't protypes; it was "jsotypes", an old and incompatible fork of protypes.  The fun was that protypes and jsotypes shared the same #include guard, so if you included them in the wrong order, things fell over.  Hence the JSUint32 hackaround we used for a bit, until bug 708735.)

Incidentally, while reviewing bug 579517 I noticed bug 579517 comment 14.  I wonder if I've explained how the stdint.h types are incompatible with the PR* types anywhere else...  :-)

Comment 17

[(no longer active)]

FWIW bug 927728 will allow us to remove prtypes.h #includes which are currently solely used for PRUnichar.

Comment 18

[Cory Moore]

Attachment: same problem

Thanks from rathamimi1982.