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

llvmPackages_{7-12}.compiler-rt: install resource files to DATADIR #123103

Merged
merged 2 commits into from May 15, 2021

Conversation

sternenseemann
Copy link
Member

This is in an effort to fix the following build failure shown by
chromium:

clang++: error: no such file or directory: '/nix/store/fhd89wrmkx6nflzjk0d6waz70bk3zc4i-clang-wrapper-12.0.0/resource-root/share/cfi_blacklist.txt'

As it turns out a change introduced via the gnu-install-dirs.patch
caused add_compiler_rt_resource_file to install resource files to
$dev/include (FULL_INCLUDEDIR) instead of $out/share (FULL_DATADIR)
which in turn meant that the clang wrappers we had didn't link those
files to its resource root at all.

Alternative fix to this would have been to link
compiler-rt.dev/include/*.txt to the wrappers resource-root/share as
well, but since this was handled inconsistently across the patch anyways
(the dfsan list is installed correctly), opt to handle this
consistently within the patch.

llvmPackages_{5,6} install the resource files to a completely different
location and need separate investigation.

lr $(nix-build -A llvmPackages_12.stdenv.cc)/resource-root/share/
/nix/store/5i59ga5gfw08mg1cxyya5dspw2m63gai-clang-wrapper-12.0.0/resource-root/share/
/nix/store/5i59ga5gfw08mg1cxyya5dspw2m63gai-clang-wrapper-12.0.0/resource-root/share/asan_blacklist.txt
/nix/store/5i59ga5gfw08mg1cxyya5dspw2m63gai-clang-wrapper-12.0.0/resource-root/share/cfi_blacklist.txt
/nix/store/5i59ga5gfw08mg1cxyya5dspw2m63gai-clang-wrapper-12.0.0/resource-root/share/dfsan_abilist.txt
/nix/store/5i59ga5gfw08mg1cxyya5dspw2m63gai-clang-wrapper-12.0.0/resource-root/share/hwasan_blacklist.txt
/nix/store/5i59ga5gfw08mg1cxyya5dspw2m63gai-clang-wrapper-12.0.0/resource-root/share/msan_blacklist.txt
Motivation for this change
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.

Copy link
Member

@primeos primeos left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@sternenseemann
Copy link
Member Author

sternenseemann commented May 15, 2021

> echo nix-build -A\ llvmPackages_{7,8,9,10,11,12}.stdenv.cc | sh | while read line
                                                          ls $line/resource-root/share/
                                                      end
asan_blacklist.txt  cfi_blacklist.txt  dfsan_abilist.txt  hwasan_blacklist.txt  msan_blacklist.txt
asan_blacklist.txt  cfi_blacklist.txt  dfsan_abilist.txt  hwasan_blacklist.txt  msan_blacklist.txt
asan_blacklist.txt  cfi_blacklist.txt  dfsan_abilist.txt  hwasan_blacklist.txt  msan_blacklist.txt
asan_blacklist.txt  cfi_blacklist.txt  dfsan_abilist.txt  hwasan_blacklist.txt  msan_blacklist.txt
asan_blacklist.txt  cfi_blacklist.txt  dfsan_abilist.txt  hwasan_blacklist.txt  msan_blacklist.txt
asan_blacklist.txt  cfi_blacklist.txt  dfsan_abilist.txt  hwasan_blacklist.txt  msan_blacklist.txt

@vcunat
Copy link
Member

vcunat commented May 15, 2021

I'd strongly suggest that we should first have some solution workaround that doesn't involve rebuilding darwin stdenv. Otherwise it's yet another few days of waiting (for chromium to work).

@sternenseemann
Copy link
Member Author

I'd strongly suggest that we should first have some solution that doesn't involve rebuilding darwin stdenv. Otherwise it yet another few days of waiting (for chromium to work).

If we only touch mkExtraBuildCommands* in llvm/*/default.nix, that should achieve that, right?

