Skip to content

ghc: check for targetPlatform.linker to determine if gold is available#126205

Merged
Ericson2314 merged 1 commit intoNixOS:haskell-updatesfrom
sternenseemann:ghc-linker-checks
Jun 8, 2021
Merged

ghc: check for targetPlatform.linker to determine if gold is available#126205
Ericson2314 merged 1 commit intoNixOS:haskell-updatesfrom
sternenseemann:ghc-linker-checks

Conversation

@sternenseemann
Copy link
Copy Markdown
Member

useLdGold previously just checked for useLLVM which (currently) implies
linker == "lld". However more accurate is to check the linker of the
targetPlatform as it actually tells us which bintools package we can
expect.

I'm not handling linker == "gold" so far. This doesn't seem to happen
in any case so far, so it is hard to tell what it would imply. Maybe
${bintools}/bin/ld would be gold in that case, so it'd be wrong to
pass -fuse-ld=gold?

Motivation for this change
Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • 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 nixpkgs-review --run "nixpkgs-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • 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
  • Fits CONTRIBUTING.md.

@github-actions github-actions bot added the 6.topic: haskell General-purpose, statically typed, purely functional programming language label Jun 8, 2021
@ofborg ofborg bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin. 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux. labels Jun 8, 2021
Copy link
Copy Markdown
Member

@cdepillabout cdepillabout Jun 8, 2021

Choose a reason for hiding this comment

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

What does targetPlatform.linker == "bfd" mean?

(Also, why does this mean that gold should be used?)


One other question I had: what made you want to change/fix this? Were you seeing a problem that targetPlatform.useLLVM wasn't catching?

Ideally I guess there would be a comment here explaining the reasoning for the check. At the very least, I don't know what is going on here (although I did sort of understand the !useLLVM check).

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

ld.bfd is the classic GNU ld that uses the bfd library which was (at least historically) behind most of binutils and gdb. ld.gold is the alternative one that is a bit odd that it is even part of the binutils project at all. Had it been started a few years later, I assume it would have been an LLVM instead (and been more like LLD).

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Interestingly, if we moved it to a separate package too, it would probably be easier to implement linker == "gold".

Copy link
Copy Markdown
Member Author

@sternenseemann sternenseemann Jun 8, 2021

Choose a reason for hiding this comment

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

One other question I had: what made you want to change/fix this? Were you seeing a problem that targetPlatform.useLLVM wasn't catching?

useLLVM implies linker == "lld", but we recently made it (theoretically) possible to have something like useLLVM without linker == "lld" (which was when linker was introduced in the first place). Thus checking linker is more accurate and future proof.

bfd implies the binutils package is used for bintools which contains ld.gold at least in nixpkgs.

@Ericson2314
Copy link
Copy Markdown
Member

linker == "gold" would be gold is the default linker everywhere, which hasn't been set up yet. I think you want to change the conditions so it's that or what we we've got, so "use gold if gold is the default, or bfd is the default and these other conditions are met".

@sternenseemann
Copy link
Copy Markdown
Member Author

linker == "gold" would be gold is the default linker everywhere, which hasn't been set up yet. I think you want to change the conditions so it's that or what we we've got, so "use gold if gold is the default, or bfd is the default and these other conditions are met".

Not sure if I quite understood you correctly, is 7babbf76889 what you had in mind?

@Ericson2314
Copy link
Copy Markdown
Member

@sternenseemann Yes that is what i was thinking, and i added some suggestions for a few other nits.

useLdGold previously just checked for useLLVM which (currently) implies
`linker == "lld"`. However more accurate is to check the `linker` of the
`targetPlatform` as it actually tells us which bintools package we can
expect.

`linker == "bfd"` implies that we are using the `binutils` package, so
gold is available, so we can use it unless musl is the libc. `linker ==
"gold"` implies that gold is the default linker already and we should
absolutely use it.
@Ericson2314 Ericson2314 merged commit 4f97d78 into NixOS:haskell-updates Jun 8, 2021
@sternenseemann sternenseemann deleted the ghc-linker-checks branch June 8, 2021 21:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

6.topic: haskell General-purpose, statically typed, purely functional programming language 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin. 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants