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

Build failure: rustc on staging #318674

Open
reckenrode opened this issue Jun 10, 2024 · 23 comments
Open

Build failure: rustc on staging #318674

reckenrode opened this issue Jun 10, 2024 · 23 comments
Labels

Comments

@reckenrode
Copy link
Contributor

Steps To Reproduce

Steps to reproduce the behavior:

  1. nix build 'github:NixOS/nixpkgs?ref=afa876d6feea138d23310cc19662b0d3364570d9#rustc' (tested only on Darwin).

Build log

https://gist.github.com/reckenrode/53c181f0c8fff37707e719734250024c

Additional context

Possibly due to #316761. #317273 may be related to the fix, but it targets machine flags not hardening flags.

Clang does provide hardeningUnsupportedFlagsByTargetPlatform, but if Rust is not invoking a wasm32-unknown-unknown target clang, I don’t think that will apply.

Notify maintainers

@risicle @alyssais

Metadata

Please run nix-shell -p nix-info --run "nix-info -m" and paste the result.

[user@system:~]$ nix-shell -p nix-info --run "nix-info -m"
 - system: `"aarch64-darwin"`
 - host os: `Darwin 23.5.0, macOS 14.5`
 - multi-user?: `no`
 - sandbox: `yes`
 - version: `nix-env (Nix) 2.18.2`
 - channels(root): `"nixpkgs"`
 - nixpkgs: `/etc/nix/inputs/nixpkgs`

I actually have the Darwin sandbox disabled, so I don’t know what’s up with nix-info.


Add a 👍 reaction to issues you find important.

@reckenrode reckenrode added the 0.kind: build failure A package fails to build label Jun 10, 2024
@reckenrode reckenrode changed the title Build failure: rustc Build failure: rustc on staging Jun 10, 2024
@alyssais
Copy link
Member

Sounds like maybe we should not enable this by default for clang stdenv until we have a mechanism to filter Clang hardening flags by target?

@risicle
Copy link
Contributor

risicle commented Jun 11, 2024

That's weird, because cc-wrapper is full of things that assume a single specific target platform, which is why I decided we could get away with hardeningUnsupportedFlagsByTargetPlatform operating at the meta level instead of having to sniff target platform at compiler invocation time (please god no). So really, I'm amazed it works at all.

The simple thing to do here is hardeningDisable = ["zerocallusedregs"] for rustc, no?

I'm sure you can try it faster than me, already having a half-built toolchain - it'll take me ~12h to bootstrap darwin staging.

(also I really thought I built past rustc on macos x86_64 before I merged this...)

@reckenrode
Copy link
Contributor Author

Setting hardeningDisable =["zerocallusedregs"] leads to a different error:

  cargo:warning=clang: warning: argument unused during compilation: '-mmacos-version-min=10.12' [-Wunused-command-line-argument]
  cargo:warning=clang: error: unsupported option '-arch' for target 'wasm32-unknown-unknown'

Is it possible to set CC_wasm32_unknown_unknown to a clang with the right flags? I tried setting it to clang-unwrapped, but that didn’t work due to not finding libc headers.

@alyssais
Copy link
Member

That's weird, because cc-wrapper is full of things that assume a single specific target platform, which is why I decided we could get away with hardeningUnsupportedFlagsByTargetPlatform operating at the meta level instead of having to sniff target platform at compiler invocation time (please god no). So really, I'm amazed it works at all.

Most of them are just irrelevant for weird targets like bpf or wasm32-unknown-unknown, and get ignored. It's only recently that Clang has started producing errors rather than either warning or ignoring them. I think it's a good experience if we're able to support at least those targets, because it's not like there's a bpf stdenv.

@risicle
Copy link
Contributor

risicle commented Jun 12, 2024

My preference would probably be to guide people towards using an unwrapped compiler in this case then - it's not clear to me what value wrapping a wasm compiler adds, and it would be nicer to avoid the complexity and not start giving people any expectations of a wrapped compiler working as a multi-target compiler. In fact I'd probably advocate the wrapper at least emitting a warning if it detects the --target flag being used. If not throw an error entirely.

I'm not sure what's going on with this new rustc error, but I can't see it being related to the zerocallusedregs addition.

@risicle
Copy link
Contributor

risicle commented Jun 23, 2024

@risicle
Copy link
Contributor

risicle commented Jun 23, 2024

Indeed if we patch that with

--- a/src/bootstrap/src/utils/cc_detect.rs        2024-06-23 14:29:18.395349230 +0100
+++ b/src/bootstrap/src/utils/cc_detect.rs    2024-06-23 14:35:42.303268205 +0100
@@ -174,6 +174,14 @@
             build.config.android_ndk.as_ref().map(|ndk| ndk_compiler(compiler, &target.triple, ndk))
         }

+        t if t.starts_with("wasm") => {
+            let root = PathBuf::from("@clangUnwrapped@/bin");
+            match compiler {
+                Language::C => Some(root.join("clang")),
+                Language::CPlusPlus => Some(root.join("clang++")),
+            }
+        }
+
         // The default gcc version from OpenBSD may be too old, try using egcc,
         // which is a gcc version from ports, if this is the case.
         t if t.contains("openbsd") => {

it will use the unwrapped clang. But now it can't find any of its headers 🫠

  cargo:warning=In file included from /private/tmp/nix-build-rustc-1.78.0.drv-1/rustc-1.78.0-src/src/llvm-project/compiler-rt/lib/builtins/absvdi2.c:13:
  cargo:warning=/private/tmp/nix-build-rustc-1.78.0.drv-1/rustc-1.78.0-src/src/llvm-project/compiler-rt/lib/builtins/int_lib.h:92:10: fatal error: 'float.h' file not found
  cargo:warning=   92 | #include <float.h>
  cargo:warning=      |          ^~~~~~~~~
  cargo:warning=1 error generated.

@alyssais
Copy link
Member

I think the best solution here might be for the clang wrapper to exec unwrapped clang if it sees -target. That way, we still have a multi-target compiler, which is good, because there are packages that rely on that, but we also don't end up using the arguments injected by the wrapper when building for a custom target (which is probably what those packages expect anyway.)

@risicle
Copy link
Contributor

risicle commented Jun 23, 2024

I think that will just put us in the same situation as with the patch - falling back to the unwrapped clang will give you a clang that can't find its stdlib headers. Also not sure how much I like the idea of making --target act like a cliff edge, drastically changing behaviour.

The quickest thing to do here is to add hardeningDisable = [ "zercallusedregs" ] to rustc. That will take us back to the situation before this breakage.

The "most correct" thing is probably to do something like point the build process at pkgsCross.wasi32.stdenv.cc for its wasm compiler, allowing any logic around choice of flags to continue to operate in nix through hardeningUnsupportedFlagsByTargetPlatform. But that adds a lot of new build-time-dependencies.

What we really want I reckon is to re-wrap our existing clang-unwrapped compiler with a wasm platform, requiring fewest additional builds.

What I can't figure out though is why this isn't a problem on linux/gcc.

@alyssais
Copy link
Member

The "most correct" thing is probably to do something like point the build process at pkgsCross.wasi32.stdenv.cc for its wasm compiler

It's generally @Ericson2314's and my intention that pkgsCross shouldn't really be used in this way, within a package. It's intended to be a convenience for using Nixpkgs, not use in packages, where it causes performance and layering problems.

@reckenrode
Copy link
Contributor Author

reckenrode commented Jun 24, 2024

The "most correct" thing is probably to do something like point the build process at pkgsCross.wasi32.stdenv.cc for its wasm compiler

It's generally @Ericson2314's and my intention that pkgsCross shouldn't really be used in this way, within a package. It's intended to be a convenience for using Nixpkgs, not use in packages, where it causes performance and layering problems.

What would be the intended way to invoke a cross-compiler in a build when its platform isn’t the default target?

@alyssais
Copy link
Member

For the common cases — wasm or bpf — there's no libc to build, so we'd just use normal clang or rustc or whatever (with a fixed wrapper), and get the benefits of a multi-arch compiler.

I don't think there are many packages that where part but not all of it needs to be compiled for e.g. aarch64-linux on x86_64-linux. Is that sort of situation something you're concerned about?

@reckenrode
Copy link
Contributor Author

reckenrode commented Jun 24, 2024

I don't think there are many packages that where part but not all of it needs to be compiled for e.g. aarch64-linux on x86_64-linux. Is that sort of situation something you're concerned about?

Wine uses MinGW to build PE DLLs. MinGW is GCC, so it has to be built as a cross-compiler.

It’s used via pkgsCross, which causes some problems on Darwin, but it has been worked around in the Wine derivation.

@alyssais
Copy link
Member

@Ericson2314 how do you envisage that working?

@risicle
Copy link
Contributor

risicle commented Jun 24, 2024

pkgsCross shouldn't really be used in this way, within a package

Agree with this, it feels dirty.

I think I have something else that will work, but will try it before I elaborate...

@Ericson2314
Copy link
Member

I think for Wine, for now, it would be OK to have a pkgsMingw just as we have a pkgsi686. It is ugly, but is less bad than saying "fuck it, pkgsCross is OK to use". I don't love this at all, of course :)

@risicle
Copy link
Contributor

risicle commented Jun 24, 2024

Hold your stomachs

--- a/pkgs/development/compilers/rust/rustc.nix
+++ b/pkgs/development/compilers/rust/rustc.nix
@@ -18,12 +18,29 @@
 # This only builds std for target and reuses the rustc from build.
 , fastCross
 , lndir
+, substituteAll
 , makeWrapper
 }:
 
 let
   inherit (lib) optionals optional optionalString concatStringsSep;
   inherit (darwin.apple_sdk.frameworks) Security;
