Do not hardcode the specific linker to use
ClosedPublic

Authored by angerman on Mar 15 2017, 8:56 PM.

Details

Summary

This should be handled appropriately by a wrapper script around the compiler,
if one wants to insist on the specific linker to be used. Otherwise this
breaks if the used compiler fails to understand this directive.

I believe that using a specific linker should be part of the compilers
toolchain, we delegate to and not hardcoded here in ghc.

Diff Detail

Repository
rGHC Glasgow Haskell Compiler
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
angerman created this revision.Mar 15 2017, 8:56 PM
dfeuer requested changes to this revision.Mar 16 2017, 2:39 AM
dfeuer added a subscriber: dfeuer.

Wasn't this a rather recent change?

This revision now requires changes to proceed.Mar 16 2017, 2:39 AM
erikd requested changes to this revision.EditedMar 16 2017, 3:10 AM

Have you tested that on Arm64 Linux? For a long time BFD ld was broken on both 32 and 63 bit Arm. If BFD ld has been fixed we probably need a version check on it.

@dfeuer I'm happy to discuss this. And if we truly want to force gold here, we need to at least guard it with check that the compiler is gcc.

In D3351#96296, @erikd wrote:

Have you tested that on Arm64 Linux? For a long time BFD ld was broken on both 32 and 63 bit Arm. If BFD ld has been fixed we probably need a version check on it.

No I have not. However, that's why I believe if ld is broken on arm, than the toolchain should take care of this. Say a wrapper around gcc, that pushes -fuse-ld=gold, but not hardcoded in here in ghc.

@erikd I've just figured that my comment may raise more questions than answers. Killing code that only compiles for arm on linux, and then saying I did not test on arm linux might sound quite bonkers.

I've run into this when trying to cross compile to arm-linux with the android ndk toolchain. So I did not technically test this on arm. Hope that clears up any potential confusion. And yes, these lines
prevent cross compilation to android.

erikd added a comment.Mar 16 2017, 3:22 AM

And yes, these lines prevent cross compilation to android.

How do you know they don't break compiles (cross and/or native) for Linux?

In D3351#96301, @erikd wrote:

And yes, these lines prevent cross compilation to android.

How do you know they don't break compiles (cross and/or native) for Linux?

I do not. With the new CI infrastructure @davean is working on, we will though; and make the
kinds of breakage due to fixes on one platform that break another hopefully less likely.

angerman updated this revision to Diff 12031.Apr 7 2017, 5:00 AM
angerman edited edge metadata.
  • rebase against master
rwbarton edited edge metadata.Apr 7 2017, 7:35 AM

Otherwise this breaks if the used compiler fails to understand this directive. (looking at you clang!)

My version of clang understands -fuse-ld=gold. In what situation did you need this patch?

Otherwise this breaks if the used compiler fails to understand this directive. (looking at you clang!)

My version of clang understands -fuse-ld=gold. In what situation did you need this patch?

$ touch x.c
$ clang -fuse-ld=gold x.c 
clang: error: invalid linker name in argument '-fuse-ld=gold'

This holds true for at least the following clangs I have:

