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)

Other
Other
defect
Not set
normal

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)

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.
Attachment #8597580 - Flags: review?(jorendorff)
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+
Attached patch Bug1158445.patch (obsolete) — Splinter Review
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)
Attached patch Bug1158445.patch (obsolete) — Splinter Review
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 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+
Attachment #8874447 - Flags: review?(nfroyd) → review+
Thank you for the review! Can you please add checkin-needed keyword?
Attachment #8597580 - Attachment is obsolete: true
(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
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)
(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)
It looks like Android implemented it in 2014: https://android.googlesource.com/platform/bionic/+/5afae64%5E%21/
(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.
Attached patch Bug1158445.patch (obsolete) — Splinter Review
Hope that this one will be ok.
Attachment #8874447 - Attachment is obsolete: true
Flags: needinfo?(petr.sumbera)
Attachment #8875615 - Flags: review?(nfroyd)
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)
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.
HPUX and AIX are not supported.
Attached patch Bug1158445.patchSplinter Review
Attachment #8875615 - Attachment is obsolete: true
Attachment #8876034 - Flags: review?(nfroyd)
Attachment #8876034 - Flags: review?(nfroyd) → review+
Can you please add checkin-needed keyword?
Flags: needinfo?(nfroyd)
(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
Assignee: andrew → petr.sumbera
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
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
(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)
(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 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?
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 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+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: