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

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

Open
K900 opened this issue Aug 8, 2022 · 13 comments
Open

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

K900 opened this issue Aug 8, 2022 · 13 comments

Comments

@K900
Copy link
Contributor

K900 commented Aug 8, 2022

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
Copy link
Member

Artturin commented Aug 18, 2022

create a issue upstream?

EDIT: someone created one 2 days ago ziglang/zig#12458

@ajs124
Copy link
Member

ajs124 commented Aug 18, 2022

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

@AndersonTorres
Copy link
Member

Where is this flag implemented?

@tavi-vi
Copy link
Contributor

tavi-vi commented Sep 8, 2022

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
Copy link
Member

ajs124 commented Sep 8, 2022

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
Copy link
Contributor

Indeed, this would help to unbreak ncdu for instance !

@sagehane
Copy link
Contributor

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 ];
}

jordanisaacs added a commit to jordanisaacs/nixpkgs that referenced this issue Jan 9, 2023
Also adds `-Dcpu=baseline` to fix: NixOS#185665
@strager
Copy link
Contributor

strager commented Feb 4, 2023

Proposed fix: #214440

@strager strager self-assigned this Feb 4, 2023
@andrewrk
Copy link
Member

andrewrk commented Feb 4, 2023

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
Copy link
Contributor

strager commented Feb 4, 2023

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
Copy link
Contributor

sagehane commented Feb 4, 2023

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.

winterqt added a commit to winterqt/nixpkgs that referenced this issue Feb 17, 2023
26b9a2f changes Zig 0.10 to build the
compiler (notably *not* its outputs, at least not by default) with
its baseline CPU target, but we should ideally do it for both versions
to increase reproducibility, as well as increase the number of users who
are able to use Hydra-provided Zig binaries.

This also adds a comment above the flag in 0.10, to explain why we're adding
the flag, as we do with the RPATH one.

See NixOS#214356 and NixOS#185665
for further context.
davidak pushed a commit to hesiod/nixpkgs that referenced this issue Feb 18, 2023
26b9a2f changes Zig 0.10 to build the
compiler (notably *not* its outputs, at least not by default) with
its baseline CPU target, but we should ideally do it for both versions
to increase reproducibility, as well as increase the number of users who
are able to use Hydra-provided Zig binaries.

This also adds a comment above the flag in 0.10, to explain why we're adding
the flag, as we do with the RPATH one.

See NixOS#214356 and NixOS#185665
for further context.
@figsoda
Copy link
Member

figsoda commented Aug 14, 2023

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

@AndersonTorres
Copy link
Member

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.

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

No branches or pull requests