[CMake] Windows stdlib cross-compilation fixes #13140

troughton · 2017-11-29T09:23:07Z

This PR contains a few changes that are needed to make the Windows stdlib cross-compile from Linux.

Namely, it:

Ensures that -Wl,-z,defs is only added for non-Windows (and non-RemoteMirror) targets
Skips building SwiftPrivate on Windows since that currently fails to link (I've filed SR-6489 to track this)
Remove the LLVM-provided -std= C++ flag and instead use CMake's CXX_STANDARD so that we can build against the Visual Studio 2017 headers (which require C++14). If someone can suggest a more elegant implementation than just replacing the string in CMAKE_CXX_FLAGS I'd be glad to hear it.

troughton · 2017-11-29T09:23:47Z

@compnerd This addresses most of the issues that I contacted you about. I haven't yet had a chance to look at what was causing the VFS overlay issue or the linking issues for the produced executables on x64, but will do so when I next get a chance.

troughton · 2017-11-30T23:59:47Z

@compnerd The VFS overlay issue was it was only being applied to Swift sources and not the C++ sources in the runtime. Remaining issues are the lld-link arguments being wrong on x64 and the runtime assertion in ManagedBufferPointer, both of which I plan to look at on the weekend.

troughton · 2017-12-08T21:14:16Z

This has been sitting here for a little while and I'm not sure who it falls under.

@davezarzycki This may be of interest with regards to #12895. Could you take a look, and possibly cc in other relevant people?

davezarzycki · 2017-12-08T22:06:56Z

Hi @troughton – I'm not sure who to CC. You might want to consider emailing swift-dev at this point.

davezarzycki

Please don't remove the hack in stdlib/public/SwiftRemoteMirror/CMakeLists.txt. It is needed on Linux unified CMake builds.

davezarzycki · 2017-12-09T17:13:02Z

Oh, I think I see what you're doing. I think that removing -Wl,-z,defs by default and then adding it back selectively is needlessly complicated. What is preventing us from limiting the default removal of -Wl,-z,defs to just Windows? (And therefore let -W,-z,defs be preserved by default on Linux.)

troughton · 2017-12-09T20:34:51Z

It needs to be passed as a linker flag for Swift libraries depending on the target SDK, not the host platform, and we don’t know what the target SDKs are until we’re in add_swift_library. Removing it for Windows (e.g. with CMAKE_SYSTEM_NAME equal to WINDOWS) would fix native compilation but not cross-compilation.

It’s definitely not a nice solution, but I’m not sure how to do it better.

davezarzycki · 2017-12-10T13:13:48Z

I still think that -Wl,-z,defs is a good default, and I think removing that flag by default is the wrong approach (especially when one is not cross compiling); but I'm not a CMake expert, so I don't know how to recommend something better.

From my experience with Makefile based projects, one should get the defaults "right" at the top level and then have subordinate Makefiles or CMakeLists opt into non-default behavior as needed.

troughton · 2017-12-17T20:51:00Z

@gottesmm I might pull out the C++14 part of it looks like the consensus is to shift Swift to C++14 on all platforms, but does the rest of this PR look okay?

gottesmm · 2017-12-17T22:11:38Z

@troughton I would prefer if we just switch swift to c++14 as well. We should only add complexity to the build if we absolutely need to do so. Let me take a look real quick at the PR.

gottesmm

Some questions.

gottesmm · 2017-12-17T22:12:45Z

cmake/modules/SwiftSharedCMakeConfig.cmake

@@ -81,8 +81,20 @@ macro(swift_common_standalone_build_config_llvm product is_cross_compiling)
  # HACK: this ugly tweaking is to prevent the propagation of the flag from LLVM
  # into swift.  The use of this flag pollutes all targets, and we are not able
  # to remove it on a per-target basis which breaks cross-compilation.
+  # Therefore, we always remove it here and then selectively re-add it to the


I have noticed this hack making the rounds recently. What is going on here? Why doesn't LLVM run into these issues as well?

The short version is Windows doesn't support -z,defs as a linker flag, and CMAKE_SHARED_LINKER_FLAGS get applied to all linker invocations, so we have to move where it gets added.

LLVM doesn't run into it because it uses the CMAKE_SYSTEM_NAME method for cross-compiling (so this flag would never get inserted in the first place for Windows), whereas Swift has a custom scheme where we don't know the SDK targets until much later in the configuration process.

This explanation should be added here. Also, it should be clear that this is due to windows.

gottesmm · 2017-12-17T22:13:00Z

cmake/modules/SwiftSharedCMakeConfig.cmake

  string(REGEX REPLACE "-Wl,-z,defs" "" CMAKE_SHARED_LINKER_FLAGS "${CMAKE_SHARED_LINKER_FLAGS}")

+  if (LLVM_COMPILER_IS_GCC_COMPATIBLE)


I am assuming this will not be in the final version?

gottesmm · 2017-12-17T22:13:54Z

cmake/modules/AddSwift.cmake

@@ -721,6 +721,7 @@ function(_add_swift_library_single target name)
    swift_windows_generate_sdk_vfs_overlay(SWIFTLIB_SINGLE_VFS_OVERLAY_FLAGS)
    foreach(flag ${SWIFTLIB_SINGLE_VFS_OVERLAY_FLAGS})
      list(APPEND SWIFTLIB_SINGLE_SWIFT_COMPILE_FLAGS -Xcc;${flag})
+      list(APPEND SWIFTLIB_SINGLE_C_COMPILE_FLAGS ${flag})


What is this for?

Windows is case-insensitive, so when you're cross-compiling from a case-sensitive filesystem a number of header paths don't resolve (since the Windows headers are very loose about case and the spellings vary between files).

The VFS overlay is a means to map between the differently-cased forms of the headers (e.g. windows.h to Windows.h). Previously, it wasn't passing the argument to use the VFS overlay to the C/C++ runtime sources correctly (so it was working for Swift import Msvcrt only).

gottesmm · 2017-12-17T22:14:06Z

cmake/modules/AddSwift.cmake

@@ -1151,6 +1152,10 @@ function(_add_swift_library_single target name)
  # import library targets when the library was added.  Use that to adjust the
  # link libraries.
  if("${SWIFTLIB_SINGLE_SDK}" STREQUAL "WINDOWS")
+    # We need to set the C++ standard from C++11 to C++14 on Windows
+    # for compilers using the GCC driver.
+    set_property(TARGET "${target}" PROPERTY CXX_STANDARD 14)


I am assuming this is gone?

gottesmm · 2017-12-17T22:14:53Z

cmake/modules/AddSwift.cmake

@@ -1453,6 +1458,11 @@ function(add_swift_library name)
        continue()
      endif()

+      # TODO Support SwiftPrivate on Windows (SR-6489)


Can you add to the comment here to describe the problem. I think just saying something like this would be fine:

TODO: Currently SwiftPrivate runs into linker problems on Windows. See SR-6489.

Done, thanks. That's definitely clearer.

gottesmm · 2017-12-17T22:15:04Z

cmake/modules/AddSwift.cmake

+       if(NOT "${sdk}" STREQUAL "WINDOWS" AND NOT "${name}" STREQUAL "swiftRemoteMirror")
+          # Add back the flags we removed from the call to HandleLLVMOptions 
+          # in SwiftSharedCMakeConfig.cmake.
+          list(APPEND swiftlib_link_flags_all "-Wl,-z,defs")


I am assuming this goes away after c++14?

This part doesn't. The flag is currently being unconditionally removed so Windows works, (#13209), so this adds it back for non-Windows targets. swiftRemoteMirror was also removing the flag manually, so I consolidated that change here.

troughton · 2017-12-17T22:36:06Z

I've removed all of the C++14 changes; they should be addressed in a separate PR (#13403 if it's Windows only, or something different again if it goes project-wide).

troughton · 2018-01-20T09:57:52Z

This PR is a lot simpler now that the C++14 changes have been separately merged in. Does it look okay to merge? As a quick recap, this PR now:

Re-enables the -Wl,-z,defs flags that are currently disabled as a Windows-specific workaround.
Disables SwiftPrivate on Windows since it fails to link.
Makes the C sources use the VFS overlay on Windows to resolve case-sensitivity issues with filenames.

troughton · 2018-01-30T20:30:01Z

@gottesmm Checking in about this PR - does it look okay to merge?

gottesmm · 2018-01-31T01:46:13Z

cmake/modules/SwiftSharedCMakeConfig.cmake

@@ -81,8 +81,20 @@ macro(swift_common_standalone_build_config_llvm product is_cross_compiling)
  # HACK: this ugly tweaking is to prevent the propagation of the flag from LLVM
  # into swift.  The use of this flag pollutes all targets, and we are not able
  # to remove it on a per-target basis which breaks cross-compilation.
+  # Therefore, we always remove it here and then selectively re-add it to the


This explanation should be added here. Also, it should be clear that this is due to windows.

gottesmm · 2018-01-31T01:54:19Z

cmake/modules/AddSwift.cmake

+       if(NOT "${sdk}" STREQUAL "WINDOWS" AND NOT "${name}" STREQUAL "swiftRemoteMirror")
+          # Add back the flags we removed from the call to HandleLLVMOptions 
+          # in SwiftSharedCMakeConfig.cmake.
+          list(APPEND swiftlib_link_flags_all "-Wl,-z,defs")


I really dislike all of the special cases in this function and additionally how this calls out windows but really you are working with the complement. The technical debt here makes me unhappy.

That being said, I think for now, this is fine.

I certainly agree it's far from an ideal approach, but yes, addressing it would take a larger refactor, and I don't think this is much worse than what surrounds it.

gottesmm · 2018-01-31T21:01:26Z

cmake/modules/SwiftSharedCMakeConfig.cmake

+  # CMAKE_SHARED_LINKER_FLAGS are determined based on the host system and 
+  # then applied to all targets. 
+  # To work around this, we unconditionally remove the flag here and then 
+  # selectively add it to the per-target link flags in AddSwift.cmake.


Nice explanation! It is exactly what I was looking for. My biggest fear in general with cmake changes is that we lose

3 last knits:

Can you fix the formatting on the text so it is 80 columns or proper paragraphs? I.e. I would do this:

# HACK: Windows doesn't support -z,defs as a linker flag. # # Normally, LLVM ...

Can you say the function in AddSwift.cmake where it is (add_swift_library, I think it is?).

I forgot if it is there, but in add_swift_library can you add a similar comment to this that points back at this code in words like you pointed at it in words here.

Erm.

knits => nits.

What I meant to say is that my biggest fear with cmake changes is forgetting why hacks were added. That is why I am asking for so much here as such things have happened in the past.

Does that look okay now? I've been editing on the GitHub web interface so gauging formatting is somewhat difficult; I don't think it exposes column width, for instance. I've also updated the comment in AddSwift.cmake so it's hopefully a little clearer.

gottesmm · 2018-01-31T22:00:03Z

Looks great to me! Can you squash the history? Then lets get this show started = ).

troughton · 2018-01-31T22:43:02Z

History squashed – sorry for the opening/closing PR noise.

gottesmm · 2018-02-01T02:39:34Z

@swift-ci clean test and merge

gottesmm · 2018-02-01T02:39:49Z

@swift-ci test and merge

Disable SwiftPrivate, remove -Wl,-z,defs, and use the VFS overlay for compiling the runtime.

troughton · 2018-02-01T09:13:27Z

Tests passed on Linux but failed compiling for ARM from macOS. Apparently my assumption that this was Windows-only was wrong; it looks like the -z,-defs option should only apply to ELF targets. Sorry; I'm not sure how I missed that.

I've updated the condition to re-add it only for ELF targets and updated the comment accordingly. I'm going to run tests locally on a macOS host to check, but I'm fairly certain that should resolve the issue. One thing I’m not sure about is the LLVM CMake files do exempt e.g. FreeBSD from the flag (although I don’t know why), so perhaps it’s safer to only add the -z,defs flag back for Linux.

davezarzycki · 2018-02-01T12:28:24Z

I don't think that LLVM's lack of -z,defs on FreeBSD says much about FreeBSD other than they have fewer maintainers and users and therefore they probably have some lingering libraries that aren't -z,defs compatible. In other words, if FreeBSD provides a "foo.a" that LLVM links against, and that library isn't -z,defs compatible, then LLVM needs to avoid the flag.

troughton · 2018-02-02T06:31:06Z

In that case, do you think it’s appropriate to include the flag for FreeBSD in Swift, and if so, does the revised patch look otherwise okay? I don’t personally have a FreeBSD system to test on.

davezarzycki · 2018-02-02T13:07:08Z

I don't have an opinion about -z,defs on FreeBSD. I mostly care about the unified build experience, so if LLVM starts building with -z,defs on FreeBSD, I'd hope that the flag survives the Swift CMake files and is used there too.

troughton · 2018-02-02T20:11:34Z

I don’t think that’s possible. You could potentially do something convoluted like caching whether the flag was added by LLVM, checking if the Swift SDK matches the host SDK, and then adding it back in that case, but then it wouldn’t be added for cross-compiling, and it seems fairly error-prone to me.

I think that the cleanest option is to maintain our own list of targets for -z,defs. If that list can be simply “all ELF targets”, then great; otherwise, we can follow LLVM’s approach and just copy their long list of special-case exemptions.

@gottesmm What approach do you want to take? If we go with the 'all ELF targets' it might break some untested targets, but it'd be simple enough for maintainers of those targets to patch the condition.

troughton · 2018-02-07T01:40:41Z

@gottesmm If the changes look okay, could you please trigger CI again? If not, I think it might be worth splitting the -z,defs changes out into a separate PR since they're the only difficult part of this PR.

gottesmm · 2018-02-20T18:41:35Z

@troughton Sorry I dropped this. The right person to review this right now is @Rostepher. Btw, have you considered asking for commit access? Then you wouldn't need to ask people to run CI testing ; ).

davezarzycki · 2018-03-16T16:20:25Z

Adding @Rostepher to the reviewers list based on @gottesmm's last comment.

compnerd · 2018-04-16T21:15:39Z

Is any of this still needed? I have most of the unit tests building and a good chunk of it passing even.

troughton · 2018-04-16T21:20:43Z

It’s still useful, but it’s really not much. At this point, the patch just re-enables a warning on Linux and adds the VFS overlay as a flag to building th C libraries.

compnerd · 2018-04-18T15:33:52Z

Yeah, I think that is useful :-)

compnerd · 2018-04-18T15:33:57Z

@swift-ci please test and merge

troughton changed the title ~~Fixes for cross-compiling the Windows stdlib from Linux~~ Fixes for cross-compiling the Windows stdlib Nov 29, 2017

troughton changed the title ~~Fixes for cross-compiling the Windows stdlib~~ Windows stdlib cross-compilation fixes Nov 29, 2017

troughton changed the title ~~Windows stdlib cross-compilation fixes~~ [CMake] Windows stdlib cross-compilation fixes Nov 30, 2017

troughton mentioned this pull request Dec 1, 2017

build: effectively revert 986beb75225a09969f71123f134967eecef0dcd5 #13209

Merged

compnerd mentioned this pull request Dec 3, 2017

Fix multi-arch cross-compiling #13108

Closed

troughton mentioned this pull request Dec 3, 2017

[driver] Implement a Windows toolchain for cross-compiling #13236

Merged

davezarzycki requested changes Dec 9, 2017

View reviewed changes

davezarzycki approved these changes Dec 10, 2017

View reviewed changes

troughton mentioned this pull request Dec 13, 2017

build: use C++14 for Windows builds #13403

Merged

gottesmm reviewed Dec 17, 2017

View reviewed changes

gottesmm reviewed Jan 31, 2018

View reviewed changes

troughton closed this Jan 31, 2018

troughton deleted the windows-cross-compile-fixes branch January 31, 2018 22:30

troughton restored the windows-cross-compile-fixes branch January 31, 2018 22:31

troughton reopened this Jan 31, 2018

troughton force-pushed the windows-cross-compile-fixes branch from a82c162 to 505c462 Compare January 31, 2018 22:41

Fix cross-compilation for the Windows stdlib

7c2230b

Disable SwiftPrivate, remove -Wl,-z,defs, and use the VFS overlay for compiling the runtime.

troughton force-pushed the windows-cross-compile-fixes branch from 505c462 to 7c2230b Compare February 1, 2018 09:08

SwiftPrivate now builds on Windows with Pull-14710

5173e3c

davezarzycki requested a review from Rostepher March 16, 2018 16:19

Rostepher approved these changes Apr 18, 2018

View reviewed changes

swift-ci merged commit 577b272 into swiftlang:master Apr 18, 2018

		string(REGEX REPLACE "-Wl,-z,defs" "" CMAKE_SHARED_LINKER_FLAGS "${CMAKE_SHARED_LINKER_FLAGS}")

		if (LLVM_COMPILER_IS_GCC_COMPATIBLE)

[CMake] Windows stdlib cross-compilation fixes #13140

[CMake] Windows stdlib cross-compilation fixes #13140

Uh oh!

Conversation

troughton commented Nov 29, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

troughton commented Nov 29, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

troughton commented Nov 30, 2017

Uh oh!

troughton commented Dec 8, 2017

Uh oh!

davezarzycki commented Dec 8, 2017

Uh oh!

davezarzycki left a comment

Choose a reason for hiding this comment

Uh oh!

davezarzycki commented Dec 9, 2017

Uh oh!

troughton commented Dec 9, 2017

Uh oh!

davezarzycki commented Dec 10, 2017

Uh oh!

troughton commented Dec 17, 2017

Uh oh!

gottesmm commented Dec 17, 2017

Uh oh!

gottesmm left a comment

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!

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!

troughton Dec 17, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

troughton commented Dec 17, 2017

Uh oh!

troughton commented Jan 20, 2018

Uh oh!

troughton commented Jan 30, 2018

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!

Choose a reason for hiding this comment

Uh oh!

gottesmm commented Jan 31, 2018

Uh oh!

troughton commented Jan 31, 2018

Uh oh!

gottesmm commented Feb 1, 2018

Uh oh!

troughton commented Nov 29, 2017 •

edited

Loading

troughton commented Nov 29, 2017 •

edited

Loading

troughton Dec 17, 2017 •

edited

Loading

troughton commented Feb 1, 2018 •

edited

Loading

troughton commented Feb 2, 2018 •

edited

Loading