rustc: build with system llvm and jemalloc #49557

symphorien · 2018-10-31T22:05:59Z

Motivation for this change

We can use our llvm instead of the bundled one, just like fedora does: https://src.fedoraproject.org/rpms/rust/blob/master/f/rust.spec

The closure size goes down a little: 780MB -> 740MB, but now part of this closure is llvm and can be shared. This also benefits compilation time.

The commit also includes some cleanup of tests which no longer exist and don't need to be removed.

I tested compilation of alacritty, firefox, thunderbird and ripgrep on x86_64-linux. (on top of master, 9f88282)

Things done

Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS)
Built on platform(s)
- NixOS
- macOS
- other Linux distributions
Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
Tested compilation of all pkgs that depend on this change using nix-shell -p nox --run "nox-review wip"
Tested execution of all binary files (usually in ./result/bin/)
Determined the impact on package closure size (by running nix path-info -S before and after)
Fits CONTRIBUTING.md.

Mic92 · 2018-11-01T01:04:26Z

pkgs/development/compilers/rust/default.nix

@@ -29,7 +29,7 @@ in rec {
      ./patches/disable-test-inherit-env.patch
    ];

-    forceBundledLLVM = true;
+    withBundledLLVM = false;


@LnL7 @dtzWill anyone who does remember why we switched away from this option? I thing this option was true at some point.

It broke the tests back then 7bd191d
It could be still ok though:

Archlinux does this as well: https://git.archlinux.org/svntogit/community.git/tree/trunk/PKGBUILD?h=packages/rust

Ubuntu uses the bundled one: https://git.launchpad.net/ubuntu/+source/rustc/tree/debian/control?h=ubuntu/cosmic

Opensuse uses the system one: https://build.opensuse.org/package/view_file/devel:languages:rust/rust/rust.spec?expand=1

Since you deal with rust backports sometimes for firefox what would you prefer @andir ?
I guess in the worst case it should be still possible to just re-enable the bundled version in backported version.

I really have no strong opinion here. I would certainly like to avoid a situation where we have to port LLVM whenever we have to back port rustc for Thunderbird/Firefox.

How much is the speedup of compilation time? If it is significant I could see this being a good thing in general.

Compiling rustc with the bundled llvm takes about 1h05 for llvm and 55 min for rustc. So this halves compilation time.
Note that fedora disables codegen tests when using the bundled llvm.

At the moment we do not run tests anyway.

Is everything from their llvm fork upstream now? That was the main reason for this IIRC.

They only seem to merge pull requests from changes that have been before in llvm.

Great, I tried to split this out into 2 separate derivations at some point but not rebuilding llvm is even better.

dtzWill · 2018-11-01T01:15:15Z

No clue offhand, does `git blame` (or similar) give any explanation?

…

On Wed, 31 Oct 2018 18:04:32 -0700, Jörg Thalheim ***@***.***> wrote: Mic92 commented on this pull request. > @@ -29,7 +29,7 @@ in rec { ./patches/disable-test-inherit-env.patch ]; - forceBundledLLVM = true; + withBundledLLVM = false; @LnL7 @dtzWill anyone who does remember why we switched away from this option? I thing this option was true at some point. -- You are receiving this because you were mentioned. Reply to this email directly or view it on GitHub: #49557 (review) part: text/html

Mic92 · 2018-11-03T20:18:20Z

I will test the aarch64 build. I also would like to see macOS tested.

Mic92 · 2018-11-04T15:54:57Z

@periklis It would be interesting to see if this pull request would solve #49708 in a more general way on macOS (or at least work around).

periklis · 2018-11-05T12:43:07Z

@Mic92 I started a build on my darwin builder. I will report the results when done

Mic92 · 2018-11-06T15:29:52Z

@danieldk maybe you can also have look at this w.r.t. to macOS.

danieldk · 2018-11-07T14:23:52Z

I'll give it a shot, my MacBook is chugging on the build now.

danieldk · 2018-11-07T14:29:55Z

Does not get very far on Darwin (Mojave, latest Xcode):

https://gist.githubusercontent.com/danieldk/32e8d9beba4c5d7e442f917a06ab0635/raw/43f164906002dfa3b1b49c04e33d19ab3c7c0102/gistfile1.txt

symphorien · 2018-11-07T14:45:38Z

Can you try to remove the stdenv.isLinux && guard on line 45 then ? This should solve ___cxa_end_catch at least and a bunch of others.

danieldk · 2018-11-07T15:07:38Z

I have added

++ optional (stdenv.isDarwin && !withBundledLLVM) "-lc++"

Since the C++ standard library is libc++ on Darwin and the --push-state and --as-needed options are not known by LLVM ld (?). Now linking fails on missing jemalloc symbols, which is interesting, since configure reports that it finds jemalloc.

symphorien · 2018-11-07T15:12:10Z

Yes --push-state is a GNU extension, I forgot.

danieldk · 2018-11-07T15:17:03Z

Does the removed comment about jemalloc prefixes from postPatch still apply?

