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.

Sign up for GitHub

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: use the same libstdc++ as the rest of nixpkgs #223664

Merged
merged 4 commits into from
Apr 7, 2023
Merged

cudaPackages: use the same libstdc++ as the rest of nixpkgs #223664

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
bd62420
cudaPackages: use same libstdc++ as the rest of nixpkgs
SomeoneSerge Mar 28, 2023
ab9c46d
cudaPackages.cudnn: fix libstdc++ mismatch
SomeoneSerge Apr 4, 2023
13c47d6
faissWithCuda: do not leak the old libstdc++
SomeoneSerge Apr 5, 2023
3af299f
cudaPackages.cudatoolkit: hotfix the libstdc++ mismatch caused by cud…
SomeoneSerge Apr 4, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 4 additions & 2 deletions pkgs/development/compilers/cudatoolkit/common.nix
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ args@
, python3 # FIXME: CUDAToolkit 10 may still need python27
, pulseaudio
, requireFile
, stdenv
, backendStdenv # E.g. gcc11Stdenv, set in extension.nix
, unixODBC
, wayland
Expand Down Expand Up @@ -121,8 +122,8 @@ backendStdenv.mkDerivation rec {
(placeholder "lib")
(placeholder "out")
"${placeholder "out"}/nvvm"
# Is it not handled by autoPatchelf automatically?
"${lib.getLib backendStdenv.cc.cc}/lib64"
# NOTE: use the same libstdc++ as the rest of nixpkgs, not from backendStdenv
"${lib.getLib stdenv.cc.cc}/lib64"
Copy link
Member

Choose a reason for hiding this comment

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

I thought we just did a bunch of work refactoring so that we could use backendStdenv's lib? why are we reverting?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Using backendStdenv results in linking against the new libstdc++ as long as no one tries to manually add some cc.cc.lib into buildInputs or otherwise override anything. However, backendStdenv.cc.cc.lib is still an "output" of the cc.cc derivation, and backendStdenv.cc.cc is gcc11...

I'm sorry I don't know how to make this cleaner at the moment, but for things at least to work we need to explicitly link normal stdenv's libstdc++

also it's time to ditch cudaPackages.cudatoolkit

Copy link
Member

Choose a reason for hiding this comment

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

ah ok thanks for explaining... so can we get rid of backendStdenv entirely then? IIUC it should no longer be used after this PR?

"${placeholder "out"}/jre/lib/amd64/jli"
"${placeholder "out"}/lib64"
"${placeholder "out"}/nvvm/lib64"
Expand Down Expand Up @@ -204,6 +205,7 @@ backendStdenv.mkDerivation rec {

mv pkg/builds/nsight_systems/target-linux-x64 $out/target-linux-x64
mv pkg/builds/nsight_systems/host-linux-x64 $out/host-linux-x64
rm $out/host-linux-x64/libstdc++.so*
''}

rm -f $out/tools/CUDA_Occupancy_Calculator.xls # FIXME: why?
Expand Down
14 changes: 10 additions & 4 deletions pkgs/development/compilers/cudatoolkit/extension.nix
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,11 +10,17 @@ final: prev: let
finalVersion = cudatoolkitVersions.${final.cudaVersion};

# Exposed as cudaPackages.backendStdenv.
# We don't call it just "stdenv" to avoid confusion: e.g. this toolchain doesn't contain nvcc.
# Instead, it's the back-end toolchain for nvcc to use.
# We also use this to link a compatible libstdc++ (backendStdenv.cc.cc.lib)
# This is what nvcc uses as a backend,
# and it has to be an officially supported one (e.g. gcc11 for cuda11).
#
# It, however, propagates current stdenv's libstdc++ to avoid "GLIBCXX_* not found errors"
# when linked with other C++ libraries.
# E.g. for cudaPackages_11_8 we use gcc11 with gcc12's libstdc++
Comment on lines +13 to +18
Copy link
Member

Choose a reason for hiding this comment

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

thanks for explaining! these comments are super useful!

# Cf. https://github.com/NixOS/nixpkgs/pull/218265 for context
backendStdenv = prev.pkgs."${finalVersion.gcc}Stdenv";
backendStdenv = final.callPackage ./stdenv.nix {
nixpkgsStdenv = prev.pkgs.stdenv;
nvccCompatibleStdenv = prev.pkgs.buildPackages."${finalVersion.gcc}Stdenv";
};

### Add classic cudatoolkit package
cudatoolkit =
Expand Down
9 changes: 5 additions & 4 deletions pkgs/development/compilers/cudatoolkit/redist/build-cuda-redist-package.nix
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
{ lib
, stdenv
, backendStdenv
, fetchurl
, autoPatchelfHook
Expand Down Expand Up @@ -30,11 +31,11 @@ backendStdenv.mkDerivation {
];

buildInputs = [
# autoPatchelfHook will search for a libstdc++ and we're giving it a
# "compatible" libstdc++ from the same toolchain that NVCC uses.
#
# autoPatchelfHook will search for a libstdc++ and we're giving it
# one that is compatible with the rest of nixpkgs, even when
# nvcc forces us to use an older gcc
# NB: We don't actually know if this is the right thing to do
backendStdenv.cc.cc.lib
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I probably should leave a warning somewhere, but if one accidentally uses backendStdenv.cc.cc.lib they'll end up breaking things by linking against the old libstdc++: .lib is just an output that cc happens to have, and wrapCCWith doesn't expose any passthru for libcxx

stdenv.cc.cc.lib
];

dontBuild = true;
Expand Down
17 changes: 17 additions & 0 deletions pkgs/development/compilers/cudatoolkit/stdenv.nix
View file
Open in desktop
Copy link
Member

Choose a reason for hiding this comment

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

why does this need to live in its own file? AFAICT it's only used in one place -- why not just inline it there?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right, so the idea seems to be that we eventually want to construct our own cudaStdenv, such that when people use it they don't have to add cuda_nvcc to nativeBuildInputs, and that the desiredata about old-gcc new-libstdc++ is satisfied automatically. I intended this file as cudaStdenv's future home

Copy link
Member

Choose a reason for hiding this comment

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

ohhh ok, makes sense

Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
{ nixpkgsStdenv
, nvccCompatibleStdenv
, overrideCC
, wrapCCWith
}:

overrideCC nixpkgsStdenv (wrapCCWith {
cc = nvccCompatibleStdenv.cc.cc;

# 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 = nixpkgsStdenv.cc.cc.lib;
})
8 changes: 5 additions & 3 deletions pkgs/development/libraries/science/math/cudnn/generic.nix
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
{
{ stdenv,
backendStdenv,
lib,
zlib,
Expand Down Expand Up @@ -26,7 +26,6 @@
maxCudaVersion,
}:
assert useCudatoolkitRunfile || (libcublas != null); let
inherit (backendStdenv) cc;
inherit (lib) lists strings trivial versions;

# majorMinorPatch :: String -> String
Expand Down Expand Up @@ -63,7 +62,10 @@ in

# Used by autoPatchelfHook
buildInputs = [
cc.cc.lib # libstdc++
# Note this libstdc++ isn't from the (possibly older) nvcc-compatible
# stdenv, but from the (newer) stdenv that the rest of nixpkgs uses
stdenv.cc.cc.lib

zlib
cudatoolkit_root
];
Expand Down
6 changes: 4 additions & 2 deletions pkgs/development/libraries/science/math/faiss/default.nix
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,17 +25,19 @@
builtins.head optLevels
, faiss # To run demos in the tests
, runCommand
}:
}@inputs:
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
}@inputs:
}:

