[cmake] Work around relocation R_X86_64_PC32 link error #2012

RLovelett · 2016-04-01T18:45:12Z

What's in this pull request?

This is the first patch to begin working around the dynamic resolution of protected symbols. This behavior was introduced in binutils 2.26.

A more complete discussion of the error and its background can be found in SR-1023.

The brief overview is that in versions of ld that would be effected by this dynamic relocation issue there are two flags: -z nocopyreloc and -z noextern-protected-data. If ld supports the flags then they are sent to the linker. This patch checks that the linker supports the flags and then optionally sets them.

The code to check if ld supports the flags is inspired by this CMake mailing list entry.

https://cmake.org/pipermail/cmake/2011-July/045525.html

Resolves bug number: (SR-1023)

~~While this pull does not entirely resolve SR-1023 I do think it is a good first step.~~

Update 1: I've now provided a Driver patch too. This branch should now work on binutils 2.26 (e.g., Ubuntu 16.04). The patch does not work on binutils builds that do not support these flags (e.g., Ubuntu 14.04/15.10). A conditional still needs to be written for the Driver to apply the new flags conditionally. I'm looking for feedback on how to best address that.

Before merging this pull request to apple/swift repository:

Test pull request on Swift continuous integration.

Triggering Swift CI

The swift-ci is triggered by writing a comment on this PR addressed to the GitHub user @swift-ci. Different tests will run depending on the specific comment that you use. The currently available comments are:

Smoke Testing

Platform	Comment
All supported platforms	@swift-ci Please smoke test
OS X platform	@swift-ci Please smoke test OS X platform
Linux platform	@swift-ci Please smoke test Linux platform

Validation Testing

Platform	Comment
All supported platforms	@swift-ci Please test
OS X platform	@swift-ci Please test OS X platform
Linux platform	@swift-ci Please test Linux platform

Note: Only members of the Apple organization can trigger swift-ci.

jckarter · 2016-04-01T18:47:59Z

Looks OK to me (though @gribozavr is a better CMake-r than me). We'll also need something in the driver to pass these flags if the linker needs them too. That would need to be some kind of runtime check, though.

RLovelett · 2016-04-01T18:48:37Z

cmake/modules/AddSwift.cmake

@@ -1774,6 +1787,18 @@ function(_add_swift_executable_single name)
    list(APPEND link_flags "-fuse-ld=gold")
  endif()

+  INCLUDE(CheckCXXCompilerFlag)


I actually don't think this block is required. I merely included it because I think it might be. Someone who understands the CMake scripts a little better might be able to quickly deduce its necessity.

I don't think this block is necessary. This function is used to configure compiler executables, which don't have any Swift code in them.

RLovelett · 2016-04-01T18:52:46Z

@jckarter I don't yet understand how the driver works in order to write that code. I'm interested in trying to fix that piece as well (as a separate pull). I just figured I'd start with the piece I do somewhat understand and can test and validate.

jckarter · 2016-04-01T18:55:21Z

One thing, there's no reason to do this check when building to target OS X; it's only necessary if targeting ELF platforms (or maybe only Linux, if the binutils brain damage didn't make it out to *BSD).

jckarter · 2016-04-01T18:56:04Z

Fair enough about making the driver change separately. It'd be good to note that in the PR.

gribozavr · 2016-04-02T06:22:00Z

cmake/modules/AddSwift.cmake

+  CHECK_CXX_COMPILER_FLAG("" LINKER_SUPPORTS_NOEXTERN_PROTECTED_DATA)
+  IF(LINKER_SUPPORTS_NOEXTERN_PROTECTED_DATA)
+      list(APPEND link_flags ${CMAKE_REQUIRED_FLAGS})
+  ENDIF()


I don't think you should include() or run tests in this function. This function is executed many times.

You should run the test once in the top-level CMakeLists.txt.

Also, our CMake code uses lowercase for function names, and two spaces for indentation, could you make it consistent?

RLovelett · 2016-04-05T02:42:00Z

I've rebased on master and pushed up a new attempt that incorporates the feedback.

@jckarter I've moved the test for the linker flag inside of an if that only executes on Linux.

@gribozavr I've created a new CMake module, CheckLinkerDynamicRelocationFlags, that defines a function that allows for testing the linker. This function is executed in the CMakeLists.txt and sets two variables LINKER_SUPPORTS_NOCOPYRELOC and LINKER_SUPPORTS_NOEXTERN_PROTECTED_DATA. AddSwift.cmake merely checks that those variables evalute to true and then turns on the appropriate flags.

I believe I've also addressed the styling changes you've requested.

gribozavr · 2016-04-05T03:38:13Z

Thank you! Could you prefix global variables with SWIFT_*? LGTM with that.

RLovelett · 2016-04-05T12:33:51Z

Updated to SWIFT_LINKER_SUPPORTS_NOCOPYRELOC and SWIFT_LINKER_SUPPORTS_NOEXTERN_PROTECTED_DATA.

Thank you for taking the time to review @jckarter and @gribozavr.

jckarter · 2016-04-05T15:50:05Z

CMakeLists.txt

@@ -419,6 +420,12 @@ if("${CMAKE_SYSTEM_NAME}" STREQUAL "Linux")
  set(SWIFT_HOST_VARIANT_SDK "LINUX")
  set(SWIFT_PRIMARY_VARIANT_SDK_default "LINUX")

+  # Support dynamic relocation in binutils >= 2.26


More precisely, we're supporting "relative relocation against protected symbols in binutils 2.26". Give them the benefit of the doubt that 2.27 might be fixed.

jckarter · 2016-04-05T15:50:49Z

LGTM too, with a fix to the comment.

rintaro · 2016-04-05T16:20:33Z

cmake/modules/CheckLinkerDynamicRelocationFlags.cmake

+function(check_linker_supports_flag flag_to_test var_name)
+  set(CMAKE_REQUIRED_FLAGS "${flag_to_test}")
+  check_cxx_compiler_flag("" LINKER_SUPPORTS_NOEXTERN_PROTECTED_DATA)
+  if(LINKER_SUPPORTS_NOEXTERN_PROTECTED_DATA)


Variable name LINKER_SUPPORTS_NOEXTERN_PROTECTED_DATA is not relevant for here.
result would be sufficient I think.

(I could have swore I replied to this already but apparently not.)

If I change it to result than the check in the CMake output is less meaningful. It displays something like

... Performing Test result Performing Test result - Success ...

as opposed to

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

If you still think it is worth it than I'm amenable to the change but I think it provides a less clear description of what is going on.

Hm, I understand that.
And I found CMake caches the result of check_cxx_compiler_flag associated with the variable name.
Because of that, if "-Wl,-z,nocopyreloc" failed, then "-Wl,-z,noextern-protected-data" fails without actual test.
To prevent that, <var> for every tests must have unique names.

For example:

function(check_linker_supports_flag flag_to_test var_name) set(CMAKE_REQUIRED_FLAGS "${flag_to_test}") check_cxx_compiler_flag("" ${var_name}) if(${${var_name}}) set("${var_name}" TRUE PARENT_SCOPE) else() set("${var_name}" FALSE PARENT_SCOPE) endif() endfunction()

I think I should clarify the problem. For example:

cmake_minimum_required(VERSION 2.8.12) include(CheckCXXCompilerFlag) check_cxx_compiler_flag("-static" RESULT) message("RESULT_STATIC: ${RESULT}") check_cxx_compiler_flag("-FOO-BAR" RESULT) message("RESULT_FOOBAR: ${RESULT}")

This CMakeLists.txt outputs:

-- Performing Test RESULT -- Performing Test RESULT - Success RESULT_STATIC: 1 RESULT_FOOBAR: 1

The second check_cxx_compiler_flag() is no-op and just returns cached RESULT value.
Generated CMakeCache.txt contains

//Test RESULT RESULT:INTERNAL=1

@rintaro wonderful catch! Yes.

-- Performing Test SWIFT_LINKER_SUPPORTS_NOCOPYRELOC -- Performing Test SWIFT_LINKER_SUPPORTS_NOCOPYRELOC - Success -- Performing Test SWIFT_LINKER_SUPPORTS_NOEXTERN_PROTECTED_DATA -- Performing Test SWIFT_LINKER_SUPPORTS_NOEXTERN_PROTECTED_DATA - Success

Much, much better. Thank you.

A more complete discussion of the error and its background can be found in SR-1023. The brief overview is that in versions of `ld` that would be effected by this dynamic relocation issue there are two flags: `-z nocopyreloc` and `-z noextern-protected-data`. If `ld` supports the flags then they are sento to the linker. This patch checks that the linker supports the flags and then optionally sets them. The code to check if `ld` supports the flags is inspired by this CMake mailing list entry. https://cmake.org/pipermail/cmake/2011-July/045525.html

RLovelett · 2016-05-25T15:58:48Z

With #2609 going in I now think this change is over-come by events. The problem that it was trying to fix was solved by #2609.

I am going to close it.

RLovelett reviewed Apr 1, 2016
View reviewed changes

gribozavr reviewed Apr 2, 2016
View reviewed changes

RLovelett force-pushed the bugfix/SR-1023-Build-Error-Relocation-R_X86_64_PC32 branch 2 times, most recently from b2fc5eb to ca5808d Compare April 5, 2016 02:36

RLovelett force-pushed the bugfix/SR-1023-Build-Error-Relocation-R_X86_64_PC32 branch from ca5808d to 84de260 Compare April 5, 2016 02:42

RLovelett force-pushed the bugfix/SR-1023-Build-Error-Relocation-R_X86_64_PC32 branch from 84de260 to 63c702c Compare April 5, 2016 12:32

jckarter reviewed Apr 5, 2016
View reviewed changes

rintaro reviewed Apr 5, 2016
View reviewed changes

RLovelett force-pushed the bugfix/SR-1023-Build-Error-Relocation-R_X86_64_PC32 branch 2 times, most recently from 5c81539 to 63ae486 Compare April 6, 2016 19:08

RLovelett mentioned this pull request Apr 6, 2016

Restore build functionality RLovelett/swift-aur#4

Merged

RLovelett force-pushed the bugfix/SR-1023-Build-Error-Relocation-R_X86_64_PC32 branch 2 times, most recently from 281b050 to 59d2e56 Compare April 15, 2016 12:44

RLovelett force-pushed the bugfix/SR-1023-Build-Error-Relocation-R_X86_64_PC32 branch 2 times, most recently from b15f541 to 2ff13c6 Compare April 26, 2016 13:40

rintaro mentioned this pull request Apr 27, 2016

[CMakeLists] Use unique <var> for check_cxx_source_runs test #2320

Merged

1 task

RLovelett force-pushed the bugfix/SR-1023-Build-Error-Relocation-R_X86_64_PC32 branch from 2ff13c6 to 650791d Compare April 29, 2016 18:04

RLovelett force-pushed the bugfix/SR-1023-Build-Error-Relocation-R_X86_64_PC32 branch from 650791d to cabe7c4 Compare May 23, 2016 13:02

Update the driver

0be7d4f

RLovelett closed this May 25, 2016

RLovelett deleted the bugfix/SR-1023-Build-Error-Relocation-R_X86_64_PC32 branch July 26, 2016 17:03

This was referenced Apr 1, 2016

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

Closed

[SR-1294] Ubuntu 16.04 swift fails to link: protected relocation issue #43902

Closed

[cmake] Work around relocation R_X86_64_PC32 link error #2012

[cmake] Work around relocation R_X86_64_PC32 link error #2012

Uh oh!

Conversation

RLovelett commented Apr 1, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What's in this pull request?

Resolves bug number: (SR-1023)

Uh oh!

jckarter commented Apr 1, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RLovelett commented Apr 1, 2016

Uh oh!

jckarter commented Apr 1, 2016

Uh oh!

jckarter commented Apr 1, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RLovelett commented Apr 5, 2016

Uh oh!

gribozavr commented Apr 5, 2016

Uh oh!

RLovelett commented Apr 5, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jckarter commented Apr 5, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RLovelett commented May 25, 2016

Uh oh!

Uh oh!

RLovelett commented Apr 1, 2016 •

edited

Loading