Support building swift/AST with MSVC on Windows #5948

hughbe · 2016-11-28T16:58:36Z

Relies on apple/swift-llvm#33 but can be merged before

hughbe · 2016-12-09T19:55:07Z

lib/AST/ASTMangler.cpp

@eeckstein PTAL, this changes your recently committed code - I think this was a case of Clang allowing something that perhaps isn't spec-ed C++?

Yeah, this just looks like a mistake.

yes, it was

hughbe · 2016-12-09T19:57:10Z

include/swift/AST/Module.h

This fixes an MSVC error: cannot instantiate abstract class FileUnit. I'm not sure how correct this is (@jrose-apple you seem to know a lot of these sorts of things)

Definitely not correct, except coincidentally. Also confused; this operator occurs within the definition of FileUnit, so I would expect it to be able to calculate the alignment.

…well, okay, maybe not, that might require two passes. *goes to check C++ standard*

Okay, this actually does look correct—"When alignof is applied to a reference type, the result shall be the alignment of the referenced type." (C++11 [expr.alignof]p3)—but if MSVC is accepting one and not the other I'm a little concerned that it's doing the wrong thing. Can you test that it accepts this code?

struct Foo { bool b; static_assert(alignof(Foo&) == alignof(bool), "aligning reference rather than Foo"); };

Specific citation allowing this (which says nothing to whether MSVC implements it): "Within the class member-specification, the class is regarded as complete within function bodies, default arguments, and brace-or-equal-initializers for non-static data members (including such things in nested classes)." (C++11 [class.mem]p2)

The following modification of your example works:

struct Foo { bool b; }; static_assert(alignof(Foo&) == alignof(bool), "aligning reference rather than Foo");

Hmm, something like this:

class FileUnit { public: // Only allow allocation of FileUnits using the allocator in ASTContext // or by doing a placement new. void *operator new(size_t Bytes, ASTContext &C, unsigned Alignment = alignOfFileUnit()); }; static inline unsigned alignOfFileUnit() { return alignof(FileUnit); }

doesn't work due to same error:

1>C:\Users\Lukey\Documents\GitHub\my-swift\swift\include\swift/AST/Module.h(757): error C2259: 'swift::FileUnit': cannot instantiate abstract class (compiling source file C:\Users\Lukey\Documents\GitHub\my-swift\swift\lib\AST\ASTPrinter.cpp)
1> C:\Users\Lukey\Documents\GitHub\my-swift\swift\include\swift/AST/Module.h(757): note: due to following members: (compiling source file C:\Users\Lukey\Documents\GitHub\my-swift\swift\lib\AST\ASTPrinter.cpp)
1> C:\Users\Lukey\Documents\GitHub\my-swift\swift\include\swift/AST/Module.h(757): note: 'void swift::FileUnit::lookupValue(swift::ModuleDecl::AccessPathTy,swift::DeclName,swift::NLKind,llvm::SmallVectorImpl<swift::ValueDecl *> &) const': is abstract (compiling source file C:\Users\Lukey\Documents\GitHub\my-swift\swift\lib\AST\ASTPrinter.cpp)
1> C:\Users\Lukey\Documents\GitHub\my-swift\swift\include\swift/AST/Module.h(556): note: see declaration of 'swift::FileUnit::lookupValue' (compiling source file C:\Users\Lukey\Documents\GitHub\my-swift\swift\lib\AST\ASTPrinter.cpp)
1> C:\Users\Lukey\Documents\GitHub\my-swift\swift\include\swift/AST/Module.h(757): note: 'void swift::FileUnit::lookupObjCMethods(swift::ObjCSelector,llvm::SmallVectorImpl<swift::AbstractFunctionDecl *> &) const': is abstract (compiling source file C:\Users\Lukey\Documents\GitHub\my-swift\swift\lib\AST\ASTPrinter.cpp)
1> C:\Users\Lukey\Documents\GitHub\my-swift\swift\include\swift/AST/Module.h(588): note: see declaration of 'swift::FileUnit::lookupObjCMethods' (compiling source file C:\Users\Lukey\Documents\GitHub\my-swift\swift\lib\AST\ASTPrinter.cpp)
1> C:\Users\Lukey\Documents\GitHub\my-swift\swift\include\swift/AST/Module.h(757): note: 'swift::Identifier swift::FileUnit::getDiscriminatorForPrivateValue(const swift::ValueDecl *) const': is abstract (compiling source file C:\Users\Lukey\Documents\GitHub\my-swift\swift\lib\AST\ASTPrinter.cpp)
1> C:\Users\Lukey\Documents\GitHub\my-swift\swift\include\swift/AST/Module.h(631): note: see declaration of 'swift::FileUnit::getDiscriminatorForPrivateValue' (compiling source file C:\Users\Lukey\Documents\GitHub\my-swift\swift\lib\AST\ASTPrinter.cpp)

Ah, that's a much more enlightening error. I forgot about the virtual function.

Does the reference do the right thing now?

class Test { alignas(16) double x; public: virtual void foo() = 0; }; static_assert(alignof(Test&) == 16, "");

(Of course, maybe MSVC doesn't support overalignment, in which case the test won't work again.)

This console application compiles and runs just fine

// ConsoleApplication2.cpp : Defines the entry point for the console application. // #include "stdafx.h" #include <string> #include <io.h> class Test { alignas(16) double x; public: virtual void foo() = 0; }; static_assert(alignof(Test&) == 16, ""); int main() { }

Okay, then in that case we can use alignof(FileUnit&) in alignOfFileUnit and we should get the correct answer.

(I think this is actually important because DeclContexts are deliberately overaligned, and we rely on that to cram things in the low bits.)

https://connect.microsoft.com/VisualStudio/feedback/details/3116815

jrose-apple · 2016-12-09T21:18:25Z

include/swift/AST/AnyFunctionRef.h

Clang still has unknown pragma warnings on, so this should be guarded by a check for MSVC.

ah I assumed if this worked with clang-cl it would work with Clang, but I was wrong - fixed

jrose-apple · 2016-12-09T21:18:48Z

include/swift/AST/Decl.h

What does MSVC consider a reference that Clang doesn't?

I get the following error

error C2280: 'void *swift::AST::ModuleDecl::__delDtor(unsigned int)': attempting to reference a deleted function

The problem seems to be that MSVC generates this __delDtor intrinsic that I don't seem to have control over.

Take a look at this link: https://msdn.microsoft.com/en-us/library/mt723604.aspx
There seems to be some mention of this error, but I'm not sure how applicable this is, so I'd appreciate your expertise

Ah, maybe you would then have to delete operator delete in all the subclasses too.

It's possible marking it virtual would work?

Ah, maybe you would then have to delete operator delete in all the subclasses too.

This doesn't work - it just changes the error message from

void swift::Decl::operator delete(void *)': attempting to reference a deleted function

to

void swift::ModuleDecl::operator delete(void *)': attempting to reference a deleted function

Making it virtual causes the error

'delete' cannot be a virtual function

All right, oh well. We can do the workaround.

This looks like an MSVC bug. I presume the deleting destructor is part of its ABI, but that entry point seems like it ought to be transitively delete-d if operator delete is deleted.

I can believe it's an MSVC bug, but I don't mind us working around it for now.

I've submitted a connect bug for this, at https://connect.microsoft.com/VisualStudio/feedback/details/3116505

jrose-apple · 2016-12-09T21:19:25Z

include/swift/AST/Expr.h

This seems really brittle. I still don't like it, but I don't have an alternative to play with right now.

I hate this too, but seems to be the best I can figure out

jrose-apple · 2016-12-09T21:20:00Z

include/swift/AST/LookupKinds.h

If you're going to do this, just write the constant unconditionally, and static_assert that it has the correct value.

I can't seem to reproduce this - I think it is a Visual Studio bug. I've removed the workaround

jrose-apple · 2016-12-09T21:20:25Z

lib/AST/ASTMangler.cpp

Yeah, this just looks like a mistake.

jrose-apple · 2016-12-09T21:21:11Z

lib/AST/NameLookup.cpp

We can just include <cassert> unconditionally. What's intrin.h for?

Oh, I see, it's the popcnt check below. I think it's fine to leave that out, since the static_assert will be checked in Clang anyway.

Sorry - I'm a tad confused. Do you suggest not changing this file, or simply to move <cassert> out of the if

Ah, both. :-) Please move <cassert> out of the #if, but don't bother asserting the popcount under MSVC (just #if it out).

fixed, thanks

hughbe · 2016-12-12T21:32:18Z

I've rebased and addressed your comments about the alignment issues

jrose-apple