Apple LLVM version 8.1.0 (clang-802.0.38)
Target: x86_64-apple-darwin16.4.0
Thread model: posix
InstalledDir: /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin
Android clang version 3.8.275480  (based on LLVM 3.8.275480)
Target: x86_64-apple-darwin16.4.0
Thread model: posix
InstalledDir: /Users/angerman/Library/Android/sdk/ndk-bundle/toolchains/llvm/prebuilt/darwin-x86_64/bin/.
clang version 4.0.0 (https://github.com/llvm-mirror/clang.git 559aa046fe3260d8640791f2249d7b0d458b5700) (http://llvm.org/git/llvm.git 3de71b0e7b7a3b5df49f05a4653d4b1ecb70bfab)
Target: x86_64-apple-darwin16.4.0
Thread model: posix
InstalledDir: /Users/angerman/Projects/Haskell/clang-static/bin

Maybe it's clang on darwin specific?

bgamari requested changes to this revision.Apr 8 2017, 7:14 PM

I would that we have a configure check for Trac #4210 (which is the reason why we can't use bfd) before we remove this logic (as well as the logic in FIND_LD). All of the pieces necessary for such a check can be found here, https://sourceware.org/bugzilla/show_bug.cgi?id=16177.

This revision now requires changes to proceed.Apr 8 2017, 7:14 PM
angerman updated this revision to Diff 12070.Apr 10 2017, 10:03 PM
angerman edited edge metadata.
  • rebase onto master
bgamari requested changes to this revision.Apr 11 2017, 12:48 PM

I stand by my statement that we should consolidate this with the check in FIND_LD and properly check for Trac #4210.

This revision now requires changes to proceed.Apr 11 2017, 12:48 PM
rwbarton requested changes to this revision.Apr 11 2017, 3:09 PM

Maybe it's clang on darwin specific?

I think you just don't have gold installed.

This change will definitely break things for people who have a buggy ld.bfd installed, which they will find out when programs compiled for the target system segfault. How should they know that they have to use gold? I really think this is worse than whatever issue you are having that causes you to want to remove this.

Maybe it's clang on darwin specific?

I think you just don't have gold installed.

This change will definitely break things for people who have a buggy ld.bfd installed, which they will find out when programs compiled for the target system segfault. How should they know that they have to use gold? I really think this is worse than whatever issue you are having that causes you to want to remove this.

You might be right, that the flag is indeed supported, but there's just no gold.

The android sdk/ndk comes with:

/Users/angerman/Library/Android/sdk/ndk-bundle/toolchains/aarch64-linux-android-4.9/prebuilt/darwin-x86_64/aarch64-linux-android/bin/ld.gold
/Users/angerman/Library/Android/sdk/ndk-bundle/toolchains/aarch64-linux-android-4.9/prebuilt/darwin-x86_64/bin/aarch64-linux-android-ld.gold
/Users/angerman/Library/Android/sdk/ndk-bundle/toolchains/aarch64-linux-android-4.9/prebuilt/darwin-x86_64/lib/bfd-plugins/LLVMgold.so
/Users/angerman/Library/Android/sdk/ndk-bundle/toolchains/arm-linux-androideabi-4.9/prebuilt/darwin-x86_64/arm-linux-androideabi/bin/ld.gold
/Users/angerman/Library/Android/sdk/ndk-bundle/toolchains/arm-linux-androideabi-4.9/prebuilt/darwin-x86_64/bin/arm-linux-androideabi-ld.gold
/Users/angerman/Library/Android/sdk/ndk-bundle/toolchains/arm-linux-androideabi-4.9/prebuilt/darwin-x86_64/lib/bfd-plugins/LLVMgold.so
/Users/angerman/Library/Android/sdk/ndk-bundle/toolchains/llvm/prebuilt/darwin-x86_64/lib64/LLVMgold.so
/Users/angerman/Library/Android/sdk/ndk-bundle/toolchains/mips64el-linux-android-4.9/prebuilt/darwin-x86_64/lib/bfd-plugins/LLVMgold.so
/Users/angerman/Library/Android/sdk/ndk-bundle/toolchains/mipsel-linux-android-4.9/prebuilt/darwin-x86_64/lib/bfd-plugins/LLVMgold.so
/Users/angerman/Library/Android/sdk/ndk-bundle/toolchains/x86-4.9/prebuilt/darwin-x86_64/bin/i686-linux-android-ld.gold
/Users/angerman/Library/Android/sdk/ndk-bundle/toolchains/x86-4.9/prebuilt/darwin-x86_64/i686-linux-android/bin/ld.gold
/Users/angerman/Library/Android/sdk/ndk-bundle/toolchains/x86-4.9/prebuilt/darwin-x86_64/lib/bfd-plugins/LLVMgold.so
/Users/angerman/Library/Android/sdk/ndk-bundle/toolchains/x86_64-4.9/prebuilt/darwin-x86_64/bin/x86_64-linux-android-ld.gold
/Users/angerman/Library/Android/sdk/ndk-bundle/toolchains/x86_64-4.9/prebuilt/darwin-x86_64/lib/bfd-plugins/LLVMgold.so
/Users/angerman/Library/Android/sdk/ndk-bundle/toolchains/x86_64-4.9/prebuilt/darwin-x86_64/x86_64-linux-android/bin/ld.gold

Thus the issue may lie with explicitly calling it gold?

Can we in the meantime change this so the cases are android specific? And then try to implement as ben suggest?

I stand by my statement that we should consolidate this with the check in FIND_LD and properly check for Trac #4210.

angerman updated this revision to Diff 12090.Apr 11 2017, 11:33 PM
angerman edited edge metadata.
  • Use FIND_LD
angerman updated this revision to Diff 12276.Apr 25 2017, 9:09 PM
  • Use FIND_LD
  • Rebase
angerman updated this revision to Diff 12355.May 2 2017, 5:06 AM
  • rebase
angerman updated this revision to Diff 12415.May 7 2017, 8:50 PM
  • Use FIND_LD
  • rebase master

@bgamari is the use of FIND_LD to your liking?

I think there are a few issues here.

On the whole I think we should address this along with Trac #13541. This has on my list (9373994acaf1b73fe0e7cf8e03594c63cec8d235 was a first step) but has been superceded by more urgent things.

aclocal.m4
575

Is the first argument really right?

2000–2001

This needs to be updated.

configure.ac
500

Shouldn't the first argument be $target?

angerman retitled this revision from Don’t force gold on me. to Do not hardcode the specific linker to use.May 9 2017, 9:21 PM
angerman edited the summary of this revision. (Show Details)
angerman updated this revision to Diff 12468.May 9 2017, 9:30 PM
  • fix comment
  • fix missing dollars
angerman marked 3 inline comments as done.May 9 2017, 9:30 PM
angerman updated this revision to Diff 12476.May 9 2017, 11:25 PM
  • fix ld discovery for android and rpi
angerman updated this revision to Diff 12477.May 9 2017, 11:25 PM
  • Fix ld discovery
  • arm-unknown-linux-gnueabihf
[...]
checking for arm-linux-gnueabihf-ld... arm-linux-gnueabihf-ld
checking for arm-linux-gnueabihf-ld.gold... no
configure: WARNING: could not find ld.gold, falling back to arm-linux-gnueabihf-ld
[...]
Configure completed successfully.

   Building GHC version  : 8.3.20170510
          Git commit id  : 57e6f837910ef150f74e53d7003ebec859e979e7

   Build platform        : x86_64-apple-darwin
   Host platform         : x86_64-apple-darwin
   Target platform       : arm-unknown-linux
[...]
   ld           : arm-linux-gnueabihf-ld
[...]
  • armv7-unknown-linux-androideabi
[...]
checking for armv7-linux-androideabi-ld... armv7-linux-androideabi-ld
checking for armv7-linux-androideabi-ld.gold... no
configure: ld is ld.gold
[...]
Configure completed successfully.

   Building GHC version  : 8.3.20170510
          Git commit id  : 57e6f837910ef150f74e53d7003ebec859e979e7

   Build platform        : x86_64-apple-darwin
   Host platform         : x86_64-apple-darwin
   Target platform       : arm-unknown-linux-android
[...]
   ld           : armv7-linux-androideabi-ld
[...]
  • aarch64-unknown-linux-android
[...]
checking for aarch64-linux-android-ld... aarch64-linux-android-ld
checking for aarch64-linux-android-ld.gold... no
configure: WARNING: could not find ld.gold, falling back to aarch64-linux-android-ld
[...]
Configure completed successfully.

   Building GHC version  : 8.3.20170510
          Git commit id  : 57e6f837910ef150f74e53d7003ebec859e979e7

   Build platform        : x86_64-apple-darwin
   Host platform         : x86_64-apple-darwin
   Target platform       : aarch64-unknown-linux-android
[...]
   ld           : aarch64-linux-android-ld
[...]

I don't know why the android sdk for aarch64 ships ld.bfd by default and ld.gold for arm. But this test should be sufficient, I believe.

I wonder if we should properly respect LD if provided? Right now, we'll try to find gold, and only fall back if we won't find it. However if someone explicitly sets LD, shouldn't we respect that choice?

bgamari accepted this revision.May 10 2017, 10:29 PM

Alright. Fair enough. I still say we should do better though.

This revision is now accepted and ready to land.May 10 2017, 10:29 PM
This revision was automatically updated to reflect the committed changes.