MFBT madvise calls fail to compile on Solaris due to void* cast

RESOLVED FIXED in Firefox 55

Status

()

defect
RESOLVED FIXED
4 years ago
2 years ago

People

(Reporter: andrew, Assigned: petr.sumbera)

Tracking

Trunk
mozilla56
Other
Other
Points:
---

Firefox Tracking Flags

(firefox40 wontfix, firefox-esr52 fix-optional, firefox54 wontfix, firefox55 fixed, firefox56 fixed)

Details

Attachments

(1 attachment, 4 obsolete attachments)

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+
Posted 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)
Posted 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.
Posted 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.
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
https://hg.mozilla.org/mozilla-central/rev/c4a44e35800a
Status: ASSIGNED → RESOLVED
Closed: 2 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.