Just a few last comments.

jrose-apple · 2016-12-12T21:35:59Z

lib/AST/ASTContext.cpp

More unguarded pragmas. Also, I find it hard to believe that we'd get a warning on this code and not the previous line, so maybe this is no longer necessary?

We do get the following warning:

C:\Users\Lukey\Documents\GitHub\my-swift\swift\lib\AST\ASTContext.cpp(282): warning C4189: 'conformance': local variable is initialized but not referenced

very weird...

jrose-apple · 2016-12-12T21:37:11Z

lib/AST/ASTDumper.cpp

(To be clear, no action requested. This just seems like a horrible oversight on MSVC's part.)

Okay, I have a much better fix that suddenly came to me when you commented. Basically, the problem was that the code referring to this << operator is in an anonymous namespace. Moving << into that anonymous namespace fixes this. Unfortunately, MSVC wasn't very helpful as it reported the error as an ambiguous overload error for unsigned char

jrose-apple · 2016-12-12T21:37:30Z

lib/AST/ASTMangler.cpp

Why the change here? These do different things. cc @eeckstein

whoops - merge conflict

jrose-apple · 2016-12-12T21:59:32Z

lib/AST/ASTDumper.cpp

Tabs snuck in here.

Thanks - this should be fixed now

hughbe · 2016-12-15T15:34:11Z

I've made a connect bug for the numTrailingObjects problem: https://connect.microsoft.com/VisualStudio/feedback/details/3116517

If anyone's curious, they can try coming up with a better workaround by compiling MSVC code online here: http://rextester.com/XIJ43194

hughbe · 2016-12-19T10:43:11Z

include/swift/AST/AnyFunctionRef.h

https://connect.microsoft.com/VisualStudio/feedback/details/3116508/msvc-shouldnt-show-errors-using-deprecated-methods-from-deprecated-methods

hughbe · 2016-12-19T10:46:23Z

include/swift/AST/Types.h

Dupe of https://connect.microsoft.com/VisualStudio/feedback/details/3116640/msvc-doesnt-support-sizeof-value-in-a-static-assert-in-a-class

hughbe · 2016-12-22T20:27:33Z

lib/AST/ASTContext.cpp

This seems to be a bug in MSVC: I've been trying hard to reproduce this came, but can't.

hughbe · 2016-12-22T20:28:10Z

lib/AST/ASTDumper.cpp

Again, this looks to be an MSVC bug, but I can't reproduce it :(

hughbe · 2017-01-09T09:08:40Z

@slavapestov PTAL. I think the only thing holding back this one getting merged is the gross hack for https://connect.microsoft.com/VisualStudio/feedback/details/3116517. I'd like to figure out a work around that doesn't make us diverge from upstream LLVM.

If anyone has some spare time, check out a simple reproduction here and help me find a nicer fix for MSVC's bug: http://rextester.com/XIJ43194. This one also lets you compile C++ online if you don't have a Windows PC

slavapestov · 2017-01-09T09:10:33Z

@swift-ci Please smoke test

hughbe commented Dec 9, 2016

View reviewed changes

jrose-apple reviewed Dec 9, 2016

View reviewed changes

jrose-apple reviewed Dec 12, 2016

View reviewed changes

This was referenced Dec 12, 2016

Fix errors and warnings building swift/SILGen on Windows using MSVC #5955

Merged

Fix errors and warnings building swift/SIL on Windows using MSVC #5954

Merged

hughbe commented Dec 19, 2016

View reviewed changes

hughbe commented Dec 22, 2016

View reviewed changes

Support building swift/AST with MSVC on Windows

7a5ef4b

slavapestov self-assigned this Jan 9, 2017

slavapestov merged commit 55f979c into swiftlang:master Jan 9, 2017

hughbe deleted the ast-msvc branch January 9, 2017 21:00

Support building swift/AST with MSVC on Windows #5948

Support building swift/AST with MSVC on Windows #5948

Uh oh!

Conversation

hughbe commented Nov 28, 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!

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!

jrose-apple Dec 12, 2016 • 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!

hughbe Dec 12, 2016 • 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!

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!

hughbe Dec 9, 2016 • 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!

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!

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!

jrose-apple Dec 12, 2016 •

edited

Loading

hughbe Dec 12, 2016 •

edited

Loading

hughbe Dec 9, 2016 •

edited

Loading