785738 - Build failure with gcc 4.2 since bug 579517

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Landry Breuil (:gaston)]

Reporting, even if gcc 4.2 is going to be unsupported.. my openbsd/amd64/gcc 4.2.1 builds started failing with 

../../dist/include/mozilla/TimeStamp.h:133: error: call of overloaded 'FromTicks(long int)' is ambiguous
../../dist/include/mozilla/TimeStamp.h:123: note: candidates are: static mozilla::TimeDuration mozilla::TimeDuration::FromTicks(int64_t)
../../dist/include/mozilla/TimeStamp.h:129: note:                 static mozilla::TimeDuration mozilla::TimeDuration::FromTicks(double)
../../dist/include/mozilla/TimeStamp.h:137: error: call of overloaded 'FromTicks(long int)' is ambiguous
../../dist/include/mozilla/TimeStamp.h:123: note: candidates are: static mozilla::TimeDuration mozilla::TimeDuration::FromTicks(int64_t)
../../dist/include/mozilla/TimeStamp.h:129: note:                 static mozilla::TimeDuration mozilla::TimeDuration::FromTicks(double)

(see http://buildbot.rhaalovely.net/builders/mozilla-central-amd64/builds/495/steps/build/logs/stdio)

Most likely a fallout of the pruint->stdint migration, since on openbsd int64_t appears to be a double. clang builds are fine apparently. The huge http://hg.mozilla.org/mozilla-central/rev/a16372ce30b5 commit is a likely candidate.

How do we want to fix it, if we want to fix it ?

Comment 1

[Boris Zbarsky [:bzbarsky]]

> since on openbsd int64_t appears to be a double

I'd really doubt that.  If that were true, the browser would be all sorts of broken.

The real issue, I would bet, is that "long int" needs to be promoted to int64_t here (because it's 32 bit in the configuration in question?) and once you're promoting it's ambiguous which promotion to use.  That said, I'm surprised LL_MAXINT is being reported as "long int".  That would imply that PR_BYTES_PER_LONG == 8, and hence there should be no weird promotion errors here.

In any case, not using LL_MAXINT and instead using the std::limits thing for int64_t or whatever the portable equivalent is would seem like the right fix.

Comment 2

[Landry Breuil (:gaston)]

I have a tentative fix :

-    if (aTicks >= double(LL_MAXINT))
-      return TimeDuration::FromTicks(LL_MAXINT);
+    if (aTicks >= std::numeric_limits<double>::max())
+      return TimeDuration::FromTicks(std::numeric_limits<double>::max());

(+ #include <limits> and same chunk for MININT)

That seems to build with gcc 4.2.1, but i guess the comment about LL_MAXINT should be amended, but how ?

Pushed that change to try in https://tbpl.mozilla.org/?tree=Try&rev=15a13dd74206

Next to that there's another print->stdint failure :

/home/landry/src/mozilla-central/netwerk/streamconv/converters/nsDirIndex.cpp:71: error: prototype for 'nsresult nsDirIndex::Ge
tLastModified(int64_t*)' does not match any in class 'nsDirIndex'
/home/landry/src/mozilla-central/netwerk/streamconv/converters/nsDirIndex.h:18: error: candidate is: virtual nsresult nsDirInde
x::GetLastModified(PRTime*)
/home/landry/src/mozilla-central/netwerk/streamconv/converters/nsDirIndex.cpp:71: error: prototype for 'nsresult nsDirIndex::Se
tLastModified(int64_t)' does not match any in class 'nsDirIndex'
/home/landry/src/mozilla-central/netwerk/streamconv/converters/nsDirIndex.h:18: error: candidate is: virtual nsresult nsDirInde
x::SetLastModified(PRTime)

How should that one be fixed ? use PRTime in netwerk/streamconv/converters/nsDirIndex.cpp:71 ?

Comment 3

[Landry Breuil (:gaston)]

Ah, the PRTime comes from the idl in netwerk/streamconv/public/nsIDirIndex.idl. istr int*_t types dont work in idl files.. but trying with int64_t there nevetheless. types in h/cpp should match the one in idl; right ?

Comment 4

[Landry Breuil (:gaston)]

With attribute int64_t lastModified in the idl file build fails with :

/home/landry/src/mozilla-central/netwerk/streamconv/converters/nsIndexedToHTML.cpp: In member function 'virtual nsresult nsIndexedToHTML::OnIndexAvailable(nsIRequest*, nsISupports*, nsIDirIndex*)':
/home/landry/src/mozilla-central/netwerk/streamconv/converters/nsIndexedToHTML.cpp:948: error: no matching function for call to 'nsIDirIndex::GetLastModified(PRTime*)'
../../../dist/include/nsIDirIndex.h:58: note: candidates are: virtual nsresult nsIDirIndex::GetLastModified(int64_t*) <near match>
/home/landry/src/mozilla-central/netwerk/streamconv/converters/nsIndexedToHTML.cpp:954: error: call of overloaded 'AppendInt(PRTime&)' is ambiguous
../../../dist/include/nsTSubstring.h:410: note: candidates are: void nsAString_internal::AppendInt(int32_t)
../../../dist/include/nsTSubstring.h:417: note:                 void nsAString_internal::AppendInt(uint32_t)
../../../dist/include/nsTSubstring.h:424: note:                 void nsAString_internal::AppendInt(int64_t)
../../../dist/include/nsTSubstring.h:431: note:                 void nsAString_internal::AppendInt(uint64_t)

So should i rollback nsDirIndex.{cpp,h} to use PRTime instead of int64_t for lastModified ?

Comment 5

[Landry Breuil (:gaston)]

Build fails with the same error if i try with PRTime instead of int64_t. A bit lost on how to fix it. Remember that on openbsd for obscure reasons (dont exactly remember the bug # now) int64 from nspr != int64_t from stdint..

Comment 6

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

INT64_MIN / UINT64_MIN probably works too

Comment 7

[(no longer active)]

(In reply to Landry Breuil (:gaston) from comment #5)
> Build fails with the same error if i try with PRTime instead of int64_t. A
> bit lost on how to fix it. Remember that on openbsd for obscure reasons
> (dont exactly remember the bug # now) int64 from nspr != int64_t from
> stdint..

That is a problem!  Can we fix that?

Comment 8

[Landry Breuil (:gaston)]

(In reply to Ehsan Akhgari [:ehsan] from comment #7)
> (In reply to Landry Breuil (:gaston) from comment #5)
> > Build fails with the same error if i try with PRTime instead of int64_t. A
> > bit lost on how to fix it. Remember that on openbsd for obscure reasons
> > (dont exactly remember the bug # now) int64 from nspr != int64_t from
> > stdint..
> 
> That is a problem!  Can we fix that?

I'd be delighted to fix that, but i already tried and failed. See bug #634793 and bug #599764 comment 20. (if the int64 used in idl is the one coming from nspr's protypes.h..)

Comment 9

[Landry Breuil (:gaston)]

using std::numeric_limits failed on windows (https://tbpl.mozilla.org/php/getParsedLog.php?id=14740823&tree=Try) and had failing tests on linux (https://tbpl.mozilla.org/php/getParsedLog.php?id=14741660&tree=Try, unrelated ?)

With hints from ms2ger trying INT64_MAX/INT64_MIN. Builds on OpenBSD.

Adding a static_cast in the offending AppendInt() call in netwerk/streamconv/converters/nsIndexedToHTML.cpp (in addition to using PRTime where appropriate in nsDirIndex.{h,cpp} seems to work too, so push both fixes to try in https://tbpl.mozilla.org/?tree=Try&rev=8b583d16ff50

Comment 10

[Landry Breuil (:gaston)]

Finally got a full patchset that got m-c to compile, pushed to try in https://tbpl.mozilla.org/?tree=Try&rev=472c42eef636.

Since this missed the uplift; as of now aurora doesn't build on OpenBSD with gcc 4.2, so once this bug is fixed on m-c i'll request approval for aurora.

The problem behind most of the failures resorts to uses of int64 (now int64_t) instead of PRTime, since PRTime is PRInt64 and != int64_t on OpenBSD. The patches attempts to fix all of them, i had to resort to reinterpret_cast<int64_t> for all calls to GetInt64 under places/ where a time was used, since there's no GetPRTime in mozIStorageStatement API. I hope that's acceptable.

As for Part 3, i'm not sure at all of the nsLocalFileOS2.cpp & nsLocalFileWin.cpp changes, try will tell.

I suppose Part 5 is unacceptable, but i have no idea right now on how to fix it. security/nss/lib/certdb/certt.h defines cert_rev_flags_per_method as a PRUint64, but security/manager/ssl/src/nsIdentityChecking.cpp now tries to assign it an int64_t.. I'm open to any idea here, but i doubt nss is willing to use int64_t.

Comment 11

[Landry Breuil (:gaston)]

All green on try, will ask for r? but not sure of part 5, nor of the commit msgs.. Will also do a full try run with tests.

Comment 12

[Landry Breuil (:gaston)]

Attachment: Use INT64_MIN/INT64_MAX

Use INT64_MAX/MIN in xpcom/ds/TimeStamp.h. Note sure the comment about the overflow is still relevant.

Comment 13

[Landry Breuil (:gaston)]

Attachment: Use PRTime in nsDirIndex

Use PRTime for lastModified in nsDirIndex to match the idl in nsIDirIndex.idl. Use a static_cast to int64_t in appendInt call.

Comment 14

[Landry Breuil (:gaston)]

Attachment: Use PRTime in nsLocalFile for modification time/lock time

Rather big patch, switches all callers of modiftime/locktime to use PRTime where appropriate. since nsIFile.idl changed i ameneded the prototypes for the corresponding getters/setters in nsLocalFile{Unix,OS2,Win}.cpp and try seems to confirm the change is fine.

Comment 15

[Landry Breuil (:gaston)]

Attachment: Use PRTime where appropriate under rdf/

Sidenote: the rdf/datasource/src/nsFileSystemDataSource.cpp change can go to Part 3/nsIFile patch/changeset for consistency since it deals with GetLastModifiedTime()

Comment 16

[Landry Breuil (:gaston)]

Attachment: Restore a pruint64 to please nss

Controversial change, restore a PRUint64 since security/nss/lib/certdb/certt.h still uses it for cert_rev_flags_per_method. Dont remember the exact error message otherwise, but it was badly erroring out. I'm open to any better suggestion, since re-adding PRUint64 will be frowned upon :)

Comment 17

[Landry Breuil (:gaston)]

Attachment: Use PRTime under places/ and add reinterpret_casts

Use PRTime where appropriate, and add ugly static/reinterpret casts to please AppendInt/GetInt64 calls.

Comment 18

[(no longer active)]

Comment on attachment 656044 [details] [diff] [review]
Use PRTime in nsLocalFile for modification time/lock time

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

Sigh...

::: xpcom/io/nsLocalFileUnix.cpp
@@ +1007,5 @@
>  
>      struct STAT sbuf;
>      if (LSTAT(mPath.get(), &sbuf) == -1)
>          return NSRESULT_FOR_ERRNO();
> +    *aLastModTimeOfLink = sbuf.st_mtime * PR_MSEC_PER_SEC;

Please use PRTime casts here.

Comment 19

[(no longer active)]

Comment on attachment 656051 [details] [diff] [review]
Restore a pruint64 to please nss

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

What is the error message which causes this to be required?

Comment 20

[Landry Breuil (:gaston)]

(In reply to Ehsan Akhgari [:ehsan] from comment #19)
> Comment on attachment 656051 [details] [diff] [review]
> Restore a pruint64 to please nss
> 
> Review of attachment 656051 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> What is the error message which causes this to be required?

security/manager/ssl/src/nsIdentityChecking.cpp: In member function 'nsresult nsNSSCertificate::hasValidEVOidTag(SECOidTag&, bool&)':
security/manager/ssl/src/nsIdentityChecking.cpp:1156: error: invalid conversion from 'uint64_t*' to 'PRUint64*'
security/manager/ssl/src/nsIdentityChecking.cpp:1163: error: invalid conversion from 'uint64_t*' to 'PRUint64*'

Iirc i tried casting it with reinterpret_cast<PRUint64*> but it also failed.

Comment 21

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 656051 [details] [diff] [review]
Restore a pruint64 to please nss

I think this is the way to go until NSS moves into this century.

Comment 22

[(no longer active)]

Comment on attachment 656051 [details] [diff] [review]
Restore a pruint64 to please nss

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

I really really think this is the wrong thing to do, and I'm only r+'ing this for the sake of the OpenBSD port, and because it seems impossible to make NSPR do the sane thing here.  Please add a big scary comment here to describe why we're using PRUint64 here, and reference bug 634793, and let's cross our fingers and hope that we can fix this some day. :(

Comment 23

[Landry Breuil (:gaston)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/fc8e76eb1479
https://hg.mozilla.org/integration/mozilla-inbound/rev/b523c75e74b8
https://hg.mozilla.org/integration/mozilla-inbound/rev/78648d135093
https://hg.mozilla.org/integration/mozilla-inbound/rev/5108cd2d7306
https://hg.mozilla.org/integration/mozilla-inbound/rev/989f2d81bd15
https://hg.mozilla.org/integration/mozilla-inbound/rev/6442238886ea

The only 'cosmetic' change i did was the nss comment :
+  // We need a PRUint64 here instead of a nice int64_t (until bug 634793 is
+  // fixed) to match the type used in security/nss/lib/certdb/certt.h for
+  // cert_rev_flags_per_method.

and moving the rdf/datasource/src/nsFileSystemDataSource.cpp change from part 5/rdf to Part 3/nsIFile patch/changeset.

Comment 24

[Landry Breuil (:gaston)]

Comment on attachment 656054 [details] [diff] [review]
Use PRTime under places/ and add reinterpret_casts

[valid for all 6 patches]
[Approval Request Comment]
Bug caused by (feature/regressing bug #):

579517 & 634793
 
User impact if declined: 

m-a wont build with gcc 4.2 on OpenBSD, see http://buildbot.rhaalovely.net/builders/mozilla-aurora-amd64 since the 28/8.

Testing completed (on m-c, etc.): 

In progress in https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=6442238886ea, will post the link when it migrates to m-c. The patchset also went to try, and i've put it on my openbsd/aurora buildslave in http://buildbot.rhaalovely.net/builders/mozilla-aurora-amd64/builds/484 to ensure it's green again there.

Risk to taking this patch (and alternatives if risky): 

Only types/casts changes that dont affect tier1 platforms, since PRTime/PRInt64 is equivalent to int64_t there.

String or UUID changes made by this patch: 
none

Comment 25

[Landry Breuil (:gaston)]

http://buildbot.rhaalovely.net/builders/mozilla-aurora-amd64/builds/484 is all green for me, so the patchset backported unmodified from central correctly fixes aurora for me.

Comment 26

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/fc8e76eb1479
https://hg.mozilla.org/mozilla-central/rev/b523c75e74b8
https://hg.mozilla.org/mozilla-central/rev/78648d135093
https://hg.mozilla.org/mozilla-central/rev/5108cd2d7306
https://hg.mozilla.org/mozilla-central/rev/989f2d81bd15
https://hg.mozilla.org/mozilla-central/rev/6442238886ea

Comment 27

[Landry Breuil (:gaston)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/b2565a78fe82
https://hg.mozilla.org/releases/mozilla-aurora/rev/b984c1076c94
https://hg.mozilla.org/releases/mozilla-aurora/rev/788cfcab1d2a
https://hg.mozilla.org/releases/mozilla-aurora/rev/87af6afc89c6
https://hg.mozilla.org/releases/mozilla-aurora/rev/7d439b0dadf6
https://hg.mozilla.org/releases/mozilla-aurora/rev/31341ab65f0c

Comment 28

[Wan-Teh Chang]

Attachment: Restore the use of PRUint64 in nsIdentityChecking.cpp to match the type used by NSS

The patch "Restore a pruint64 to please nss" (attachment 656051 [details] [diff] [review])
is a partial fix to the problem introduced by the automated change
of PRUint64 to uint64_t in
http://hg.mozilla.org/mozilla-central/diff/a16372ce30b5/security/manager/ssl/src/nsIdentityChecking.cpp

This patch completes the fix and also removes the incorrect comment.

Those three PRUint64 variables are being assigned to NSS structure
members that have the PRUint64 type. Therefore they should be PRUint64.
uint64_t is nicer in general, but not when we are working with something
that is declared to be a PRUint64.

Comment 29

[(no longer active)]

Comment on attachment 689950 [details] [diff] [review]
Restore the use of PRUint64 in nsIdentityChecking.cpp to match the type used by NSS

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

I don't understand the motivation behind this patch.  After your other patch here, PRUint64 and unit64_t will be the same type in OpenBSD as well, right?  With that, the compiler will be more than happy to let us use uint64_t.  Now it's true that for an API standpoint, it's more kosher to not do that, but we've decided to use stdint types everywhere.  This patch really defeats the purpose of this bug.  :-)

(You don't need to ask for superreview on patches that do not change our APIs...)

Comment 30

[Wan-Teh Chang]

Comment on attachment 689950 [details] [diff] [review]
Restore the use of PRUint64 in nsIdentityChecking.cpp to match the type used by NSS

If I use a library Foo, with this function and this structure:

typedef int FooInt32;

struct FooPoint {
  FooInt32 x,
  FooInt32 y
};

void FOO_GetXCoordinate(const FooPoint *point, FooInt32 *x);

I will write my code using FooInt32 as the type of an x
coordinate variable, unless it is inconvenient to do so:

  FooInt32 x;

  FOO_GetXCoordinate(point, &x);

In nsIdentityChecking.cpp, it is very natural to use
PRUint64 for the three variables in question, because
they are used with NSS, which uses NSPR's fixed-width
integer types.

As another example, in Windows code it is very natural
to use DWORD variables with Win32 functions, even though
we know DWORD is the same as unsigned int. Can you imagine
someone proposing changing DWORD to unsigned int globally
in the source tree?

I don't understand why you object to the use of
PRUint64 in code that directly deals with NSS. Please
reconsider. Thanks.

Comment 31

[Landry Breuil (:gaston)]

To my understanding PRTypes use outside of nspr/nss in m-c should die (after all, that was the intent of the stdint types migration).

I'd rather go the opposite way and revert https://hg.mozilla.org/integration/mozilla-inbound/rev/989f2d81bd15 now that it works on OpenBSD, instead of reintroducing PRUint64 here.

Comment 32

[(no longer active)]

Unfortunately, typedef doesn't actually define a new type, it just defines another name for the same type, so this issue does not matter as far as the compiler is concerned.  It matters a lot to the cleanliness of our code base, though, since we don't want everyone to decide whether they should use an NSPR type of a C++ type, and when in doubt, you should always prefer the latter.  I think comment 31 reflects what we want to do here instead.