+  clangRewrapped = stdenv.cc.override (old: rec {
+    stdenvNoCC = old.stdenvNoCC.override {
+      targetPlatform = old.stdenvNoCC.targetPlatform // {
+        parsed = {
+          cpu.name = "wasm32";
+          vendor.name = "unknown";
+          kernel.name = "unknown";
+          abi.name = "unknown";
+        };
+        config = "wasm32-unknown-unknown";
+      };
+    };
+    bintools = old.bintools.override {
+      inherit stdenvNoCC;
+    };
+  });
 in stdenv.mkDerivation (finalAttrs: {
   pname = "${targetPackages.stdenv.cc.targetPrefix}rustc";
   inherit version;
@@ -194,7 +211,12 @@ in stdenv.mkDerivation (finalAttrs: {
   # the rust build system complains that nix alters the checksums
   dontFixLibtool = true;
 
-  inherit patches;
+  patches = patches ++ [
+    (substituteAll {
+      src = ./wasm-clang-unwrapped.patch;
+      clangUnwrapped = clangRewrapped;
+    })
+  ];
 
   postPatch = ''
     patchShebangs src/etc
--- /dev/null
+++ b/pkgs/development/compilers/rust/wasm-clang-unwrapped.patch
@@ -0,0 +1,19 @@
+--- a/src/bootstrap/src/utils/cc_detect.rs        2024-06-23 14:29:18.395349230 +0100
++++ b/src/bootstrap/src/utils/cc_detect.rs    2024-06-23 14:35:42.303268205 +0100
+@@ -174,6 +174,14 @@
+             build.config.android_ndk.as_ref().map(|ndk| ndk_compiler(compiler, &target.triple, ndk))
+         }
+
++        t if t.starts_with("wasm") => {
++            let root = PathBuf::from("@clangUnwrapped@/bin");
++            match compiler {
++                Language::C => Some(root.join("wasm32-unknown-unknown-clang")),
++                Language::CPlusPlus => Some(root.join("wasm32-unknown-unknown-clang++")),
++            }
++        }
++
+         // The default gcc version from OpenBSD may be too old, try using egcc,
+         // which is a gcc version from ports, if this is the case.
+         t if t.contains("openbsd") => {
+
+

WFM macos 12 x86_64.

Obvs would need something slightly different for non-clang stdenvs - or maybe not because they don't appear to be broken and nobody seems to know why.

@risicle
Copy link
Contributor

risicle commented Jun 30, 2024

The deeper I dig the weirder this gets:

On x86_64 linux, after the build:

$ file build/x86_64-unknown-linux-gnu/stage2-std/wasm32-unknown-unknown/release/build/compiler_builtins-ee0249720c2fbd55/out/f671b77bc351d22a-absvdi2.o
ELF 64-bit LSB relocatable, x86-64, version 1 (SYSV), not stripped

(same for *.o in that dir)

x86_64 darwin after the build (if zerocallusedregs temporarily disabled to avoid error):

$ file build/x86_64-apple-darwin/stage2-std/wasm32-unknown-unknown/release/build/compiler_builtins-c6397cafd08482ba/out/424303ac06eede49-absvdi2.o
WebAssembly (wasm) binary module version 0x1 (MVP)

(again, same for *.o in that dir)

So.. on linux is it just blindly compiling these compiler builtins for the host platform? Are we actually assured that the end result works for wasm32?

(all on staging-next)

@K900
Copy link
Contributor

K900 commented Jul 1, 2024

Same issue on nvcc, by the way.

@risicle
Copy link
Contributor

risicle commented Jul 1, 2024

Suggest same stopgap.

@K900
Copy link
Contributor

K900 commented Jul 1, 2024

Done in #323849

@risicle
Copy link
Contributor

risicle commented Jul 1, 2024

Feels like this issue is becoming a general "make it easy/possible to re-wrap a compiler for a different target" point of reference.

Incidentally @K900 how does the ffmpeg situation react to being given an un-wrapped clang for cuda-llvm mode?

@Atemu
Copy link
Member

Atemu commented Nov 22, 2024

I noticed the workaround for this is still in ffmpeg.

Is this still an issue?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

7 participants