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

haskell: add the linkWithGold function to link packages with ld.gold #24692

Merged
merged 1 commit into from
Apr 8, 2017

Conversation

basvandijk
Copy link
Member

@mention-bot
Copy link

@basvandijk, thanks for your PR! By analyzing the history of the files in this pull request, we identified @peti, @domenkozar and @pikajude to be potential reviewers.

@basvandijk
Copy link
Member Author

@peti it would be interesting turning on linkWithGold by default (for GHC >= 7.8) and let hydra build the package set and see how much will break. I would expect that we need to add a few --ghc-option=-optl-lm here and there.

@nh2
Copy link
Contributor

nh2 commented Apr 7, 2017

I would expect that we need to add a few --ghc-option=-optl-lm here and there.

@basvandijk I think those -lms should then be integrated into the upstream cabal packages, as that is the clean way to do it.

@basvandijk basvandijk force-pushed the haskell-link-with-gold branch from 5f4309a to 9ebb7cb Compare April 7, 2017 07:14
@basvandijk
Copy link
Member Author

basvandijk commented Apr 7, 2017

@nh2 agreed.

I forced-pushed a commit to this PR that adds a reference to a PR for double-conversion to explicitly link with libm.

@basvandijk
Copy link
Member Author

BTW I initially used the following implementation but abandoned it because the current implementation is simpler and doesn't require rebuilding all Haskell packages. But it might be interesting to compare the two...

Also support linking the double-conversion package with ld.gold.
@basvandijk basvandijk force-pushed the haskell-link-with-gold branch from 9ebb7cb to c865b8e Compare April 7, 2017 07:44
@basvandijk
Copy link
Member Author

@nh2 it would be great to also have your linkWithLLD available. Are you working on a PR for that BTW?

@nh2
Copy link
Contributor

nh2 commented Apr 7, 2017

it would be great to also have your linkWithLLD available. Are you working on a PR for that BTW?

@basvandijk do you mean to add this example as a nixpkgs package?

@basvandijk
Copy link
Member Author

@nh2 no, I mean I would like to have a function like my linkWithGold but then for linking with lld. It could be called linkWithLLD and work something like:

linkWithLLD = drv : appendConfigureFlag drv
    "--with-gcc=clang --with-ld=clang";

What is probably missing is that clang should be brought in scope.

@nh2
Copy link
Contributor

nh2 commented Apr 7, 2017

@basvandijk Yes; and pkgs.lld would also be needed, and flags to build to pass ld-option, cc-option, ghc-option etc.

I'm not currently working on that, so if you'd like to do it please go ahead, it would be awesome!

Also, if you come across a problem involving linking functions from libstc++, ping me.

@basvandijk
Copy link
Member Author

Ok this sounds like a nice weekend project. I'll see if I have some time for it. Thanks for the info!

@ljli
Copy link
Contributor

ljli commented Apr 7, 2017

Great! Any thoughts on making it the default where possible? We pass --split-sections by default for GHC > 8.0.1, which apparently requires the gold linker to take effect and thus I assume doesn't do anything currently. Wrong info, still desirable though to reduce compile time.

@nh2
Copy link
Contributor

nh2 commented Apr 8, 2017

@basvandijk I found that Haskell executables built with lld in nix that also link -l system shared libraries don't link correctly, with entries in ldd in resulting executbles being not found (rpaths are missing). That's because ld and gold have wrappers that do the following:

# Second, for each directory in the library search path (-L...),

 # First, find all -L... switches.

    # Second, for each directory in the library search path (-L...),
    # see if it contains a dynamic library used by a -l... flag.  If
    # so, add the directory to the rpath.

lld currently doesn't have such a wrapper.

@nh2
Copy link
Contributor

nh2 commented Apr 8, 2017

Also this type of patch or setting to --enable-new-dtags will likely be needed.

@aristidb aristidb merged commit e893646 into NixOS:master Apr 8, 2017
@basvandijk
Copy link
Member Author

@aristidb thanks for the merge. Could this also be cherry-picked on release-17.03? I would like to use this in production.

@nh2
Copy link
Contributor

nh2 commented Apr 8, 2017

I've created a separate issue #24744 for the problem that lld doesn't work correctly with nix.

@domenkozar
Copy link
Member

domenkozar commented Jun 6, 2017

I wonder if linkWithGold should also pass --ghc-option=-optl-lm to the package it's compiling. Maybe also --ghc-option=-optl-pthread.

@nh2
Copy link
Contributor

nh2 commented Jun 6, 2017

I wonder if linkWithGold should also pass --ghc-option=-optl-lm to the package it's compiling. Maybe also --ghc-option=-optl-pthread.

@domenkozar Personally I think it shouldn't, unless it's a temporary hack. It should be fixed upstream, as ld happily accepts these flags and that's better than implicitly linking those anyway.

@domenkozar
Copy link
Member

Every package that uses Prelude.sqrt would need that flag added to cabal file. That sounds unfeasible to me :)

@nh2
Copy link
Contributor

nh2 commented Jun 6, 2017

Every package that uses Prelude.sqrt would need that flag added to cabal file.

@domenkozar With "upstream" I mean the package that declares the function, not the one that uses it (that would be infeasible, users shouldn't have to rely on the implementation detail with what C function a Haskell function is implemented).

So in that case I think base should have that extra-libraries: m.

Do you think this will make any problem?

@domenkozar
Copy link
Member

If we can get base fixed, I'm all for it.

@domenkozar
Copy link
Member

There's a bug when you depend on a library that is linked with gold: https://ghc.haskell.org/trac/ghc/ticket/13810#comment:3

@nh2
Copy link
Contributor

nh2 commented Jul 20, 2017

I would expect that we need to add a few --ghc-option=-optl-lm here and there.

@basvandijk I think those -lms should then be integrated into the upstream cabal packages, as that is the clean way to do it.

I have now confirmed that --ghc-option=-optl-lm should not be used.

It wil pass it also to those linker invocations that create the individual Haskell module .o files, which will then fail with e.g.

/nix/store/iz98xp5p48jayn034wq89y30plpx5xns-binutils-2.27/bin/ld.gold: fatal error: cannot mix -r with dynamic object /nix/store/7crrmih8c52r8fbnqb933dxrsp44md93-glibc-2.25/lib/libm.so.6

in some cases.

Putting extra-libraries: m into the executable section of the .cabal file is the only working solution (or more precisely, any library that uses functions from libm, but note that is hard to determine because nothing in the toolchain will complain when you forget it on a library that uses function from libm, so you'll only get missing symbols at the link of the final executable).

nh2 added a commit to input-output-hk/cardano-sl that referenced this pull request Jul 21, 2017
…stead.

Fixes

  ld.gold: fatal error: cannot mix -r with dynamic object

error for dynamic linking.

See NixOS/nixpkgs#24692 (comment)
nh2 added a commit to input-output-hk/cardano-sl that referenced this pull request Jul 27, 2017
…raries: m` instead.

Fixes

  ld.gold: fatal error: cannot mix -r with dynamic object

error for dynamic linking (also in profiling).

See NixOS/nixpkgs#24692 (comment)

Cherry-Picked-From: 8f81fb0
Cherry-Picked-By: Niklas Hambüchen <[email protected]>
@domenkozar
Copy link
Member

Anyone remembers why we didn't enable gold by default?

@nh2
Copy link
Contributor

nh2 commented Apr 10, 2019

@domenkozar No, I don't know. I know why we aren't using lld as default: #24744

@adamse
Copy link
Contributor

adamse commented Feb 19, 2020

@domenkozar, @nh2: I think #80466 is the missing piece for gold as the default linker.

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

Successfully merging this pull request may close these issues.

7 participants