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 ?

> 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.

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 ?

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 ?

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 ?

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..

INT64_MIN / UINT64_MIN probably works too

(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?

(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..)

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

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.

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.

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

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

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.

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

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 :)

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

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 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?

(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 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 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. :(

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 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

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.

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

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

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 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 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.

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.

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.