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

hughbe · 2016-11-28T17:49:14Z

Various control path warnings
Various deleted function errors
Various sign warnings
Various function argument missing warnings
Relies on apple/swift-llvm#33 but PRs can be merged in any order

hughbe · 2016-12-12T23:34:03Z

include/swift/SIL/SILAllocated.h

@@ -28,8 +28,11 @@ class SILAllocated {
  void *operator new(size_t) = delete;
  void *operator new[](size_t) = delete;

+  // Work around MSVC error: attempting to reference a deleted function.
+#if !defined(_MSC_VER) || defined(__clang__)


This is as a result of an MSVC ABI bug

... That has been fixed in VS 2017 :D

hughbe · 2016-12-12T23:34:24Z

include/swift/SIL/SILInstruction.h

  size_t numTrailingObjects(
+#if defined(_MSC_VER) && !defined(__clang__)


There it is again, the horrible hack.

hughbe · 2016-12-12T23:39:31Z

lib/SIL/TypeLowering.cpp

@@ -1210,8 +1212,8 @@ TypeConverter::~TypeConverter() {
 void *TypeLowering::operator new(size_t size, TypeConverter &tc,
                                 IsDependent_t dependent) {
  return dependent
-    ? tc.DependentBPA.Allocate(size, alignof(TypeLowering))
-    : tc.IndependentBPA.Allocate(size, alignof(TypeLowering));
+    ? tc.DependentBPA.Allocate(size, alignof(TypeLowering&))


This fixes the "cannot instantiate abstract class" error seen in #5948. The error happens for each alignof(TypeLowering) check. @jrose-apple FYI

2>C:\Users\Lukey\Documents\GitHub\my-swift\swift\lib\SIL\TypeLowering.cpp(1215): error C2259: 'swift::Lowering::TypeLowering': cannot instantiate abstract class
2> C:\Users\Lukey\Documents\GitHub\my-swift\swift\lib\SIL\TypeLowering.cpp(1215): note: due to following members:
2> C:\Users\Lukey\Documents\GitHub\my-swift\swift\lib\SIL\TypeLowering.cpp(1215): note: 'swift::SILValue swift::Lowering::TypeLowering::emitLoadOfCopy(swift::SILBuilder &,swift::SILLocation,swift::SILValue,swift::IsTake_t) const': is abstract
2> C:\Users\Lukey\Documents\GitHub\my-swift\swift\include\swift/SIL/TypeLowering.h(194): note: see declaration of 'swift::Lowering::TypeLowering::emitLoadOfCopy'
2> C:\Users\Lukey\Documents\GitHub\my-swift\swift\lib\SIL\TypeLowering.cpp(1215): note: 'void swift::Lowering::TypeLowering::emitStoreOfCopy(swift::SILBuilder &,swift::SILLocation,swift::SILValue,swift::SILValue,swift::IsInitialization_t) const': is abstract
2> C:\Users\Lukey\Documents\GitHub\my-swift\swift\include\swift/SIL/TypeLowering.h(207): note: see declaration of 'swift::Lowering::TypeLowering::emitStoreOfCopy'
2> C:\Users\Lukey\Documents\GitHub\my-swift\swift\lib\SIL\TypeLowering.cpp(1215): note: 'void swift::Lowering::TypeLowering::emitStore(swift::SILBuilder &,swift::SILLocation,swift::SILValue,swift::SILValue,swift::StoreOwnershipQualifier) const': is abstract
2> C:\Users\Lukey\Documents\GitHub\my-swift\swift\include\swift/SIL/TypeLowering.h(218): note: see declaration of 'swift::Lowering::TypeLowering::emitStore'
2> C:\Users\Lukey\Documents\GitHub\my-swift\swift\lib\SIL\TypeLowering.cpp(1215): note: 'swift::SILValue swift::Lowering::TypeLowering::emitLoad(swift::SILBuilder &,swift::SILLocation,swift::SILValue,swift::LoadOwnershipQualifier) const': is abstract
2> C:\Users\Lukey\Documents\GitHub\my-swift\swift\include\swift/SIL/TypeLowering.h(225): note: see declaration of 'swift::Lowering::TypeLowering::emitLoad'
2> C:\Users\Lukey\Documents\GitHub\my-swift\swift\lib\SIL\TypeLowering.cpp(1215): note: 'void swift::Lowering::TypeLowering::emitCopyInto(swift::SILBuilder &,swift::SILLocation,swift::SILValue,swift::SILValue,swift::IsTake_t,swift::IsInitialization_t) const': is abstract
2> C:\Users\Lukey\Documents\GitHub\my-swift\swift\include\swift/SIL/TypeLowering.h(230): note: see declaration of 'swift::Lowering::TypeLowering::emitCopyInto'
2> C:\Users\Lukey\Documents\GitHub\my-swift\swift\lib\SIL\TypeLowering.cpp(1215): note: 'void swift::Lowering::TypeLowering::emitDestroyAddress(swift::SILBuilder &,swift::SILLocation,swift::SILValue) const': is abstract
2> C:\Users\Lukey\Documents\GitHub\my-swift\swift\include\swift/SIL/TypeLowering.h(240): note: see declaration of 'swift::Lowering::TypeLowering::emitDestroyAddress'
2> C:\Users\Lukey\Documents\GitHub\my-swift\swift\lib\SIL\TypeLowering.cpp(1215): note: 'void swift::Lowering::TypeLowering::emitDestroyRValue(swift::SILBuilder &,swift::SILLocation,swift::SILValue) const': is abstract
2> C:\Users\Lukey\Documents\GitHub\my-swift\swift\include\swift/SIL/TypeLowering.h(246): note: see declaration of 'swift::Lowering::TypeLowering::emitDestroyRValue'
2> C:\Users\Lukey\Documents\GitHub\my-swift\swift\lib\SIL\TypeLowering.cpp(1215): note: 'void swift::Lowering::TypeLowering::emitLoweredDestroyValue(swift::SILBuilder &,swift::SILLocation,swift::SILValue,swift::Lowering::TypeLowering::LoweringStyle) const': is abstract
2> C:\Users\Lukey\Documents\GitHub\my-swift\swift\include\swift/SIL/TypeLowering.h(254): note: see declaration of 'swift::Lowering::TypeLowering::emitLoweredDestroyValue'
2> C:\Users\Lukey\Documents\GitHub\my-swift\swift\lib\SIL\TypeLowering.cpp(1215): note: 'void swift::Lowering::TypeLowering::emitDestroyValue(swift::SILBuilder &,swift::SILLocation,swift::SILValue) const': is abstract
2> C:\Users\Lukey\Documents\GitHub\my-swift\swift\include\swift/SIL/TypeLowering.h(290): note: see declaration of 'swift::Lowering::TypeLowering::emitDestroyValue'
2> C:\Users\Lukey\Documents\GitHub\my-swift\swift\lib\SIL\TypeLowering.cpp(1215): note: 'swift::SILValue swift::Lowering::TypeLowering::emitLoweredCopyValue(swift::SILBuilder &,swift::SILLocation,swift::SILValue,swift::Lowering::TypeLowering::LoweringStyle) const': is abstract
2> C:\Users\Lukey\Documents\GitHub\my-swift\swift\include\swift/SIL/TypeLowering.h(296): note: see declaration of 'swift::Lowering::TypeLowering::emitLoweredCopyValue'
2> C:\Users\Lukey\Documents\GitHub\my-swift\swift\lib\SIL\TypeLowering.cpp(1215): note: 'swift::SILValue swift::Lowering::TypeLowering::emitCopyValue(swift::SILBuilder &,swift::SILLocation,swift::SILValue) const': is abstract
2> C:\Users\Lukey\Documents\GitHub\my-swift\swift\include\swift/SIL/TypeLowering.h(324): note: see declaration of 'swift::Lowering::TypeLowering::emitCopyValue'

As long as we have a complete type, it seems to be doing the right thing.

Okay, good to know.

hughbe · 2016-12-22T12:03:14Z

@jckarter PTAL if you have time and are not on holiday. I don't think I should add llvm_unreachable to the deleted methods here, as I'm fairly sure MSVC would call them, resulting in assertions when the code is executed

jckarter · 2016-12-22T17:02:43Z

include/swift/SIL/SILOpenedArchetypesTracker.h

+#if defined(_MSC_VER)
+#pragma warning(push)
+#pragma warning(disable: 4521)
+#endif


Is the warning legitimate, or an MSVC bug? Can we slap an enable_if on a generic constructor somewhere to avoid formally having multiple copy ctors?

I'm inclined to think this is an MSVC bug. I think it is related to
The line SILOpenedArchetypesTracker &operator = (const SILOpenedArchetypesTracker &) = delete; but have been unable to put a repo together.

I'm not surre I fully understand the enable_if comment you made, sorry

I was referring to the not-uncommon situation where there's a formal copy constructor and a generic constructor that could also be called with a const T & argument, thereby also potentially fulfilling the role of copy constructor.

slavapestov · 2017-01-11T06:34:24Z

@swift-ci Please smoke test

slavapestov · 2017-01-11T06:34:48Z

@jckarter Does this LGTM?

hughbe · 2017-01-11T08:32:41Z

I need to add an enable_if to a generic copy constructor to fix an MSVC warning that I've silenced. I think considering this PR has been around for a while and is decently large, it would be good to get this merged in, and then I can deal with the MSVC warning in another PR. It also keeps getting merge conflicts, so would be good to stop having to deal with that!

jckarter · 2017-01-11T17:49:02Z

include/swift/SIL/SILInstruction.h

      typename TrailingBase::template OverloadToken<Operand>) const {
+#endif


I think it would be better to consolidate the workaround in a single typedef somewhere, without the conditional.

// Equivalent to `typename TrailingBase::template OverloadToken<Operand>`. // We have to fully elaborate the type because of an MSVC bug. using OperandOverloadToken = llvm::trailing_objects_internal::TrailingObjectsBase::OverloadToken<Operand>;

jckarter · 2017-01-11T17:52:04Z

include/swift/SIL/SILArgument.h

  void operator delete(void *Ptr, size_t) = delete;
+#endif


It might be nice to consolidate these into a compatibility macro in a swift/Basic header:

#if !defined(_MSC_VER) || defined(__clang__) # define SWIFT_DELETE_OPERATOR_DELETE(...) \ void operator delete(__VA_ARGS__) = delete #else // Due to an MSVC bug, deleting an operator delete // also causes the destructor to be deleted. # define SWIFT_DELETE_OPERATOR_DELETE(...) #endif

jckarter · 2017-01-11T17:53:53Z

I think we should avoid conditionals where possible by consolidating the repeated cases into compatibility macros. Other than that, LGTM.

slavapestov · 2017-01-11T23:41:48Z

@hughbe Do you mind making the cleanup suggested by @jckarter in a separate patch?

hughbe · 2017-01-11T23:55:14Z

Fine by me thanks for getting this in

hughbe changed the title ~~Fix errors and warnings building libSwiftSIL on Windows using MSVC~~ Fix errors and warnings building swift/SIL on Windows using MSVC Nov 28, 2016

hughbe commented Dec 12, 2016

View reviewed changes

jckarter reviewed Dec 22, 2016

View reviewed changes

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

36100bf

slavapestov self-assigned this Jan 11, 2017

jckarter reviewed Jan 11, 2017

View reviewed changes

slavapestov merged commit 5c0afe9 into swiftlang:master Jan 11, 2017

hughbe deleted the sil-msvc branch January 11, 2017 23:55

		size_t numTrailingObjects(
		#if defined(_MSC_VER) && !defined(__clang__)

		typename TrailingBase::template OverloadToken<Operand>) const {
		#endif

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

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

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!

hughbe commented Dec 22, 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!

slavapestov commented Jan 11, 2017

Uh oh!

slavapestov commented Jan 11, 2017

Uh oh!

hughbe commented Jan 11, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jckarter Jan 11, 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!

jckarter commented Jan 11, 2017

Uh oh!

slavapestov commented Jan 11, 2017

Uh oh!

hughbe commented Jan 11, 2017

Uh oh!

Uh oh!

jckarter Jan 11, 2017 •

edited

Loading