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

hughbe · 2016-11-28T17:56:11Z

Reference deleted function error
Constructor overload error
Iterator internal stdlib error
Disable assertion failing on MSVC

hughbe · 2016-12-12T22:19:51Z

lib/SILGen/SILGenApply.cpp

@@ -174,7 +174,10 @@ class Callee {
  const Kind kind;

  // Move, don't copy.
+  // Work around MSVC error: attempting to reference a deleted function.
+#if !defined(_MSC_VER) || defined(__clang__)


I'd love to get rid of this. The problem here is caused by this code:

Callee getCallee() { assert(ApplyCallee && "did not find callee?!"); return *std::move(ApplyCallee); }

This gives me the error

1>C:\Users\Lukey\Documents\GitHub\my-swift\swift\lib\SILGen\SILGenApply.cpp(1496): error C2280: 'anonymous-namespace'::Callee::Callee(const anonymous-namespace'::Callee &)': attempting to reference a deleted function
1> C:\Users\Lukey\Documents\GitHub\my-swift\swift\lib\SILGen\SILGenApply.cpp(179): note: see declaration of '`anonymous-namespace'::Callee::Callee'

Note: replacing delete with default below fixes any errors (@jrose-apple)

Replacing delete with default is wrong, since the type is intended to be move-only. Does llvm::Optional not support move semantics when built for MSVC? Does std::move(*ApplyCallee) work?

Yes it does @jckarter. Thanks!!

You might want to take a look at #5948 in which I try to work around a similar error. Unfortunately, this time the code that triggers this is compiler generated __delDtor(unsigned int)

jckarter · 2016-12-12T22:41:28Z

lib/SILGen/SILGenExpr.cpp

@@ -1693,7 +1693,7 @@ RValue RValueEmitter::visitMemberRefExpr(MemberRefExpr *E, SGFContext C) {
  // and releases.
  if (auto *N = E->getBase()->getType()->getNominalOrBoundGenericNominal())
    if (auto RV = NominalTypeMemberRefRValueEmitter(E, C, N).emit(SGF))
-      return RValue(std::move(RV).getValue());
+      return RValue(const_cast<RValue&&>(std::move(RV).getValue()));


Why is this necessary? An rvalue reference-to-const has almost no reason to exist.

Hmm, seems like a Visual Studio bug, rather than something MSVC specific. I can reproduce this from the IDE in intellisense but not MSVC. I wrote this in about a month ago so I think I just assumed IDE errors == MSVC errors for this.

The error is:

swift::lowering::RValue(const swift::Lowering::RValue &) is inaccessible

... I've reverted this cahnge

hughbe · 2016-12-19T10:41:08Z

@gottesmm FYI

compnerd · 2016-12-19T15:41:56Z

@swift-ci please test

swift-ci · 2016-12-19T15:42:49Z

Build failed
Jenkins build - Swift Test Linux Platform
Git Commit - 2c65cec0c91436ba7704c4946b9cace7ec683382
Test requested by - @compnerd

swift-ci · 2016-12-19T16:43:42Z

Build failed
Jenkins build - Swift Test OS X Platform
Git Commit - 2c65cec0c91436ba7704c4946b9cace7ec683382
Test requested by - @compnerd

hughbe · 2016-12-19T16:53:16Z

What's causing these failures - it doesn't look like things are actually failing in the CI?

jrose-apple · 2016-12-19T17:25:55Z

"Wrong SHA" failures are a workaround for a bug with the Jenkins GitHub plugin we're using. There are general plans to upgrade the CI system so meanwhile we just put in an expedient hack to fail and automatically retry the build.

hughbe · 2016-12-21T19:30:13Z

Tests seem to have passed, this was never merged

hughbe · 2016-12-21T19:30:46Z

Derp, you did a test only trigger

jrose-apple · 2016-12-21T21:13:34Z

Ah, yeah, hoping for final sign-off from @jckarter.

jckarter · 2016-12-21T21:38:30Z

include/swift/SILOptimizer/PassManager/PassPipeline.h

@@ -56,7 +56,9 @@ class SILPassPipelinePlan final {
  SILPassPipelinePlan() = default;
  ~SILPassPipelinePlan() = default;
  SILPassPipelinePlan(const SILPassPipelinePlan &) = default;
+#if !defined(_MSC_VER) || defined(__clang__)
  SILPassPipelinePlan(SILPassPipelinePlan &&) = delete;


If MSVC doesn't like the = delete, can we at least leave an llvm_unreachable implementation behind? It must have been deleted for a reason, and it'd be nice to still check dynamically.

I'm not sure that's the right thing to do.

The errors are

1>C:\Users\hbellamy\Documents\GitHub\my-swift\swift\lib\SILOptimizer\PassManager\PassPipeline.cpp(104): error C2280: 'swift::SILPassPipelinePlan::SILPassPipelinePlan(swift::SILPassPipelinePlan &&)': attempting to reference a deleted function
1> C:\Users\hbellamy\Documents\GitHub\my-swift\swift\include\swift/SILOptimizer/PassManager/PassPipeline.h(51): note: see declaration of 'swift::SILPassPipelinePlan::SILPassPipelinePlan'

It comes from this code

SILPassPipelinePlan SILPassPipelinePlan::getDiagnosticPassPipeline(SILOptions Options) { SILPassPipelinePlan P; if (SILViewSILGenCFG) { addCFGPrinterPipeline(P, "SIL View SILGen CFG"); } // If we are asked do debug serialization, instead of running all diagnostic // passes, just run mandatory inlining with dead transparent function cleanup // disabled. if (Options.DebugSerialization) { addMandatoryDebugSerialization(P); return P; } // Lower all ownership instructions right after SILGen for now. addOwnershipModelEliminatorPipeline(P); // Otherwise run the rest of diagnostics. addMandatoryOptPipeline(P); if (SILViewGuaranteedCFG) { addCFGPrinterPipeline(P, "SIL View Guaranteed CFG"); } return P; }

@gottesmm FYI

I've pushed a test fix as a seperate commit - I'm not sure if it's correct, but it seems to get rid of the build errors

jckarter · 2016-12-21T21:38:34Z

lib/SILGen/SILGenApply.cpp

+      // extent.params.begin(), extent.params.end())" due to a missing "<"
+      // parameter.
+      for (auto param : extent.Params) {
+        innerParams.append(std::initializer_list<SILParameterInfo>({ param }));


innerParams.push_back(param); would be simpler.

jckarter · 2016-12-21T21:39:03Z

lib/SILGen/SILGenApply.cpp

@@ -4239,7 +4247,9 @@ namespace {
        // with specialized emitters above, enum constructors use
        // the AST-level abstraction pattern, to ensure that function
        // types in payloads are re-abstracted correctly.
+#if !defined(_MSC_VER) || defined(__clang__)
        assert(!AssumedPlusZeroSelf);


We should look into what's making this assertion fail. It could indicate a serious problem.

For now, I'm going to revert this change. It's possible this is related to my changes elsewhere in my working branch. However, in previous PRs I have seen assertions happening as they assume a define left to right ordering of method results passed to a method, so could be something more

hughbe · 2016-12-22T11:59:01Z

Thanks Joe!
Updated the PR to address your feedback. I also rebased, and fixed several more MSVC errors. Some are known.
New ones are

delctype(Value) for anonymous enum: https://connect.microsoft.com/VisualStudio/feedback/details/3117167
Deleted reference for SILPassPipelinePlan: https://connect.microsoft.com/VisualStudio/feedback/details/3117181

jckarter · 2016-12-22T16:59:32Z

lib/SILOptimizer/PassManager/PassPipeline.cpp

@@ -101,7 +101,7 @@ SILPassPipelinePlan::getDiagnosticPassPipeline(SILOptions Options) {
  // disabled.
  if (Options.DebugSerialization) {
    addMandatoryDebugSerialization(P);
-    return P;
+    return const_cast<const SILPassPipelinePlan&>(P);


What is this cast for? This probably defeats NRVO. Looking at the code here, I suspect the deleted move constructor was intended to prevent people from writing pessimizing std::moves, but Clang has a warning for that now. AFAICT it should be safe to just remove the SILPassPipelinePlan(SILPassPipelinePlan &&) = delete; declaration.

I was just experimenting with ideas to fix the error. I can remove the delete decleration if you'd like

I don't see any obvious reason to keep it. Pinging @gottesmm since he put it there.

Removed, thanks

…MSVC - https://connect.microsoft.com/VisualStudio/feedback/details/3116505/msvc-fails-to-compile-code-that-compiles-with-clang-reports-attempting-to-reference-a-deleted-function-error-from-destructor - https://connect.microsoft.com/VisualStudio/feedback/details/3116636/msvc-reports-ambiguous-symbol-error-for-friend-class-declaration-in-an-anonymous-namespace

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

jckarter · 2016-12-22T18:33:42Z

Looks good, thanks @hughbe !

jrose-apple · 2016-12-22T18:49:05Z

@swift-ci Please smoke test

hughbe commented Dec 12, 2016

View reviewed changes

jckarter reviewed Dec 12, 2016

View reviewed changes

jckarter requested changes Dec 21, 2016

View reviewed changes

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

95af3aa

jckarter reviewed Dec 22, 2016

View reviewed changes

hughbe added 2 commits December 22, 2016 18:26

Fix errors building swift/SIL on Windows with MSVC

63d6b15

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

jckarter approved these changes Dec 22, 2016

View reviewed changes

jrose-apple merged commit 8f3c7eb into swiftlang:master Dec 22, 2016

hughbe deleted the silgen-msvc branch December 23, 2016 10:25

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

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

Uh oh!

Conversation

hughbe commented Nov 28, 2016

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 commented Dec 19, 2016

Uh oh!

compnerd commented Dec 19, 2016

Uh oh!

swift-ci commented Dec 19, 2016

Uh oh!

swift-ci commented Dec 19, 2016

Uh oh!

hughbe commented Dec 19, 2016

Uh oh!

jrose-apple commented Dec 19, 2016

Uh oh!

hughbe commented Dec 21, 2016

Uh oh!

hughbe commented Dec 21, 2016

Uh oh!

jrose-apple commented Dec 21, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

hughbe commented Dec 22, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

jckarter commented Dec 22, 2016

Uh oh!

jrose-apple commented Dec 22, 2016

Uh oh!

Uh oh!

hughbe Dec 12, 2016 •

edited

Loading

hughbe Dec 22, 2016 •

edited

Loading

hughbe commented Dec 22, 2016 •

edited

Loading