Linking hack for macOS Sierra #27536

Ericson2314 · 2017-07-21T05:40:44Z

Motivation for this change

Sierra imposes and arbitrary limit on mach-o header sizes, preventing too many libraries form being linkned. Following @copumpkin's suggestion, we recursively divide the libraries between children using -reexport-l until the header size is presumed small enough.

"Fixes" #22810

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.

mention-bot · 2017-07-21T05:40:46Z

@Ericson2314, thanks for your PR! By analyzing the history of the files in this pull request, we identified @pikajude, @edolstra, @vcunat, @LnL7 and @copumpkin to be potential reviewers.

Ericson2314 · 2017-07-21T05:43:15Z

I'm getting ld: file not found: /nix/store/vjbaqsdys43yay0ynzi00v82d9r8xyw4-clang-4.0.0/lib/libLTO.dylib for reasons I know not.

LnL7 · 2017-07-21T08:55:41Z

We don't have LTO support, not sure why you're running into that tho. See #19312

shlevy · 2017-07-21T12:58:34Z

@Ericson2314 The patch is not much shorter than the wrapper itself, any chance we can either factor out the common bits better or just have a second wrapper? 😕

domenkozar · 2017-07-21T13:01:58Z

Another approach would be patching Cabal (although I'm not sure how much would break): commercialhaskell/stack#3209 (comment)

EDIT: not to discourage this work (thank you John!), I'm still thinking about possible alternatives since it's mostly Haskell that's affected.

Ericson2314 · 2017-07-21T13:13:48Z

@shlevy Ah, second inner wrapper with mutual recursion makes a lot of sense—didn't think of that I'll switch.

domenkozar · 2017-07-21T13:27:21Z

@Ericson2314 so @copumpkin proposed to patch GHC linker and upstream the change, which I think makes more sense. This would fix the issue for non-Nix users which is clearly a more general solution (for Haskell, but not for non-Haskell in Nix).

shlevy · 2017-07-21T13:38:08Z

If this is working I'd rather not have to wait for the GHC linker change... We can always turn this off once we're sure the GHC fix is sufficient

domenkozar · 2017-07-21T13:41:20Z

Another potential solution is to use haskell/cabal#3979

shlevy · 2017-07-21T13:53:48Z

@domenkozar is that different from #25537 ?

domenkozar · 2017-07-21T13:54:21Z

@shlevy it's not, but then I don't understand how you're still seeing the problem with nixpkgs? :)

shlevy · 2017-07-21T14:03:03Z

@domenkozar Kept adding deps to our haskell packages and hit it again 😄

domenkozar · 2017-07-21T14:05:26Z

I'm now exploring the use of https://downloads.haskell.org/~ghc/8.0.2/docs/html/users_guide/phases.html#ghc-flag--staticlib

Ericson2314 · 2017-07-21T15:09:33Z

I think this is a good thing to have around Nixpkgs regardless. Our need is Haskell, but the problem is not Haskell-specific (that is, if other languages can get on our level of code reuse 😏).

shlevy · 2017-07-21T17:41:37Z

pkgs/build-support/cc-wrapper/default.nix

+    '' else ''
+      wrap ${prefix}ld ${./ld-wrapper.sh}
+      export ldWrapper=${prefix}ld
+      wrap ${prefix}ld-reexport-delegate ${./macos-sierra-reexport-hack.sh}


Looks like reexport-delegate isn't referenced anywhere but from this script itself, how does this get called? 😕

Yeah I had those messed up; fixed later.

Ericson2314 · 2017-07-21T18:19:14Z

~~Ok, back to the weird LTO error now.~~ Fixed. -lto_library was not being properly filtered out.

Ericson2314 · 2017-07-21T22:40:15Z

Now, I get linking failures without a message. Pushing a commit with debug stuff in case anyone else wants to look.

Ericson2314 · 2017-07-26T13:54:44Z

By the way, yesterday got back to __LINKEDIT problems.

Ericson2314 · 2017-07-31T21:01:57Z

CC @orivej --- you've done some great ld-wrapper work, including improving further on style things I pointed out with the old code, so pinging you lest I forget anything here.

Probably best to override Haskell packages set, or anything else linking a lot of libraries, with this.

As described in NixOS#18461, MacOS no longer accepts dylibs which only reexport other dylibs, because their symbol tables are empty. To get around this, we define an object file with a single "private extern" symbol, which hopefully won't clobber anything.

…ude path

Ericson2314 · 2017-07-31T21:05:37Z

pkgs/build-support/cc-wrapper/macos-sierra-reexport-hack.bash

+    fi
+
+    # first half of libs
+    @binPrefix@ld -macosx_version_min 10.10 -arch x86_64 -dylib \


@copumpkin @LnL7 / other Darwin people. I just cargo-culted -macosx_version_min 10.10 from elsewhere, and it's probably necessary. What should it be, if it is passed anything at all?

you can use $MACOSX_DEPLOYMENT_TARGET, that's defined in the stdenv.

Also add appropriate `meta.platforms = ...` to each derivation.

Ericson2314 · 2017-08-01T06:02:24Z

Anyone have any opinions on how I've organized things (especially since this last commit)?

orivej · 2017-08-01T12:01:16Z

pkgs/stdenv/darwin/default.nix

@@ -297,7 +297,7 @@ in rec {
    initialPath = import ../common-path.nix { inherit pkgs; };
    shell       = "${pkgs.bash}/bin/bash";

-    cc = import ../../build-support/cc-wrapper {
+    cc = lib.callPackageWith {} ../../build-support/cc-wrapper {


What does this do?

It adds the override field so I can replace cc while leaving everything else the same. Probably should add a note for that.

shlevy

Overall LGTM!

shlevy · 2017-08-01T12:08:18Z

pkgs/build-support/cc-wrapper/macos-sierra-reexport-hack.bash

+    case "${!n}" in
+        -l*) let overflowCount+=1 ;;
+        -reexport-l*) let overflowCount+=1 ;;
+        *) ;;


Should we handle -- here or elsewhere?

Oh I wasn't aware of it; looking up.

You mean just plain paths to dylibs passed in? I didn't see any -- in the man page.

I just mean the standard -- to separate command line flags from files that start with --, no idea if ld supports it but most tools do.

@shlevy Well I don't yet handle libraries passed in by path yet at all, in fact.

@shlevy Well I don't yet handle libraries passed in by path yet at all, in fact. I'll look into -- and that.

Eh, let's just do this as followup if needed.

shlevy · 2017-08-01T12:10:40Z

pkgs/test/macos-sierra-shared/default.nix

+    if bad-asdf
+    then echo "bad-asdf can succeed on non-sierra, OK" >&2
+    else echo "bad-asdf should fail on sierra, OK" >&2
+    fi


I don't love that we won't be able to catch regressions due to the exe just no longer breaking the limit for some reason

Neither do I. But I couldn't think of anything better without much more work.

orivej · 2017-08-01T15:08:29Z

pkgs/build-support/cc-wrapper/macos-sierra-reexport-hack.bash

+                ;;
+            -l)
+                echo "cctools LD does not support '-L foo' or '-l foo'" >&2
+                exit 1


How can a general purpose linker just fail to process these args? This is bound to break linking something.

Well, I suppose the man page could be wrong. I thought i tested too but let me double check.

Note that this isn't a failure to parse -Lpath or -llib, just -L path and -l lib. The latter is supported by the GNU toolchain nowadays but can't be expected to work everywhere (and, FWIW, from my understanding this is a limitation of the real linker program, not this script)

I didn't know that macOS ld does not support these args without spaces, then there is no problem. This is funny, since it has quite a few arguments that start with -l (-lazy-l* -lazy_library -lazy_framework -lto_library).

@orivej Yes I should special case the rest of those--I only did -lto_library.

Ericson2314 · 2017-08-03T21:29:09Z

Somebody just ping me if this needs more features

The workaround that this change deletes was initially contributed in NixOS#25537 to mitigate NixOS#22810 until a more permanent solution could be devised. That more permanent solution was eventually contributed in NixOS#27536, which now obviates the initial workaround, so this change removes it.

Ericson2314 requested review from copumpkin and shlevy July 21, 2017 05:40

Ericson2314 force-pushed the appease-sierra-linker branch from 80f8d67 to d1c0e3e Compare July 21, 2017 15:11

shlevy reviewed Jul 21, 2017

View reviewed changes

Ericson2314 force-pushed the appease-sierra-linker branch 2 times, most recently from f3e7d5d to 1286947 Compare July 21, 2017 18:15

Ericson2314 force-pushed the appease-sierra-linker branch 2 times, most recently from d590e3f to 8360abc Compare July 21, 2017 20:50

Ericson2314 force-pushed the appease-sierra-linker branch 2 times, most recently from 4713d0a to 40825d8 Compare July 25, 2017 19:45

Ericson2314 force-pushed the appease-sierra-linker branch 3 times, most recently from f0c4e68 to 5a15ea8 Compare July 27, 2017 19:39

Ericson2314 and others added 7 commits July 31, 2017 17:02

cc-wrapper: WIP linking hack for mac OS

0c37778

Probably best to override Haskell packages set, or anything else linking a lot of libraries, with this.

cc-wrapper: Use two scripts instead of patch

9242c8b

cc-wrapper: Beef up sierra tests to actually use functions

f671b85

cc-wrapper-test: Forgot to extern "C" now that main is C++

539dcb8

cc-wrapper-test: Don't use assert

a9dd855

cc-wrapper-test: Use $CXX else the C++ std lib won't be on the incl…

eeed181

…ude path

Ericson2314 commented Jul 31, 2017

View reviewed changes

Ericson2314 force-pushed the appease-sierra-linker branch 2 times, most recently from 9967a96 to 134b713 Compare August 1, 2017 01:20

macos-sierra-shared: Make live code and add to nascent Darwin channel

14e05c3

Also add appropriate `meta.platforms = ...` to each derivation.

Ericson2314 force-pushed the appease-sierra-linker branch from 134b713 to 14e05c3 Compare August 1, 2017 01:28

orivej reviewed Aug 1, 2017

View reviewed changes

shlevy approved these changes Aug 1, 2017

View reviewed changes

orivej reviewed Aug 1, 2017

View reviewed changes

Ericson2314 force-pushed the appease-sierra-linker branch from 14e05c3 to 85ce161 Compare August 1, 2017 20:32

Ericson2314 added 2 commits August 3, 2017 16:37

cc-wrapper: Handle more -l.... gotchas

2e61d8b

cc-wrapper: Don't hardcode MacOS min version for delegates

cd6c452

Ericson2314 force-pushed the appease-sierra-linker branch from b8d2065 to cd6c452 Compare August 3, 2017 20:45

Ericson2314 merged commit cd6c452 into NixOS:master Aug 3, 2017

Ericson2314 added a commit that referenced this pull request Aug 3, 2017

Merge PR #27536

8ba6012

Ericson2314 deleted the appease-sierra-linker branch August 3, 2017 21:28

shlevy mentioned this pull request Nov 11, 2017

macOS Sierra and Haskell "malformed mach-o: load commands size" #22810

Closed

bitemyapp mentioned this pull request Feb 27, 2018

GHC panic when building Stack on macOS Sierra commercialhaskell/stack#2577

Closed

avieth mentioned this pull request May 1, 2018

[CBR-51] use stack lts-11.2 (#2735) input-output-hk/cardano-sl#2745

Merged

Gabriella439 mentioned this pull request Dec 1, 2022

haskellPackages.mkDerivation: Remove Darwin dynamic libraries workaround #203963

Closed

1 task

Linking hack for macOS Sierra #27536

Linking hack for macOS Sierra #27536

Conversation

Ericson2314 commented Jul 21, 2017

Motivation for this change

Things done

mention-bot commented Jul 21, 2017

Ericson2314 commented Jul 21, 2017

LnL7 commented Jul 21, 2017

shlevy commented Jul 21, 2017

domenkozar commented Jul 21, 2017 • edited Loading

Ericson2314 commented Jul 21, 2017

domenkozar commented Jul 21, 2017 • edited Loading

shlevy commented Jul 21, 2017

domenkozar commented Jul 21, 2017

shlevy commented Jul 21, 2017

domenkozar commented Jul 21, 2017

shlevy commented Jul 21, 2017

domenkozar commented Jul 21, 2017

Ericson2314 commented Jul 21, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ericson2314 commented Jul 21, 2017 • edited Loading

Ericson2314 commented Jul 21, 2017

Ericson2314 commented Jul 26, 2017 • edited Loading

Ericson2314 commented Jul 31, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ericson2314 commented Aug 1, 2017

Choose a reason for hiding this comment

Ericson2314 Aug 1, 2017 • edited Loading

Choose a reason for hiding this comment

shlevy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ericson2314 Aug 1, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ericson2314 Aug 1, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ericson2314 commented Aug 3, 2017

domenkozar commented Jul 21, 2017 •

edited

Loading

domenkozar commented Jul 21, 2017 •

edited

Loading

Ericson2314 commented Jul 21, 2017 •

edited

Loading

Ericson2314 commented Jul 26, 2017 •

edited

Loading

Ericson2314 Aug 1, 2017 •

edited

Loading

Ericson2314 Aug 1, 2017 •

edited

Loading

Ericson2314 Aug 1, 2017 •

edited

Loading