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

valgrind: Fix darwin build #29254

Merged
merged 1 commit into from Sep 17, 2017
Merged

valgrind: Fix darwin build #29254

merged 1 commit into from Sep 17, 2017

Conversation

knedlsepp
Copy link
Member

Motivation for this change

Fix the darwin build of valgrind and valgrind-light as part of #29221

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option build-use-sandbox in nix.conf on non-NixOS)
  • Built on platform(s)
    • NixOS
    • macOS
    • Linux
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nox --run "nox-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Fits CONTRIBUTING.md.

@LnL7 LnL7 added 6.topic: darwin Running or building packages on Darwin 9.needs: port to stable A PR needs a backport to the stable release. labels Sep 12, 2017
@LnL7
Copy link
Member

LnL7 commented Sep 12, 2017

I'm still seeing an issue with the build

clang -DHAVE_CONFIG_H -I. -I..  -I.. -I../include -I../include -I../VEX/pub -I../VEX/pub -DVGA_amd64=1 -DVGO_darwin=1 -DVGP_amd64_darwin=1 -DVGPV_amd64_darwin_vanilla=1 -I../coregrind -DVG_LIBDIR="\"/nix/store/kmrrafjalnj4mbc4b0ykxmbppp6f84cv-valgrind-3.13.0/lib/valgrind"\" -DVG_PLATFORM="\"amd64-darwin\""    -O2 -g -std=gnu99 -Wall -Wmissing-prototypes -Wshadow -Wpointer-arith -Wstrict-prototypes -Wmissing-declarations -Wcast-align -Wcast-qual -Wwrite-strings -Wempty-body -Wformat -Wformat-security -Wignored-qualifiers -fno-stack-protector -fno-strict-aliasing -fno-builtin -Wno-cast-align -Wno-self-assign -Wno-tautological-compare -mmacosx-version-min=10.6 -fno-stack-protector   -c -o m_syswrap/libcoregrind_amd64_darwin_a-syswrap-linux.o `test -f 'm_syswrap/syswrap-linux.c' || echo './'`m_syswrap/syswrap-linux.c
clang -DHAVE_CONFIG_H -I. -I..  -I.. -I../include -I../include -I../VEX/pub -I../VEX/pub -DVGA_amd64=1 -DVGO_darwin=1 -DVGP_amd64_darwin=1 -DVGPV_amd64_darwin_vanilla=1 -I../coregrind -DVG_LIBDIR="\"/nix/store/kmrrafjalnj4mbc4b0ykxmbppp6f84cv-valgrind-3.13.0/lib/valgrind"\" -DVG_PLATFORM="\"amd64-darwin\""    -O2 -g -std=gnu99 -Wall -Wmissing-prototypes -Wshadow -Wpointer-arith -Wstrict-prototypes -Wmissing-declarations -Wcast-align -Wcast-qual -Wwrite-strings -Wempty-body -Wformat -Wformat-security -Wignored-qualifiers -fno-stack-protector -fno-strict-aliasing -fno-builtin -Wno-cast-align -Wno-self-assign -Wno-tautological-compare -mmacosx-version-min=10.6 -fno-stack-protector   -c -o m_syswrap/libcoregrind_amd64_darwin_a-syswrap-linux-variants.o `test -f 'm_syswrap/syswrap-linux-variants.c' || echo './'`m_syswrap/syswrap-linux-variants.c
clang -DHAVE_CONFIG_H -I. -I..  -I.. -I../include -I../include -I../VEX/pub -I../VEX/pub -DVGA_amd64=1 -DVGO_darwin=1 -DVGP_amd64_darwin=1 -DVGPV_amd64_darwin_vanilla=1 -I../coregrind -DVG_LIBDIR="\"/nix/store/kmrrafjalnj4mbc4b0ykxmbppp6f84cv-valgrind-3.13.0/lib/valgrind"\" -DVG_PLATFORM="\"amd64-darwin\""    -O2 -g -std=gnu99 -Wall -Wmissing-prototypes -Wshadow -Wpointer-arith -Wstrict-prototypes -Wmissing-declarations -Wcast-align -Wcast-qual -Wwrite-strings -Wempty-body -Wformat -Wformat-security -Wignored-qualifiers -fno-stack-protector -fno-strict-aliasing -fno-builtin -Wno-cast-align -Wno-self-assign -Wno-tautological-compare -mmacosx-version-min=10.6 -fno-stack-protector   -c -o m_syswrap/libcoregrind_amd64_darwin_a-syswrap-darwin.o `test -f 'm_syswrap/syswrap-darwin.c' || echo './'`m_syswrap/syswrap-darwin.c
m_syswrap/syswrap-darwin.c:9869:8: error: unknown type name 'dyld_kernel_image_info_t'
       dyld_kernel_image_info_t dyld_cache_image;
       ^
1 error generated.
make[3]: *** [Makefile:5666: m_syswrap/libcoregrind_amd64_darwin_a-syswrap-darwin.o] Error 1
make[3]: Leaving directory '/private/tmp/nix-build-valgrind-3.13.0.drv-0/valgrind-3.13.0/coregrind'
make[2]: *** [Makefile:1827: all] Error 2
make[2]: Leaving directory '/private/tmp/nix-build-valgrind-3.13.0.drv-0/valgrind-3.13.0/coregrind'
make[1]: *** [Makefile:829: all-recursive] Error 1
make[1]: Leaving directory '/private/tmp/nix-build-valgrind-3.13.0.drv-0/valgrind-3.13.0'
make: *** [Makefile:698: all] Error 2
builder for ‘/nix/store/c0x9gb1bknxnfhwl1b9f6b4sqgvvk2dr-valgrind-3.13.0.drv’ failed with exit code 2

@knedlsepp
Copy link
Member Author

Hm. Ok. I'm on 10.11.6 and I've got Xcode Command line tools installed. (It seems that sometimes this is a problem) I've heard that sandboxed builds would help me getting more reproducible builds. How can I configure my nix, so I'll get the same build errors?

@periklis
Copy link
Contributor

Is this bzero-patch not needed anymore?

@knedlsepp
Copy link
Member Author

As far as I can tell the patch was merged in: https://sourceware.org/git/?p=valgrind.git;a=commitdiff;h=f6d7adf7e983dea468c8cf337630acce902934e3

@knedlsepp
Copy link
Member Author

knedlsepp commented Sep 15, 2017

@LnL7: I suppose the reason for the build error is the following:
Your build determines you are on macOS 10.12. This makes valgrind use the dyld_kernel_image_info_t type of xnu that was only recently introduced (It seems xnu-3789.1.32?), but we don't ship a xnu in nixpkgs that recent.
So assuming this analysis is correct, I guess this narrows our options down to:

  • Make the valgrind build think the macOS version is 10.11 regardless the actual system
  • Add a more recent xnu for 10.12. (Which I'm pretty sure you could teach me considering your contributions )

What would you suggest?

@knedlsepp
Copy link
Member Author

Ok. I've tried to wrap my head around the apple-source-releases.
I think I understand which changes would be necessary to this file to get a more current xnu built. As I'm however a little confused about this file, I'd hope to get some clarifications first.
Initially I thought that as there are multiple os versions listed in the versions attribute that we probably build different versions of all those attributes depending on the host OS.
But now that I've read through it, I have the feeling even though this is split up into all those osx-10.11.6, osx-10.9.5, ... that this os-x versions is actually only ever used in the generation of the store path, but the build is the same no matter the host version.
What is the rationale behind using:
/nix/store/wyyddffl27q7kspvfl4d4h9yagp2smyi-xnu-osx-10.11.6 instead of /nix/store/wyyddffl27q7kspvfl4d4h9yagp2smyi-xnu-3248.60.10?
It seems to make this file more complicated than it actually needs to be. Can't we simply use consistent versions listed on one of those release pages?

The bzero-patch was merged upstream in
git-svn-id: svn://svn.valgrind.org/valgrind/trunk@16103, so it does no
longer apply.

Additionally - to make the build succeed on darwin systems more recent
than our nixpkgs.darwin.xnu kernel version - we need to teach the build
the version of the xnu headers we provide, instead of letting the build
figure out the actual system version using `uname -r`.
@knedlsepp
Copy link
Member Author

@LnL7: Since I've learned that upgrading the xnu kernel might break support for macOS < 10.12, I've instead opted for teaching the valgrind build how to figure out which version of xnu we use in nixpkgs. Build seems to work fine on 10.11.6 at least.

preConfigure = stdenv.lib.optionalString stdenv.isDarwin (
let OSRELEASE = ''
$(awk -F '"' '/#define OSRELEASE/{ print $2 }' \
<${xnu}/Library/Frameworks/Kernel.framework/Headers/libkern/version.h)'';
Copy link
Member

Choose a reason for hiding this comment

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

we should add something for this to the xnu drv

Copy link
Member Author

Choose a reason for hiding this comment

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

I've been thinking about that, since we actually already encode the macOS version. Sadly those don't easily transpose to the kernel version. Additionally there's only valgrind, IOKit, libpthread and network_cmds that depend directly on xnu(as far as I can tell.), so it might not be worth the effort.

Copy link
Member

@LnL7 LnL7 Sep 15, 2017

Choose a reason for hiding this comment

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

Yeah, first I was thinking about an attribute on xnu, but that could easily get out of sync. Adding a it to nix-support is the nicest alternative I could come up with, but that would be a mass-rebuild.

Copy link
Member Author

Choose a reason for hiding this comment

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

This also seems like a fine solution.
Just to get an even broader view on the issue I'd like to mention that there also was an idea to replace uname itself: #25240

@LnL7
Copy link
Member

LnL7 commented Sep 15, 2017

@knedlsepp Indeed, using the same versions of the sources is important for reproducibility and compatibility. We still support 10.10, even tho all of the hydra.org macs are running a more recent version. That's why we define MACOSX_DEPLOYMENT_TARGET=10.10 in the stdenv.

Getting rid of the impurity that looks at the kernel version of the build host is definitely the preferred solution. But we could bump the version of xnu if that doesn't break things for older versions of OSX.

@LnL7 LnL7 merged commit 036036a into NixOS:master Sep 17, 2017
@LnL7 LnL7 added the 8.has: port to stable A PR already has a backport to the stable release. label Sep 17, 2017
@knedlsepp knedlsepp deleted the fix-valgrind branch September 17, 2017 18:24
@veprbl
Copy link
Member

veprbl commented Sep 19, 2017

Thank you for your work!

Does this work on Sierra? On my system I get (say running valgrind ls):

valgrind: mmap-FIXED(0x0, 253952) failed in UME (load_segment1) with error 12 (Cannot allocate memory).

I'm running 10.12.6 with no CLT or XCode. I've also tried building valgrind from current master (b9df4c8dec4d) instead of version 3.13.0. That didn't help.

@knedlsepp
Copy link
Member Author

Hm. While this is unfortunate, I suppose it's quite likely that my patch wasn't that helpful after all. Valgrind probably has good reasons to do things differently for 10.12. I guess instead I should have tried bumping the xnu version as this is something I could actually test on my 10.11 system

@veprbl
Copy link
Member

veprbl commented Sep 19, 2017

I've tried bumping xnu and got stuck with build error in coreutils (built as a stdenv dependency):

Undefined symbols for architecture x86_64:
  "_renameatx_np", referenced from:
      _renameat2 in libcoreutils.a(renameat2.o)
ld: symbol(s) not found for architecture x86_64

To fix that I've tried to bump Libsystem and Libc, but that didn't help.

Other possibility would be to supply bumped xnu only for valgrind. Not sure if that can work.

@veprbl veprbl removed the 9.needs: port to stable A PR needs a backport to the stable release. label Mar 14, 2019
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: port to stable A PR already has a backport to the stable release.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants