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

Status: RESOLVED FIXED

(Core :: MFBT, defect)

Description

[Andrew Paprocki]

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.

Comment 1

[Andrew Paprocki]

Attachment: Fix madvise() cast to caddr_t on Solaris

Comment 2

[Jason Orendorff [: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.

Comment 3

[Petr Sumbera]

Attachment: Bug1158445.patch

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)

Comment 4

[Petr Sumbera]

Attachment: Bug1158445.patch

Included another occurrence of madvise() in MFBT.

Comment 5

[Jan de Mooij [:jandem]]

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.

Comment 6

[Petr Sumbera]

Thank you for the review! Can you please add checkin-needed keyword?

Comment 7

[Nathan Froyd [:froydnj]]

(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!

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/51c81beac6aa
posix_madvise() should be used instead of madvise in Poison code; r=froydnj

Comment 9

[Sebastian Hengst [:aryx] (needinfo on intermittent or backout)]

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

Comment 10

[Nathan Froyd [:froydnj]]

(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.

Comment 11

[Andrew Paprocki]

It looks like Android implemented it in 2014:

https://android.googlesource.com/platform/bionic/+/5afae64%5E%21/

Comment 12

[Nathan Froyd [:froydnj]]

(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.

Comment 13

[Petr Sumbera]

Attachment: Bug1158445.patch

Hope that this one will be ok.

Comment 14

[Nathan Froyd [:froydnj]]

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?

Comment 15

[Petr Sumbera]

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

[Mike Hommey [:glandium]]

HPUX and AIX are not supported.

Comment 17

[Petr Sumbera]

Attachment: Bug1158445.patch

Comment 18

[Petr Sumbera]

Can you please add checkin-needed keyword?

Comment 19

[Chris Peterson [:cpeterson]]

(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.

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c4a44e35800a
posix_madvise() should be used instead of madvise on Solaris. r=froydnj, r=jandem

Comment 21

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/c4a44e35800a

Comment 22

[Julien Cristau [:jcristau] (back August 2)]

(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?

Comment 23

[Chris Peterson [: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.

Comment 24

[Chris Peterson [: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.

Comment 25

[Petr Sumbera]

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

[Julien Cristau [:jcristau] (back August 2)]

Comment on attachment 8876034 [details] [diff] [review]
Bug1158445.patch

use posix_madvise instead of madvise on solaris, beta55+

esr52- per comment 25.

Comment 27

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/3ac23bd29301