should define PRUint64 as unsigned long long like __APPLE__

RESOLVED FIXED in 4.9.5

Status

P2
normal
RESOLVED FIXED
8 years ago
6 years ago

People

(Reporter: m_kato, Assigned: wtc)

Tracking

other
4.9.5
x86_64
OpenBSD
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments, 6 obsolete attachments)

http://fxr.watson.org/fxr/source/arch/amd64/include/_types.h?v=OPENBSD#L54,64,74,82

   51 /* LONGLONG */
   52 typedef long long               __int64_t;
   53 /* LONGLONG */
   54 typedef unsigned long long      __uint64_t;
Blocks: 633857
(Assignee)

Comment 1

8 years ago
Makoto: thank you for the bug report.  Is there a compilation error?
The __APPLE__ exception was added to fix a compilation error.
(In reply to comment #1)
> Makoto: thank you for the bug report.  Is there a compilation error?
> The __APPLE__ exception was added to fix a compilation error.

Landry knows about this.  He reports it in bug 633857 comment #12 etc.
Blocks: 599764
(Assignee)

Comment 3

8 years ago
Thank you.  But no one should assume that PRUint64 and uint64_t are
exactly the same.  It is better to fix the compilation errors using patches
like the one in bug 633857 comment 12, unless it becomes painful.
OS: OpenBSD → All
(Assignee)

Updated

8 years ago
OS: All → OpenBSD
Attachment #513315 - Flags: review?(landry)
Attachment #513315 - Flags: review?(wtc)
(Assignee)

Comment 5

8 years ago
Comment on attachment 513315 [details] [diff] [review]
fix

Makato: thank you for the patch.  Please see my comment 3
for why I marked this patch review-.

Please see the original comment for 64-bit Mac OS X:

>  * On 64-bit Mac OS X, uint64 needs to be defined as unsigned long long to
>  * match uint64_t, otherwise our uint64 typedef conflicts with the uint64
>  * typedef in cssmconfig.h, which CoreServices.h includes indirectly.

Note that the 'uint64' type (defined by NSPR in "obsolete/protypes.h")
is involved in that problem.

In contrast, the 64-bit OpenBSD problem involves only int64_t, which NSPR
doesn't define:

>+ * On 64-bit OpenBSD (x86-64 and Sparc), int64_t is defined as long long.
>+ * To avoid compile error, use long long instead of.
Attachment #513315 - Flags: review?(wtc) → review-
I'm not comfortable changing conditionally nsprtypes.h only for openbsd/64bits when the patch in bug 633857 comment #12 also fixes it in a simpler way (provided it doesn't break other platforms, of course).
Changing that typedef might aswell subtly break other stuff somewhere else in the tree.

From my point of view, when types conflict, the 'official ones' from types.h should be used.

> In contrast, the 64-bit OpenBSD problem involves only int64_t

Nope, the build failure is 'error: invalid conversion from 'PRUint64*' to 'uint64_t*''
As a sidenote, it changes nothing on OpenBSD/i386, with or without https://bug599764.bugzilla.mozilla.org/attachment.cgi?id=478732 and https://bug633857.bugzilla.mozilla.org/attachment.cgi?id=512802 it builds and runs the same.
(Assignee)

Comment 8

