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

makeBinaryWrapper fixes for Darwin #150079

Merged
merged 5 commits into from
Dec 17, 2021

Conversation

bergkvist
Copy link
Member

@bergkvist bergkvist commented Dec 10, 2021

Motivation for this change

See discussion on #124556

TODO:

  • Switch from stdenv.cc.cc -> stdenv.cc
  • Fix chdir test breaking on darwin due to /tmp being a symlink to /private/tmp
  • Make sure OfBorg makeBinaryWrapper tests are not skipped
  • Use address sanitizer in tests only due to performance impact and compilation problems
  • Make sure tests work on aarch64-darwin and macOS 12 (Monterey)
Things done
  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 22.05 Release Notes (or backporting 21.11 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
    • (Release notes changes) Ran nixos/doc/manual/md-to-db.sh to update generated release notes
  • Fits CONTRIBUTING.md.

Future work/Next PRs for makeBinaryWrapper

  • Add (sound) static analysis of generated C code
  • Print usage notes when makeWrapper is called with 0 or 1 argument(s), since it requires at least 2 arguments.
  • Add substitute for --run COMMAND from makeWrapper
  • Start using binary wrapper for interpreters - so that we no longer need to special for darwin case in makeScriptWriter ( writers.makeScriptWriter: fix on Darwin\MacOS #93757)

@ofborg ofborg bot added the 6.topic: darwin Running or building packages on Darwin label Dec 10, 2021
@domenkozar
Copy link
Member

@roberth

@ofborg ofborg bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10 labels Dec 10, 2021
@tfc
Copy link
Contributor

tfc commented Dec 10, 2021

I have been trying to find more complex derivations that depend on makewrapper working where i substitute the normal wrapper with the binary wrapper.

While doing that i discovered that polkit does not build, with the following error message:

==1657==ASan runtime does not come first in initial library list; you should either link runtime to your application or manually preload it with LD_PRELOAD.

that, together with the performance discussion convinces me towards dropping (at least the address) sanitizers.

@bergkvist
Copy link
Member Author

bergkvist commented Dec 10, 2021

Okay, it seems like makeBinaryWrapper tests are now working on x86_64-linux, aarch64-linux and x86_64-darwin - and tests are running in OfBorg. For some reason the address sanitizer doesn't seem to cause build issues in the test, only when executed in a nix-shell.

$ env NIX_PATH=nixpkgs=$PWD nix-shell -p makeBinaryWrapper --run "makeWrapper /usr/local/bin/python3 a"
In file included from <built-in>:362:
<command line>:1:9: error: '_FORTIFY_SOURCE' macro redefined [-Werror,-Wmacro-redefined]
#define _FORTIFY_SOURCE 2
        ^
<built-in>:350:9: note: previous definition is here
#define _FORTIFY_SOURCE 0
        ^
1 error generated.

Regardless, I have now disabled the address sanitizer by default - and only enabled it in the tests. (undefined sanitizer is still enabled by default)

aarch64-darwin seems to fail due to something with CODESIGN_ALLOCATE. Since I don't have an M1-macbook it is difficult for me to debug what has happened here. Any ideas?

@bergkvist
Copy link
Member Author

bergkvist commented Dec 10, 2021

Another problem I just detected:

When setting sandbox = true on macOS Big Sur (x86), an trying to run the tests, I'm getting:

$ nix-build -A makeBinaryWrapper.passthru.tests
these derivations will be built:
  /nix/store/ypa249hapa52hclxgzn2hm587cbmqcbc-make-binary-wrapper.sh.drv
  /nix/store/7gr9icmimx2rq5kprzzg7vnp8jxv43wc-hook.drv
  /nix/store/amhyywm2pb9kd82hpflfm352qsprim5d-envcheck.drv
  /nix/store/0f7laklp2k624a58y4dy15b4gzp8ak4r-test-wrapper_suffix.drv
  /nix/store/2557sm5fipgcvs6w6ldlrwf5jdz05506-test-wrapper_env.drv
  /nix/store/fmy4z0rw8j6sl1ivxnix2cyf7njjikc4-test-wrapper_inherit-argv0.drv
  /nix/store/jdyvcgkr6agjys9sw7cjq8byglpyln53-test-wrapper_invalid-env.drv
  /nix/store/ky3a5d7j078g8g4aas7c60jkhcxdc798-test-wrapper_add-flags.drv
  /nix/store/mv4d4vrm36gb1grmvrnfjs2c50l2i8fb-test-wrapper_argv0.drv
  /nix/store/qhz3i0dwgdjaj7lyn243s0nly02xd76s-test-wrapper_basic.drv
  /nix/store/r95rh6safj8ph5bh7mmnqdf09ivfy3c2-test-wrapper_chdir.drv
  /nix/store/xy7wphhy2gp6f80llqkl9xxlwj957aa7-test-wrapper_combination.drv
  /nix/store/zjlg7i2fi1kz6g9ch3dhbyyypndivr2g-test-wrapper_prefix.drv
  /nix/store/5q526ykiwqsngw0qgydhyx8kl7jhd335-make-binary-wrapper-test.drv
error: while setting up the build environment: getting attributes of path '/usr/lib/libSystem.B.dylib': No such file or directory

Seems like something is expecting libSystem.B.dylib to exist in the file system - but all system shared libraries were removed from the file system in the macOS Big Sur upgrade.

To check whether a system library exists in macOS Big Sur it is possible to use:

// From #include <mach-io/dyld>
static bool _dyld_shared_cache_contains_path(const char *path)

@bergkvist
Copy link
Member Author

bergkvist commented Dec 14, 2021

Trying to use sanitizers in tests on aarch64-darwin I would get:

# undefined behaviour sanitizer:
ld: file not found: /nix/store/bp55vzlmcyqcx9n08pkzslvks10zybwc-clang-wrapper-11.1.0/resource-root/lib/darwin/libclang_rt.ubsan_osx_dynamic.dylib

# address sanitizer:
ld: file not found: /nix/store/bp55vzlmcyqcx9n08pkzslvks10zybwc-clang-wrapper-11.1.0/resource-root/lib/darwin/libclang_rt.asan_osx_dynamic.dylib

# As we can see, they sanitizer libraries does not seem to exist:
$ ls /nix/store/bp55vzlmcyqcx9n08pkzslvks10zybwc-clang-wrapper-11.1.0/resource-root/lib/darwin/
libclang_rt.cc_kext.a  libclang_rt.osx.a

So I decided to disable sanitizers in aarch64-darwin tests.

The strange codesigner-errors disappeared once I added cc to the list of deps for makeBinaryWrapper, although then a new error occured:

$ nix-build -A makeBinaryWrapper.passthru.tests
these 12 derivations will be built:
  /nix/store/c878h9lm9pp97zz67149q87l947dpyna-envcheck.drv
  /nix/store/803j7571q1xz5dxmsrq9px988ig69ck5-test-wrapper_inherit-argv0.drv
  /nix/store/8x3yfs11shxrfbzwcd7zr3sbxcddxi6h-test-wrapper_env.drv
  /nix/store/dc95gz3a0dfgwfrxwg9iswhmf1hrr1bj-test-wrapper_basic.drv
  /nix/store/inlb4hrw8z7g3zh4y3qpkc0vhsa5q4wv-test-wrapper_invalid-env.drv
  /nix/store/ka9jrbh2zbsxkpp8mqv6mz2mjgr86675-test-wrapper_add-flags.drv
  /nix/store/n72dam3rxisk54fskjr3b81jvxn45vf6-test-wrapper_chdir.drv
  /nix/store/ppg4728yk0wjcmirqxra9ls8xmjq4pj5-test-wrapper_argv0.drv
  /nix/store/qayllqssv41m7ykfx4dhyfqcnkp1swgp-test-wrapper_suffix.drv
  /nix/store/rw661pcarbcznk6m3xfqnx2pcs9pkpf1-test-wrapper_prefix.drv
  /nix/store/xys5hq6xjm984fwzxm56c3zam6ij0g7d-test-wrapper_combination.drv
  /nix/store/6f2x1i7c6v38gmc6l7xbf36b04w1xzbf-make-binary-wrapper-test.drv
building '/nix/store/c878h9lm9pp97zz67149q87l947dpyna-envcheck.drv'...
/private/tmp/nix-build-envcheck.drv-0/ccXY1W9e.s:22:15: error: index must be an integer in range [-256, 255].
        ldr     x3, [x3, ___stack_chk_guard];momd
                         ^
/private/tmp/nix-build-envcheck.drv-0/ccXY1W9e.s:65:15: error: index must be an integer in range [-256, 255].
        ldr     x1, [x1, ___stack_chk_guard];momd
                         ^

error: builder for '/nix/store/c878h9lm9pp97zz67149q87l947dpyna-envcheck.drv' failed with exit code 1;
       last 7 log lines:
       > /private/tmp/nix-build-envcheck.drv-0/ccXY1W9e.s:22:15: error: index must be an integer in range [-256, 255].
       >         ldr     x3, [x3, ___stack_chk_guard];momd
       >                          ^
       > /private/tmp/nix-build-envcheck.drv-0/ccXY1W9e.s:65:15: error: index must be an integer in range [-256, 255].
       >         ldr     x1, [x1, ___stack_chk_guard];momd
       >                          ^
       >
       For full logs, run 'nix log /nix/store/c878h9lm9pp97zz67149q87l947dpyna-envcheck.drv'.
error: 1 dependencies of derivation '/nix/store/ka9jrbh2zbsxkpp8mqv6mz2mjgr86675-test-wrapper_add-flags.drv' failed to build
error: 1 dependencies of derivation '/nix/store/6f2x1i7c6v38gmc6l7xbf36b04w1xzbf-make-binary-wrapper-test.drv' failed to build

I don't really have a good explanation for these error messages.

For some reason, changing from nativeBuildInputs = [ makeBinaryWrapper ] to buildInputs = [ makeBinaryWrapper ] in the tests made this error go away.

But at least the errors are gone now, and the tests seem to succeed on aarch64-darwin, x86_64-darwin, aarch64-linux, x86_64-linux.

Sanitizers are now only enabled in tests (except for aarch64-darwin) by default - meaning you can expect a ~10x speedup of the wrappers compared to the bash wrappers.

Copy link
Member

@ncfavier ncfavier left a comment

Choose a reason for hiding this comment

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

Diff looks good to me

Copy link
Contributor

@andrew-d andrew-d left a comment

Choose a reason for hiding this comment

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

👍 for the sanitizers change

Copy link
Contributor

@doronbehar doronbehar left a comment

Choose a reason for hiding this comment

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

I have only comments regarding the commit history. All santizers related changes could be squashed into 1 commit. And please follow the commit messages guidelines:

https://github.com/NixOS/nixpkgs/blob/master/CONTRIBUTING.md

Ideally this should have been enforced in the original PR.

"${stdenv.cc.cc}/bin" does not contain a cc-symlink, since this is the unwrapped version of the compiler - however "${stdenv.cc}/bin" does.
Move makeBinaryWrapper tests to pkgs.makeBinaryWrapper.passthru.tests, since OfBorg doesn't discover/skips the tests in the previous location.
On macOS, /tmp is a symlink to /private/tmp. When performing cd /tmp, and checking cwd - it won't match since it follows the symlink.

This caused test breakage on macOS but not Linux. Instead, use a folder which is not a symlink, and consistent across Linux and macOS.
Although sanitizers can catch and prevent undefined behaviour during runtime, it has a significant impact on performance. They also cause issues on macOS where they can make compilation fail. The future goal is to instead utilize static analysis to prevent undefined behaviour as makeBinaryWrapper evolves.
Sanitizers don't seem to be present on aarch64-darwin/macOS 12 (Monterey), so they are removed from the aarch64-darwin tests.

Switching from nativeBuildInputs to buildInputs and adding cc to the deps list caused some strange error messages to go away.
@bergkvist
Copy link
Member Author

@doronbehar Can you verify that the commit messages/commits look correctly formatted now? I also combined the sanitizer removal into a single commit.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
6.topic: darwin Running or building packages on Darwin 8.has: clean-up 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10 10.rebuild-linux: 1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants