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

[SR-1023] Build Error: Relocation R_X86_64_PC32 #43635

Closed
swift-ci opened this issue Mar 22, 2016 · 38 comments
Closed

[SR-1023] Build Error: Relocation R_X86_64_PC32 #43635

swift-ci opened this issue Mar 22, 2016 · 38 comments
Labels

Comments

@swift-ci
Copy link
Collaborator

@swift-ci swift-ci commented Mar 22, 2016

Previous ID SR-1023
Radar None
Original Reporter rlovelett (JIRA User)
Type Bug
Status Closed
Resolution Done

Attachment: Download

Environment

Arch Linux (4.4.5-1-ARCH)
Clang version 3.7.1 (tags/RELEASE_371/final)
GNU ld (GNU Binutils) 2.26.0.20160302

Additional Detail from JIRA
Votes 4
Component/s
Labels Bug
Assignee rlovelett (JIRA)
Priority Medium

md5: 887c0afbe82cca6279b355238f4a2f89

is duplicated by:

  • SR-1294 Ubuntu 16.04 swift fails to link: protected relocation issue

relates to:

  • SR-1316 swift-package-manager link doesn't seem to respect the gold linker flag on Ubuntu

Issue Description:

Linking libswiftCore.so using binutils 2.26 fails during the relocation of a protected symbol. This issue is not present in 2.25 of binutils.