(As a side note, seems like darwin stdenv doesn't link the resource files into the resource root at all).

@vcunat
Copy link
Member

vcunat commented May 15, 2021

In the worst case we could make the change linux-only for now or something.

@r-rmcgibbo
Copy link

Result of nixpkgs-review pr 123103 at 01971cf6 run on aarch64-linux 1

28 packages marked as broken and skipped:
  • clang-sierraHack
  • clang-sierraHack-stdenv
  • darwin.maloader
  • embree
  • embree2
  • gnome.gnome-documents
  • idris2
  • intel-compute-runtime
  • jucipp
  • libsForQt512.discover
  • ...
4 packages failed to build:
363 packages skipped due to time constraints:
  • adapta-gtk-theme
  • adoptopenjdk-icedtea-web
  • almanah
  • apostrophe
  • appleseed
  • arc-theme
  • arx-libertatis
  • asciidoc-full
  • asciidoc-full-with-plugins
  • astroid
  • ...
30 packages built successfully:
  • clang (clang_7 ,llvmPackages.clang ,llvmPackages.libstdcxxClang ,llvmPackages_7.clang ,llvmPackages_7.libstdcxxClang)
  • clangStdenv (gnustep.stdenv ,llvmPackages.stdenv ,llvmPackages_7.stdenv)
  • clang_9 (haskellPackages.llvmPackages.clang ,haskellPackages.llvmPackages.libstdcxxClang ,llvmPackages_9.clang ,llvmPackages_9.libstdcxxClang)
  • fcitx5-mozc
  • glib-networking (gnome.glib-networking ,gnome.glib_networking)
  • gnome.gnome-shell-extensions
  • gnustep.back
  • gnustep.base
  • gnustep.gorm
  • gnustep.gui
  • gnustep.libobjc
  • gnustep.make
  • gnustep.projectcenter
  • gnustep.system_preferences
  • llvmPackages_9.stdenv (haskellPackages.llvmPackages.stdenv)
  • innernet
  • parinfer-rust (kakounePlugins.parinfer-rust)
  • kime
  • libproxy
  • mediatomb
  • openshift
  • ostree
  • python38Packages.johnnycanencrypt
  • python38Packages.tumpa
  • python39Packages.johnnycanencrypt
  • python39Packages.tumpa
  • spidermonkey_68
  • tests.cc-wrapper-clang (tests.cc-wrapper-clang-7)
  • txr
  • unar

Note that build failures may predate this PR, and could be nondeterministic or hardware dependent.
Please exercise your independent judgement. Does something look off? Please file an issue or reach out on IRC.

@jonringer
Copy link
Contributor

jonringer commented May 15, 2021

In the worst case we could make the change linux-only for now or something.

The linux builders should be able to chew through it in a day. Darwin will suffer greatly as it takes many days for it to complete a stdenv rebuild; and darwin has been delaying staging-next enough already. Delaying staging-next any further is now taking away from doing stabilization from the next staging-next.

@jonringer
Copy link
Contributor

For linux, this only causes ~600 rebuilds [53/84/617 built ..... I'm fine with merging it assuming that it address the chromium issue.

sternenseemann added a commit to sternenseemann/nixpkgs that referenced this pull request May 15, 2021
Likely due to an oversight, after 7869d16
some of the resource files of compiler-rt are installed to $dev/include
instead of $out/share which breaks the assumption of multiple cc
wrappers in llvmPackages. To resolve this without causing a mass rebuild
on darwin, link the files from the wrong location into the correct place
in clang's resource root.

The proper fix for this is NixOS#123103 which however causes a darwin stdenv
rebuild and should go through staging first.
@sternenseemann
Copy link
Member Author

I'm trying to find a workaround which saves us from the darwin stdenv rebuild. If that doesn't pan out we'll have to have two different patches for darwin and linux I guess.

In any case I would propose that we try to merge the proper fix (whatever it'll look like) with the next staging-next PR then?

@jonringer
Copy link
Contributor

@GrahamcOfBorg build chromium

@Ericson2314
Copy link
Member

Yikes :(. I'm sorry this one got in there.

sternenseemann added a commit to sternenseemann/nixpkgs that referenced this pull request May 15, 2021
7869d16 changed how resource files are
installed. Likely by accident, now some of the resource files are
installed to $dev/include instead of $out/share. This causes the cc
wrapper's resource-root to miss those files from compiler-rt as they are
in a different place than expected.

This commit fixes all instances of this incorrect installation for
llvmPackages_10, 11 and 12 which are the only llvm package sets which
link ${targetLlvmLibraries.compiler-rt.out}/share to the resource-root.

For the other llvm package set this will likely also need to be fixed,
but it doesn't have to have immediate urgency and doing it in two steps
allows us to (hopefully) fix the chromium build without causing a darwin
stdenv rebuild.

The full fix can be found in NixOS#123103 and should probably be included in
the next staging-next rotation.
@jonringer
Copy link
Contributor

Yea, not sure why I thought it was a good idea for ofborg to try and build chromium

@jonringer
Copy link
Contributor

Since the reduced changes were merged into staging-next. Should this be rebased on staging, so that the stdenv rebuild can be done on the next staging-next?

@sternenseemann
Copy link
Member Author

Sounds good to me.

This is in an effort to fix the following build failure shown by
chromium:

    clang++: error: no such file or directory: '/nix/store/fhd89wrmkx6nflzjk0d6waz70bk3zc4i-clang-wrapper-12.0.0/resource-root/share/cfi_blacklist.txt'

As it turns out a change introduced via the gnu-install-dirs.patch
caused `add_compiler_rt_resource_file` to install resource files to
$dev/include (FULL_INCLUDEDIR) instead of $out/share (FULL_DATADIR)
which in turn meant that the clang wrappers we had didn't link those
files to its resource root at all.

Alternative fix to this would have been to link
compiler-rt.dev/include/*.txt to the wrappers resource-root/share as
well, but since this was handled inconsistently across the patch anyways
(the dfsan list is installed correctly), opt to handle this
consistently within the patch.

llvmPackages_{5,6} install the resource files to a completely different
location and need separate investigation.
This is done for 10-12, but not for the earlier llvm package sets.
@sternenseemann sternenseemann changed the title [staging-next] llvmPackages_{7-12}.compiler-rt: install resource files to DATADIR llvmPackages_{7-12}.compiler-rt: install resource files to DATADIR May 15, 2021
@sternenseemann sternenseemann changed the base branch from staging-next to staging May 15, 2021 16:36
@Ericson2314 Ericson2314 merged commit 4236fb1 into NixOS:staging May 15, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants