Bug 796941 (dieprtypesdie)

Tracking bug for removal of the prtypes.h inclusions in our tree

NEW
Unassigned

Status

()

Core
General
5 years ago
19 days ago

People

(Reporter: Ehsan, Unassigned)

Tracking

(Depends on: 7 bugs)

Trunk
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

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.
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.
Depends on: 796948

Updated

5 years ago
Depends on: 792379
Depends on: 797106
Depends on: 797238
Depends on: 796129
Depends on: 792581
Depends on: 797445
Depends on: 798172
Depends on: 798564
Depends on: 798595
Depends on: 798599

Updated

5 years ago
Depends on: 798573
Depends on: 798431
OS: Mac OS X → All
Hardware: x86 → All
Depends on: 712936
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.
(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.  :(
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.
(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.
Depends on: 799958
(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...
(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.
(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.
(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?
(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.
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).
(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.
(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.
(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.
(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?
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...  :-)
Depends on: 801466
Depends on: 803441
Depends on: 829685
Depends on: 830112
Depends on: 883436
Depends on: 888323
Depends on: 801209
Depends on: 895047
Depends on: 895141
Depends on: 895168

Updated

4 years ago
Depends on: 923249
Depends on: 927728
FWIW bug 927728 will allow us to remove prtypes.h #includes which are currently solely used for PRUnichar.
Depends on: 928038
Depends on: 928040
Depends on: 928041
Depends on: 928043
Depends on: 928046
Depends on: 928049
Depends on: 928052
Depends on: 928053
Depends on: 928054
Depends on: 928712

Updated

4 years ago
Depends on: 928741
Depends on: 935789
Depends on: 943156
Depends on: 956995
Depends on: 957002
Depends on: 1296176
Depends on: 1296178
Depends on: 1296182

Updated

8 months ago
Depends on: 1297402

Updated

8 months ago
Depends on: 1298855
Depends on: 1308094
Depends on: 1308100
Depends on: 1308101
Depends on: 1308103
Depends on: 1308104
Depends on: 1308105
Depends on: 1308106

Updated

20 days ago
Depends on: 1353143

Updated

19 days ago
Depends on: 1353544
You need to log in before you can comment on or make changes to this bug.