[511/536] Linking CXX shared library lib/swift/linux/x86_64/libswiftCore.so
FAILED: : && /usr/bin/clang++  -fPIC -fno-stack-protector -fPIC -fvisibility-inlines-hidden -Wall -W -Wno-unused-parameter -Wwrite-strings -Wcast-qual -Wmissing-field-initializers -Wcovered-switch-default -Wnon-virtual-dtor -Wdelete-non-virtual-dtor -Werror=date-time -std=c++11 -fcolor-diagnostics -ffunction-sections -fdata-sections -Wdocumentation -Wimplicit-fallthrough -Wunreachable-code -Woverloaded-virtual -O3  -Wl,-z,defs   -target x86_64-unknown-linux-gnu -isysroot / -lpthread -ldl  -L/home/ryan/build/Ninja-ReleaseAssert/swift-linux-x86_64/./lib/swift/linux/x86_64 -L/home/ryan/build/Ninja-ReleaseAssert/swift-linux-x86_64/./bin/../lib/swift/linux/x86_64 -L/home/ryan/build/Ninja-ReleaseAssert/swift-linux-x86_64/./bin/../lib/swift/linux -shared -Wl,-soname,libswiftCore.so -o lib/swift/linux/x86_64/libswiftCore.so lib/swift/linux/x86_64/swift_begin.o stdlib/public/core/linux/x86_64/Swift.o lib/swift/linux/x86_64/swift_end.o -L/home/ryan/build/Ninja-ReleaseAssert/llvm-linux-x86_64/lib  -L/home/ryan/build/Ninja-ReleaseAssert/llvm-linux-x86_64/./lib lib/swift/linux/x86_64/libswiftRuntime.a lib/swift/linux/x86_64/libswiftStdlibStubs.a -licuuc -licui18n -lbsd -Wl,-rpath,"\$ORIGIN:/usr/lib/swift/linux" && :
/usr/bin/ld: stdlib/public/core/linux/x86_64/Swift.o: relocation R_X86_64_PC32 against protected symbol `_TMPSa' can not be used when making a shared object
/usr/bin/ld: final link failed: Bad value

Using git bisect I was able to determine the commit that introduced the behavior of binutils between 2.25 and 2.26 to be ca3fe95e469b9daec153caa2c90665f5daaec2b5.

ca3fe95e469b9daec153caa2c90665f5daaec2b5 is the first bad commit
commit ca3fe95e469b9daec153caa2c90665f5daaec2b5
Author: H.J. Lu <hjl.tools@gmail.com>
Date:   Thu Mar 5 06:34:39 2015 -0800

    Add extern_protected_data and set it for x86
    
    With copy relocation, address of protected data defined in the shared
    library may be external.  This patch adds extern_protected_data and
    changes _bfd_elf_symbol_refs_local_p to return false for protected data
    if extern_protected_data is true.
    
    bfd/
    
        PR ld/pr15228
        PR ld/pr17709
        * elf-bfd.h (elf_backend_data): Add extern_protected_data.
        * elf32-i386.c (elf_backend_extern_protected_data): New.
        Defined to 1.
        * elf64-x86-64.c (elf_backend_extern_protected_data): Likewise.
        * elflink.c (_bfd_elf_adjust_dynamic_copy): Don't error on
        copy relocs against protected symbols if extern_protected_data
        is true.
        (_bfd_elf_symbol_refs_local_p): Don't return true on protected
        non-function symbols if extern_protected_data is true.
        * elfxx-target.h (elf_backend_extern_protected_data): New.
        Default to 0.
        (elfNN_bed): Initialize extern_protected_data with
        elf_backend_extern_protected_data.
    
    ld/testsuite/
    
        PR ld/pr15228
        PR ld/pr17709
        * ld-i386/i386.exp (i386tests): Add a test for PR ld/17709.
        * ld-i386/pr17709-nacl.rd: New file.
        * ld-i386/pr17709.rd: Likewise.
        * ld-i386/pr17709a.s: Likewise.
        * ld-i386/pr17709b.s: Likewise.
        * ld-i386/protected3.d: Updated.
        * ld-i386/protected3.s: Likewise.
        * ld-x86-64/pr17709-nacl.rd: New file.
        * ld-x86-64/pr17709.rd: Likewise.
        * ld-x86-64/pr17709a.s: Likewise.
        * ld-x86-64/pr17709b.s: Likewise.
        * ld-x86-64/protected3.d: Updated.
        * ld-x86-64/protected3.s: Likewise.
        * ld-x86-64/x86-64.exp (x86_64tests): Add a test for PR ld/17709.

:040000 040000 7fc861c288f9bed44c6444d1b04e2f6e688aeeaf fca3f6ce979e7c00ed44c04f506880015235806d M  bfd
:040000 040000 a5e358abb99b2b4089765f16904f9ebc496c0f12 7ccba1e77448a0155e56e8155073b40804b00daa M  ld

To perform the bisect I used 2bd25930221dea4bf33c13a89c111514491440e2 as the last known good commit and 71090e7a9dde8d28ff5c4b44d6d83e270d1088e4 as the last known bad commit.

@jckarter
Copy link
Member

@jckarter jckarter commented Mar 22, 2016

Is there a way then for the compiler to tell ld "no, really, always resolve this symbol to the shared library's version", short of passing -Bsymbolic or something potentially invasive to non-Swift object files?

@jckarter
Copy link
Member

@jckarter jckarter commented Mar 22, 2016

rlovelett (JIRA User) Thanks for figuring out where the problem is!

@jckarter
Copy link
Member

@jckarter jckarter commented Mar 22, 2016

The fix for this is in the driver or (hopefully) in the compiler, not the runtime.

@swift-ci
Copy link
Collaborator Author

@swift-ci swift-ci commented Mar 22, 2016

Comment by Ryan Lovelett (JIRA)

@jckarter can you expand on that? I know that this might not be a starter bug but I'd like to finally fix something around here instead of just filing bugs.

When you say the driver do you mean this? Also which compiler, do you mean the clang compiler or the actual Swift compiler?

@jckarter
Copy link
Member

@jckarter jckarter commented Mar 22, 2016

If you have something like this:

int foo() { bar(); }
int bar() { }

Then the default behavior for an ELF shared library is for the call to bar from foo to be dynamically resolved by the dynamic linker, on the off chance that the symbol might be overridden by some other shared library. This dynamic resolution incurs load-time and runtime overhead that we don't want for Swift symbols, and we use relocations that dynamic linkers don't support, so we use protected visibility, which is supposed to tell the linker to resolve references to the protected symbol statically. It sounds like GNU ld broke this intentionally because "copy" relocations could theoretically cause a definition to be copied out of its home dylib, but we shouldn't run into situations where that's necessary in Swift. We really want references to symbols within a single dynamic library to be statically resolved.

@swift-ci
Copy link
Collaborator Author

@swift-ci swift-ci commented Mar 23, 2016

Comment by Ryan Lovelett (JIRA)

First thanks for explaining that. Very helpful. Not sure if I am capable of fixing this yet.

But I'm trying to read more about the topic. I wonder do you think this issue is related? https://llvm.org/bugs/show_bug.cgi?id=23104

@jckarter
Copy link
Member

@jckarter jckarter commented Mar 23, 2016

Yeah, that's the same issue.

@jckarter
Copy link
Member

@jckarter jckarter commented Mar 23, 2016

I'd start by asking H.J. or the GNU ld list what options we have for requiring local references to protected symbols. If -Bsymbolic is the only choice, then so be it.

@jckarter
Copy link
Member

@jckarter jckarter commented Mar 23, 2016

It occurs to me that another approach here might be, instead of using protected visibility, to use a hidden alias symbol for local references within an image. So for each public symbol we'd have:

; Public symbol
@_Tfoo = global i32 0
; Hidden alias
@_tfoo = hidden alias i32 *@_Tfoo

and we'd reference @_tfoo locally.

@jckarter
Copy link
Member

@jckarter jckarter commented Mar 23, 2016

The problem with that would be that, if the symbol were copied into the executable's region, we'd have inconsistent metadata pointers. I hope there's a way to make copy relocations illegal on a symbol.

@jckarter
Copy link
Member

@jckarter jckarter commented Mar 24, 2016

I asked the binutils list:

https://sourceware.org/ml/binutils/2016-03/msg00312.html
https://sourceware.org/ml/binutils/2016-03/msg00313.html

H.J. recommends invoking the linker with -z nocopyreloc.

@octoploid
Copy link
Mannequin

@octoploid octoploid mannequin commented Mar 24, 2016

@swift-ci
Copy link
Collaborator Author

@swift-ci swift-ci commented Apr 1, 2016

Comment by Ryan Lovelett (JIRA)

While I can't say I know what works. I can say something that doesn't. I tried applying the following patch to send -z nocopyreloc through to the linker (as suggested).

diff --git a/cmake/modules/AddSwift.cmake b/cmake/modules/AddSwift.cmake
index 9234f51..69d959e 100644
--- a/cmake/modules/AddSwift.cmake
+++ b/cmake/modules/AddSwift.cmake
@@ -1164,6 +1164,8 @@ function(_add_swift_library_single target name)
     list(APPEND link_flags "-fuse-ld=gold")
   endif()
 
+  list(APPEND link_flags "-Wl,-z,nocopyreloc")
+
   # Configure plist creation for OS X.
   set(PLIST_INFO_PLIST "Info.plist" CACHE STRING "Plist name")
   if(APPLE AND SWIFTLIB_SINGLE_IS_STDLIB)

I'll concede this might not be the correct way to pass things through to the linker. But I did notice it appears on the arguments sent to clang++.

[510/544] Linking CXX shared library lib/swift/linux/x86_64/libswiftCore.so
FAILED: : && /home/ryan/Source/build/Ninja-ReleaseAssert/llvm-linux-x86_64/bin/clang++  -fPIC -fno-stack-protector -fPIC -fvisibility-inlines-hidden -Wall -W -Wno-unused-parameter -Wwrite-strings -Wcast-qual -Wmissing-field-initializers -Wcovered-switch-default -Wnon-virtual-dtor -Wdelete-non-virtual-dtor -Werror=date-time -std=c++11 -fcolor-diagnostics -ffunction-sections -fdata-sections -Wdocumentation -Wimplicit-fallthrough -Wunreachable-code -Woverloaded-virtual -O3  -Wl,-z,defs   -target x86_64-unknown-linux-gnu -isysroot / -lpthread -ldl -Wl,-z,nocopyreloc  -L/home/ryan/Source/build/Ninja-ReleaseAssert/swift-linux-x86_64/./lib/swift/linux/x86_64 -L/home/ryan/Source/build/Ninja-ReleaseAssert/swift-linux-x86_64/./bin/../lib/swift/linux/x86_64 -L/home/ryan/Source/build/Ninja-ReleaseAssert/swift-linux-x86_64/./bin/../lib/swift/linux -shared -Wl,-soname,libswiftCore.so -o lib/swift/linux/x86_64/libswiftCore.so lib/swift/linux/x86_64/swift_begin.o stdlib/public/core/linux/x86_64/Swift.o lib/swift/linux/x86_64/swift_end.o -L/home/ryan/Source/build/Ninja-ReleaseAssert/llvm-linux-x86_64/lib  -L/home/ryan/Source/build/Ninja-ReleaseAssert/llvm-linux-x86_64/./lib lib/swift/linux/x86_64/libswiftRuntime.a lib/swift/linux/x86_64/libswiftStdlibStubs.a -licuuc -licui18n -Wl,-rpath,"\$ORIGIN:/usr/lib/swift/linux" && :
/usr/bin/ld: stdlib/public/core/linux/x86_64/Swift.o: relocation R_X86_64_PC32 against protected symbol `_TMPSa' can not be used when making a shared object

