1589975 - build failure due to use of undefined mmap flag (MAP_NORESERVE)

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[Marco Perez [:mpk]]

It looks like the changes introduced in bug 1527597 broke trunk builds on FreeBSD.
FreeBSD removed the mmap flag MAP_NORESERVE about 5 years ago from its code.
(See https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=193961 for more details.)

OpenBSD might be affected as well. The code probably builds fine on NetBSD, though.

Comment 1

[Landry Breuil (:gaston)]

m-c still fails on OpenBSD on a skia failure:

91:16.79 In file included from Unified_cpp_gfx_sfntly_cpp_src2.cpp:20:
91:16.79 /build/buildslave-amd64/mozilla-central-amd64/build/gfx/sfntly/cpp/src/sfntly/table/core/cmap_table.cc:442:21: error: reinterpret_cast from 'nullptr_t' to 'sfntly::ReadableFontData *' is not allowed
91:16.79     : CMap::Builder(reinterpret_cast<ReadableFontData*>(NULL),

as for MAP_NORESERVE, it's defined in sys/mman.h (cf http://cvsweb.openbsd.org/src/sys/sys/mman.h?rev=1.34&content-type=text/x-cvsweb-markup) but with this comment:

/*
 * Legacy defines for userland source compatibility.
 * Can be removed once no longer needed in base and ports.
 */

and the flag isnt documented in http://man.openbsd.org/mmap ... so i would say it shouldnt be used.

Comment 2

[Jan de Mooij [:jandem]]

We could do something like this:

int flags = ...;
#ifdef MAP_NORESERVE
flags |= MAP_NORESERVE;
#endif

Or check for the BSDs explicitly. Another option is to just check for Solaris, but it seems nice to pass the flag on Linux/Mac...

Comment 3

[Marco Perez [:mpk]]

Attachment: unbreak builds on systems not supporting the MAP_NORESERVE mmap flag

Passing the flag wherever it seems to be supported (and ignoring it wherever it isn't) seems to be the sanest way forward at this point. This way we avoid accidental build breakage in the most cases, I think.

Of course this might prevent us from looking for a cleaner solution in case platforms without support for the MAP_NORESERVE flag suffer from the original problem that affected Solaris builds (see bug 1527597). But I wouldn't want to open a can of worms.

As a regular (trunk) builder and occasional contributor I think it's important to keep the number of bugs that prevent us from building at a minimum. But since I'm no official maintainer I'd like to hear Jan's and Landry's opinion first, before requesting a review of the patch.

Comment 4

[Jan Beich]

Comment on attachment 9102977 [details] [diff] [review]
unbreak builds on systems not supporting the MAP_NORESERVE mmap flag

I had the same idea but not style. `flags` variable is not necessary:
```c++
  void* p = MozTaggedAnonymousMmap(randomAddr, bytes, PROT_NONE,
#  ifdef MAP_NORESERVE
                                   MAP_NORESERVE |
#  endif
                                       MAP_PRIVATE | MAP_ANON,
                                   -1, 0, "js-executable-memory");
```
or (which is more common)
```c++
#ifndef MAP_NORESERVE
#  define MAP_NORESERVE 0
#endif
```

Comment 5

[Jan de Mooij [:jandem]]

I prefer the flags variable over "expression splicing" (not sure if there's a better term). Defining MAP_NORESERVE is an interesting idea.

Comment 6

[Jan Beich]

(In reply to Marco Perez from comment #3)

Of course this might prevent us from looking for a cleaner solution in case platforms without support for the MAP_NORESERVE flag suffer from the original problem that affected Solaris builds (see bug 1527597). But I wouldn't want to open a can of worms.

On FreeBSD swap reservation is controlled by vm.overcommit (bitfield). When vm.overcommit=0 (default) behavior is similar to MAP_NORESERVE.

$ swapinfo
Device                1K-blocks     Used    Avail Capacity
$ sysctl vm.overcommit=1
<No swap -> every reservation fails -> system meltdown/crash>

$ swapinfo
Device                1K-blocks     Used    Avail Capacity
/dev/zvol/tank/swap    9437184        0  9437184     0%
/dev/zvol/tank/swap1  20971520        0 20971520     0%
Total                 30408704        0 30408704     0%

$ sysctl vm.overcommit=1
$ firefox
JavaScript warning: moz-extension://a6112896-bb84-455c-805b-34720f48074d/js/hntrie.js, line 516: WebAssembly.Memory failed to reserve a large virtual memory region. This may be due to low configured virtual memory limits on this system.
JavaScript error: , line 0: uncaught exception: out of memory
JavaScript warning: moz-extension://a6112896-bb84-455c-805b-34720f48074d/lib/publicsuffixlist/publicsuffixlist.js, line 577: WebAssembly.Memory failed to reserve a large virtual memory region. This may be due to low configured virtual memory limits on this system.
JavaScript warning: , line 0: WebAssembly.Memory failed to reserve a large virtual memory region. This may be due to low configured virtual memory limits on this system.
^C

$ sysctl vm.overcommit=2
$ limits -w 10m firefox
JavaScript warning: , line 0: WebAssembly.Memory failed to reserve a large virtual memory region. This may be due to low configured virtual memory limits on this system.
^C

Comment 7

[Marco Perez [:mpk]]

Thanks a lot for the thorough explanation on vm.overcommit. So it looks like FreeBSD (out of the box) should be safe from the problems Solaris suffered from.

Defining MAP_NORESERVE to 0 seems like an elegant aproach.

Comment 8

[Marco Perez [:mpk]]

Comment on attachment 9102977 [details] [diff] [review]
unbreak builds on systems not supporting the MAP_NORESERVE mmap flag

I was preparing an updated patch when it occurred to me that maybe we should define MAP_NORESERVE in a more central location, just in case it finds its way into the code somewhere else in the tree.

Comment 9

[Pascal Chevrel:pascalc (PTO until April 26)]

Marking as fix-optional for 71 to remove it from our weekly triage meeting but I would take a NPOTB patch for uplift.

Comment 10

[Marco Perez [:mpk]]

Attachment: unbreak_build__MAP_NORESERVE__v2.patch

Ok, it looks like harfbuzz dealt with the issue in a similar way and rust went to great lengths to solve the problem across a wide range of platforms. This patch employs Jan's 2nd proposal, but higher up in the non-windows section of the file. Even if the flag ever gets used in another function (within js/src/jit/ProcessExecutableMemory.cpp), then it hopefully won't break the build again.

Comment 11

[Marco Perez [:mpk]]

Comment on attachment 9103686 [details] [diff] [review]
unbreak_build__MAP_NORESERVE__v2.patch

Ooops! Ididn't realize patch review requires phabricator now. (It's been a while since my last patch.)
I'll provide the new patch via phabricator shortly. Sorry for the bugspam...

Comment 12

[Marco Perez [:mpk]]

Attachment: Bug 1589975 - Unbreak build on platforms not supporting the mmap flag MAP_NORESERVE. r=jandem

Unbreak builds on (non-Windows) platforms where mmap doesn't support the MAP_NORESERVE flag.
We know for sure that at least FreeBSD has been affected since the fix for bug #1527597 landed.

Comment 13

[Marco Perez [:mpk]]

Attachment: Bug 1589975 - Unbreak build on platforms not supporting the mmap flag MAP_NORESERVE. r=jandem

--
user: Marco Perez Kistler <[e-mail]>
branch 'default'
changed js/src/jit/ProcessExecutableMemory.cpp

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7a71fb6a8d55
Unbreak build on platforms not supporting the mmap flag MAP_NORESERVE. r=jandem

Comment 15

[Jan de Mooij [:jandem]]

Comment on attachment 9103914 [details]
Bug 1589975 - Unbreak build on platforms not supporting the mmap flag MAP_NORESERVE. r=jandem

Beta/Release Uplift Approval Request

• User impact if declined: Build failures on some platforms (FreeBSD for example).
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Shouldn't have any effect on our Tier 1 platforms.
• String changes made/needed:

Comment 16

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/7a71fb6a8d55

Comment 17

[Pascal Chevrel:pascalc (PTO until April 26)]

Comment on attachment 9103914 [details]
Bug 1589975 - Unbreak build on platforms not supporting the mmap flag MAP_NORESERVE. r=jandem

No impact on the builds mozilla ships and this covered by tests, approved for 71 beta 5, thanks.

Comment 18

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/releases/mozilla-beta/rev/f9408680c703