788012 - Build failure after bug 579517

Status: RESOLVED FIXED

(MailNews Core :: Build Config, defect)

Description

[Landry Breuil (:gaston)]

Similar to but 785738, c-c now fails on openbsd/clang due to prtime being different from int64_t, and probably others. 

/var/buildslave-mozilla/comm-central-amd64/build/mailnews/base/src/nsMsgPurgeService.cpp:199:17: error: no matching function for call to 'PR_ParseTimeString'
                PR_ParseTimeString(curFolderLastPurgeTimeString.get(), false, &theTime);
                ^~~~~~~~~~~~~~~~~~
../../../mozilla/dist/include/prtime.h:244:20: note: candidate function not viable: no known conversion from 'int64_t *' (aka 'long long *') to 'PRTime *' (aka 'long *') for 3rd argument; 
NSPR_API(PRStatus) PR_ParseTimeString (
                   ^
/var/buildslave-mozilla/comm-central-amd64/build/mailnews/base/src/nsMsgPurgeService.cpp:290:11: error: no matching function for call to 'PR_ParseTimeString'
          PR_ParseTimeString(curJunkFolderLastPurgeTimeString.get(), false, &theTime);
          ^~~~~~~~~~~~~~~~~~
../../../mozilla/dist/include/prtime.h:244:20: note: candidate function not viable: no known conversion from 'int64_t *' (aka 'long long *') to 'PRTime *' (aka 'long *') for 3rd argument; 
NSPR_API(PRStatus) PR_ParseTimeString (
                   ^
2 errors generated.

Working on a patch...

Comment 1

[Landry Breuil (:gaston)]

Attachment: Fix build on OpenBSD after bug 579517

Dunno if i should ask a c-c peer .. this diff fixes the build for me on openbsd/clang

Comment 2

[Landry Breuil (:gaston)]

/home/landry/src/comm-central/mailnews/imap/src/nsAutoSyncManager.cpp:108:20: error: cannot initialize a parameter of type 'PRTime *' (aka 'long *') with an rvalue of type 'int64_t *' (aka 'long long *')

/home/landry/src/comm-central/mailnews/local/src/nsMsgBrkMBoxStore.cpp:177:38: error: cannot initialize a parameter of type 'PRTime *' (aka 'long *') with an rvalue of type 'int64_t *' (aka 'long long *')
/home/landry/src/comm-central/mailnews/local/src/nsMsgMaildirStore.cpp:613:11: error: call to member function 'AppendInt' is ambiguous
newName.AppendInt(PR_Now());

Were the corresponding failure messages.

Comment 3

[Landry Breuil (:gaston)]

comm-aurora with gcc 4.2 is affected too, the same fix will probably do the trick :

/usr/ports/pobj/comm-aurora/mailnews/base/src/nsMsgPurgeService.cpp: In member function 'nsresult nsMsgPurgeService::PerformPurge()':
/usr/ports/pobj/comm-aurora/mailnews/base/src/nsMsgPurgeService.cpp:199: error: invalid conversion from 'int64_t*' to 'PRTime*'
/usr/ports/pobj/comm-aurora/mailnews/base/src/nsMsgPurgeService.cpp:290: error: invalid conversion from 'int64_t*' to 'PRTime*'

Comment 4

[Landry Breuil (:gaston)]

Attachment: Fix build on OpenBSD after bug 579517 (c-a patch)

[Approval Request Comment]
Regression caused by (bug #): 579517
User impact if declined: c-a failure on OpenBSD/gcc 4.2.1
Testing completed (on c-c, etc.): in progress
Risk to taking this patch (and alternatives if risky): none, wont affect tier1 platforms for which those types are the same.

Comment 5

[(no longer active)]

Comment on attachment 658384 [details] [diff] [review]
Fix build on OpenBSD after bug 579517

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

I'm technically not a peer of anything in c-c, but I know some of them.  ;-)

Comment 6

[(no longer active)]

Comment on attachment 658463 [details] [diff] [review]
Fix build on OpenBSD after bug 579517 (c-a patch)

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

(You don't need to ask for a separate review on branch patches if they're practically the same as trunk patches...)

Comment 7

[Landry Breuil (:gaston)]

https://hg.mozilla.org/comm-central/rev/66edec66017c

Comment 8

[Landry Breuil (:gaston)]

it seems i've missed one, dunno how it wasnt catched.. or the PRInt64 was brought by new code ?

/var/buildslave-mozilla/comm-central-amd64/build/mailnews/imap/src/nsImapMailFolder.cpp:8261:26: error: cannot initialize a parameter of type 'int64_t *' (aka 'long long *') with an rvalue of type 'PRInt64 *' (aka 'long *')
          seekable->Tell(&curOfflineStorePos);
                         ^~~~~~~~~~~~~~~~~~~

Comment 9

[Justin Wood (:Callek)]

Comment on attachment 658384 [details] [diff] [review]
Fix build on OpenBSD after bug 579517

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

So I know this already landed, but I'm not sure why it is ONLY failing for you, when it works fine on all the official mozilla machines.

I don't have the best comprehension of this C++ magic, so I want Neil [or another real peer of this code] to give it a proper look before *anyone* approves it on the branch

::: mailnews/local/src/nsMsgMaildirStore.cpp
@@ +609,5 @@
>    }
>  
>    // generate new file name
>    nsAutoCString newName;
> +  newName.AppendInt(static_cast<int64_t>(PR_Now()));

In addition to the above this worries me, PR_Now returns PRTime, which is typedefed to PRInt64 which is *not* always an int64_t compat type, where it can be long long, among other things (see: http://mxr.mozilla.org/comm-central/source/mozilla/nsprpub/pr/include/prtypes.h#340 )

So this static_cast feels dangerous if its needed, since it circumvents the normal checks and bounds here

Comment 10

[Landry Breuil (:gaston)]

(In reply to Justin Wood (:Callek) from comment #9)
> Comment on attachment 658384 [details] [diff] [review]
> Fix build on OpenBSD after bug 579517
> 
> Review of attachment 658384 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> So I know this already landed, but I'm not sure why it is ONLY failing for
> you, when it works fine on all the official mozilla machines.

Because on OpenBSD PRInt64 != int64_t, and on all official mozilla machines the types are equivalent and thus don't produce those failures. See bug 785738 and bug 634793 for the root cause of all my pain.

> In addition to the above this worries me, PR_Now returns PRTime, which is
> typedefed to PRInt64 which is *not* always an int64_t compat type, where it
> can be long long, among other things (see:
> http://mxr.mozilla.org/comm-central/source/mozilla/nsprpub/pr/include/
> prtypes.h#340 )
> 
> So this static_cast feels dangerous if its needed, since it circumvents the
> normal checks and bounds here

The same 'fixes' were applied on m-c.

Comment 11

[Landry Breuil (:gaston)]

Attachment: fix another print64/int64_t mixup

That one fixes the aforementioned error. c-c builds again for me on openbsd. Pending discussion on the other issue....

Comment 12

[Justin Wood (:Callek)]

Comment on attachment 659464 [details] [diff] [review]
fix another print64/int64_t mixup

I am not an expert so I still want Neil to look at this bug but this command anyway until he does

Comment 13

[Justin Wood (:Callek)]

(In reply to Justin Wood (:Callek) from comment #12)
> Comment on attachment 659464 [details] [diff] [review]
> fix another print64/int64_t mixup
> 
> I am not an expert so I still want Neil to look at this bug but this command
> anyway until he does

Bah - THIS is why I should never try to review from mobile. Let me repeat what I said, only clearer and correctly:

I'm no expert with Cpp unfortunately, so I'd still like to keep the previous review request on Neil open to help check my sanity in my above comments. However given the other patch landed and this unblocks you, lets land this self-contained change anyway.

Comment 14

[Landry Breuil (:gaston)]

 https://hg.mozilla.org/comm-central/rev/e3c08e7b8ebc

i of course need to recheck aurora to see if this chunk needs to be added to the patch waiting approval...

Comment 15

[[e-mail]]

Comment on attachment 658384 [details] [diff] [review]
Fix build on OpenBSD after bug 579517

>-  newName.AppendInt(PR_Now());
>+  newName.AppendInt(static_cast<int64_t>(PR_Now()));
Callek: AppendInt takes an int32_t, uint32_t, int64_t or uint64_t. If PR_Now() doesn't return one of those, you'll get an ambiguous cast, so you have to cast it manually.

Comment 16

[Landry Breuil (:gaston)]

(In reply to Landry Breuil (:gaston) from comment #14)
>  https://hg.mozilla.org/comm-central/rev/e3c08e7b8ebc
> 
> i of course need to recheck aurora to see if this chunk needs to be added to
> the patch waiting approval...

That pruint64 is not present in comm-aurora, so my approval-comm-aurora request stands for attachment 658463 [details] [diff] [review].

Comment 17

[Landry Breuil (:gaston)]

Callek, ping comm-aurora a? would be a pity to miss next uplift.

Comment 18

[Justin Wood (:Callek)]

(In reply to Landry Breuil (:gaston) from comment #17)
> Callek, ping comm-aurora a? would be a pity to miss next uplift.

I can't approve mailnews/ patches -- standard8: ping

Comment 19

[Mark Banner (:standard8)]

So is this bug fixed now? Also, exactly which patches need approval?

Comment 20

[Landry Breuil (:gaston)]

att 658463 needs approval, the bug is fixed in c-c but pending in c-a.

Comment 21

[Mark Banner (:standard8)]

Ok, thanks, if its fixed in comm-central, then we normally mark the bug as fixed - branches are tracked by the status field.

Comment 22

[Florian Quèze [:florian]]

https://hg.mozilla.org/releases/comm-aurora/rev/c49b8f67479f

Comment 23

[Landry Breuil (:gaston)]

Comment on attachment 659464 [details] [diff] [review]
fix another print64/int64_t mixup

After a recheck, it seems this chunk is needed too on comm-aurora. In the hope i can slip it in before the next uplift...

[Approval Request Comment]
Regression caused by (bug #): 579517
User impact if declined: c-a failure on OpenBSD/gcc 4.2.1
Testing completed (on c-c, etc.): done, fixes build
Risk to taking this patch (and alternatives if risky): none, wont affect tier1 platforms for which those types are the same.

Comment 24

[Justin Wood (:Callek)]

Comment on attachment 659464 [details] [diff] [review]
fix another print64/int64_t mixup

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

I can't officially set this [yet - filed Bug 798996 for that], but I'm giving it a+=me

Comment 25

[Justin Wood (:Callek)]

http://hg.mozilla.org/releases/comm-aurora/rev/e0369f027737