Closed
Bug 1158445
Opened 8 years ago
Closed 6 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•8 years ago
|
||
Attachment #8597580 -
Flags: review?(jorendorff)
Comment 2•8 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•6 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•6 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•6 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•6 years ago
|
Attachment #8874447 -
Flags: review?(nfroyd) → review+
Assignee | ||
Comment 6•6 years ago
|
||
Thank you for the review! Can you please add checkin-needed keyword?
![]() |
||
Updated•6 years ago
|
Attachment #8597580 -
Attachment is obsolete: true
![]() |
||
Comment 7•6 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•6 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•6 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•6 years ago
|
||
It looks like Android implemented it in 2014: https://android.googlesource.com/platform/bionic/+/5afae64%5E%21/
![]() |
||
Comment 12•6 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•6 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•6 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•6 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•6 years ago
|
||
HPUX and AIX are not supported.
Assignee | ||
Comment 17•6 years ago
|
||
Attachment #8875615 -
Attachment is obsolete: true
Attachment #8876034 -
Flags: review?(nfroyd)
![]() |
||
Updated•6 years ago
|
Attachment #8876034 -
Flags: review?(nfroyd) → review+
Assignee | ||
Comment 18•6 years ago
|
||
Can you please add checkin-needed keyword?
Flags: needinfo?(nfroyd)
Comment 19•6 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•6 years ago
|
Updated•6 years ago
|
Assignee: andrew → petr.sumbera
Comment 20•6 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•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/c4a44e35800a
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
Comment 22•6 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•6 years ago
|
Comment 23•6 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•6 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•6 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•6 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•6 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-beta/rev/3ac23bd29301
You need to log in
before you can comment on or make changes to this bug.
Description
•