Copy link
Contributor Author

Choose a reason for hiding this comment

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

stdenv = if cudaSupport then backendStdenv else stdenv;

This causes infinite recursion in (whatever version of Nix I've tried this with a month ago)

Copy link
Member

@samuela samuela Apr 6, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

What about something like

effectiveStdenv = if cudaSupport then backendStdenv else stdenv;

?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried that earlier in tensorflow, I think. Sandro pointed out that this way it's really easy for someone to accidentally refer to the old stdenv since it's still in the scope

Copy link
Member

Choose a reason for hiding this comment

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

ok that's a fair point. I'm happy to go either way


assert cudaSupport -> nvidia-thrust.cudaSupport;

let
pname = "faiss";
version = "1.7.2";

inherit (cudaPackages) cudaFlags;
inherit (cudaPackages) cudaFlags backendStdenv;
inherit (cudaFlags) cudaCapabilities dropDot;

stdenv = if cudaSupport then backendStdenv else inputs.stdenv;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@samuela look, this worked 😆 😅

import faiss
import zmq

works now.

So yeah, further on the roadmap is to add hooks for nvcc and FindCUDAToolkit.cmake, and rename this into cudaStdenv

Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
stdenv = if cudaSupport then backendStdenv else inputs.stdenv;
stdenv = if cudaSupport then backendStdenv else stdenv;


cudaJoined = symlinkJoin {
name = "cuda-packages-unsplit";
paths = with cudaPackages; [
Expand Down