1150659 - Build breakage on OpenBSD since jemalloc was enabled by default in bug 1134039

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Landry Breuil (:gaston)]

I dont know mozilla's stance wrt jemalloc, i never really looked into it since we're pretty happy with our own malloc, and i also find it somewhat stupid to not rely on the operating system malloc... but that said, build is broken by default for me since the 22 when bug 1134039 landed:

 1:29.84 js/src> configure: error: --enable-jemalloc not supported on i386-unknown-openbsd5.7

So i need to know if i should sprinkle --disable-jemalloc all around in my mozconfig, or if the commit can be amended for the unsupported platforms, or if there's a better way...

Comment 1

[Landry Breuil (:gaston)]

Attachment: disable-jemalloc

Here's what i had in mind, and a similar thing for toplevel configure.in; In the OpenBSD community, nobody trusts jemalloc as it is "finely tuned to turn off ASLR" (not my words) and the (supposed) performance improvements are not worth the risks/regression it brings. Personally, i think that not relying on the OS's malloc is wrong, so can we please disable it for OpenBSD at least ?

Comment 2

[Mike Hommey [:glandium]]

Comment on attachment 8587959 [details] [diff] [review]
disable-jemalloc

Review of attachment 8587959 [details] [diff] [review]:
-----------------------------------------------------------------

You should try something like this instead:

diff --git a/js/src/configure.in b/js/src/configure.in
index 978d370..61be6bb 100644
--- a/js/src/configure.in
+++ b/js/src/configure.in
@@ -3020,29 +3020,26 @@ if test "$MOZ_MEMORY"; then
   fi
   dnl The generic feature tests that determine how to compute ncpus are long and
   dnl complicated.  Therefore, simply define special cpp variables for the
   dnl platforms we have special knowledge of.
   case "${target}" in
   *-darwin*)
     AC_DEFINE(MOZ_MEMORY_DARWIN)
     ;;
-  *-*freebsd*)
+  *-*bsd*)
     AC_DEFINE(MOZ_MEMORY_BSD)
     ;;
   *-android*|*-linuxandroid*)
     AC_DEFINE(MOZ_MEMORY_LINUX)
     AC_DEFINE(MOZ_MEMORY_ANDROID)
     ;;
   *-*linux*)
     AC_DEFINE(MOZ_MEMORY_LINUX)
     ;;
-  *-netbsd*)
-    AC_DEFINE(MOZ_MEMORY_BSD)
-    ;;
   *-solaris*)
     AC_DEFINE(MOZ_MEMORY_SOLARIS)
     ;;
   *-mingw*)
     AC_DEFINE(MOZ_MEMORY_WINDOWS)
     ;;
   *)
     AC_MSG_ERROR([--enable-jemalloc not supported on ${target}])

Comment 3

[Jan Beich]

(In reply to Landry Breuil (:gaston) from comment #1)
>@@ -2999,16 +2999,21 @@ dnl ====================================
> dnl = Enable jemalloc
> dnl ========================================================
> MOZ_MEMORY=1

Yuck! mozjemalloc has bitrotten support for Tier3. I need to disable it on FreeBSD as well until bug 762451.

> MOZ_ARG_DISABLE_BOOL(jemalloc,
> [  --disable-jemalloc       Don't replace memory allocator with jemalloc],
>     MOZ_MEMORY=)
> 
> case "${OS_TARGET}" in
>+OpenBSD)
>+  dnl we dont want to use jemalloc on OpenBSD
>+  MOZ_MEMORY=

Opting out should be before MOZ_ARG_* handling to allow experimental builds with --enable-jemalloc. Example use case is debugging regressions when using system malloc (being non-default) in Firefox or in malloc itself (either one).

(In reply to Mike Hommey [:glandium] from comment #2)
> -  *-*freebsd*)
> +  *-*bsd*)
>      AC_DEFINE(MOZ_MEMORY_BSD)
>      ;;

DragonFly doesn't have BSD suffix in |uname -s|. Not sure if it's affected by bug 788955 comment 25.

https://github.com/DragonFlyBSD/DeltaPorts/blob/master/ports/www/firefox/dragonfly/patch-configure.in

Comment 4

[Jan Beich]

configure:MOZ_JEMALLOC="" vs. js/src/configure.in:MOZ_JEMALLOC=1 leads to an interesting error:

Traceback (most recent call last):
  File "./config.status", line 989, in <module>
    config_status(**args)
  File "python/mozbuild/mozbuild/config_status.py", line 149, in config_status
    summary = the_backend.consume(definitions)
  File "python/mozbuild/mozbuild/backend/base.py", line 180, in consume
    for obj in objs:
  File "python/mozbuild/mozbuild/frontend/emitter.py", line 168, in emit
    objs = list(self._emit_libs_derived(contexts))
  File "python/mozbuild/mozbuild/frontend/emitter.py", line 204, in _emit_libs_derived
    self._link_libraries(context, obj, variable)
  File "python/mozbuild/mozbuild/frontend/emitter.py", line 317, in _link_libraries
    context)
mozbuild.frontend.reader.SandboxValidationError:
==============================
ERROR PROCESSING MOZBUILD FILE
==============================

The error occurred while processing the following file or one of the files it includes:

    js/src/shell/moz.build

The error occurred when validating the result of the execution. The reported error is:

    USE_LIBS contains "memory", which does not match any LIBRARY_NAME in the tree.

http://buildbot.rhaalovely.net/builders/mozilla-aurora-netbsd-amd64/builds/72/steps/configure/logs/stdio
http://buildbot.rhaalovely.net/builders/mozilla-aurora-freebsd-amd64/builds/585/steps/configure/logs/stdio

Comment 5

[Jan Beich]