@jckarter
Copy link
Member

@jckarter jckarter commented Apr 1, 2016

The argument's been ongoing on the binutils list; it sounds like this may be a mistake on their part they'll backpedal. In the meantime, it sounds like we also need to pass -z noextern-protected-data to the linker as well to prevent it from treating a library's own protected symbols as external.

@swift-ci
Copy link
Collaborator Author

@swift-ci swift-ci commented Apr 1, 2016

Comment by Ryan Lovelett (JIRA)

That does get it to compile now; thanks for that. 👍

diff --git a/cmake/modules/AddSwift.cmake b/cmake/modules/AddSwift.cmake
index 4010aac..8bb7c10 100644
--- a/cmake/modules/AddSwift.cmake
+++ b/cmake/modules/AddSwift.cmake
@@ -1164,6 +1164,9 @@ function(_add_swift_library_single target name)
     list(APPEND link_flags "-fuse-ld=gold")
   endif()
 
+  list(APPEND link_flags "-Wl,-z,nocopyreloc")
+  list(APPEND link_flags "-Wl,-z,noextern-protected-data")
+
   # Configure plist creation for OS X.
   set(PLIST_INFO_PLIST "Info.plist" CACHE STRING "Plist name")
   if(APPLE AND SWIFTLIB_SINGLE_IS_STDLIB)

I too have been following that thread on the binutils list that you started. I agree that it appears that someone does not think that should have been merged. Though I don't have enough visibility into the project to determine if the person who thinks it shouldn't be there actually has the authority to revert it.

My larger question is what is the Swift position on this? Is Swift going to accept patches to work-around these issues or is it just not going to support Ubuntu 16.04 until binutils is properly patched?

Let's say it does get reverted/patched upstream. That doesn't seem like a guarentee that'll make it into Ubuntu 16.04. I targeted that release because it is due in ~21 days and is going to ship with some version of binutils-2.26; likely one that will not link properly.

@jckarter
Copy link
Member

@jckarter jckarter commented Apr 1, 2016

I think the thing for us to do would be to have a CMake check that we're using a version of GNU ld that takes those -z flags, and pass them if they're accepted.

@swift-ci
Copy link
Collaborator Author

@swift-ci swift-ci commented Apr 1, 2016

Comment by Ryan Lovelett (JIRA)

Ok I think I can work towards a patch for that.

Additionally, this sort of patch only works for the main Swift build.

What about other libraries? Are they all going to need to provide a similar patch or is there a way to provide this capability uniformly via swiftc?

@jckarter
Copy link
Member

@jckarter jckarter commented Apr 1, 2016

Yeah, the driver should be updated to invoke the linker with the necessary flags. @mxcl or @ddunbar, does swift build also drive the linker itself without using swiftc in any circumstances?

@ddunbar
Copy link
Member

@ddunbar ddunbar commented Apr 1, 2016

Not for Swift code. Pure C language targets link with Clang, but I don't think that is an issue here.

Note that a CMake check is not really correct here (for the driver), because IIRC this applies when `swiftc` is compiling, at which point it might have a different linker. Ideally the checks in the driver would be based on the linker in use at the time.

