Closed
Bug 1158445
Opened 10 years ago
Closed 8 years ago
MFBT madvise calls fail to compile on Solaris due to void* cast
Categories
(Core :: MFBT, defect)
Tracking
()
RESOLVED
FIXED
mozilla56
Tracking | Status | |
---|---|---|
firefox40 | --- | wontfix |
firefox-esr52 | --- | fix-optional |
firefox54 | --- | wontfix |
firefox55 | --- | fixed |
firefox56 | --- | fixed |
People
(Reporter: andrew, Assigned: petr.sumbera)
Details
Attachments
(1 file, 4 obsolete files)
1.25 KB,
patch
|
froydnj
:
review+
jcristau
:
approval-mozilla-beta+
jcristau
:
approval-mozilla-esr52-
|
Details | Diff | Splinter Review |
The madvise() calls in mfbt/Poison.cpp and mfbt/tests/TestPoisonArea.cpp fail to compile on Solaris due to the void* cast. The first parameter to madvise() on Solaris should be type caddr_t.
Reporter | ||
Comment 1•10 years ago
|
||
Attachment #8597580 -
Flags: review?(jorendorff)
Comment 2•10 years ago
|
||
Comment on attachment 8597580 [details] [diff] [review]
Fix madvise() cast to caddr_t on Solaris
Review of attachment 8597580 [details] [diff] [review]:
-----------------------------------------------------------------
OK.
A few pre-existing hacks for Solaris use #ifdef __SUNPRO_CC, but what we care about here is the OS, not the compiler. #ifdef __sun should be fine.
Attachment #8597580 -
Flags: review?(jorendorff) → review+
Assignee | ||
Comment 3•8 years ago
|
||
I think better solution than using #ifdefs is to use posix_madvise() which have the same parameters on all platforms.
The only problem with posix_madvise() is semantic problem on Linux for MADV_DONTNEED [1]. Which is not the case here.
[1] http://man7.org/linux/man-pages/man3/posix_madvise.3.html (see NOTES)
Attachment #8869987 -
Flags: review?(jorendorff)
Assignee | ||
Comment 4•8 years ago
|
||
Included another occurrence of madvise() in MFBT.
Attachment #8869987 -
Attachment is obsolete: true
Attachment #8869987 -
Flags: review?(jorendorff)
Attachment #8874447 -
Flags: review?(jorendorff)
Attachment #8874447 -
Flags: review?(jdemooij)
Comment 5•8 years ago
|
||
Comment on attachment 8874447 [details] [diff] [review]
Bug1158445.patch
Review of attachment 8874447 [details] [diff] [review]:
-----------------------------------------------------------------
LGTM but I'm not an MFBT peer so I'll request an additional review from Nathan.
Attachment #8874447 -
Flags: review?(nfroyd)
Attachment #8874447 -
Flags: review?(jorendorff)
Attachment #8874447 -
Flags: review?(jdemooij)
Attachment #8874447 -
Flags: review+
![]() |
||
Updated•8 years ago
|
Attachment #8874447 -
Flags: review?(nfroyd) → review+
Assignee | ||
Comment 6•8 years ago
|
||
Thank you for the review! Can you please add checkin-needed keyword?
![]() |
||
Updated•8 years ago
|
Attachment #8597580 -
Attachment is obsolete: true
![]() |
||
Comment 7•8 years ago
|
||
(In reply to Petr Sumbera from comment #6)
> Thank you for the review! Can you please add checkin-needed keyword?
Started a try run just to make sure things build OK, and I'll commit after everything comes back green:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=291b69b183a35831ab0cd28e9bebc730bf00386e
Thanks for the patch!
Pushed by nfroyd@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/51c81beac6aa
posix_madvise() should be used instead of madvise in Poison code; r=froydnj
![]() |
||
Comment 9•8 years ago
|
||
Backed out for bustage on Android in mfbt/Poison.cpp:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ea32af8eee1459bdcbe85c8a3e7b78b374802d1c
Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=51c81beac6aa43f61b67adc5bc0692476759246f&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=105213827&repo=mozilla-inbound
[task 2017-06-07T15:53:35.149179Z] 15:53:35 INFO - In file included from /home/worker/workspace/build/src/obj-firefox/mfbt/Unified_cpp_mfbt0.cpp:47:0:
[task 2017-06-07T15:53:35.153086Z] 15:53:35 INFO - /home/worker/workspace/build/src/mfbt/Poison.cpp: In function 'bool ProbeRegion(uintptr_t, uintptr_t)':
[task 2017-06-07T15:53:35.153153Z] 15:53:35 INFO - /home/worker/workspace/build/src/mfbt/Poison.cpp:132:62: error: 'POSIX_MADV_NORMAL' was not declared in this scope
[task 2017-06-07T15:53:35.153200Z] 15:53:35 INFO - if (posix_madvise(reinterpret_cast<void*>(aRegion), aSize, POSIX_MADV_NORMAL)) {
[task 2017-06-07T15:53:35.153251Z] 15:53:35 INFO - ^
[task 2017-06-07T15:53:35.153316Z] 15:53:35 INFO - /home/worker/workspace/build/src/mfbt/Poison.cpp:132:79: error: 'posix_madvise' was not declared in this scope
[task 2017-06-07T15:53:35.153376Z] 15:53:35 INFO - if (posix_madvise(reinterpret_cast<void*>(aRegion), aSize, POSIX_MADV_NORMAL)) {
[task 2017-06-07T15:53:35.153430Z] 15:53:35 INFO - ^
[task 2017-06-07T15:53:35.153472Z] 15:53:35 INFO - /home/worker/workspace/build/src/mfbt/Poison.cpp:137:1: error: control reaches end of non-void function [-Werror=return-type]
[task 2017-06-07T15:53:35.153487Z] 15:53:35 INFO - }
[task 2017-06-07T15:53:35.153501Z] 15:53:35 INFO - ^
[task 2017-06-07T15:53:35.153522Z] 15:53:35 INFO - cc1plus: all warnings being treated as errors
Flags: needinfo?(nfroyd)
![]() |
||
Comment 10•8 years ago
|
||
(In reply to Sebastian Hengst [:aryx][:archaeopteryx] (needinfo on intermittent or backout) from comment #9)
> Backed out for bustage on Android in mfbt/Poison.cpp:
>
> https://hg.mozilla.org/integration/mozilla-inbound/rev/
> ea32af8eee1459bdcbe85c8a3e7b78b374802d1c
>
> Push with failures:
> https://treeherder.mozilla.org/#/jobs?repo=mozilla-
> inbound&revision=51c81beac6aa43f61b67adc5bc0692476759246f&filter-
> resultStatus=testfailed&filter-resultStatus=busted&filter-
> resultStatus=exception&filter-resultStatus=retry&filter-
> resultStatus=usercancel&filter-resultStatus=runnable
> Failure log:
> https://treeherder.mozilla.org/logviewer.html#?job_id=105213827&repo=mozilla-
> inbound
Yup, forgot to test Android. :( Looks like posix_madvise isn't available there, so we'll have to come up with a different solution.
Flags: needinfo?(nfroyd) → needinfo?(petr.sumbera)
Reporter | ||
Comment 11•8 years ago
|
||
It looks like Android implemented it in 2014:
https://android.googlesource.com/platform/bionic/+/5afae64%5E%21/
![]() |
||
Comment 12•8 years ago
|
||
(In reply to Andrew Paprocki from comment #11)
> It looks like Android implemented it in 2014:
>
> https://android.googlesource.com/platform/bionic/+/5afae64%5E%21/
We're including <sys/mman.h>, it appears, so perhaps the version of Android we're targeting doesn't support it.
Assignee | ||
Comment 13•8 years ago
|
||
Hope that this one will be ok.
Attachment #8874447 -
Attachment is obsolete: true
Flags: needinfo?(petr.sumbera)
Attachment #8875615 -
Flags: review?(nfroyd)
![]() |
||
Comment 14•8 years ago
|
||
Comment on attachment 8875615 [details] [diff] [review]
Bug1158445.patch
Review of attachment 8875615 [details] [diff] [review]:
-----------------------------------------------------------------
At this point, why not just go with Andrew's original patch, as the intent there is more obvious?
Attachment #8875615 -
Flags: review?(nfroyd)
Assignee | ||
Comment 15•8 years ago
|
||
Ok. I can do that. I might try whether we can use XP_SOLARIS instead of __sun. But the same problem with madvise beside Solaris have also HPUX and AIX. And at least AIX seems to have posix_madvise so it would be help it too.
Comment 16•8 years ago
|
||
HPUX and AIX are not supported.
Assignee | ||
Comment 17•8 years ago
|
||
Attachment #8875615 -
Attachment is obsolete: true
Attachment #8876034 -
Flags: review?(nfroyd)
![]() |
||
Updated•8 years ago
|
Attachment #8876034 -
Flags: review?(nfroyd) → review+
Assignee | ||
Comment 18•8 years ago
|
||
Can you please add checkin-needed keyword?
Flags: needinfo?(nfroyd)
Comment 19•8 years ago
|
||
(In reply to Petr Sumbera from comment #18)
> Can you please add checkin-needed keyword?
Thanks for the reminder! :)
This is a straightforward fix. We can ask to uplift it to Beta 55 after it lands in Nightly.
Flags: needinfo?(nfroyd)
Keywords: checkin-needed
Updated•8 years ago
|
Updated•8 years ago
|
Assignee: andrew → petr.sumbera
Comment 20•8 years ago
|
||
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c4a44e35800a
posix_madvise() should be used instead of madvise on Solaris. r=froydnj, r=jandem
Keywords: checkin-needed
Comment 21•8 years ago
|
||
bugherder |
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
Comment 22•8 years ago
|
||
(In reply to Chris Peterson [:cpeterson] from comment #19)
> (In reply to Petr Sumbera from comment #18)
> > Can you please add checkin-needed keyword?
>
> Thanks for the reminder! :)
>
> This is a straightforward fix. We can ask to uplift it to Beta 55 after it
> lands in Nightly.
Want to do that now?
Flags: needinfo?(cpeterson)
Updated•8 years ago
|
Comment 23•8 years ago
|
||
(In reply to Julien Cristau [:jcristau] from comment #22)
> > This is a straightforward fix. We can ask to uplift it to Beta 55 after it
> > lands in Nightly.
>
> Want to do that now?
Sure. I'll request Beta uplift.
I don't know if we want this for ESR 52. It is not a security fix, but it is a small patch that fixes a Solaris compilation error. OTOH, this Solaris compilation error was reported two years ago and nobody seemed to care until Petr stepped forward to fix it.
Flags: needinfo?(cpeterson)
Comment 24•8 years ago
|
||
Comment on attachment 8876034 [details] [diff] [review]
Bug1158445.patch
[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration:
It is not a security fix, but it is a small patch that fixes a Solaris compilation error.
Fix Landed on Version: Nightly 56.
[Feature/Bug causing the regression]:
[User impact if declined]: No Firefox builds on Solaris!
[Is this code covered by automated tests?]: No. Solaris is a tier 3 platform so Mozilla does not compile or test it.
[Has the fix been verified in Nightly?]: I don't know if anyone has tried to compile Nightly 56 for Solaris.
[Needs manual test from QE? If yes, steps to reproduce]: No.
[List of other uplifts needed for the feature/fix]: No other uplifts.
[Is the change risky?]: No.
[Why is the change risky/not risky?]: Low risk because it just adds some code that is not compile on any of Mozilla's tier 1 platforms.
[String changes made/needed]: No string changes.
Attachment #8876034 -
Flags: approval-mozilla-esr52?
Attachment #8876034 -
Flags: approval-mozilla-beta?
Assignee | ||
Comment 25•8 years ago
|
||
Better not to backport this into 52ESR now. It wouldn't work on Solaris without Bug 1354510 (which defines XP_SOLARIS). Let's focus on next ESR instead. Thank you!
Comment 26•8 years ago
|
||
Comment on attachment 8876034 [details] [diff] [review]
Bug1158445.patch
use posix_madvise instead of madvise on solaris, beta55+
esr52- per comment 25.
Attachment #8876034 -
Flags: approval-mozilla-esr52?
Attachment #8876034 -
Flags: approval-mozilla-esr52-
Attachment #8876034 -
Flags: approval-mozilla-beta?
Attachment #8876034 -
Flags: approval-mozilla-beta+
Comment 27•8 years ago
|
||
bugherder uplift |
You need to log in
before you can comment on or make changes to this bug.
Description
•