(In reply to Jan Beich from comment #4)
> configure:MOZ_JEMALLOC="" vs. js/src/configure.in:MOZ_JEMALLOC=1 leads...

Sorry typos. I meant configure.in:MOZ_MEMORY="" vs. js/src/configure.in:MOZ_MEMORY=1, i.e. memory/mozjemalloc/ lives outside of js/src/ and may not be built without JS_STANDALONE.

Comment 6

[Jan Beich]

Attachment: consistent defaults, v0

Maybe sync MOZ_ARG_* and MOZ_MEMORY=1 between configure.in and js/src/configure.in ?

https://treeherder.mozilla.org/#/jobs?repo=try&revision=44e7c305ba90

Comment 7

[Landry Breuil (:gaston)]

(In reply to Mike Hommey [:glandium] from comment #2)
> Comment on attachment 8587959 [details] [diff] [review]
> disable-jemalloc
> 
> Review of attachment 8587959 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> You should try something like this instead:

As i said, i dont really care about jemalloc. The way it works hides possible possible use-after-free and overflows, so we should always be able to override that and use our own system malloc, since it *will* uncover more bugs in firefox itself, which would be hidden by jemalloc otherwise.

So yeah, we might "fix" jemalloc-by-default using such a diff, but the background issue will still be there. People might want to use their system malloc for various reasons (debugging, as jan said), and you shouldnt remove that option from them.

Comment 8

[Landry Breuil (:gaston)]

Comment on attachment 8588264 [details] [diff] [review]
consistent defaults, v0

I dont really understand all the intricacies between MOZ_JEMALLOC3 and MOZ_MEMORY but with that diff i can run configure, and have MOZ_MEMORY properly defaulting to unset on OpenBSD.

Comment 9

[Mike Hommey [:glandium]]

(In reply to Landry Breuil (:gaston) from comment #7)
> As i said, i dont really care about jemalloc. The way it works hides
> possible possible use-after-free

No it doesn't. It even poisons freed memory.

> and overflows

No allocator can prevent overflows.

Comment 10

[Landry Breuil (:gaston)]

(In reply to Mike Hommey [:glandium] from comment #9)
> (In reply to Landry Breuil (:gaston) from comment #7)
> > As i said, i dont really care about jemalloc. The way it works hides
> > possible possible use-after-free
> 
> No it doesn't. It even poisons freed memory.

And what do you think our own malloc do ? Oh well. I dont have the arguments to debate this, and bikesheds are not my thing. Other OpenBSD developers who understand allocators *might* chime in, but i know it's a lost cause.

Now even if i try to 'fix' jemalloc the way you propose in comment 2, the build is broken as shown by jan in comment 4.

Comment 11

[Mike Hommey [:glandium]]

Comment on attachment 8588264 [details] [diff] [review]
consistent defaults, v0

Review of attachment 8588264 [details] [diff] [review]:
-----------------------------------------------------------------

::: configure.in
@@ +7145,5 @@
> +      MOZ_MEMORY=1
> +    fi
> +    ;;
> +  esac
> +fi

This should just be MOZ_MEMORY=1 everywhere but osx 32 bits. OpenBSD can use --disable-jemalloc in their ports if they really want to disable it. And the native jemalloc code path should be changed, I think, not to be triggered on MOZ_MEMORY being empty.

Note that mingw builds currently don't enable jemalloc, but I don't know why. Jacek, do you know?

Comment 12

[Landry Breuil (:gaston)]

Comment on attachment 8588264 [details] [diff] [review]
consistent defaults, v0

Patch got bitrotted and doesnt apply anymore.....

Can we do something about this issue please ? Tree not building on all BSDs is not nice.

Comment 13

[Jacek Caban]

(In reply to Mike Hommey [:glandium] from comment #11)
> Note that mingw builds currently don't enable jemalloc, but I don't know
> why. Jacek, do you know?

I don't know, probably some build problems. I will try to experiment with enabling it.

Comment 14

[Landry Breuil (:gaston)]

Can we move forward please and actually commit anything that fixes the breakage ? It's been a week and configure not working for all BSDs might hide actual build issues piling on top from showing up in buildbot...

Comment 15

[Jan Beich]

Attachment: consistent defaults, v0

(In reply to Landry Breuil (:gaston) from comment #12)
> Patch got bitrotted and doesnt apply anymore.....

Hmm, maybe my local commits affected |diff -U8| context. Trying again
and fixing whitespace while here. If it doesn't help apply the change
from the Try build in comment 6.

(In reply to Mike Hommey [:glandium] from comment #11)

Can you stop cancelling review without explaining why? This is not the
same as r- or r+ on condition and can be interpreted as "not willing
to be a reviewer for the patch".

https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/How_to_Submit_a_Patch#Addressing_review_comments

> ::: configure.in
> @@ +7145,5 @@
> > +      MOZ_MEMORY=1
> > +    fi
> > +    ;;
> > +  esac
> > +fi
> 
> This should just be MOZ_MEMORY=1 everywhere but osx 32 bits.

Do you expect platform-specific codepaths in mozjemalloc to just
magically work after years being unused and untested since the fork?

Maybe Landry can test --enable-jemalloc after applying comment 2
and the attached patch. My experience is in bug 1153683.

> OpenBSD can use --disable-jemalloc in their ports if they really
> want to disable it.

Same as bug 952828 comment 14. I'd force the quirk/tuning downstream
as well if there was no active maintainer here. ;)

> And the native jemalloc code path should be changed, I think, not to
> be triggered on MOZ_MEMORY being empty.

This belongs in a different bug. Note, native jemalloc if in libc
cannot be disabled sans memory stats/tuning under -DMOZ_MEMORY.

Comment 16

[Mike Hommey [:glandium]]

Comment on attachment 8591419 [details] [diff] [review]
consistent defaults, v0

Review of attachment 8591419 [details] [diff] [review]:
-----------------------------------------------------------------

Let's see how bug 1155438 changes things first.

Comment 17

[Jan Beich]

(In reply to Mike Hommey [:glandium] from comment #16)
> Let's see how bug 1155438 changes things first.

That bug is about related issue. It only addresses the error in comment 4. Other issues such as comment 0 (and comment 2) or exposed bug 1153683 still remain. Not to mention disabling jemalloc only for Tier3 and only for non-standalone is confusing, as if Tier3 runtime receives enough care for such distinction.

About consistency, I wonder if bug 702250 doesn't affect standalone JS.

Comment 18

[Jan Beich]

Attachment: consistent defaults, v0

Rebased.

Comment 19

[Landry Breuil (:gaston)]

Comment on attachment 8598107 [details] [diff] [review]
consistent defaults, v0

Fwiw, i like this.

Comment 20

[Jan Beich]

Landry, are you going to fix this? If not Assignee is misleading.

Comment 21

[Landry Breuil (:gaston)]

I dont remember what made the issue/configure error go away, but i'm far from looking into this again...

Comment 22

[Benjamin Bouvier [:bbouvier] (inactive)]

Just to double-check: are you saying the issue doesn't reproduce? If so, maybe we could close this bug.

Comment 23

[Jan Beich]

It still affects at least FreeBSD. Standalone SpiderMonkey crashes due to bug 1153683 caused by implicit --enable-jemalloc.

Comment 24

[Landry Breuil (:gaston)]

The 'original' issue (ie jemalloc being forced on everywhere) see comment #0, configure: error: --enable-jemalloc not supported on i386-unknown-openbsd5.7) has been probably fixed since then, ie jemalloc is not be enabled by default on OpenBSD now.

If i pass --enable-jemalloc manually it fails:
0:35.68 ERROR: --enable-jemalloc is not supported on OpenBSD

Comment 25

[Jan Beich]

Have you tried standalone build?

  $ cd js/src
  $ autoconf-2.13
  $ ./configure
  $ gmake
  $ js/src/shell/js

According build/moz.configure/memory.configure it should fail for OpenBSD as if --enable-jemalloc was explicitly passed.

  @depends('--enable-jemalloc', target, build_project, c_compiler)
  def jemalloc(value, target, build_project, c_compiler):
      if value.origin != 'default':
	  return bool(value) or None

      if build_project == 'js':
	  return True

Comment 26

[Landry Breuil (:gaston)]

[14:33] openbsd-amd64:~/src/mozilla-central/js/src/ $CC=clang CXX=clang++ ./configure
Reexecuting in the virtualenv
checking for a shell... /bin/sh
checking for host system type... x86_64-unknown-openbsd6.0
checking for target system type... x86_64-unknown-openbsd6.0
checking for the Android toolchain directory... not found
checking whether cross compiling... no
checking for pkg_config... /usr/bin/pkg-config
checking for pkg-config version... 0.27.1
checking for yasm... /usr/local/bin/yasm
checking yasm version... 1.3.0
checking for android platform directory... no
checking for the target C compiler... /usr/local/bin/clang
checking whether the target C compiler can be used... yes
checking the target C compiler version... 3.9.0
checking the target C compiler works... yes
checking for the target C++ compiler... /usr/local/bin/clang++
checking whether the target C++ compiler can be used... yes
checking the target C++ compiler version... 3.9.0
checking the target C++ compiler works... yes
checking for the host C compiler... /usr/local/bin/clang
checking whether the host C compiler can be used... yes
checking the host C compiler version... 3.9.0
checking the host C compiler works... yes
checking for the host C++ compiler... /usr/local/bin/clang++
checking whether the host C++ compiler can be used... yes
checking the host C++ compiler version... 3.9.0
checking the host C++ compiler works... yes
checking for 64-bit OS... yes
ERROR: --enable-jemalloc is not supported on OpenBSD
[14:34] openbsd-amd64:~/src/mozilla-central/js/src/

Comment 27

[Jan Beich]

Probably not an issue after https://hg.mozilla.org/mozilla-central/rev/e8ac1244375a