Last Comment Bug 788012 - Build failure after bug 579517
: Build failure after bug 579517
Status: RESOLVED FIXED
:
Product: MailNews Core
Classification: Components
Component: Build Config (show other bugs)
: unspecified
: x86 OpenBSD
: -- normal (vote)
: Thunderbird 18.0
Assigned To: Landry Breuil (:gaston)
:
Mentors:
Depends on: 634793
Blocks: stdint
  Show dependency treegraph
 
Reported: 2012-09-03 13:45 PDT by Landry Breuil (:gaston)
Modified: 2012-10-08 00:05 PDT (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
unaffected
fixed
fixed
fixed


Attachments
Fix build on OpenBSD after bug 579517 (4.88 KB, patch)
2012-09-04 23:16 PDT, Landry Breuil (:gaston)
ehsan: review+
neil: review+
bugspam.Callek: feedback-
Details | Diff | Splinter Review
Fix build on OpenBSD after bug 579517 (c-a patch) (4.87 KB, patch)
2012-09-05 06:09 PDT, Landry Breuil (:gaston)
ehsan: review+
standard8: approval‑comm‑aurora+
Details | Diff | Splinter Review
fix another print64/int64_t mixup (1.10 KB, patch)
2012-09-08 02:27 PDT, Landry Breuil (:gaston)
bugspam.Callek: review+
standard8: approval‑comm‑aurora+
Details | Diff | Splinter Review

Description Landry Breuil (:gaston) 2012-09-03 13:45:34 PDT
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) 2012-09-04 23:16:33 PDT
Created attachment 658384 [details] [diff] [review]
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) 2012-09-04 23:51:23 PDT
/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) 2012-09-05 05:19:08 PDT
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) 2012-09-05 06:09:09 PDT
Created attachment 658463 [details] [diff] [review]
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 :Ehsan Akhgari 2012-09-05 16:44:54 PDT
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 :Ehsan Akhgari 2012-09-05 16:45:58 PDT
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) 2012-09-06 12:39:39 PDT
https://hg.mozilla.org/comm-central/rev/66edec66017c
Comment 8 Landry Breuil (:gaston) 2012-09-08 00:16:02 PDT
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) 2012-09-08 00:24:36 PDT
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) 2012-09-08 01:10:02 PDT
(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) 2012-09-08 02:27:16 PDT
Created attachment 659464 [details] [diff] [review]
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) 2012-09-08 02:33:27 PDT
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) 2012-09-08 16:09:46 PDT
(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) 2012-09-09 12:59:18 PDT
 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 neil@parkwaycc.co.uk 2012-09-10 05:13:30 PDT
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) 2012-09-10 22:28:49 PDT
(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) 2012-10-05 02:28:22 PDT
Callek, ping comm-aurora a? would be a pity to miss next uplift.
Comment 18 Justin Wood (:Callek) 2012-10-05 02:29:42 PDT
(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) 2012-10-05 03:02:50 PDT
So is this bug fixed now? Also, exactly which patches need approval?
Comment 20 Landry Breuil (:gaston) 2012-10-05 03:44:09 PDT
att 658463 needs approval, the bug is fixed in c-c but pending in c-a.
Comment 21 Mark Banner (:standard8) 2012-10-05 08:35:20 PDT
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] [:flo] 2012-10-05 11:19:06 PDT
https://hg.mozilla.org/releases/comm-aurora/rev/c49b8f67479f
Comment 23 Landry Breuil (:gaston) 2012-10-07 04:48:04 PDT
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) 2012-10-07 20:18:54 PDT
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) 2012-10-07 20:51:11 PDT
http://hg.mozilla.org/releases/comm-aurora/rev/e0369f027737

Note You need to log in before you can comment on or make changes to this bug.