Skip to content

[WIP] Implement WindowsToolchain #252

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

Closed
wants to merge 11 commits into from

Conversation

stevapple
Copy link
Contributor

Implemented the Windows-specific toolchain.

@stevapple
Copy link
Contributor Author

CC @compnerd , I hope to get SPM fully usable on Windows through swift-driver.

@@ -258,3 +279,5 @@ extension DarwinToolchain {
}

}

// TODO: See whether Windows needs `addArgsToLinkStdlib`.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it will definitely needs arguments for that. At the very least, you need to ensure that the ucrt link matches the stdlib.

@stevapple
Copy link
Contributor Author

stevapple commented Sep 14, 2020

I’m working on the linker part, and now I’m troubled by the following two question:

  • Can we use things like link.exe and lib.exe in Cygwin and MingW? That is, is link.exe and lib.exe available to everyone using Swift for Windows?
  • Do we also have llvm-lib.exe and lldb-link.exe in Cygwin and MingW toolchains?(we don’t need this)

It’s fairly easy to configure the MSVC linkers and their LLVM drop-in substitutes (I think it can be done today), but what about not using msvc here?

@stevapple
Copy link
Contributor Author

stevapple commented Sep 14, 2020

Another question that I think only @compnerd is capable of answering... Are we going to include clangLibrary (Currently only clangHeaders are included) in Windows Toolchain? If the answer is “yes”, then things will be much easier because we don’t need to search for clang libraries in the whole system.

The dedicated place for it is %TOOLCHAIN_ROOT%/usr/lib/clang/11.0.0/lib/windows, and %TOOLCHAIN_ROOT%/usr/lib/clang/11.0.0 should be symlinked into %TOOLCHAIN_ROOT%/usr/lib/swift/clang.

@stevapple stevapple changed the title Implement WindowsToolchain [WIP] Implement WindowsToolchain Sep 14, 2020
@compnerd
Copy link
Member

The MinGW path should not use the WindowsToolchain but rather the GNUToolchain much Like the model in clang. MinGW is substantially different in ABI that you cannot mix and match the behavior. In fact there's a large number of other things that won't work - like linking because MinGW would require auto link extract that the Windows path does not use as the MINGW doesn't support linker directives. The other thing is that binutils isn't supported by swift because of issues with relocation handling. In general, MinGW isn't ready yet, and would require a lot more than just tweaking the driver.

No, there's no reason to include clangrt, that isn't used on windows.

@stevapple
Copy link
Contributor Author

clang_rt is required for sanitizer support.

    // Check that we're one of the known supported targets for sanitizers.
    if !(targetTriple.isWindows || targetTriple.isDarwin || targetTriple.os == .linux)

I’m seeing this line of code. It should mean that we are going to provide sanitizer support on Windows?

@stevapple
Copy link
Contributor Author

stevapple commented Sep 16, 2020

Finally gets a solution for sanitizer support. I’m going to filter out any sanitizer other than ASAN. This means if anyone builds his copy with clang_rt, he’ll be able to use ASAN. Other sanitizers will be blocked even if their libraries exist.

What do you think? @compnerd @owenv

Copy link
Contributor

@owenv owenv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for updating this! It looks pretty good to me, but I'm not very familiar with Windows so I wasn't able to review it thoroughly.

It would be really great if we could include some basic tests alongside this change as well, at least for the platform-specific linking. I think a lot of the windows-related integration tests from the main Swift repo will continue to fail until MSVC runtime handling is added, I haven't looked into how complicated that is. Maybe a couple cases could be added to testLinking in SwiftDriverTests.swift that use the windows target?

@stevapple
Copy link
Contributor Author

It would be really great if we could include some basic tests alongside this change as well, at least for the platform-specific linking. I think a lot of the windows-related integration tests from the main Swift repo will continue to fail until MSVC runtime handling is added, I haven't looked into how complicated that is. Maybe a couple cases could be added to testLinking in SwiftDriverTests.swift that use the windows target?

Of course I will. However, there is still an unimplemented function (see the only unresolved review😂) which should be implemented before implementing tests.

@stevapple
Copy link
Contributor Author

Shall we use MD or MT as default for Windows? @compnerd

@compnerd
Copy link
Member

I don't think that we should have a default for the value. It must match the value for the runtime that you are building against. I suppose that one possibility is to query the SDKSettings.plist for the correct value like I have done in swift-package-manager. But, really, I think that we should have the user specify the correct value.

@stevapple
Copy link
Contributor Author

stevapple commented Sep 18, 2020

I‘m thinking it necessary. It’s one of our goals to allow user run swiftc without any additional options, which means we should be able to infer every parameter we need from the context.

We can have multiple ways to infer though. plist is one of them, but maybe we also need a hard-coded default.

@compnerd
Copy link
Member

If you are adamant that you must have a default, then you must serialize the compile options into the Swift.swiftmodule and then deserialize it to find the default at runtime. Otherwise, whatever default you choose is the wrong one - my runtime will always be the inverse of whatever the default is, and therefore you have just built a binary that will arbitrarily crash.

@stevapple
Copy link
Contributor Author

I‘d like to know if SDKSettings.plist is built alongside with Windows SDK. If it is (which means every copy of Windows SDK contains SDKSettings.plist), we won’t need an extra default. That would be great and simple.

@compnerd
Copy link
Member

It is not, it is only something that is built on my builds of the toolchain; anyone building the toolchain manually will have to deal with that themselves.

@shahmishal shahmishal closed this Oct 5, 2020
@shahmishal
Copy link
Member

The Swift project moved the default branch to main and deleted master branch, so GitHub automatically closed the PR. Please re-create the pull request with main branch.

More detail about the branch update - https://forums.swift.org/t/updating-branch-names/40412

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants