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

[swiftc/msvc] Compiling with MSVC library #1516

Merged
merged 1 commit into from Jul 9, 2016

Conversation

Projects
None yet
8 participants
@tinysun212
Contributor

tinysun212 commented Mar 2, 2016

What's in this pull request?

Some modifications for the ms-extension option of the clang.exe in the Visual Studio 2015 development environment

This patch is only for swiftc.exe. For the standard library or build script, other PR's will be used in later.
I used the library set of Visual Studio 2015 Update 1 and recent version of swift-clang as the compiler. If you are using the real MSVC compiler, more patch might be required.

Related bug number: (SR-34) Port Swift to Windows


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

View changes

include/swift/Basic/Malloc.h Outdated
#include <cstdlib>
#endif
namespace swift {
// FIXME: Use C11 aligned_alloc or Windows _aligned_malloc if available.

This comment has been minimized.

@jckarter

jckarter Mar 2, 2016

Member

You can trim down this fixme a bit now.

@@ -481,6 +481,16 @@ struct ScalarTraits<uint32_t> {
static bool mustQuote(StringRef) { return false; }
};
#if defined(_MSC_VER)
// In MSVC, 'unsigned long' is 32bit size and different from uint32_t,
// and it is used to define swift::sys::ProcessId.

This comment has been minimized.

@jckarter

jckarter Mar 2, 2016

Member

It might be better to change ProcessId to use a stdint typedef.

This comment has been minimized.

@tinysun212

tinysun212 Mar 2, 2016

Contributor

ProcessId in swift defined as typedef llvm::sys::ProcessInfo::ProcessId ProcessId;.
LLVM ProcessInfo coded.

struct ProcessInfo {
#if defined(LLVM_ON_UNIX)
  typedef pid_t ProcessId;
#elif defined(LLVM_ON_WIN32)
  typedef unsigned long ProcessId; // Must match the type of DWORD on Windows.
  .....

Changing ProcessId in swift means changing or ignoring LLVM definition.
I think reusing the LLVM definition is better.

This comment has been minimized.

@jckarter

jckarter Mar 2, 2016

Member

Yeah, you're right. Understandable that LLVM wouldn't want to include all of windows.h just for DWORD.

@jckarter

View changes

lib/IDE/CodeCompletion.cpp Outdated
namespace {
class ExprParentFinder : public ASTWalker {
#if defined(_MSC_VER)
// MSVC needed '::' (http://reviews.llvm.org/D4443)
friend class ::CodeCompletionTypeContextAnalyzer;

This comment has been minimized.

@jckarter

jckarter Mar 2, 2016

Member

Does it work portably if you say friend class ::swift::CodeCompletionTypeContextAnalyzer; ?

This comment has been minimized.

@tinysun212

tinysun212 Mar 2, 2016

Contributor

It reports error : no class named 'CodeCompletionTypeContextAnalyzer' in namespace 'swift'.
CodeCompletion.cpp is not surrounded by namespace swift { }.

This comment has been minimized.

@jckarter

jckarter Mar 2, 2016

Member

It should probably be inside the anonymous namespace, then.

@gribozavr

View changes

lib/ClangImporter/ClangImporter.cpp Outdated
@@ -2039,7 +2039,9 @@ auto ClangImporter::Implementation::importFullName(
if (!shouldImportAsInitializer(method, initPrefixLength,
result.InitKind)) {
// We cannot import this as an initializer anyway.
return { };
// Please don't use 'return { }' instead of this. MSVC compiler
// reports 'implicit initialization of field ErrorInfo'

This comment has been minimized.

@gribozavr

gribozavr Mar 2, 2016

Collaborator

Please remove the comment, we don't leave such things in the source.

@gribozavr

This comment has been minimized.

Collaborator

gribozavr commented Mar 2, 2016

Some modifications for the ms-compatibility mode of the clang-cl.exe in the Visual Studio 2015 development environment

Why is that interesting to support? If you can use clang, why not use clang in its native mode?

@tinysun212

This comment has been minimized.

Contributor

tinysun212 commented Mar 2, 2016

For build and link with MS C runtime library, including their headers are necessary. Without ms-compatibility mode, they are cannot be compiled.

@jckarter

This comment has been minimized.

Member

jckarter commented Mar 2, 2016

Could we use native Clang mode for swiftc, but MS compatibility mode for the runtime?

@jckarter

This comment has been minimized.

Member

jckarter commented Mar 2, 2016

Or could we avoid needing to parse Visual C++'s libraries by using libc++? Do pure C windows.h-level headers really need MSVC compatibility?

@tinysun212

This comment has been minimized.

Contributor

tinysun212 commented Mar 3, 2016

I'm confused about options in comments.
I enabled two options -fms-extension, -fms-compatibility. The former required for understanding MS includes (even for stdio.h). I'll check if any issue occurs when the latter option is removed.

@jckarter

This comment has been minimized.

Member

jckarter commented Mar 3, 2016

Sounds good. AIUI -fms-extension should be fine, since that enables purely additive MSVC features, whereas -fms-compatibility causes Clang to deliberately emulate MSVC bugs. We'd prefer not to have to deal with the latter.

@tinysun212 tinysun212 force-pushed the tinysun212:pr-swiftc-msvc-1 branch Mar 3, 2016

@tinysun212

This comment has been minimized.

Contributor

tinysun212 commented Mar 3, 2016

After remove -fms-compatibility option, some files can be removed from PR. Also, I modified some comments and rebranched.

@jckarter

View changes

lib/Immediate/Immediate.cpp Outdated
#if defined(_MSC_VER)
#include "Windows.h"
#define dlopen(x, y) LoadLibrary(x)

This comment has been minimized.

@jckarter

jckarter Mar 4, 2016

Member

Instead of a macro, can we use a wrapper function with a conditional implementation?

@jckarter

View changes

lib/ClangImporter/ClangImporter.cpp Outdated
@@ -2039,7 +2039,7 @@ auto ClangImporter::Implementation::importFullName(
if (!shouldImportAsInitializer(method, initPrefixLength,
result.InitKind)) {
// We cannot import this as an initializer anyway.
return { };
return ImportedName();

This comment has been minimized.

@jckarter

jckarter Mar 4, 2016

Member

Is this change still necessary?

This comment has been minimized.

@tinysun212

tinysun212 Mar 4, 2016

Contributor

Yes. It is probably related to the template library.

@jckarter

View changes

lib/Sema/CSDiag.cpp Outdated
void filterList(CallArgParam actualArgs) {
return filterList(ArrayRef<CallArgParam>(actualArgs));
}
#endif

This comment has been minimized.

@jckarter

jckarter Mar 4, 2016

Member

Is this change necessary without -fms-compatibility?

This comment has been minimized.

@tinysun212

tinysun212 Mar 4, 2016

Contributor

Yes, it is.

This comment has been minimized.

@lattner

lattner Mar 4, 2016

Collaborator

Please don't add #ifdefs for this. If there is a problem resolving the overload, just rename the methods to be distinct, and update the callers.

@jckarter

View changes

lib/SILOptimizer/Mandatory/ConstantPropagation.cpp Outdated
});
#endif

This comment has been minimized.

@jckarter

jckarter Mar 4, 2016

Member

Is this change necessary without -fms-compatibility?

This comment has been minimized.

@tinysun212

tinysun212 Mar 4, 2016

Contributor

Yes, it also couldn't be removed.

@tinysun212 tinysun212 changed the title from [swiftc/msvc] Compiling with MSVC to [swiftc/msvc] Compiling with MSVC library Mar 4, 2016

@tinysun212

This comment has been minimized.

Contributor

tinysun212 commented Mar 6, 2016

After removing -fms-compatablitity-version=19, return { }; in ClangImporter.cpp is revoked. I think the option is not ignored even if -fno-ms-compatibility was used.

But the patch which the [](){} are required in ConstantPropagation.cpp could not be revoked by removing the option. I think it is related to the <functional> header implemention.

Two or more default parameters of functional in constructor were not resolved. Removing ifdefs in ConstantPropagation.cpp, I changed one constructor of three default parameters in Local.h to two constructors of one default parameter.

@tinysun212

This comment has been minimized.

Contributor

tinysun212 commented Mar 7, 2016

For the patch of ConstantPropagation.cpp, I'll check more if it can be revoked.

@tinysun212

This comment has been minimized.

Contributor

tinysun212 commented Mar 7, 2016

The error of return { }; in ClangImporter.cpp was not for the bug emulation in ms-compatibility. I think it was the difference between c++11 and c++14. I wrote following simple code and tested it.

tt.cpp
------

enum class MyType { SampleValue };

class Opt {
public:
    explicit Opt() {}
};

struct Astr {
    MyType  myt = MyType::SampleValue;
    Opt  opt;
};

Astr Foo2() {
    return{};
}
$ clang++ -c  tt.cpp  -std=c++11   -->  NO ERROR
$ clang++ -c  tt.cpp  -std=c++14
tt.cpp:15:9: error: chosen constructor is explicit in copy-initialization
        return{};
               ^
tt.cpp:6:11: note: constructor declared here
        explicit Opt() {}
                 ^
tt.cpp:11:7: note: in implicit initialization of field 'opt' with omitted initializer
        Opt  opt;
             ^
1 error generated.

By verifying clang -v, when the -fms-compatablitity-version=19 option is used, clang driver inserted -std=c++14 option to the fronted.

Anyway, we will not use the compatibility option, no problem.

@tinysun212

This comment has been minimized.

Contributor

tinysun212 commented Mar 7, 2016

For the patch of ConstantPropagation.cpp, it can be revoked, but has a little problem.

I have two clang version for Windows.
One is the clang.exe I built from swift-clang repository, and I made this PR with it.
Other is the clang 3.7 with MS CodeGen which released by MS last year.

MS clang and MS cl compiler recognize well two or more functional default parameter in constructor.
But my clang was not recognize it. I think MS clang has some different code not yet applied to swift-clang.

I cannot use MS clang for ConstantPropagation.cpp now, it generates a fatal error at other position instead. I want to keep this patch.

@gottesmm

This comment has been minimized.

Member

gottesmm commented Mar 7, 2016

@tinysun212 Can you smoosh together the patches?

@gribozavr

This comment has been minimized.

Collaborator

gribozavr commented Mar 7, 2016

Do we really need to make any changes to the compiler? Why can't we use clang.exe to build the compiler, and clang-cl.exe to build the runtime?

@tinysun212 tinysun212 force-pushed the tinysun212:pr-swiftc-msvc-1 branch to 25e2b92 Mar 8, 2016

@stefanstefan2001

This comment has been minimized.

stefanstefan2001 commented Apr 23, 2016

Ping?

@tinysun212

This comment has been minimized.

Contributor

tinysun212 commented Apr 26, 2016

@jxyang has more changes than me in his repository.

@tinysun212

This comment has been minimized.

Contributor

tinysun212 commented May 9, 2016

@gribozavr, The difference between clang.exe and clang-cl.exe is the driver options for clang frontend. Anyway, this PR is suitable to clang.exe with the option -fms-extension and not use'-fms-compatibility. In clang-cl.exe, both of them are enabled.

To build the compiler swift.exe on Windows (MSVC, not Cygwin), we should use the MS standard C++ library and headers. We can not drop the option -fms-extension because the library can't be used without the option.

Unfortunately, the option -fms-extension caused some code changes, but this is the minimum patch to build the compiler on Windows.

@tinysun212

This comment has been minimized.

Contributor

tinysun212 commented May 16, 2016

ping?

@tinysun212 tinysun212 referenced this pull request Jun 17, 2016

Merged

Basic: support `_aligned_malloc` on Windows #3054

0 of 1 task complete
@CodaFi

This comment has been minimized.

Collaborator

CodaFi commented Jul 6, 2016

@tinysun212 It looks like you have merge conflicts. Could you please rebase onto master.

@@ -23,7 +23,7 @@ using namespace swift;
using namespace swift::sys;
// Include the correct TaskQueue implementation.
#if LLVM_ON_UNIX && !defined(__CYGWIN__)
#if LLVM_ON_UNIX && !defined(__CYGWIN__) && !defined(_MSC_VER)

This comment has been minimized.

@jrose-apple

jrose-apple Jul 6, 2016

Member

Why is LLVM_ON_UNIX even set for MSVC?

for (auto &envPair : Cmd->getExtraEnvironment())
for (auto &envPair : Cmd->getExtraEnvironment()) {
#if defined(_MSC_VER)
std::string envStr = envPair.first;

This comment has been minimized.

@jrose-apple

jrose-apple Jul 6, 2016

Member

Nitpick: please use an llvm::SmallString instead of std::string, so that smaller settings don't require an extra heap allocation.

@tinysun212 tinysun212 force-pushed the tinysun212:pr-swiftc-msvc-1 branch from 25e2b92 Jul 7, 2016

@tinysun212

This comment has been minimized.

Contributor

tinysun212 commented Jul 7, 2016

@CodaFi, @jrose-apple, thanks for your reviews, it is rebased and changed.

@@ -485,14 +485,18 @@ class CastOptimizer {
public:
CastOptimizer(std::function<void (SILInstruction *I, ValueBase *V)> ReplaceInstUsesAction,

This comment has been minimized.

@jrose-apple

jrose-apple Jul 7, 2016

Member

Can I ask why this change was necessary?

This comment has been minimized.

@tinysun212

tinysun212 Jul 7, 2016

Contributor

Using the clang with the ms-extension option (not ms-compatibility option), the line of the constructor having two default value of dummy lambda expression is compiled, but three default value constructor is not compiled.

This comment has been minimized.

@jrose-apple

jrose-apple Jul 7, 2016

Member

That's very strange. Mind leaving a comment before I merge?

@jrose-apple

This comment has been minimized.

Member

jrose-apple commented Jul 7, 2016

Looks reasonable to me. Thanks, @tinysun212!

@swift-ci Please test

[swiftc/msvc] Compiling with MSVC
Some modifications for the ms-extension option of the clang.exe in the Visual Studio 2015 development environment

This patch is only for swiftc.exe. I used the library set of Visual Studio 2015 Update 1 and recent version of swift-clang as the compiler. If you are using the real MSVC compiler, more patch might be required.

@tinysun212 tinysun212 force-pushed the tinysun212:pr-swiftc-msvc-1 branch to 32ae84c Jul 8, 2016

@tinysun212

This comment has been minimized.

Contributor

tinysun212 commented Jul 8, 2016

@jrose-apple, I applied your reviews.

@jrose-apple jrose-apple merged commit cccfbf4 into apple:master Jul 9, 2016

@jrose-apple

This comment has been minimized.

Member

jrose-apple commented Jul 9, 2016

Thanks, merged!

@tinysun212

This comment has been minimized.

Contributor

tinysun212 commented Jul 9, 2016

Thanks, this merging I have long awaited. ^^

@tinysun212 tinysun212 deleted the tinysun212:pr-swiftc-msvc-1 branch Oct 21, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment