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

cudaPackages.backendStdenv.cc: gccForLibs #275947

Merged
merged 8 commits into from
Jan 13, 2024
31 changes: 30 additions & 1 deletion pkgs/build-support/cc-wrapper/default.nix
Original file line number Diff line number Diff line change
Expand Up @@ -110,6 +110,9 @@ let
gccForLibs_solib = getLib gccForLibs
+ optionalString (targetPlatform != hostPlatform) "/${targetPlatform.config}";

# Analogously to cc_solib and gccForLibs_solib
libcxx_solib = "${lib.getLib libcxx}/lib";
Comment on lines +113 to +114
Copy link
Contributor

Choose a reason for hiding this comment

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

I would have expected to see this

Suggested change
# Analogously to cc_solib and gccForLibs_solib
libcxx_solib = "${lib.getLib libcxx}/lib";
# Analogously to cc_solib and gccForLibs_solib
libcxx_solib = getLib libcxx
+ optionalString (targetPlatform != hostPlatform) "/${targetPlatform.config}";

Why do we (or do we not) nest when cross-compiling (beside the pain it causes our tooling, apparently #279952 (comment)).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Bc it was that way for darwin and i kept it, just moved to a variable


# The following two functions, `isGccArchSupported` and
# `isGccTuneSupported`, only handle those situations where a flag
# (`-march` or `-mtune`) is accepted by one compiler but rejected
Expand Down Expand Up @@ -261,6 +264,25 @@ stdenv.mkDerivation {
inherit bintools;
inherit cc libc libcxx nativeTools nativeLibc nativePrefix isGNU isClang;

# Expose the C++ standard library we're using. See the comments on "General
# libc++ support". This is also relevant when using older gcc than the
# stdenv's, as may be required e.g. by CUDAToolkit's nvcc.
cxxStdlib =
let
givenLibcxx = libcxx.isLLVM or false;
givenGccForLibs = useGccForLibs && gccForLibs.langCC or false;
in
if (!givenLibcxx) && givenGccForLibs then
{ kind = "libstdc++"; package = gccForLibs; solib = gccForLibs_solib; }
else if givenLibcxx then
{ kind = "libc++"; package = libcxx; solib = libcxx_solib;}
else
# We're probably using the `libstdc++` that came with our `gcc`.
# TODO: this is maybe not always correct?
# TODO: what happens when `nativeTools = true`?
Comment on lines +281 to +282
Copy link
Contributor

Choose a reason for hiding this comment

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

Any further notes or comments on the two TODOs? If they're still TODO, would you mind adding a bit more description to them or making an issue to follow up on them so they're not lost?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

{ kind = "libstdc++"; package = cc; solib = cc_solib; }
;

emacsBufferSetup = pkgs: ''
; We should handle propagation here too
(mapc
Expand Down Expand Up @@ -440,6 +462,13 @@ stdenv.mkDerivation {
echo "-L${gccForLibs}/lib/gcc/${targetPlatform.config}/${gccForLibs.version}" >> $out/nix-support/cc-ldflags
echo "-L${gccForLibs_solib}/lib" >> $out/nix-support/cc-ldflags
''
# The above "fix" may be incorrect; gcc.cc.lib doesn't contain a
# `target-triple` dir but the correct fix may be to just remove the above?
#
# For clang it's not necessary (see `--gcc-toolchain` below) and for other
# situations adding in the above will bring in lots of other gcc libraries
# (i.e. sanitizer libraries, `libatomic`, `libquadmath`) besides just
# `libstdc++`; this may actually break clang.
Comment on lines +465 to +471
Copy link
Contributor

Choose a reason for hiding this comment

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

Which above fix is this referring to? The other edit you made in the file is almost 200 lines earlier. If it's referring to that, can we move the comment there? This block seems out of place.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

These were left after cherry picking

This comment was marked as outdated.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@rrbutani why did you find it necessary to comment out dc6a8f9#diff-ad7b67d4a92a2db43281c9c45aa74c32341a554fbca0b6b25be593f4648abb5eR330?

I kept things as they were after @danielfullmer's PR


# TODO We would like to connect this to `useGccForLibs`, but we cannot yet
# because `libcxxStdenv` on linux still needs this. Maybe someday we'll
Expand Down Expand Up @@ -564,7 +593,7 @@ stdenv.mkDerivation {
echo "$ccLDFlags" >> $out/nix-support/cc-ldflags
echo "$ccCFlags" >> $out/nix-support/cc-cflags
'' + optionalString (targetPlatform.isDarwin && (libcxx != null) && (cc.isClang or false)) ''
echo " -L${lib.getLib libcxx}/lib" >> $out/nix-support/cc-ldflags
echo " -L${libcxx_solib}" >> $out/nix-support/cc-ldflags
''

##
Expand Down
79 changes: 0 additions & 79 deletions pkgs/development/compilers/cudatoolkit/extension.nix

This file was deleted.

37 changes: 14 additions & 23 deletions pkgs/development/cuda-modules/backend-stdenv.nix
Original file line number Diff line number Diff line change
Expand Up @@ -2,38 +2,29 @@
lib,
nvccCompatibilities,
cudaVersion,
buildPackages,
pkgs,
overrideCC,
stdenv,
wrapCCWith,
stdenvAdapters,
}:

let
gccMajorVersion = nvccCompatibilities.${cudaVersion}.gccMaxMajorVersion;
# We use buildPackages (= pkgsBuildHost) because we look for a gcc that
# runs on our build platform, and that produces executables for the host
# platform (= platform on which we deploy and run the downstream packages).
# The target platform of buildPackages.gcc is our host platform, so its
# .lib output should be the libstdc++ we want to be writing in the runpaths
# Cf. https://github.com/NixOS/nixpkgs/pull/225661#discussion_r1164564576
nixpkgsCompatibleLibstdcxx = buildPackages.gcc.cc.lib;
nvccCompatibleCC = buildPackages."gcc${gccMajorVersion}".cc;

cc = wrapCCWith {
cc = nvccCompatibleCC;

# This option is for clang's libcxx, but we (ab)use it for gcc's libstdc++.
# Note that libstdc++ maintains forward-compatibility: if we load a newer
# libstdc++ into the process, we can still use libraries built against an
# older libstdc++. This, in practice, means that we should use libstdc++ from
# the same stdenv that the rest of nixpkgs uses.
# We currently do not try to support anything other than gcc and linux.
libcxx = nixpkgsCompatibleLibstdcxx;
};
cudaStdenv = overrideCC stdenv cc;
cudaStdenv = stdenvAdapters.useLibsFrom stdenv pkgs."gcc${gccMajorVersion}Stdenv";
passthruExtra = {
inherit nixpkgsCompatibleLibstdcxx;
nixpkgsCompatibleLibstdcxx = lib.warn "cudaPackages.backendStdenv.nixpkgsCompatibleLibstdcxx is misnamed, deprecated, and will be removed after 24.05" cudaStdenv.cc.cxxStdlib.package;
# cc already exposed
};
assertCondition = true;
in

# We should use libstdc++ at least as new as nixpkgs' stdenv's one.
assert let
cxxStdlibCuda = cudaStdenv.cc.cxxStdlib.package;
cxxStdlibNixpkgs = stdenv.cc.cxxStdlib.package;
in
((stdenv.cc.cxxStdlib.kind or null) == "libstdc++")
-> lib.versionAtLeast cxxStdlibCuda.version cxxStdlibNixpkgs.version;

lib.extendDerivation assertCondition passthruExtra cudaStdenv
19 changes: 16 additions & 3 deletions pkgs/development/cuda-modules/cuda/overrides.nix
Original file line number Diff line number Diff line change
Expand Up @@ -85,7 +85,20 @@ attrsets.filterAttrs (attr: _: (builtins.hasAttr attr prev)) {
);

cuda_nvcc = prev.cuda_nvcc.overrideAttrs (
oldAttrs: {
oldAttrs:
let
# This replicates the logic in stdenvAdapters.useLibsFrom, except we use
# gcc from pkgsHostTarget and not from buildPackages.
ccForLibs-wrapper = final.pkgs.stdenv.cc;
gccMajorVersion = final.nvccCompatibilities.${cudaVersion}.gccMaxMajorVersion;
cc = final.pkgs.wrapCCWith {
cc = final.pkgs."gcc${gccMajorVersion}".cc;
useCcForLibs = true;
gccForLibs = ccForLibs-wrapper.cc;
};
cxxStdlibDir = ccForLibs-wrapper.cxxStdlib.solib;
in
{

outputs = oldAttrs.outputs ++ lists.optionals (!(builtins.elem "lib" oldAttrs.outputs)) [ "lib" ];

Expand Down Expand Up @@ -119,8 +132,8 @@ attrsets.filterAttrs (attr: _: (builtins.hasAttr attr prev)) {
cat << EOF >> bin/nvcc.profile

# Fix a compatible backend compiler
PATH += ${lib.getBin final.backendStdenv.cc}/bin:
LIBRARIES += "-L${lib.getLib final.backendStdenv.nixpkgsCompatibleLibstdcxx}/lib"
PATH += ${lib.getBin cc}/bin:
LIBRARIES += "-L${cxxStdlibDir}/lib"

# Expose the split-out nvvm
LIBRARIES =+ -L''${!outputBin}/nvvm/lib
Expand Down
1 change: 0 additions & 1 deletion pkgs/development/python-modules/jaxlib/default.nix
Original file line number Diff line number Diff line change
Expand Up @@ -282,7 +282,6 @@ let
# loading multiple extensions in the same python program due to duplicate protobuf DBs.
# 2) Patch python path in the compiler driver.
preBuild = lib.optionalString cudaSupport ''
export NIX_LDFLAGS+=" -L${backendStdenv.nixpkgsCompatibleLibstdcxx}/lib"
patchShebangs ../output/external/xla/third_party/gpus/crosstool/clang/bin/crosstool_wrapper_driver_is_not_gcc.tpl
'' + lib.optionalString stdenv.isDarwin ''
# Framework search paths aren't added by bintools hook
Expand Down
24 changes: 24 additions & 0 deletions pkgs/stdenv/adapters.nix
Original file line number Diff line number Diff line change
Expand Up @@ -237,6 +237,30 @@ rec {
});
});

useLibsFrom = modelStdenv: targetStdenv:
let
ccForLibs = modelStdenv.cc.cc;
cc = pkgs.wrapCCWith {
/* NOTE: cc.cc is the unwrapped compiler. Should we respect the old
* wrapper instead? */
cc = targetStdenv.cc.cc;
Comment on lines +244 to +246
Copy link
Contributor Author

Choose a reason for hiding this comment

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


/* NOTE(originally by rrbutani):
* Normally the `useCcForLibs`/`gccForLibs` mechanism is used to get a
* clang based `cc` to use `libstdc++` (from gcc).
*
* Here we (ab)use it to use a `libstdc++` from a different `gcc` than our
* `cc`.
*
* Note that this does not inhibit our `cc`'s lib dir from being added to
* cflags/ldflags (see `cc_solib` in `cc-wrapper`) but this is okay: our
* `gccForLibs`'s paths should take precedence. */
useCcForLibs = true;
gccForLibs = ccForLibs;
};
in
overrideCC targetStdenv cc;

useMoldLinker = stdenv: let
bintools = stdenv.cc.bintools.override {
extraBuildCommands = ''
Expand Down
7 changes: 7 additions & 0 deletions pkgs/top-level/python-packages.nix
Original file line number Diff line number Diff line change
Expand Up @@ -14143,6 +14143,13 @@ self: super: with self; {
grpc = compat.grpcTF;
grpcio = compat.grpcioTF;
tensorboard = compat.tensorboardTF;

# Tensorflow 2.13 doesn't support gcc13:
# https://github.com/tensorflow/tensorflow/issues/61289
#
# We use the nixpkgs' default libstdc++ to stay compatible with other
# python modules
stdenv = pkgs.stdenvAdapters.useLibsFrom stdenv pkgs.gcc12Stdenv;
};

tensorflow-datasets = callPackage ../development/python-modules/tensorflow-datasets { };
Expand Down