NixOS / nixpkgs

Nix Packages collection & NixOS
MIT License
17.57k stars 13.73k forks source link

zig: builds for -march=native by default #185665

Open K900 opened 2 years ago

K900 commented 2 years ago

Describe the bug

zig as currently used in nixpkgs builds for the current machine's native CPU architecture instead of baseline x86-64, resulting in SIGILLs on older machines.

Steps To Reproduce

  1. nix build nixpkgs#ncdu --rebuild on a machine with AVX512
  2. Disassemble the resulting binary
  3. Observe AVX512

Expected behavior

The resulting binaries are not built with extended instructions.

Additional context

Do we really need to wrap zig for this?

Notify maintainers

@aiotter @andrewrk @AndersonTorres

Artturin commented 2 years ago

create a issue upstream?

EDIT: someone created one 2 days ago https://github.com/ziglang/zig/issues/12458

ajs124 commented 2 years ago

upstream basically says "not an issue", so we should probably just fix this in nixpkgs

AndersonTorres commented 2 years ago

Where is this flag implemented?

tavi-vi commented 2 years ago

Ideally, I think -Dtarget= should be used, it removes the cpu nondeterminism (zig uses a baseline cpu model when a target is specified) along with potentially architecture and OS nondeterminism when cross compiling. And if nondeterminism is the concern here, some provisions might need to be made to prevent compiling in Debug mode (the default) as only the Release{Fast,Safe,Small} modes are guaranteed to reproducibly build

ajs124 commented 2 years ago

grepping for zig build in nixpkgs currently gives us this:

pkgs/applications/misc/mepo/default.nix:    zig build test
pkgs/applications/misc/mepo/default.nix:    zig build -Drelease-safe=true -Dcpu=baseline --prefix $out install
pkgs/applications/misc/rivercarro/default.nix:    zig build -Drelease-safe -Dcpu=baseline --prefix $out install
pkgs/applications/window-managers/river/default.nix:    zig build -Drelease-safe -Dcpu=baseline ${lib.optionalString xwaylandSupport "-Dxwayland"} -Dman-pages --prefix $out install
pkgs/development/tools/zls/default.nix:    zig build -Drelease-safe -Dcpu=baseline --prefix $out install
pkgs/games/blackshades/default.nix:    zig build -Drelease-fast -Dcpu=baseline --prefix $out install
pkgs/tools/misc/clipbuzz/default.nix:    zig build -Drelease-safe -Dcpu=baseline --prefix $out install
pkgs/tools/misc/findup/default.nix:    zig build -Drelease-safe -Dcpu=baseline --prefix $out

all of those look like they're doing the right thing, but

  1. that doesn't cover software like ncdu, which uses a makefile, which didn't/doesn't have these flags
  2. it's manually specified for every package

To fix both these shortcomings, we should probably create a zig setupHook, so including zig in your nativeBuildInputs calls zig build automatically and a wrapper, so the flags are always correct, even if a makefile is used which calls zig without the appropriate flags.

The real-world issue this would fix is #185644 and it would ideally prevent any such issues from occurring again in the future.

GaetanLepage commented 1 year ago

Indeed, this would help to unbreak ncdu for instance !

sagehane commented 1 year ago

I just wanted to make sure if -Dcpu=baseline really is the right way to address this. Does this work when cross-compiling Zig packages with Nix?

I tried building ncdu for aarch64-linux and the 1.x version (the non-Zig version) worked fine when called with qemu but the 2.x version gave me this error:

ld.lld: error: /build/ncdu-2.1.2/zig-cache/o/38c185940cff86169251500b2f734d09/ncdu.o is incompatible with elf64-littleaarch64

I'm not sure if I even did the cross-compilation correctly though, based it on the expression from https://nixos.wiki/wiki/Cross_Compiling and did something like:

with import <nixpkgs> {
  crossSystem = {
    config = "aarch64-unknown-linux-gnu";
  };
};

mkShell {
  buildInputs = [ ncdu ncdu_1 ];
}
strager commented 1 year ago

Proposed fix: #214440

andrewrk commented 1 year ago

Hi, upstream here. I'd like to provide some perspective from this side of things, and then we can meet in the middle, for the benefit of our mutual users.