$ nm /nix/store/42lsjkifndxxzn4i8c3iyl0rijrsan0v-jemalloc-5.1.0/lib/libjemalloc.dylib  | grep calloc
00000000000045a0 T _calloc
00000000000023b0 T _je_bootstrap_calloc
000000000004a4b0 t _zone_calloc

symphorien · 2018-11-07T15:19:34Z

Ah maybe, I removed it because it was not needed on linux.

danieldk · 2018-11-07T15:42:38Z

That was indeed it, it finished the stage0 compiler, will keep you posted.

danieldk · 2018-11-07T17:56:27Z

Here is the modified diff that compiles on Darwin:

https://gist.github.com/danieldk/127bf63c7ebcff0d7b0e9c21d482dd04

Changes:

Add linker flag -lc++ to NIX_LDFLAGS on Darwin.
In postPatch, remove je_ prefix in jemalloc symbol names on Darwin.

Tested:

nix-index derivation: succeeds

fd derivation: fails in unit tests with:

error: process didn't exit successfully: `/private/tmp/nix-build-fd-7.2.0.drv-0/source/target/debug/deps/tests-090b3e427c9f00ad` (signal: 11, SIGSEGV: invalid memory reference)

Some of my own crates (so far, so good).

This seems to solve the failing doctest problem.

danieldk · 2018-11-07T18:47:47Z

The segmentation fault happens in free():

* thread #2, name = 'test_absolute_path', stop reason = EXC_BAD_ACCESS (code=1, address=0x190178c63c)
  * frame #0: 0x00000001000c6b0b tests-090b3e427c9f00ad`free + 347

Full stack trace: https://gist.github.com/danieldk/5ee20615aa7ad61cba2a460ea5d08ac7

Mic92 · 2018-11-07T19:32:21Z

So it crashes in jemalloc for some reason. What I am surprised about is that I do not see any jemalloc dependencies in the homebrew build: https://github.com/Homebrew/homebrew-core/blob/76dbba870f4e74d76392d992d96bd044daf435a9/Formula/rust.rb

Their jemalloc also comes without a prefix: https://github.com/Homebrew/homebrew-core/blob/master/Formula/jemalloc.rb#L26

Do they use the system allocator on macos? I don't see any vendoring done by rust regarding jemalloc.

@alyssais do you know if they install a jemalloc by default in homebrew?

symphorien · 2018-11-07T19:38:09Z

There is a bundled jemalloc, which I remove to reduce compilation time. This was commented out before the PR. So maybe we should stick to the bundled jemalloc on darwin for the two or so upcoming rustc versions.
Afterwards, the build system will change wrt jemalloc because of rust-lang/rust#55238 . At this point we may reevaluate unbundling jemalloc on darwin.

danieldk · 2018-11-07T19:39:19Z

@Mic92 I think Rust imports jemalloc itself through jemalloc_sys.

I am now rebuilding jemalloc and rustc with the je_ prefixes to see if that helps (I guess there are symbol clashes between C [mc]alloc/free and jemalloc's equivalents without the prefix).

danieldk · 2018-11-07T20:51:32Z

The fd unit tests pass without any segfaults when jemalloc is compiled with the je_ prefix for jemalloc functions:

diff --git a/pkgs/development/libraries/jemalloc/common.nix b/pkgs/development/libraries/jemalloc/common.nix
index 593f4411f19..6cff824061e 100644
--- a/pkgs/development/libraries/jemalloc/common.nix
+++ b/pkgs/development/libraries/jemalloc/common.nix
@@ -12,7 +12,7 @@ stdenv.mkDerivation (rec {
   # By default, jemalloc puts a je_ prefix onto all its symbols on OSX, which
   # then stops downstream builds (mariadb in particular) from detecting it. This
   # option should remove the prefix and give us a working jemalloc.
-  configureFlags = stdenv.lib.optional stdenv.isDarwin "--with-jemalloc-prefix=";
+  #configureFlags = stdenv.lib.optional stdenv.isDarwin "--with-jemalloc-prefix=";
   doCheck = true;
 
   enableParallelBuilding = true;

From the jemalloc documentation:

By default, the prefix is "", except on OS X, where it is "je_". On OS X,
jemalloc overlays the default malloc zone, but makes no attempt to actually
replace the "malloc", "calloc", etc. symbols.

Just like fedora does: https://src.fedoraproject.org/rpms/rust/blob/master/f/rust.spec Also some cleanup of tests which were removed but no longer exist.

@danieldk

jemalloc with stripped prefix would cause segfaults in free: NixOS#49557 (comment) Thanks to @danieldk for darwin testing/debugging.

symphorien · 2018-11-08T12:21:57Z

Thanks @danieldk !
I rebased on a newer staging and added a commit taking into account what you found:

add -lc++ to NIX_LDFLAGS
make jemalloc parametric to strip or not the je_ prefix and forcibly use the prefixed version on rustc

This new commit does not change hashes on linux.

danieldk · 2018-11-08T12:23:58Z

Thanks! I'll do another test run.

danieldk · 2018-11-08T14:23:07Z

Everything I tested before works well 👍 .

andir · 2018-11-08T14:28:11Z

I am running a rebuild of this on x86_64-linux. It sounds really promising! :)

alyssais · 2018-11-09T09:14:52Z

@alyssais do you know if they install a jemalloc by default in homebrew?

Homebrew doesn't install anything "by default". The only things that'll get a Homebrew jemalloc are formulae that have depends_on "jemalloc" (brew uses jemalloc).

I don't see anything Rust or LLVM-related.

Mic92 · 2018-11-09T11:11:23Z

aarch64 build is scheduled.

Mic92 · 2018-11-09T11:24:36Z

aarch64 build has been rescheduled (the community builder was rebooted)

Mic92 · 2018-11-12T14:41:15Z

aarch64 successfully builded fd

periklis · 2018-11-13T08:56:35Z

@Mic92 can we enable tests for nix-index, alacritty and bat again?

periklis · 2018-11-13T08:57:14Z

ic bat is enabled in #50236

alyssais · 2018-11-13T09:03:25Z

can we enable tests for nix-index, alacritty and bat again?

Only in staging, presumably.

jemalloc with stripped prefix would cause segfaults in free: NixOS#49557 (comment) This commit has been adjusted from the one on master to only carry the relevant changes (the new optional `stripPrefix`) flag while still keeping the other flags that were removed on master since they are no loner supported by the version used there. (cherry picked from commit 973eca7)

symphorien requested review from LnL7 and Mic92 as code owners October 31, 2018 22:05

GrahamcOfBorg added 6.topic: rust 10.rebuild-darwin: 101-500 10.rebuild-linux: 501+ labels Oct 31, 2018

Mic92 reviewed Nov 1, 2018

View reviewed changes

Mic92 mentioned this pull request Nov 3, 2018

alacritty: broken darwin build #49693

Closed

rustc: build with system llvm and jemalloc

4b0d441

Just like fedora does: https://src.fedoraproject.org/rpms/rust/blob/master/f/rust.spec Also some cleanup of tests which were removed but no longer exist.

GrahamcOfBorg added the 2.status: merge conflict label Nov 8, 2018

rustc: fix build with unbundled jemalloc and llvm on darwin

973eca7

jemalloc with stripped prefix would cause segfaults in free: NixOS#49557 (comment) Thanks to @danieldk for darwin testing/debugging.

symphorien force-pushed the rust-system-libs branch from 306b86b to 973eca7 Compare November 8, 2018 12:17

GrahamcOfBorg removed the 2.status: merge conflict label Nov 8, 2018

Mic92 merged commit 3fd80c6 into NixOS:staging Nov 12, 2018

symphorien mentioned this pull request Jan 20, 2019

rust: 1.31.0 -> 1.32.0 #54323

Merged

10 tasks

symphorien deleted the rust-system-libs branch May 18, 2019 16:01

rustc: build with system llvm and jemalloc #49557

rustc: build with system llvm and jemalloc #49557

Conversation

symphorien commented Oct 31, 2018

Motivation for this change

Things done

Mic92 Nov 1, 2018

Choose a reason for hiding this comment

Mic92 Nov 1, 2018 • edited

Choose a reason for hiding this comment

andir Nov 1, 2018

Choose a reason for hiding this comment

symphorien Nov 1, 2018

Choose a reason for hiding this comment

Mic92 Nov 1, 2018

Choose a reason for hiding this comment

LnL7 Nov 17, 2018

Choose a reason for hiding this comment

Mic92 Nov 17, 2018 • edited

Choose a reason for hiding this comment

LnL7 Nov 17, 2018

Choose a reason for hiding this comment

dtzWill commented Nov 1, 2018 via email

Mic92 commented Nov 3, 2018

Mic92 commented Nov 4, 2018 • edited

periklis commented Nov 5, 2018

Mic92 commented Nov 6, 2018

danieldk commented Nov 7, 2018

danieldk commented Nov 7, 2018

symphorien commented Nov 7, 2018

danieldk commented Nov 7, 2018

symphorien commented Nov 7, 2018

danieldk commented Nov 7, 2018 • edited

symphorien commented Nov 7, 2018

danieldk commented Nov 7, 2018

danieldk commented Nov 7, 2018 • edited

danieldk commented Nov 7, 2018

Mic92 commented Nov 7, 2018 • edited

symphorien commented Nov 7, 2018

danieldk commented Nov 7, 2018 • edited

danieldk commented Nov 7, 2018

symphorien commented Nov 8, 2018

danieldk commented Nov 8, 2018

danieldk commented Nov 8, 2018

andir commented Nov 8, 2018

alyssais commented Nov 9, 2018

Mic92 commented Nov 9, 2018

Mic92 commented Nov 9, 2018

Mic92 commented Nov 12, 2018

periklis commented Nov 13, 2018

periklis commented Nov 13, 2018

alyssais commented Nov 13, 2018

Mic92 Nov 1, 2018 •

edited

Mic92 Nov 17, 2018 •

edited

Mic92 commented Nov 4, 2018 •

edited

danieldk commented Nov 7, 2018 •

edited

danieldk commented Nov 7, 2018 •

edited

Mic92 commented Nov 7, 2018 •

edited

danieldk commented Nov 7, 2018 •

edited