8 years ago
Landry: thank you for the comment.  (The int64_t is a typo
in Makoto's patch.)

When types conflict, the type expected by the function
should be used.  If a function expects a PRUint64,
PRUint64 should be used, even though PRUint64 is not
a Standard C type.  (I assume that's what you meant by
an "official" type from types.h.)

In nsWebMReader::GetBuffered, the nestegg_tstamp_scale
call expects a uint64_t* (note the *), but the
mBufferedState->CalculateBufferedForRange call expects
a PRUint64.  By simply declaring timecodeScale as a
uint64_t, we satisfy nestegg_tstamp_scale while relying
on automatic conversion of uint64_t to PRUint64 for
mBufferedState->CalculateBufferedForRange.
Status: NEW → RESOLVED
Last Resolved: 8 years ago
OS: OpenBSD → All
Resolution: --- → WONTFIX
(Assignee)

Updated

8 years ago
OS: All → OpenBSD
Attachment #513315 - Flags: review?(landry)
I was going to report again that issue, until i stumbled upon this previous try. For the record, this bite me again several times, see at least #599764, #714332, #685820 & #717733, and this was leading to issues in porting third-party nspr users (chromium, fwiw).

Saying "But no one should assume that PRUint64 and uint64_t are exactly the same." seems a bit weird to me... isn't nspr goal to be portable, and defining "similar" types mismatching with system ones against that goal ?

On OpenBSD 32/64bits systems, uint64_t is unsigned long long, and int64_t is long long. As it is with stock nspr, PRUint64 and PRint64 are defined respectively as unsigned long/long, thus not matching system types (although still being on 64 bits). It's to be checked, but i'm pretty sure other BSD oses use similar types...

For the record, nspr on OpenBSD is now shipped with that bug fixed since some months : http://anoncvs.estpak.ee/cgi-bin/cgit/openbsd-ports/commit/devel/nspr?id=a8f9cf6c935bb4fe033200cdb6a95c2d428e2602

So could this be reconsidered for inclusion ?
Status: RESOLVED → REOPENED
Resolution: WONTFIX → ---
Note that NSPR clients on BSD that use C++, and use the NSPR integer types in exposed APIs, would have to break ABI if this change occurred.  Maybe that's an acceptable cost for every client.  I'd be a little surprised if it were, myself.
Blocks: 651461
Blocks: 650665
Blocks: 717733
Blocks: 714332
Blocks: 685820
Blocks: 785738
(In reply to Wan-Teh Chang from comment #3)
> Thank you.  But no one should assume that PRUint64 and uint64_t are
> exactly the same.  It is better to fix the compilation errors using patches
> like the one in bug 633857 comment 12, unless it becomes painful.

This is getting increasingly painful, especially since landing of bug 579517 which uncovered lots of int64_t (which were previously int64, from protypes.h) uses in place of PRTime (which is PRInt64), and thus leading to lots of new failures on OpenBSD (see bug 785738)

Can we pretty please reconsider it ?

Comment 13

6 years ago
Wan-Teh, I'd like to object to comment 3.  There is absolutely no reason for anybody to assume that PRUint64 and uint64_t are different types.  This really doesn't make any sense, since in C/C++, typedef does not *define* new types, it merely adds alternate names for them, and C/C++ compilers are required to perform all of the static and dynamic type checking using the underlying types.  Resisting this idea leads us to do horrible hacks (see bug 785738 comment 22 for an example), and I'm extremely unhappy that NSPR refuses the correct fix here.
Blocks: 788012
(In reply to Ehsan Akhgari [:ehsan] from comment #13)
> There is absolutely no reason for anybody to assume that PRUint64 and uint64_t are different types.

That's not what he said.  It's that nobody should be assuming anything about the actual type of PRUint64 -- that it's the same type as uint64_t, that it's the same type as long, that it's the same type as long long, or anything -- even that it's different from any of these types.  <stdint.h> has the same limitations -- you can't assume uint32_t is the same as unsigned int or unsigned long on 32-bit platforms (although it's probably one or the other).

NSPR provides PR{Ui,I}nt{8,16,32,64} typedefs.  It does not provide an API guarantee of their being the same types as the corresponding <stdint.h> types that might or might not be provided by any libc implementation for the platform, or to particular fundamental C types, or even to any fundamental C types at all.  It may not be helpful for them to be different.  But let's be clear: it would be an NSPR API change to offer that guarantee.

Note that the obsolete "native" NSPR types -- {u,}int{8,16,32,64 -- similarly provide no guarantees of sameness with <stdint.h> types or fundamental C types.  Indeed they're even less compatible, at least partly because NSPR defines them using system headers on some platforms.  (Which caused no end of hilarity, of the sort at issue with BSD here, when the JSAPI used types with those names.)

The correct fix (at least if NSPR is unwilling to contemplate an API change, as is the case so far) is to not use NSPR APIs that take pointers to PR{Ui,I}nt{8,16,32,64} values (or pointers to them, recursively).  More effort, sure, but it also seems like a good opportunity to continue reducing our dependence on NSPR.

Comment 15

6 years ago
(In reply to comment #14)
> (In reply to Ehsan Akhgari [:ehsan] from comment #13)
> > There is absolutely no reason for anybody to assume that PRUint64 and uint64_t are different types.
> 
> That's not what he said.  It's that nobody should be assuming anything about
> the actual type of PRUint64 -- that it's the same type as uint64_t, that it's
> the same type as long, that it's the same type as long long, or anything --
> even that it's different from any of these types.  <stdint.h> has the same
> limitations -- you can't assume uint32_t is the same as unsigned int or
> unsigned long on 32-bit platforms (although it's probably one or the other).

We don't need to rely on the guarantee that both PRUint64 and uint64_t map to unsigned long long, for example, and I was not talking about that.  Unless I've been making a huge mistake, PRUint64 is supposed to be an unsigned 64-bit integer, and uint64_t is also supposed to be an unsigned 64-bit integer.  So it is an entirely reasonable assumption that these types would be possible to be used interchangeably, an assumption which holds everywhere except for OpenBSD.

Now, one might argue that NSPR intentionally intends for those two types to map to two different underlying types in the general case, and I would maintain that such an argument is very hard to back by any reasoning that I can think of.  The only case where the underlying types need to be different are for platforms without native 64-bit integer support, and we're not talking about those.

> NSPR provides PR{Ui,I}nt{8,16,32,64} typedefs.  It does not provide an API
> guarantee of their being the same types as the corresponding <stdint.h> types
> that might or might not be provided by any libc implementation for the
> platform, or to particular fundamental C types, or even to any fundamental C
> types at all.  It may not be helpful for them to be different.  But let's be
> clear: it would be an NSPR API change to offer that guarantee.

Hmm, well, I can make guesses on the semantics of this API just as well as anybody, but the documentation page for PRUint64 <https://developer.mozilla.org/en-US/docs/PRUint64> just refers the reader back to the prtypes.h header, which has inconsistent behavior across platforms (see <http://mxr.mozilla.org/mozilla-central/source/nsprpub/pr/include/prtypes.h#334> for reference).  So... yeah sure, it would be an API change.  But I would mostly like to think about it as more of an API fix.

> Note that the obsolete "native" NSPR types -- {u,}int{8,16,32,64 -- similarly
> provide no guarantees of sameness with <stdint.h> types or fundamental C types.
>  Indeed they're even less compatible, at least partly because NSPR defines them
> using system headers on some platforms.  (Which caused no end of hilarity, of
> the sort at issue with BSD here, when the JSAPI used types with those names.)

I can definitely sympathize.

> The correct fix (at least if NSPR is unwilling to contemplate an API change, as
> is the case so far) is to not use NSPR APIs that take pointers to
> PR{Ui,I}nt{8,16,32,64} values (or pointers to them, recursively).  More effort,
> sure, but it also seems like a good opportunity to continue reducing our
> dependence on NSPR.

I wish it were that easy.  We can't really change NSS at will either.  But anyways, this is sort of outside of the scope of this bug.  This bug has been filed about the inconsistency of the PRUint64 definition on OpenBSD, and I still have not seen any good reason why this should not be fixed, besides basically resisting change.
(In reply to Ehsan Akhgari [:ehsan] from comment #15)
> We don't need to rely on the guarantee that both PRUint64 and uint64_t map
> to unsigned long long, for example, and I was not talking about that.

Sure you do.  Supposing they're different, passing PRUint64 where uint64_t is expected will mostly just work (with an implicit conversion -- so things like the explicit keyword will break, but barely).  But unsigned long* and unsigned long long* (and however many further levels of indirection) are required not to be interchangeable, even if unsigned long and unsigned long long are structurally identical.  Many NSPR APIs take pointers to PR{Ui,I}nt{8,16,32,64}, so to make the assumption this bug requests, those types must have the same underlying type as <stdint.h> types.

And yes, it is just that easy.  :-)  Which doesn't mean it's not hard as well, but the core of the solution is simple.
This is becoming a burden on Gecko maintainers, not to mention on Landry.  Basically any code which does computations on PRTime is going to break on BSD, and then we're going to have to review and land follow-ups.  It's a waste of everyone's time.

Is there pain caused by this proposed fix, or do you object on philosophical grounds alone?  I feel like comment 16 pretty strongly debunks the idea from comment 3 that "nobody should assume PRUint64 and uint64_t are the same".
Attachment #513315 - Flags: review- → review?(wtc)
(Assignee)

Updated

6 years ago
Priority: -- → P2
Target Milestone: --- → 4.9.4
(Assignee)

Updated

6 years ago
Target Milestone: 4.9.4 → 4.9.5
(Assignee)

Comment 18

6 years ago
Created attachment 687356 [details] [diff] [review]
Define NSPR types as <stdint.h> types

Please test this patch on OpenBSD. Are you still interested
in fixing this bug? Based on mfbt/StandardInteger.h, I assume
Visual C++ 2008 and older are the only compilers that don't
have <stdint.h>. So this patch uses a "blacklist" approach.
Attachment #687356 - Flags: superreview?(justin.lebar+bug)
Attachment #687356 - Flags: review?(jwalden+bmo)
(Assignee)

Updated

6 years ago
Attachment #513315 - Attachment is obsolete: true
Attachment #513315 - Flags: review?(wtc)
(Assignee)

Comment 19

6 years ago
Created attachment 687404 [details] [diff] [review]
Define NSPR types as <stdint.h> types, v1.1

Fix a typo in the patch (a comment was missing the closing "*/").
Attachment #687356 - Attachment is obsolete: true
Attachment #687356 - Flags: superreview?(justin.lebar+bug)
Attachment #687356 - Flags: review?(jwalden+bmo)
Attachment #687404 - Flags: superreview?(justin.lebar+bug)
Attachment #687404 - Flags: review?(jwalden+bmo)
Comment on attachment 687404 [details] [diff] [review]
Define NSPR types as <stdint.h> types, v1.1

FWIW I'm not a super-reviewer, but I'm happy to do a regular review!
Attachment #687404 - Flags: superreview?(justin.lebar+bug)
Attachment #687404 - Flags: review?(justin.lebar+bug)
Attachment #687404 - Flags: feedback?(landry)
(Assignee)

Comment 21

6 years ago
Created attachment 687535 [details] [diff] [review]
Define NSPR exact-width integer types as <stdint.h> exact-width types, v2

I've tested this patch on several platforms, including
on Windows with Visual C++ 2008 and 2012. I made two
changes.

1. Add a test for the RC_INVOKED macro to handle old
Visual C++. prtypes.h is used by the Windows resource
compiler (rc.exe) during the NSPR build. The resource
compiler does not define the _MSC_VER macro, so we
can't use _MSC_VER to determine if <stdint.h> is
present when being compiled by the resource compiler.
Fortunately the exact definition of the PRIntxxx types
is irrelevant to the resource compiler, so we can
simply act as if <stdint.h> didn't exists.

2. Remove the old special case for __APPLE__.
Attachment #687404 - Attachment is obsolete: true
Attachment #687404 - Flags: review?(jwalden+bmo)
Attachment #687404 - Flags: review?(justin.lebar+bug)
Attachment #687404 - Flags: feedback?(landry)
Attachment #687535 - Flags: review?(jwalden+bmo)
Attachment #687535 - Flags: feedback?(landry)
(Assignee)

Comment 22

6 years ago
Comment on attachment 687535 [details] [diff] [review]
Define NSPR exact-width integer types as <stdint.h> exact-width types, v2

Note: the prdepend.h change in this patch can be ignored
in your code review. (NSPR's makefiles don't have header
file dependencies, so this kind  of dummy change to
prdepend.h is a hack to cause every .c file in NSPR to
be recompiled.)
Attachment #687535 - Flags: review?(justin.lebar+bug)
>+#ifdef _PR_HAVE_STDINT_H
It's unfortunate that NSPR abuses reserved identifiers :(
Here on OpenBSD  with clang it blows on dom/indexedDB/Key.cpp :


/src/mozilla-central/dom/indexedDB/Key.cpp:394:29: error: use of undeclared identifier 'UINT64_C'
  uint64_t number = pun.u & PR_UINT64(0x8000000000000000) ?
                            ^
/usr/obj/m-c/dist/include/nspr/prtypes.h:365:22: note: expanded from macro 'PR_UINT64'
#define PR_UINT64(x) UINT64_C(x)
                     ^
/src/mozilla-central/dom/indexedDB/Key.cpp:396:30: error: use of undeclared identifier 'UINT64_C'
                    (pun.u | PR_UINT64(0x8000000000000000));
                             ^
/usr/obj/m-c/dist/include/nspr/prtypes.h:365:22: note: expanded from macro 'PR_UINT64'
#define PR_UINT64(x) UINT64_C(x)
                     ^
/src/mozilla-central/dom/indexedDB/Key.cpp:418:20: error: use of undeclared identifier 'UINT64_C'
  pun.u = number & PR_UINT64(0x8000000000000000) ?
                   ^
/usr/obj/m-c/dist/include/nspr/prtypes.h:365:22: note: expanded from macro 'PR_UINT64'
#define PR_UINT64(x) UINT64_C(x)
                     ^
/src/mozilla-central/dom/indexedDB/Key.cpp:419:22: error: use of undeclared identifier 'UINT64_C'
          (number & ~PR_UINT64(0x8000000000000000)) :
                     ^
/usr/obj/m-c/dist/include/nspr/prtypes.h:365:22: note: expanded from macro 'PR_UINT64'
#define PR_UINT64(x) UINT64_C(x)
                     ^
4 errors generated.


UINT64_C is defined in stdint.h, but within some ifdefs :

#if !defined(__cplusplus) || defined(__STDC_CONSTANT_MACROS)


see http://www.openbsd.org/cgi-bin/cvsweb/src/sys/sys/stdint.h?rev=1.7;content-type=text%2Fplain

i see __STDC_CONSTANT_MACROS being defined in g++'s cstdint, but nowhere in clang 3.1. Will see if gcc 4.6.3 changes something.
IndexedDB should just use UINT64_C. Filed bug 818689.
(Assignee)

Comment 26

6 years ago
Masatoshi: thank you for the comment. The leading underscore in
_PR_HAVE_STDINT_H is intended to stress the fact that this macro is
for internal use by NSPR headers. Is there a naming convention for
such macros used by libraries? Otherwise I'll have to document that
this macro is for internal use.

Landry: thank you for the test results. I saw that the INT{N}_MIN and
INT{N}_MAX macros are also defined inside a similar ifdef to prevent
their use in C++:

  #if !defined(__cplusplus) || defined(__STDC_LIMIT_MACROS)

I suspect the intention is to encourage C++ code to use
std::numeric_limits<intN_t>::min() and std::numeric_limits<intN_t>::max()
instead. I can do that in prtypes.h if __cplusplus is defined.

But I don't know how to replace the INT{N}_C and UINT{N}_C macros
with C++ facilities. Does anyone know?

Comment 27

6 years ago
(In reply to comment #26)
> Masatoshi: thank you for the comment. The leading underscore in
> _PR_HAVE_STDINT_H is intended to stress the fact that this macro is
> for internal use by NSPR headers. Is there a naming convention for
> such macros used by libraries? Otherwise I'll have to document that
> this macro is for internal use.

IIRC identifier names beginning with an underscore are reserved for compiler libraries.

> Landry: thank you for the test results. I saw that the INT{N}_MIN and
> INT{N}_MAX macros are also defined inside a similar ifdef to prevent
> their use in C++:
> 
>   #if !defined(__cplusplus) || defined(__STDC_LIMIT_MACROS)
> 
> I suspect the intention is to encourage C++ code to use
> std::numeric_limits<intN_t>::min() and std::numeric_limits<intN_t>::max()
> instead. I can do that in prtypes.h if __cplusplus is defined.

To be clear, that is not possible in *every* case, until compilers get support for constexpr.  Example:

char foo[std::numeric_limits<int16_t>::max()];

> But I don't know how to replace the INT{N}_C and UINT{N}_C macros
> with C++ facilities. Does anyone know?

You can just add a correct type suffix (e.g., 'u' for unsigned int, 'ul' for unsigned long, etc.)  For enforcing a sized type, you should use a static_cast (e.g., static_cast<uint16_t>(10)).
(In reply to Ehsan Akhgari [:ehsan] from comment #27)
> You can just add a correct type suffix (e.g., 'u' for unsigned int, 'ul' for
> unsigned long, etc.)
Are all supported compilers agree with the suffix for 64-bit types? (ul, ull, ui64, or whatever)
With gcc 4.6.3 it fails with :

/src/mozilla-central/dom/plugins/base/nsPluginStreamListenerPeer.cpp: In member function 'virtual nsresult nsPluginStreamListenerPeer::OnDa
ports*, nsIInputStream*, uint64_t, uint32_t)':
/src/mozilla-central/dom/plugins/base/nsPluginStreamListenerPeer.cpp:896:34: error: 'INT64_C' was not declared in this scope
/src/mozilla-central/dom/plugins/base/nsPluginStreamListenerPeer.cpp: In member function 'virtual nsresult nsPluginStreamListenerPeer::OnStrts*, nsresult)':
/src/mozilla-central/dom/plugins/base/nsPluginStreamListenerPeer.cpp:977:32: error: 'INT64_C' was not declared in this scope
Fwiw i've cross-checked, and on NetBSD and linux those macros are also defined within a same #if block. A linux stdint.h header even gives the explanation :

/* The ISO C99 standard specifies that in C++ implementations these
   should only be defined if explicitly requested.  */

#if !defined __cplusplus || defined __STDC_CONSTANT_MACROS

/* The ISO C99 standard specifies that in C++ implementations these
   macros should only be defined if explicitly requested.  */

#if !defined __cplusplus || defined __STDC_LIMIT_MACROS

So, if we want to use them from c++, better #define those ?
It seems they're already defined in a bunch of places in the tree : 

http://mxr.mozilla.org/mozilla-central/search?string=__STDC_LIMIT_MACROS
http://mxr.mozilla.org/mozilla-central/search?string=__STDC_CONSTANT_MACROS

interestingly mozilla-config.h.in defines __STDC_LIMIT_MACROS but not the CONSTANT one. Maybe it should go there ?

I wonder how your patch could build on linux or macosx without that #define...
I have patchwork in bug 812218 and bug 730805 that would define __STDC_LIMIT_MACROS for everyone, before any code is compiled or any headers are imported.  There's a holdup on figuring out why bug 812218 busts Windows PGO, tho.  I doubt it's too hard to debug, but I simply haven't had time to figure it out yet.
Comment on attachment 687535 [details] [diff] [review]
Define NSPR exact-width integer types as <stdint.h> exact-width types, v2

Clearing r? since this doesn't seem to be working properly on BSD.
Attachment #687535 - Flags: review?(justin.lebar+bug)
(Assignee)

Comment 34

6 years ago
Comment on attachment 687535 [details] [diff] [review]
Define NSPR exact-width integer types as <stdint.h> exact-width types, v2

I am abandoning this patch but leave it for future reference.
It will be useful when C11-compatible <stdint.h> is common,
which does not require __STDC_LIMIT_MACROS and __STDC_CONSTANT_MACROS
to be defined for C++.

Note: The <stdint.h> in Visual C++ 2010 and 2012 and the current version
of Xcode do not test the __STDC_LIMIT_MACROS and __STDC_CONSTANT_MACROS.

Thanks for the additional comments from Landry and Ehsan. I researched
this issue last night and came to the following conclusions.

1. The __STDC_LIMIT_MACROS and __STDC_CONSTANT_MACROS tests in the
<stdint.h> headers on some platforms come from the C99 standard. I
think it is unfortunate and doesn't make sense, which may be why
these two macros are gone in the C11 standard (I verified that with
the last draft of C11 because the standard costs more than US$200).

2. It would be bad for NSPR to require NSPR users to define
__STDC_LIMIT_MACROS and __STDC_CONSTANT_MACROS. It would not be a
robust solution for NSPR to define __STDC_LIMIT_MACROS and
__STDC_CONSTANT_MACROS before including <stdint.h>. If <stdint.h>
is already included before prtypes.h is included, it will be too late.

3. So the best solution today is to manually match the intN_t and
uintN_t typedefs on all platforms. I studied the <stdint.h> headers
of several platforms. All of them define the 8, 16, and 32-bit types
in the same way. So int64_t and uint64_t are the only issue. I found
three strategies.
- Use the "shortest" 64-bit integer type: this strategy is used by
  Linux/glibc, FreeBSD, and NetBSD. and is the current strategy of
  NSPR.
- Just use long long: this strategy is used by Apple (Mac OS X and
  iOS). OpenBSD, and Android. Note that not all *BSD use the same
  strategy.
- Use a compiler-supported 64-bit integer type: although I saw this
  code in some <stdint.h> headers, I believe this is obsolete code
  and was only necessary before long long was officially added in C99.
  NSPR uses this strategy for Visual C++ because older versions of
  Visual C++ only have __int64.

These findings showed PRInt64/PRUint64 are only different from
int64_t/uint64_t on OpenBSD and Android. This explains why the
sloppy code only broke OpenBSD among the "desktop" platforms, but
I don't know why the sloppy code didn't seem to break Android.
Attachment #687535 - Attachment description: Define NSPR types as <stdint.h> types, v2 → Define NSPR exact-width integer types as <stdint.h> exact-width types, v2
Attachment #687535 - Flags: review?(jwalden+bmo)
Attachment #687535 - Flags: feedback?(landry)
(Assignee)

Comment 35

6 years ago
(In reply to Landry Breuil (:gaston) from comment #31)
>
> interestingly mozilla-config.h.in defines __STDC_LIMIT_MACROS but not the
> CONSTANT one. Maybe it should go there ?

Yes, I think mozilla-config.h.in should also define __STDC_CONSTANT_MACROS.
But I'll let Waldo figure out the best way to define those macros in Mozilla.
Status: REOPENED → ASSIGNED
(Assignee)

Comment 36

6 years ago
Created attachment 689528 [details] [diff] [review]
Add OpenBSD and Android to the list of exceptions

Please review and test this patch. Thanks.

I guess some of you are saying "I told you so" now :-)
Attachment #689528 - Flags: superreview?(ehsan)
Attachment #689528 - Flags: review?(justin.lebar+bug)
Attachment #689528 - Flags: feedback?(landry)
Comment on attachment 689528 [details] [diff] [review]
Add OpenBSD and Android to the list of exceptions

Please use defined(__OpenBSD__), OPENBSD is not defined by default. iirc only ipc/ uses it internally, and takes care of defining it.

Fwiw, with __OpenBSD__ in the patch, m-c builds for me on OpenBSD/amd64, and i'm even able to revert https://hg.mozilla.org/integration/mozilla-inbound/rev/989f2d81bd15 , build still succeeds.
Attachment #689528 - Flags: feedback?(landry) → feedback+

Comment 38

6 years ago
Comment on attachment 689528 [details] [diff] [review]
Add OpenBSD and Android to the list of exceptions

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

Looks great, thanks a lot for doing this.  :-)
Attachment #689528 - Flags: superreview?(ehsan) → superreview+
(Assignee)

Comment 39

6 years ago
Created attachment 689878 [details] [diff] [review]
Add OpenBSD and Android to the list of exceptions, v2 (checked in)

I tweaked the comments and changed defined(OPENBSD) to defined(__OpenBSD__).

Landry: NSPR's public headers defines OPENBSD here:
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/nsprpub/pr/include/md/_openbsd.cfg&rev=3.12&mark=13-15#13

So it is correct to test defined(OPENBSD) and that's what NSPR public
headers have been doing. But it seems like a good idea to migrate away
from this practice, so I made your suggested change. (Mac OS X and
Android happened to be the two exceptions where NSPR doesn't define
its own platform-name macros, which is why NSPR must test their
compiler-predefined macros.)

Patch checked in on the NSPR trunk (NSPR 4.9.5).

Checking in config/prdepend.h;
/cvsroot/mozilla/nsprpub/config/prdepend.h,v  <--  prdepend.h
new revision: 1.20; previous revision: 1.19
done
Checking in pr/include/prlong.h;
/cvsroot/mozilla/nsprpub/pr/include/prlong.h,v  <--  prlong.h
new revision: 3.20; previous revision: 3.19
done
Checking in pr/include/prtypes.h;
/cvsroot/mozilla/nsprpub/pr/include/prtypes.h,v  <--  prtypes.h
new revision: 3.52; previous revision: 3.51
done
Attachment #689528 - Attachment is obsolete: true
Attachment #689528 - Flags: review?(justin.lebar+bug)
(Assignee)

Comment 40

6 years ago
Created attachment 689925 [details] [diff] [review]
Verify PRInt{N} matches int{N}_t

Ideally this code should be an NSPR test program and causes a
compiler error. Since not all NSPR users build the NSPR tests,
I have to add it to NSPR proper. I also can't find a GCC flag
to make this warning an error.
Attachment #689925 - Flags: review?(ehsan)

Comment 41

6 years ago
Comment on attachment 689925 [details] [diff] [review]
Verify PRInt{N} matches int{N}_t

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

I have one reservation about this patch.  Are you sure that the C type checking is smart enough to error out if these underlying types are actually not the same?  I know that C has a more relaxed type checker, but I don't remember exactly to what extent...

r=me if this really does what you mean!
Attachment #689925 - Flags: review?(ehsan) → review+
(Assignee)

Comment 42

6 years ago
(In reply to Ehsan Akhgari [:ehsan] from comment #41)
>
> I have one reservation about this patch.  Are you sure that the C type
> checking is smart enough to error out if these underlying types are actually
> not the same?

I think so. I have tested with gcc 4.4.3 on Linux (with a change prtypes.h
to make PRInt64 and int64_t different).

I will repeat my experiments with clang on Mac and Visual C++ this weekend
and report back.

Of course, if you are willing to add this piece of code to Mozilla, that'll
be even better. But I don't feel comfortable asking Mozilla to do this for
NSPR.

Comment 43

6 years ago
(In reply to comment #42)
> (In reply to Ehsan Akhgari [:ehsan] from comment #41)
> >
> > I have one reservation about this patch.  Are you sure that the C type
> > checking is smart enough to error out if these underlying types are actually
> > not the same?
> 
> I think so. I have tested with gcc 4.4.3 on Linux (with a change prtypes.h
> to make PRInt64 and int64_t different).
> 
> I will repeat my experiments with clang on Mac and Visual C++ this weekend
> and report back.

OK, sounds good.

> Of course, if you are willing to add this piece of code to Mozilla, that'll
> be even better. But I don't feel comfortable asking Mozilla to do this for
> NSPR.

Where exactly would you want us to put it?  I definitely don't mind having that check outside of NSPR at all, if you can think of a good location for it.
(Assignee)

Comment 44

6 years ago
Created attachment 690664 [details] [diff] [review]
TestPRIntN.cpp: Verify PRInt{N} matches int{N}_t

Ehsan: I found that this piece of code at most causes a compiler
warning in C. Visual C++ doesn't warn about this at the /W3 warning
level. However, in C++ this piece of code causes a compilation error.
Since NSPR test programs are all C code today, I changed this into
a C++ test program for Mozilla.

I first tried adding this test to mfbt/tests/ because StandardInteger.h
comes from mfbt. Unfortunately mfbt is now built before nspr, so by the
time this test gets built, NSPR's headers haven't been published to
dist/include yet. I don't want to change the build order just for this
test.

I then looked for a Mozilla module that uses both mfbt and NSPR.
security, xpcom, and netwerk are all good candidates. I didn't find
any C++ test program under security, so this narrows down the
candidates to three directories:
- http://mxr.mozilla.org/mozilla-central/source/xpcom/tests/
- http://mxr.mozilla.org/mozilla-central/source/xpcom/tests/external/
- http://mxr.mozilla.org/mozilla-central/source/netwerk/test/

I verified all of them would work.

Which one do you think is the best home for this test? Thanks.
Attachment #689925 - Attachment is obsolete: true
(Assignee)

Updated

6 years ago
Attachment #690664 - Flags: feedback?(ehsan)

Comment 45

6 years ago
Comment on attachment 690664 [details] [diff] [review]
TestPRIntN.cpp: Verify PRInt{N} matches int{N}_t

Let's use xpcom/tests, I guess!
Attachment #690664 - Flags: feedback?(ehsan) → feedback+
(Assignee)

Comment 46

6 years ago
Created attachment 691004 [details] [diff] [review]
TestPRIntN.cpp: Verify PRInt{N} matches int{N}_t, v2

Add the TestPRIntN.cpp test to xpcom/tests to make sure that
PRInt{N} matches int{N}_t on all platforms.
Attachment #690664 - Attachment is obsolete: true
Attachment #691004 - Flags: superreview?(ehsan)
Attachment #691004 - Flags: review?(justin.lebar+bug)
Comment on attachment 691004 [details] [diff] [review]
TestPRIntN.cpp: Verify PRInt{N} matches int{N}_t, v2

Okay!
Attachment #691004 - Flags: review?(justin.lebar+bug) → review+

Comment 48

6 years ago
Comment on attachment 691004 [details] [diff] [review]
TestPRIntN.cpp: Verify PRInt{N} matches int{N}_t, v2

sr not needed.
Attachment #691004 - Flags: superreview?(ehsan)
(Assignee)

Comment 49

6 years ago
Created attachment 692134 [details] [diff] [review]
Mozilla patch to update to NSPR_4_9_5_BETA1 and add TestPRIntN.cpp

Ehsan: could you push this patch to mozilla-inbound for me?
I don't know how to use the try server. Thanks.

After applying the patch, please run these commands:

    hg add xpcom/tests/TestPRIntN.cpp
    hg remove nsprpub/admin/repackage.sh
    
Patch description:

Bug 634793: update NSPR to NSPR_4_9_5_BETA1 and add the
TestPRIntN.cpp test to verify PRInt{N} matches int{N}_t.

Thank you!
Attachment #692134 - Flags: review?(ehsan)
(Assignee)

Comment 50

6 years ago
Comment on attachment 692134 [details] [diff] [review]
Mozilla patch to update to NSPR_4_9_5_BETA1 and add TestPRIntN.cpp

Ehsan: I figured out how to use the try server:
https://tbpl.mozilla.org/?tree=Try&rev=61ba9101b5af

I will try to push this patch myself today.
Attachment #692134 - Flags: review?(ehsan)

Comment 51

6 years ago
Try run for 61ba9101b5af is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=61ba9101b5af
Results (out of 18 total builds):
    success: 18
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/wtc@google.com-61ba9101b5af
(Assignee)

Comment 52

6 years ago
Comment on attachment 692134 [details] [diff] [review]
Mozilla patch to update to NSPR_4_9_5_BETA1 and add TestPRIntN.cpp

Patch pushed to mozilla-inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3173175efff1
https://hg.mozilla.org/mozilla-central/rev/3173175efff1
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago6 years ago
Resolution: --- → FIXED
\o/ \o/ \o/

Comment 55

6 years ago
Try run for 61ba9101b5af is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=61ba9101b5af
Results (out of 19 total builds):
    exception: 1
    success: 18
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/wtc@google.com-61ba9101b5af

Comment 56

6 years ago
FWIW, I removed the OpenBSD hack which is now unnecessary with this fix in https://hg.mozilla.org/integration/mozilla-inbound/rev/63d127cca94c.
You need to log in before you can comment on or make changes to this bug.