Zig packages do not necessarily have an optimization mode or a target. Consider, for example, a package which uses the zig build system, but only uses it compiles a tool for the host and then run it at build time to produce documentation, and the only thing installed to $prefix is some HTML files. In such case there will be no optimization flags or target flags exposed.

On the other hand, most zig projects, at least today, do install target-specific build artifacts, and that is why the zig build system has a way to ask for optimization mode and target with a standard set of flags. However, nix cannot assume the existence of -Dtarget, -Dcpu, or -Drelease options.

Regarding determinism - Zig guarantees deterministic builds when the optimization mode is a release build rather than a debug build. When building a Zig package, nix must always choose a release mode, rather than debug mode. It would be unthinkable to ship debug builds to users who install applications via nix. Zig packages generally will choose between one or two options when exposing optimization options:

  1. They have a preferred release mode, in which case they expose a boolean -Drelease flag, and make their own choice in the build script.
  2. They leave the decision up to the user, in which case, -Drelease-safe, -Drelease-small, or -Drelease-fast can be used.

I'd like to push back on the shortcomings mentioned earlier in this thread:

that doesn't cover software like ncdu, which uses a makefile, which didn't/doesn't have these flags

ncdu at least now has adjusted its makefile to address this problem, and is already fixed in the nix package. This problem is solved. I argue that its makefile should have been patched rather than patching the zig compiler itself, when the problem existed.

it's manually specified for every package

I fundamentally disagree with this being an issue. Each package might be differently configured, and that is why there is a nix file for each package, so that each package's special needs can be addressed. I think that it is desirable for there to be this layer of dialogue between nix and a zig package's build script, which could advertise a rich array of options, of which target CPU and optimization mode are only scratching the surface.

I just wanted to make sure if -Dcpu=baseline really is the right way to address this. Does this work when cross-compiling Zig packages with Nix?

I think a better way would be accessing the desired target architecture, operating system along with minimum/maximum OS version, and ABI, along with potentially the glibc version, from nix, and then specifying these via the standard -Dtarget option. I'm happy to assist in getting this working.

I mentioned this over in #214440, but I'll say it again here: Consider that, while it is crucial for zig to use the baseline CPU when zig is being used by nix to build nix packages for binary distributions, another important use case is nix users directly using the zig binary provided by nix to develop their own upstream projects. Please, let us not have a repeat of #18995.

BTW I have been a happy NixOS user for many years now. I'm available to discuss this further here as well as #nixos on libera.

strager commented 1 year ago

Zig guarantees deterministic builds when the optimization mode is a release build rather than a debug build

This is not my experience with the Zig toolchain:

$ nix-shell -I ~/Projects/ -p zig_0_9 --run 'zig build-exe -O ReleaseFast hello.zig'
$ shasum -a 256 hello                            
19be6b12d8033b1a735add02e757806926961878ef030fb5d29525d7a04e9b36  hello

$ nix-shell -I ~/Projects/ -p zig_0_9 --run 'qemu-x86_64-static -cpu Broadwell-v2 $(which zig) build-exe -O ReleaseFast hello.zig'
$ shasum -a 256 hello
f4f7e010412f1a461ef32ced02c050770a169e74fda84f57b43f2a696b5f8ed0  hello

$ nix-shell -I ~/Projects/ -p zig_0_9 --run 'qemu-x86_64-static -cpu Broadwell-v4 $(which zig) build-exe -O ReleaseFast hello.zig'
$ shasum -a 256 hello
f4f7e010412f1a461ef32ced02c050770a169e74fda84f57b43f2a696b5f8ed0  hello

$ cat hello.zig
const std = @import("std");

pub fn main() anyerror!void {
    std.debug.print("Hello, world!", .{});
}
sagehane commented 1 year ago

Zig guarantees deterministic builds when the optimization mode is a release build rather than a debug build

Pretty sure this means that release builds are deterministic for the given target. It does not mean that release builds are portable by default.

figsoda commented 1 year ago

Can this be closed now since the introduction of zig.hook?

AndersonTorres commented 1 year ago

Well, given that it was solved even before this - the hook merely made the fixup more portable - I believe it can be finally closed as solved.