@jckarter
Copy link
Member

@jckarter jckarter commented Apr 1, 2016

Good point @ddunbar. As long as we use -z nocopyreloc whenever there are Swift dynamic libraries involved in the linking, we should be OK.

@swift-ci
Copy link
Collaborator Author

@swift-ci swift-ci commented Apr 1, 2016

Comment by Ryan Lovelett (JIRA)

I've attached a patch (0001-Work-around-relocation-R_X86_64_PC32-link-error.patch) that works around the linker issues for only the Swift toolchain.

It checks to see if ld has the flags necessary and then sets them accordingly.

On OSX it looks something like this in the CMake logs:

...
-- Performing Test LINKER_SUPPORTS_NOCOPYRELOC
-- Performing Test LINKER_SUPPORTS_NOCOPYRELOC - Failed
-- Performing Test LINKER_SUPPORTS_NOEXTERN_PROTECTED_DATA
-- Performing Test LINKER_SUPPORTS_NOEXTERN_PROTECTED_DATA - Failed
...

on Arch

...
-- Performing Test LINKER_SUPPORTS_NOCOPYRELOC
-- Performing Test LINKER_SUPPORTS_NOCOPYRELOC - Success
-- Performing Test LINKER_SUPPORTS_NOEXTERN_PROTECTED_DATA
-- Performing Test LINKER_SUPPORTS_NOEXTERN_PROTECTED_DATA - Success
...

I've also submitted the patch as pull request.

@swift-ci
Copy link
Collaborator Author

@swift-ci swift-ci commented Apr 5, 2016

Comment by Ryan Lovelett (JIRA)

@jckarter I'm getting ready to start the driver patch. Can you point me to docs or anything that might help me to understand how the driver works?

@jckarter
Copy link
Member

@jckarter jckarter commented Apr 5, 2016

I don't think we have any real docs, but @belkadan is the expert in all things driver.

@belkadan
Copy link
Contributor

@belkadan belkadan commented Apr 5, 2016

If you're just adding flags to the linker invocation, the right place is lib/Driver/ToolChains.cpp (note the plural). It should be pretty easy to go from there.

@swift-ci
Copy link
Collaborator Author

@swift-ci swift-ci commented Apr 6, 2016

Comment by Ryan Lovelett (JIRA)

@belkadan that was perfect guidance.

I now have a proof of concept patch that restores compilation for XCTest and SwiftPM (which up until now had been broken).

diff --git a/lib/Driver/ToolChains.cpp b/lib/Driver/ToolChains.cpp
index 5e20ac3..eb4b846 100644
--- a/lib/Driver/ToolChains.cpp
+++ b/lib/Driver/ToolChains.cpp
@@ -1268,6 +1268,9 @@ toolchains::GenericUnix::constructInvocation(const LinkJobAction &job,
     Arguments.push_back(context.Args.MakeArgString("--target=" + getTriple().str()));
   }
 
+  Arguments.push_back("-Wl,-z,nocopyreloc");
+  Arguments.push_back("-Wl,-z,noextern-protected-data");
+
   // Add the runtime library link path, which is platform-specific and found
   // relative to the compiler.
   llvm::SmallString<128> RuntimeLibPath;

That patch however does not take in account the fact that {ld} may not support those flags (as is the case for binutils < 2.26). For the CMake patch the fix was to check if the linker supports those flags by attempting to compile with the flags present. That does not seem like a great method for the driver.

Presumably we could ask the linker for its version and then do a version comparison but that feels brittle. Are there any preferred methods for doing this?

@trfiala
Copy link
Mannequin

@trfiala trfiala mannequin commented Apr 25, 2016

Poke. We need this resolved to get Ubuntu 16.04 working. CI will be switching over to using this soon.

@jckarter
Copy link
Member

@jckarter jckarter commented Apr 25, 2016

Can you use gold in CI in the meantime?

@trfiala
Copy link
Mannequin

@trfiala trfiala mannequin commented Apr 25, 2016

If that works around it, that's fine. There's a part of the Swift build process that still can't use the gold linker (maybe building the standard libraries? It's where we collect the protocol conformances and use a linker script that isn't supported by gold).

I'll give it a shot.

@jckarter
Copy link
Member

@jckarter jckarter commented Apr 25, 2016

We ought to be able to use gold everywhere now. We don't use a linker script anymore AFAIK.

@trfiala
Copy link
Mannequin

@trfiala trfiala mannequin commented Apr 25, 2016

> We ought to be able to use gold everywhere now. We don't use a linker script anymore AFAIK.

Oh, I totally missed that if that's the case.

I had fished gold through the build process (as an option, --enable-gold-linker=1 in build-script-impl). But I had it ignored for the portion that used to require the linker script. I wonder if that got adjusted. I'll have to take a look when I try to enable it for the CI.

@swift-ci
Copy link
Collaborator Author

@swift-ci swift-ci commented Apr 25, 2016

Comment by Ryan Lovelett (JIRA)

@jckarter unless SR-941 has been resolved (I'll admit that I have not checked recently) I don't think using gold will work everywhere.

I would like to ask why the approach proposed in my pull-request is not being pursued/reviewed? AFAIK, it works just fine on any platform that uses the newer binutils; which includes Ubuntu 16.04. The only problem it has is that it will not work on older binutils (e.g., those found on Ubuntu 14.04/15.10). So the work necessary to be done is just to devise a way to allow the Driver to understand if the linker needs these special flags, much like the CMake patches do.

@belkadan
Copy link
Contributor

@belkadan belkadan commented Apr 25, 2016

It's because I don't have a good answer to "how should the Driver do feature checks" and haven't had time to look for one. :-(

@swift-ci
Copy link
Collaborator Author

@swift-ci swift-ci commented Apr 25, 2016

Comment by Ryan Lovelett (JIRA)

That is a fair answer and seems reasonable. Though why rush to support Ubuntu 16.04 before a permanent solution is found?

FWIW I want to say I am for just declaring Swift uses the gold linker everywhere and dropping support for the "standard" linker. Then we could close this as a WONTFIX and pursue SR-941 and other issues like it.

@trfiala
Copy link
Mannequin

@trfiala trfiala mannequin commented Apr 26, 2016

So I tried building on 16.04 with the gold linker flag, and that still fails, but here now:

Link /home/tfiala/src/lldb-github/build/buildbot_linux/swiftpm-linux-x86_64/.bootstrap/bin/swift-build
/usr/bin/ld: /home/tfiala/src/lldb-github/build/buildbot_linux/swiftpm-linux-x86_64/.bootstrap/lib/PackageDescription.a(Package.o): relocation R_X86_64_PC32 against protected symbol `_TMLCC18PackageDescription7Package10Dependency' can not be used when making a shared object
/usr/bin/ld: final link failed: Bad value
clang: error: linker command failed with exit code 1 (use -v to see invocation)

So it looks like maybe the gold linker setting isn't getting used by the swift-package-manager side. I'll file a separate ticket for that: SR-1316.

@trfiala
Copy link
Mannequin

@trfiala trfiala mannequin commented Apr 26, 2016

SR-1316 is blocking a work-around for fixing this on Ubuntu 16.04: the gold linker isn't getting used in what looks to be a swift-driven linker step.

@swift-ci
Copy link
Collaborator Author

@swift-ci swift-ci commented May 4, 2016

Comment by Ryan Lovelett (JIRA)

I'm un-assigning this one from myself. Without more guidance I'm not sure what I can do to the propsed pull request.

I'll come back if I have an idea. For now the patch works for me.

@swift-ci
Copy link
Collaborator Author

@swift-ci swift-ci commented May 25, 2016

Comment by Ryan Lovelett (JIRA)

With #2609 going in I think this issue has sufficiently been resolved. I'm going to close this issue.

@swift-ci
Copy link
Collaborator Author

@swift-ci swift-ci commented May 25, 2016

Comment by Ryan Lovelett (JIRA)

This issue was sufficiently solved by the patch in #2609

@swift-ci swift-ci transferred this issue from apple/swift-issues Apr 25, 2022
This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants