[swiftc] Fixed for Cygwin #3841

tinysun212 · 2016-07-28T23:23:03Z

What's in this pull request?

Resolved bug number: (SR-)

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
All supported platforms	@swift-ci Please smoke test and merge
OS X platform	@swift-ci Please smoke test OS X platform
Linux platform	@swift-ci Please smoke test Linux platform

A smoke test on macOS does the following:

Builds the compiler incrementally.
Builds the standard library only for macOS. Simulator standard libraries and
device standard libraries are not built.
lldb is not built.
The test and validation-test targets are run only for macOS. The optimized
version of these tests are not run.

A smoke test on Linux does the following:

Builds the compiler incrementally.
Builds the standard library incrementally.
lldb is built incrementally.
The swift test and validation-test targets are run. The optimized version of these
tests are not run.
lldb is tested.

Validation Testing

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

Lint Testing

Language	Comment
Python	@swift-ci Please Python lint

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

Fixed for the difference of Cygwin with other Windows variants (MSVC,
Itanium, MinGW).

The platform name is renamed to "cygwin" from "windows" which is used
for searching the standard libraries.
The consideration for DLL storage class (DllExport/DllImport) is not
required for Cygwin and MinGW. There is no problem when linking in
these environment.
Cygwin should use large memory model as default.(This may be changed
if someone ports to 32bit)
Cygwin and MinGW should use the autolink feature in the sameway of
Linux due to the linker's limit.

gribozavr · 2016-07-28T23:26:01Z

tools/driver/CMakeLists.txt

@@ -38,6 +38,11 @@ if(SWIFT_HOST_VARIANT MATCHES "${SWIFT_DARWIN_VARIANTS}")
      LINK_FLAGS " -lobjc -Wl,-force_load,\"${SWIFT_REPL_ARCLITE}\"")
 endif()

+if("${CMAKE_SYSTEM_NAME}" STREQUAL "CYGWIN")
+  set_property(TARGET swift APPEND_STRING PROPERTY
+      LINK_FLAGS " -Wl,--allow-multiple-definition")


Why is this change necessary?

Linker generates the error of multiple definition of guard variable for swift::ImmutablePointerSetFactory<swift::SILInstruction>::EmptyPtrSet in libswiftSILOptimizer.a.
And the EmptyPtrSet is defined at include/swift/Basic/ImmutablePointerSet.h.

The symbol is defined in the object files compiled from all lib/SILOptimizer/ARC/*.cpp (10 files). I think it is related to 'template instantiation'. But I don't know whether those sources are good or bad.

I added this flag instead of touching sources.

The GNU ld linker has a very poor approximation of COFF support. It fails constantly under a myriad of conditions, including COMDAT symbols. The --allow-multiple-definitions is attempting to compensate for a poor implementation of a COFF linker. To make things worse, this is something which is hardcoded into the gcc specs IIRC, and has even partially made its way into clang :-(. I for one welcome our new LLVM overlords^W^W^W^Wlld.

I'll remove this part. Updating clang to 3.8.1 with recent released cygwin package, the link flag can be removed.

With some test code, I found the Cygwin clang compiler 3.7.1 generates wrong section type for the data section which has a guard variable for template-static-member. That leads to linker's multiple definition message.

jrose-apple · 2016-07-29T15:40:53Z

@compnerd Mind looking this over as well?

compnerd · 2016-07-30T00:29:46Z

lib/Basic/Platform.cpp

+
+    if (triple.isWindowsGNUEnvironment())
+      return "mingw";
+


I think it would be nicer to use a switch here instead. That way if we run into similar issues in the future, its easier to extend.

Did you mean a nested switch in case llvm::Triple::Win32: ?

switch (triple.getEnvironment()) { case llvm::Triple::Cygnus: return "cygwin"; case llvm::Triple::GNU: return "mingw": default: return "windows"; }

I can not imagine how cover those with single switch.

Yes, a nested switch. You should be able to do a covered switch there, and have most of the environments be unreachable.

tinysun212 · 2016-08-25T09:31:04Z

ping?

compnerd · 2016-08-25T14:40:40Z

lib/IRGen/TypeLayoutVerifier.cpp

@@ -43,7 +43,7 @@ irgen::emitTypeLayoutVerifier(IRGenFunction &IGF,
                                              /*var arg*/ false);
  auto verifierFn = IGF.IGM.Module.getOrInsertFunction(
      "_swift_debug_verifyTypeLayoutAttribute", verifierFnTy);
-  if (IGF.IGM.Triple.isOSBinFormatCOFF())
+  if (IGF.IGM.Triple.isOSBinFormatCOFF() && !IGF.IGM.Triple.isOSCygMing())


Why not use IGF.IGM.useDLLStorage()?

I missed it. Updated it just.

tinysun212 · 2016-10-13T06:06:56Z

Any more reviews ?

slavapestov · 2017-01-13T03:08:56Z

Looks like this needs rebase and conflict resolution too

tinysun212 · 2017-01-17T16:23:05Z

@slavapestov I rebased and resolved conflicts this PR.

jrose-apple · 2017-01-17T17:26:48Z

This was mostly waiting for @compnerd's sign-off, I think.

compnerd · 2017-01-18T02:23:52Z

lib/IRGen/IRGenModule.cpp

@@ -440,6 +440,7 @@ llvm::Constant *swift::getRuntimeFn(llvm::Module &Module,
    fn->setCallingConv(cc);

    if (llvm::Triple(Module.getTargetTriple()).isOSBinFormatCOFF() &&


I think that this should use useDllStorage() instead of adding the negation check so that this is more similar to the previous case.

compnerd · 2017-01-18T02:24:00Z

lib/IRGen/IRGenModule.cpp

@@ -519,7 +520,8 @@ llvm::Constant *swift::getWrapperFn(llvm::Module &Module,
    auto *globalFnPtr = new llvm::GlobalVariable(
        Module, fnPtrTy, false, llvm::GlobalValue::ExternalLinkage, nullptr,
        symbol);
-    if (llvm::Triple(Module.getTargetTriple()).isOSBinFormatCOFF())
+    if (llvm::Triple(Module.getTargetTriple()).isOSBinFormatCOFF() &&
+        !llvm::Triple(Module.getTargetTriple()).isOSCygMing())


Thanks, I'll change them.

compnerd · 2017-01-18T02:25:05Z

lib/IRGen/IRGenModule.cpp

-      TargetInfo.OutputObjectFormat == llvm::Triple::MachO ||
-      Triple.isPS4()) {
+  if ((TargetInfo.OutputObjectFormat == llvm::Triple::COFF &&
+       (!Triple.isOSCygMing() || Triple.getArch() == llvm::Triple::thumb)) ||


Why the special case for cygwin on Windows ARM here?

Test script - https://github.com/apple/swift/blob/master/test/IRGen/autolink-coff.swift - asserts the target thumbv7--windows-gnu to use the MSVC-style autolink, which uses .drectve section.
I created the autolink-coff-x86.swift for MinGW (x86--windows-gnu) to test the Linux-style autolink.
If we drop/modify the autolink test case for thumbv7--windows-gnu , the code will be simpler.

Lets change the test for that target. Really, the only Windows ARM target that I know of which is tested/works is Windows itanium (with clang) or msvc (with msvc).

Fixed for the difference of Cygwin with other Windows variants (MSVC, Itanium, MinGW). - The platform name is renamed to "cygwin" from "windows" which is used for searching the standard libraries. - The consideration for DLL storage class (DllExport/DllImport) is not required for Cygwin and MinGW. There is no problem when linking in these environment. - Cygwin should use large memory model as default.(This may be changed if someone ports to 32bit) - Cygwin and MinGW should use the autolink feature in the sameway of Linux due to the linker's limit.

tinysun212 · 2017-01-18T21:19:09Z

@compnerd The test script autolink-coff.swift was changed and other reviews were applied.

slavapestov · 2017-01-18T23:55:08Z

@swift-ci Please smoke test

slavapestov · 2017-01-18T23:55:48Z

Ok, I'm merging this as soon as @cmpnerd gives the go-ahead (or does @compnerd already have commit access)?

slavapestov · 2017-01-20T00:55:34Z

@swift-ci Please smoke test

compnerd · 2017-01-20T04:17:01Z

@slavapestov yeah, I do have commit access :-).

gribozavr reviewed Jul 28, 2016
View reviewed changes

compnerd reviewed Jul 30, 2016
View reviewed changes

tinysun212 force-pushed the pr-swiftc-cygwin-2 branch from 32d22d6 to d3b5697 Compare July 30, 2016 07:56

tinysun212 mentioned this pull request Jul 30, 2016

[Autolink] Autolinking on COFF for Cygwin/MinGW #3887

Merged

1 task

tinysun212 force-pushed the pr-swiftc-cygwin-2 branch from d3b5697 to 5f25625 Compare August 1, 2016 21:20

tinysun212 changed the title ~~[swiftc/driver] Fixes for Cygwin~~ [swiftc] Fixed for Cygwin Aug 2, 2016

tinysun212 force-pushed the pr-swiftc-cygwin-2 branch 2 times, most recently from 8d605a8 to 1fb9249 Compare August 14, 2016 23:44

compnerd reviewed Aug 25, 2016
View reviewed changes

tinysun212 force-pushed the pr-swiftc-cygwin-2 branch from ac75188 to ba7e600 Compare October 8, 2016 03:19

slavapestov self-assigned this Jan 13, 2017

tinysun212 force-pushed the pr-swiftc-cygwin-2 branch from ba7e600 to 3cdf40b Compare January 17, 2017 16:20

compnerd reviewed Jan 18, 2017

View reviewed changes

tinysun212 force-pushed the pr-swiftc-cygwin-2 branch from 3cdf40b to b8dd577 Compare January 18, 2017 20:48

slavapestov merged commit 7b5cf4a into swiftlang:master Jan 20, 2017

hughbe mentioned this pull request Feb 15, 2017

Change the generated code model to Large on Windows 64 bit #7489

Merged

tinysun212 deleted the pr-swiftc-cygwin-2 branch October 20, 2018 23:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[swiftc] Fixed for Cygwin #3841

[swiftc] Fixed for Cygwin #3841

tinysun212 commented Jul 28, 2016

gribozavr Jul 28, 2016

tinysun212 Jul 29, 2016

compnerd Jul 29, 2016

tinysun212 Aug 1, 2016

jrose-apple commented Jul 29, 2016

compnerd Jul 30, 2016

tinysun212 Jul 30, 2016

compnerd Jul 30, 2016

tinysun212 commented Aug 25, 2016

compnerd Aug 25, 2016

tinysun212 Sep 11, 2016

tinysun212 commented Oct 13, 2016

slavapestov commented Jan 13, 2017

tinysun212 commented Jan 17, 2017

jrose-apple commented Jan 17, 2017

compnerd Jan 18, 2017

compnerd Jan 18, 2017

tinysun212 Jan 18, 2017

compnerd Jan 18, 2017

tinysun212 Jan 18, 2017

compnerd Jan 18, 2017

tinysun212 commented Jan 18, 2017 •

edited

Loading

slavapestov commented Jan 18, 2017

slavapestov commented Jan 18, 2017

slavapestov commented Jan 20, 2017

compnerd commented Jan 20, 2017

		@@ -440,6 +440,7 @@ llvm::Constant *swift::getRuntimeFn(llvm::Module &Module,
		fn->setCallingConv(cc);

		if (llvm::Triple(Module.getTargetTriple()).isOSBinFormatCOFF() &&

[swiftc] Fixed for Cygwin #3841

[swiftc] Fixed for Cygwin #3841

Conversation

tinysun212 commented Jul 28, 2016

What's in this pull request?

Resolved bug number: (SR-)

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jrose-apple commented Jul 29, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tinysun212 commented Aug 25, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tinysun212 commented Oct 13, 2016

slavapestov commented Jan 13, 2017

tinysun212 commented Jan 17, 2017

jrose-apple commented Jan 17, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tinysun212 commented Jan 18, 2017 • edited Loading

slavapestov commented Jan 18, 2017

slavapestov commented Jan 18, 2017

slavapestov commented Jan 20, 2017

compnerd commented Jan 20, 2017

tinysun212 commented Jan 18, 2017 •

edited

Loading