Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Closed
wants to merge 1 commit into from
Closed

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

wants to merge 1 commit into from

Conversation

thefloweringash
Copy link
Member

@thefloweringash thefloweringash commented Aug 1, 2020 •
edited

compiler-rt is no longer included in clang, so the current result of
building bootstrap tools doesn't include compiler-rt at all.

Motivation for this change

I want to update bootstrap tools soon as part of #19906, but just doing the update results in a subtly broken early toolchain. It looks like the bootstrap tools builder needs a small update. This is the first part that puts the required parts into the tarball, there's a follow up that fixes the unpacking stage.

This defines a new set of bootstrap tools, see #94427 for the "other half" that uses the updated bootstrap tools.

Possibly related #39743

cc @matthewbauer and @LnL7 who seem to work on the bootstrap files

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • 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 nixpkgs-review --run "nixpkgs-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)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.

@thefloweringash
stdenv/darwin: put compiler-rt into bootstrap tarball
37e14de 
compiler-rt is no longer included in clang, so the current result of
building bootstrap tools doesn't include compiler-rt at all.
@ofborg ofborg bot added the 6.topic: darwin Running or building packages on Darwin label Aug 1, 2020
@thefloweringash thefloweringash mentioned this pull request Aug 1, 2020
Closed
10 tasks
@thefloweringash
Copy link
Member Author

The main problem I'm seeing with trying to update the bootstrap tools without both of these changes is

$ nix-build -A perlPackages.LocaleGettext
[...]
running tests
check flags: SHELL=/nix/store/2zpjcwvc73cmirzvw7g461s96ri6dr55-bash-4.4-p23/bin/bash VERBOSE=y test
"/nix/store/z72xsh2ks2w337q2mr8kanhb47hln76l-perl-5.30.3/bin/perl" -MExtUtils::Command::MM -e 'cp_nonempty' -- gettext.bs blib/arch/auto/Locale/gettext/gettext.bs 644
PERL_DL_NONLAZY=1 "/nix/store/z72xsh2ks2w337q2mr8kanhb47hln76l-perl-5.30.3/bin/perl" "-MExtUtils::Command::MM" "-MTest::Harness" "-e" "undef *Test::Harness::Switches; test_harness(0, 'blib/lib', 'blib/arch')" t/*.t
t/bind.t ....... Can't load '/private/tmp/nix-build-perl5.30.3-gettext-1.07.drv-1/Locale-gettext-1.07/blib/arch/auto/Locale/gettext/gettext.bundle' for module Locale::gettext: dlopen(/private/tmp/nix-build-perl5.30.3-gettext-1.07.drv-1/Locale-gettext-1.07/blib/arch/auto/Locale/gettext/gettext.bundle, 2): Symbol not found: ___isOSVersionAtLeast
  Referenced from: /nix/store/p4s4w4q1lwvcla9p88d9ikvvwfxl66fg-curl-7.71.1/lib/libcurl.4.dylib
  Expected in: flat namespace
 in /nix/store/p4s4w4q1lwvcla9p88d9ikvvwfxl66fg-curl-7.71.1/lib/libcurl.4.dylib at /nix/store/z72xsh2ks2w337q2mr8kanhb47hln76l-perl-5.30.3/lib/perl5/5.30.3/darwin-thread-multi-2level/DynaLoader.pm line 197.
 at t/bind.t line 6.
Compilation failed in require at t/bind.t line 6.
BEGIN failed--compilation aborted at t/bind.t line 6.
t/bind.t ....... Dubious, test returned 2 (wstat 512, 0x200)
Failed 1/1 subtests
[...]

Full log: https://gist.github.com/thefloweringash/10f236a3d9fee59298bdee2ec47480e8

The curl with the undefined ___isOSVersionAtLeast symbol is built from stage 4. Maybe the most significant change is the cc-wrapper setup in #94427.

$ nix show-derivation /nix/store/p4s4w4q1lwvcla9p88d9ikvvwfxl66fg-curl-7.71.1/lib/libcurl.4.dylib | head -2
{
  "/nix/store/cy80vkg7r3m4gi0c3lhlhk4a5dshlrnw-curl-7.71.1.drv": {

$ nix-store -q --references /nix/store/cy80vkg7r3m4gi0c3lhlhk4a5dshlrnw-curl-7.71.1.drv | grep stdenv
/nix/store/2p866jj5z5l1qgaz6h4a63pb1azakxs7-bootstrap-stage4-stdenv-darwin.drv

@thefloweringash thefloweringash marked this pull request as ready for review August 17, 2020 11:45
@LnL7 LnL7 added this to Big Sur in Darwin Sep 17, 2020
@thefloweringash thefloweringash mentioned this pull request Sep 23, 2020
Merged
14 tasks
@thefloweringash
Copy link
Member Author

Quick sanity check:

libclang_rt differences from current bootstrap tools to currently generated bootstrap tools

-lib/clang/CLANG_VERSION/lib/darwin/libclang_rt.10.4.a
-lib/clang/CLANG_VERSION/lib/darwin/libclang_rt.asan_osx_dynamic.dylib
-lib/clang/CLANG_VERSION/lib/darwin/libclang_rt.cc_kext.a
-lib/clang/CLANG_VERSION/lib/darwin/libclang_rt.osx.a
-lib/clang/CLANG_VERSION/lib/darwin/libclang_rt.profile_osx.a
-lib/clang/CLANG_VERSION/lib/darwin/libclang_rt.safestack_osx.a
-lib/clang/CLANG_VERSION/lib/darwin/libclang_rt.stats_client_osx.a
-lib/clang/CLANG_VERSION/lib/darwin/libclang_rt.stats_osx_dynamic.dylib
-lib/clang/CLANG_VERSION/lib/darwin/libclang_rt.ubsan_osx_dynamic.dylib

libclang_rt differences from currently generated bootstrap tools (HEAD~), to generated bootstrap tools as of this PR (HEAD)

+lib/darwin/libclang_rt.10.4.a
+lib/darwin/libclang_rt.asan_osx_dynamic.dylib
+lib/darwin/libclang_rt.cc_kext.a
+lib/darwin/libclang_rt.fuzzer_no_main_osx.a
+lib/darwin/libclang_rt.fuzzer_osx.a
+lib/darwin/libclang_rt.lsan_osx_dynamic.dylib
+lib/darwin/libclang_rt.osx.a
+lib/darwin/libclang_rt.profile_osx.a
+lib/darwin/libclang_rt.safestack_osx.a
+lib/darwin/libclang_rt.stats_client_osx.a
+lib/darwin/libclang_rt.stats_osx_dynamic.dylib
+lib/darwin/libclang_rt.ubsan_minimal_osx.a
+lib/darwin/libclang_rt.ubsan_minimal_osx_dynamic.dylib
+lib/darwin/libclang_rt.ubsan_osx.a
+lib/darwin/libclang_rt.ubsan_osx_dynamic.dylib
+lib/darwin/libclang_rt.xray-basic_osx.a
+lib/darwin/libclang_rt.xray-fdr_osx.a
+lib/darwin/libclang_rt.xray-profiling_osx.a
+lib/darwin/libclang_rt.xray_osx.a
+lib/libclang_rt.10.4.a
+lib/libclang_rt.asan_osx_dynamic.dylib
+lib/libclang_rt.cc_kext.a
+lib/libclang_rt.fuzzer_no_main_osx.a
+lib/libclang_rt.fuzzer_osx.a
+lib/libclang_rt.lsan_osx_dynamic.dylib
+lib/libclang_rt.osx.a
+lib/libclang_rt.profile_osx.a
+lib/libclang_rt.safestack_osx.a
+lib/libclang_rt.stats_client_osx.a
+lib/libclang_rt.stats_osx_dynamic.dylib
+lib/libclang_rt.ubsan_minimal_osx.a
+lib/libclang_rt.ubsan_minimal_osx_dynamic.dylib
+lib/libclang_rt.ubsan_osx.a
+lib/libclang_rt.ubsan_osx_dynamic.dylib
+lib/libclang_rt.xray-basic_osx.a
+lib/libclang_rt.xray-fdr_osx.a
+lib/libclang_rt.xray-profiling_osx.a
+lib/libclang_rt.xray_osx.a

In the above the lib/libclang_rt* files are symlinks to their darwin/ counterparts.

@matthewbauer matthewbauer requested a review from LnL7 October 6, 2020 19:08
@matthewbauer
Copy link
Member

/cc @LnL7

LnL7
LnL7 approved these changes Oct 7, 2020
View reviewed changes
Copy link
Member

@LnL7 LnL7 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mentioned this before on irc, but I think this is more of a bootstrapping issue. Keeping as much as possible out of the bootstrap tarball makes bootstrapping more flexible and simplifies introducing a new architecture.

All that said I don't see a particularly big problem with adding compiler-rt.

@thefloweringash
Copy link
Member Author

I mentioned this before on irc, but I think this is more of a bootstrapping issue

As I understand it, clang_rt is a required component of a clang toolchain providing "builtins", and clang may choose to compile code to call a function defined in clang_rt. In practice, we have two instances (assuming #94427 is merged): one from bootstrap tools, and one rebuilt at stage 3. We might be able to omit the bootstrap tools version until we build a new one, but I'm not in favor of this, since it means using a partial clang toolchain in the early stages. As for how it works currently, the cc value for the entire bootstrap process comes from the bootstrap tarball, so the bootstrap clang v4 with integrated compiler-rt is used from beginning to end

All that said I don't see a particularly big problem with adding compiler-rt.

Not sure if I made this clear, but the current bootstrap tools contain compiler-rt. I think it was removed from the generator by #39743 as part of a clang refactoring. This change is to intended to keep the structure of the current bootstrap tools.

@PetarKirov
Copy link

What is the status of the PR? I'm not an expert in this area, but the change (including more of the already build llvmPackages code) doesn't seem like it can introduce issues (except for increasing the surface area of the bootstrap tarball), so I think it's worth including as it solves real issues.

@SuperSandro2000
Copy link
Member

@ofborg eval

@thefloweringash
Copy link
Member Author

This was subsumed by bootstrap changes in #105026.

@thefloweringash thefloweringash mentioned this pull request Jul 9, 2021
Merged
11 tasks
@thefloweringash thefloweringash deleted the darwin-bootstrap-compiler-rt branch July 10, 2021 02:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bobrik bobrik bobrik approved these changes

@LnL7 LnL7 LnL7 approved these changes

Assignees
No one assigned
Labels
6.topic: darwin Running or building packages on Darwin 6.topic: stdenv Standard environment 10.rebuild-darwin: 0 10.rebuild-linux: 0
Projects
Darwin
  
Big Sur
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@thefloweringash @matthewbauer @PetarKirov @SuperSandro2000 @bobrik @LnL7