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

clang_{5,6,7}: add --sysroot argument pointing to gcc toolchain prefix #53668

Open
wants to merge 1 commit into
base: staging
Choose a base branch
from

Conversation

@Twey
Copy link
Contributor

@Twey Twey commented Jan 8, 2019

Motivation for this change

Fixes #24697.

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)
  • Assured whether relevant documentation is up to date
  • Fits CONTRIBUTING.md.

@Twey Twey requested a review from matthewbauer as a code owner Jan 8, 2019
@matthewbauer
Copy link
Member

@matthewbauer matthewbauer commented Jan 8, 2019

Can we just set sysroot to empty? Otherwise we have to be careful of not propagating a dependcy on gcc.

@Twey
Copy link
Contributor Author

@Twey Twey commented Jan 8, 2019

Non-existent directories are invalid, plus clang without this patch seems to pick up some libraries from GCC that I'm not sure shouldn't be visible. GCC is passed in anyway as --gcc-toolchain so it seems like a safe option.

@@ -23,6 +23,7 @@ let
ln -s "${cc}/lib/clang/${release_version}/include" "$rsrc"
ln -s "${targetLlvmLibraries.compiler-rt.out}/lib" "$rsrc/lib"
echo "-resource-dir=$rsrc" >> $out/nix-support/cc-cflags
echo "--sysroot=${tools.clang-unwrapped.gcc}" >> $out/nix-support/cc-cflags
Copy link
Member

@LnL7 LnL7 Jan 8, 2019

Choose a reason for hiding this comment

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

clang-unwrapped.gcc is only valid linux with a gcc compiler if it needs to point to that then it must be conditionalized properly and same with the line below actually

default_cxx_stdlib_compile = optionalString (targetPlatform.isLinux && !(cc.isGNU or false) && !nativeTools && cc ? gcc)
"-isystem $(echo -n ${cc.gcc}/include/c++/*) -isystem $(echo -n ${cc.gcc}/include/c++/*)/$(${cc.gcc}/bin/gcc -dumpmachine)";

How does this relate to the -isystem flags set in the stdenv?

Copy link
Contributor Author

@Twey Twey Jan 9, 2019

Choose a reason for hiding this comment

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

-i is just for header files, and the sysroot doesn't affect paths given with absolute paths, just the search path for libraries given by name and toolchain binaries called during compilation (e.g. ld). I don't think these things interact.

Copy link
Contributor Author

@Twey Twey Jan 9, 2019

Choose a reason for hiding this comment

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

I'm a bit suspicious of these flags because I think they're included even if you want to use libc++, but I think that's the topic of a separate PR.

Copy link
Member

@LnL7 LnL7 Jan 9, 2019

Choose a reason for hiding this comment

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

It's the default, the variables are configured by the libc++ / libstdcxxHook hooks instead. I'm pretty sure we could actually get rid of this attribute now.

However doesn't this change have the problem you mentioned and break libcxxStdenv then? Whether libstdc++ or libc++ should be used and what version is determined by the extraPackages added to the cc-wrapper. In it's current form, if I'm not mistaken, this change would result in llvmPackages_7.stdenv using a --sysroot that points to llvmPackages_5.clang-unwrapped (default stdenv on darwin) which doesn't seem right to me.


targetLlvmLibraries.libcxx
targetLlvmLibraries.libcxxabi

Copy link
Contributor Author

@Twey Twey Jan 11, 2019

Choose a reason for hiding this comment

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

I've tested this (on Linux) and I don't believe it breaks libcxxStdlib. It puts libstdc++ on the library search path to be sure, but ultimately which stdlib is linked in is controlled by -stdlib as it's always been.

I'm less sure about Darwin and I don't have a system to test on, but on Linux llvmPackages_7.stdenv still gets the gcc sysroot. Does Darwin use libcxxStdenv by default? If that's so then I think it's also fine. I think the default sysroot is /, so this shouldn't break anything pure — in the worst case it adds the gcc libraries (including libstdc++) to the library search path, which are not necessary because they're not going to be linked in. But I don't think anything even gains gcc as an input that didn't have it before — if the toolchain isGNU then cc refers to a gcc that's already pulled in as clang-unwrapped.gcc, and otherwise it refers to clang itself I guess, which is fine since we're not going to be relying on libstdc++.

Copy link
Contributor Author

@Twey Twey Jan 11, 2019

Choose a reason for hiding this comment

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

Currently libstdc++ is given by the sysroot; if we want to change that behaviour maybe it should go in a separate PR?

@Twey Twey force-pushed the clang-purity branch from d7cb3a6 to d493473 Jan 9, 2019
@Twey
Copy link
Contributor Author

@Twey Twey commented Jan 9, 2019

Specifically it needs libstdc++.a to successfully compile C++ unless you're using libc++. I've changed it to use stdenv.cc.cc, though, which I believe is safe across platforms?

@Twey
Copy link
Contributor Author

@Twey Twey commented Jan 24, 2019

Is this okay? We're depending on this patch in-house (we use Clang on non-NixOS systems, and we need it to not grab system libraries).

@LnL7
Copy link
Member

@LnL7 LnL7 commented Jan 24, 2019

I don't believe it is for a clang based systems, but I might be wrong since I don't fully understand what that flag is supposed to do. Why does this need to point to gcc instead of clang itself?

EDIT:

output '/nix/store/2866q70dfvg3x6ici9sgw8vsinqrzhvl-stdenv-darwin' is not allowed to refer to the following paths:
     /nix/store/2ws9cmamvr7xyvdg4d2nnd1bmr1zjrrq-bootstrap-tools

@matthewbauer
Copy link
Member

@matthewbauer matthewbauer commented Jan 24, 2019

What the closure impact for this? I would much prefer this being --sysroot=/homeless-shelter or something like that.

TBH I don't understand why this is an issue. Don't you want your compiler to look in all of the directories for libraries? They shouldn't be given priority over LDFLAGS, but they definitely should be searched.

@LnL7
Copy link
Member

@LnL7 LnL7 commented Jan 24, 2019

That makes me even more confused, it shouldn't be pointing to the stdenv compiler at all in that case. If the path must exist $out or ${cc.cc} would make much more sense to me.

@Twey
Copy link
Contributor Author

@Twey Twey commented Jan 28, 2019

@matthewbauer Non-existent directories are invalid. We can make an empty derivation or something to point to — or point it to clang itself — but then we need to make further modifications to make sure all the gcc libraries (e.g. libstdc++) are found, which is currently done implicitly through the sysroot.

We don't want the compiler to be looking in global directories like /lib and /usr/lib, because this is impure. It means that, for example, nix-shell --pure or builds without sandboxing can exhibit impure behaviour where they succeed/fail based on external system libraries, which is a big issue for a reproducible build system.

@Twey
Copy link
Contributor Author

@Twey Twey commented Jan 28, 2019

As a concrete example, I ran across this issue when a Meson-based build that worked fine on my NixOS system failed on my coworker's Debian system, because he happened to have /usr/lib/libfoo.a and Meson uses the Clang library search paths to find and replace libraries in the linker command line.

@matthewbauer
Copy link
Member

@matthewbauer matthewbauer commented Feb 12, 2019

The issue with this is clang forms its own stdenv in many cases. This is by default on Darwin and is also used frequently on linux through clangStdenv. We don't want the GCC part leaking through in this case. It sounds like Meson is doing something really weird here though.

@Twey
Copy link
Contributor Author

@Twey Twey commented Feb 12, 2019

I agree it's weird, but Meson relies heavily on this behaviour and Clang shouldn't be giving impure paths in response to requests anyway — we should patch this out as we patch out other impure references in packages. Would you rather we had a patch to the source? This issue is currently blocking upgrade of nixpkgs at my company.

@matthewbauer matthewbauer reopened this Feb 15, 2019
@@ -23,6 +23,7 @@ let
ln -s "${cc}/lib/clang/${release_version}/include" "$rsrc"
ln -s "${targetLlvmLibraries.compiler-rt.out}/lib" "$rsrc/lib"
echo "-resource-dir=$rsrc" >> $out/nix-support/cc-cflags
echo "--sysroot=${stdenv.cc.cc}" >> $out/nix-support/cc-cflags
Copy link
Member

@matthewbauer matthewbauer Feb 15, 2019

Choose a reason for hiding this comment

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

Maybe this can work. But i think it should be cc not stdenv.cc.cc so that it comes from clang.

Copy link
Contributor Author

@Twey Twey Feb 16, 2019

Choose a reason for hiding this comment

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

Long-term I'd rather patch out the compiled-in search paths altogether and figure out a better way to switch between C++ stdlibs. As stands, though, this patch is designed to preserve the existing behaviour of finding gcc via the sysroot and using that to pick up libstdc++. Setting the sysroot to (clang) cc, or indeed any valid path, will fix the purity issue, but breaks any builds that expect clang++ to work out of the box — all C++ programs will need to explicitly include their stdlib as a dependency. Currently libstdc++ is found by --gcc-toolchain, but setting --sysroot overrides that, so we just put it back by explicitly setting the sysroot to gcc.

Copy link
Member

@matthewbauer matthewbauer Aug 1, 2019

Choose a reason for hiding this comment

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

Hmm... The wrapper should have handling for this through default_cxx_stdlib_compile. Is that maybe not working as expected:

https://github.com/NixOS/nixpkgs/blob/master/pkgs/build-support/cc-wrapper/default.nix#L48

My concern with using stdenv here is it pull in the parent's stdenv which could mess up things like cross compilation. But I definitely want to find a way to get rid of the --gcc-toolchain thing in the future, so perhaps we can handle them together.

@Twey Twey force-pushed the clang-purity branch 2 times, most recently from 85a98d8 to 8e85822 Feb 20, 2019
Copy link
Member

@matthewbauer matthewbauer left a comment

I'm okay with merging this now, as the changes seem like a step in the right direction. Two things are still needed though:

  • either base off staging and test on darwin or make conditional on linux.
  • add llvm 7

@Twey Twey force-pushed the clang-purity branch from 8e85822 to 77f454e Aug 23, 2019
@FRidh FRidh requested a review from LnL7 Nov 3, 2019
@FRidh FRidh changed the base branch from master to staging Nov 3, 2019
@stale
Copy link

@stale stale bot commented Dec 25, 2020

I marked this as stale due to inactivity. → More info

@veprbl veprbl added this to WIP in Staging via automation Nov 14, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Staging
  
WIP
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants