llvm: factor out compiler-rt, fix libstdcxxStdenv sanitizer headers #39743

Ralith · 2018-04-30T19:03:01Z

Motivation for this change

This fixes the libcxxStdenv build of projects that use e.g. the AddressSanitizer API to improve error detection when using custom allocators, and improves factoring.

Things done

Tested using sandboxing (nix.useSandbox on NixOS, or option build-use-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/)
Fits CONTRIBUTING.md.

dtzWill · 2018-04-30T22:06:19Z

LGTM.

It's unfortunate that no one knows why we built this as part of LLVM in the first place. Did something need it? xD.

But a variety of very basic testing shows things working as they should, on both glibc and musl.

Do none of the other components need or benefit from compiler-rt?

Also, might be worth doing the same for LLVM5 and seeing what the Darwin builders think of the change-- to exercise clang-based stdenv's and get ahead of possible problems when eventually moving to LLVM6 as default.

Does this sound reasonable, @NixOS/darwin-maintainers ? Have some build cycles to spare? :)

dtzWill · 2018-04-30T22:10:06Z

Would it be much work to put together a little test for the behavior this fixes, and add it as a cc-wrapper test or something? So we can (try to) avoid regressing on it.

Just a thought :).

Ralith · 2018-04-30T22:53:45Z

It's unfortunate that no one knows why we built this as part of LLVM in the first place. Did something need it? xD.

IIRC we hunted down the party responsible (@shlevy?) in IRC and they were reasonably confident that it was just a workaround for historical brokenness in the LLVM build system--its degree of support for compiling components separately has varied in the past.

Would it be much work to put together a little test for the behavior this fixes, and add it as a cc-wrapper test or something? So we can (try to) avoid regressing on it.

Such a test should be simple, especially if you can point me to an example cc-wrapper test to model it on, as I haven't written a NixOS test before. Is there any danger of planting a land mine for support of compilers that don't implement the usual sanitizers, though? I imagine e.g. icc might be an issue.

Ralith · 2018-04-30T23:50:58Z

Factoring compiler-rt out of LLVM doesn't have to happen simultaneously with fixing the bug in the clang package, but they're both going to touch clang so it's probably worth getting the rebuilds out of the way in one go.

dtzWill · 2018-04-30T23:52:03Z

We have various cc-wrapper tests that are specifically for clang -- various versions, using libc++ and libstdc++. I think perhaps they could go there? https://github.com/NixOS/nixpkgs/tree/master/pkgs/test/cc-wrapper

Anyway this can happen in a follow-up and as time allows :).

shlevy · 2018-05-01T13:22:16Z

Yes, I definitely wanted to compile this separately from the get-go but it didn't work way back when I added it.

dtzWill · 2018-05-01T23:02:00Z

Yes, I definitely wanted to compile this separately from the get-go but it didn't work way back when I added it.

Oh, well that settles that! I didn't mean to place blame in any way--often details of a decision are lost over time :). Anyway thanks for clearing that up!

dtzWill · 2018-05-09T20:35:30Z

Basic testing suggests this is fine but I'm a bit too swamped to more thoroughly review/test. Don't block on my account, sorry!

Ralith · 2018-05-18T20:38:32Z

Is this blocked?

matthewbauer · 2018-05-18T20:52:07Z

Can you rebase to staging? Otherwise looks good. The build farm is a little busy right now but I can merge after that clears up.

Ericson2314 · 2018-05-18T21:01:18Z

Woah!!! Very very very excited this exists.

Ericson2314 · 2018-05-22T22:00:28Z

pkgs/development/compilers/llvm/6/compiler-rt.nix

+  inherit version;
+  src = fetch "compiler-rt" "16m7rvh3w6vq10iwkjrr1nn293djld3xm62l5zasisaprx117k6h";
+
+  nativeBuildInputs = [ cmake python llvm ];


Pretty sure LLVM is just a build tool here? So it is a nativeBuildInput.

Reallllyyy??? No wonder you were asking about using a stand-in for llvm-config, wonder what the details here are.

Ericson2314 · 2018-05-22T22:00:45Z

OK I did the merge. This should be good now.

dtzWill

Generally LGTM, haven't tested extensively but don't expect much to break.

TYVM for moving the musl patching over 👍.

dtzWill · 2018-05-22T22:11:18Z

pkgs/development/compilers/llvm/6/compiler-rt.nix

+  inherit version;
+  src = fetch "compiler-rt" "16m7rvh3w6vq10iwkjrr1nn293djld3xm62l5zasisaprx117k6h";
+
+  nativeBuildInputs = [ cmake python llvm ];


Reallllyyy??? No wonder you were asking about using a stand-in for llvm-config, wonder what the details here are.

dtzWill · 2018-05-22T22:14:35Z

pkgs/development/compilers/llvm/6/clang/default.nix

@@ -51,13 +53,17 @@ let
    outputs = [ "out" "lib" "python" ];

    # Clang expects to find LLVMgold in its own prefix
-    # Clang expects to find sanitizer libraries in its own prefix
+    # Clang expects to find sanitizer libraries and headers in its own prefix
    postInstall = ''


Anyone have details about this?

Also discussed elsewhere in this PR (#39743 (comment)) -- I'd just like to know about the issues, as it'd be awfully nice to not have to link these bits over... (perhaps combined with a wrapper or so).

I'll see if it's possible to get away with -L and -isystem for these. The approach taken here was based on the existing code and the expectations of the clang developers, but it isn't necessarily impossible to do otherwise. If it works out, how would I go about injecting them gracefully?

e: Wait, now I think I get what @Ericson2314 was suggesting. Will experiment.

Sounds good! If it's a big deal, we can look into it later--don't mean to push all of our LLVM-related TODO's onto your PR! 😁

But if you don't mind taking a look that'd be great! Thanks!

dtzWill · 2018-05-23T00:21:45Z

FWIW initial musl testing looks good! Woo! 👍

Ericson2314 · 2018-05-23T15:49:11Z

OK rebased on some careful cherry-picks so there's no conflicts. If we can make clang not depend on compiler-rt, then we can build compiler-rt with the newly-built clang, which will be good (nearly sufficient!) for cross compilation.

Ericson2314 · 2018-05-23T15:54:40Z

Also the separate "tools" and "libraries" part of obsidiansystems@acf2ed5 would be nice to have. I might try to incorperate that.

Ralith · 2018-05-23T17:38:38Z

Contrary to my initial reaction, depsTargetTarget does, in fact, seem to be adequate for compiler-rt!

Ericson2314 · 2018-05-24T06:51:09Z

Only the wrapper script knows about compiler-rt, and both tests pass!

…er headers Build compiler-rt separate from llvm and clang, and just add as an extra library in the wrapper.

llvm 6: Master version of #39743

GrahamcOfBorg added 10.rebuild-darwin: 11-100 10.rebuild-linux: 501+ labels Apr 30, 2018

Mic92 requested a review from dtzWill April 30, 2018 20:22

Ralith force-pushed the compiler-rt branch from 2e0778f to 8774d07 Compare May 7, 2018 20:46

matthewbauer closed this May 18, 2018

matthewbauer reopened this May 18, 2018

Ralith force-pushed the compiler-rt branch from 8774d07 to 10127f4 Compare May 18, 2018 20:58

Ralith requested review from Ericson2314, FRidh, nbp, orivej, peti, ttuegel and zimbatm as code owners May 18, 2018 20:58

Ralith changed the base branch from master to staging May 18, 2018 21:01

GrahamcOfBorg added 6.topic: GNOME GNOME desktop environment and its underlying platform 6.topic: golang 6.topic: haskell 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS labels May 18, 2018

peti removed their request for review May 22, 2018 08:29

Ericson2314 mentioned this pull request May 22, 2018

glibc: don't use bootstrap libgcc_s #36948

Merged

8 tasks

Ericson2314 force-pushed the compiler-rt branch from db4787b to a8ebccb Compare May 22, 2018 21:58

Ericson2314 reviewed May 22, 2018

View reviewed changes

GrahamcOfBorg removed the 2.status: merge conflict This PR has merge conflicts with the target branch label May 22, 2018

dtzWill reviewed May 22, 2018

View reviewed changes

ttuegel removed their request for review May 22, 2018 23:19

Ericson2314 force-pushed the compiler-rt branch from a8ebccb to d37ad05 Compare May 23, 2018 15:47

Merge remote-tracking branch 'upstream/master' into ios-mini-staging

e5175fb

Ericson2314 force-pushed the compiler-rt branch from c7528e8 to 38b8a3e Compare May 23, 2018 20:36

GrahamcOfBorg added the 6.topic: stdenv Standard environment label May 23, 2018

Ericson2314 force-pushed the compiler-rt branch 2 times, most recently from 64106b4 to d9458a9 Compare May 24, 2018 06:41

GrahamcOfBorg removed the 6.topic: stdenv Standard environment label May 24, 2018

Ralith and others added 3 commits May 24, 2018 02:57

llvm and clang 6: factor out compiler-rt, fix libstdcxxStdenv sanitiz…

16da44b

…er headers Build compiler-rt separate from llvm and clang, and just add as an extra library in the wrapper.

tests.cc-wrapper: verify building with sanitizers

46eeef1

Merge remote-tracking branch 'upstream/staging' into compiler-rt

205fc55

Ericson2314 force-pushed the compiler-rt branch from 2c430a0 to 205fc55 Compare May 24, 2018 06:58

Ericson2314 merged commit 11d26c7 into NixOS:staging May 24, 2018

Ralith deleted the compiler-rt branch May 24, 2018 20:16

Ralith mentioned this pull request May 24, 2018

clang_6: fix sanitizers under libstdc++ #41065

Merged

8 tasks

Ericson2314 added a commit that referenced this pull request May 26, 2018

Merge pull request #41081 from obsidiansystems/compiler-rt-master

713d580

llvm 6: Master version of #39743

orivej mentioned this pull request Jun 6, 2018

tests.cc-wrapper: skip known-broken sanitizer cases #41284

Merged

8 tasks

thefloweringash mentioned this pull request Aug 1, 2020

stdenv/darwin: put compiler-rt into bootstrap tarball #94426

Closed

10 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

llvm: factor out compiler-rt, fix libstdcxxStdenv sanitizer headers #39743

llvm: factor out compiler-rt, fix libstdcxxStdenv sanitizer headers #39743

Ralith commented Apr 30, 2018

dtzWill commented Apr 30, 2018

dtzWill commented Apr 30, 2018

Ralith commented Apr 30, 2018

Ralith commented Apr 30, 2018

dtzWill commented Apr 30, 2018

shlevy commented May 1, 2018

dtzWill commented May 1, 2018

dtzWill commented May 9, 2018

Ralith commented May 18, 2018

matthewbauer commented May 18, 2018

Ericson2314 commented May 18, 2018 •

edited

Loading

Ericson2314 May 22, 2018

dtzWill May 22, 2018

Ericson2314 commented May 22, 2018

dtzWill left a comment

dtzWill May 22, 2018

dtzWill May 22, 2018

Ralith May 22, 2018 •

edited

Loading

dtzWill May 23, 2018

dtzWill commented May 23, 2018 •

edited

Loading

Ericson2314 commented May 23, 2018

Ericson2314 commented May 23, 2018

Ralith commented May 23, 2018

Ericson2314 commented May 24, 2018

llvm: factor out compiler-rt, fix libstdcxxStdenv sanitizer headers #39743

llvm: factor out compiler-rt, fix libstdcxxStdenv sanitizer headers #39743

Conversation

Ralith commented Apr 30, 2018

Motivation for this change

Things done

dtzWill commented Apr 30, 2018

dtzWill commented Apr 30, 2018

Ralith commented Apr 30, 2018

Ralith commented Apr 30, 2018

dtzWill commented Apr 30, 2018

shlevy commented May 1, 2018

dtzWill commented May 1, 2018

dtzWill commented May 9, 2018

Ralith commented May 18, 2018

matthewbauer commented May 18, 2018

Ericson2314 commented May 18, 2018 • edited Loading

Ericson2314 May 22, 2018

Choose a reason for hiding this comment

dtzWill May 22, 2018

Choose a reason for hiding this comment

Ericson2314 commented May 22, 2018

dtzWill left a comment

Choose a reason for hiding this comment

dtzWill May 22, 2018

Choose a reason for hiding this comment

dtzWill May 22, 2018

Choose a reason for hiding this comment

Ralith May 22, 2018 • edited Loading

Choose a reason for hiding this comment

dtzWill May 23, 2018

Choose a reason for hiding this comment

dtzWill commented May 23, 2018 • edited Loading

Ericson2314 commented May 23, 2018

Ericson2314 commented May 23, 2018

Ralith commented May 23, 2018

Ericson2314 commented May 24, 2018

Ericson2314 commented May 18, 2018 •

edited

Loading

Ralith May 22, 2018 •

edited

Loading

dtzWill commented May 23, 2018 •

edited

Loading