Windows [AST], [Serialization] and [IRGen] Fixes #11111
Conversation
… as per LLVM assertion in Verifier.cpp
troughton
changed the title
| @@ -35,7 +35,7 @@ namespace syntax { | |||
| /// a mapping from lib/AST nodes to lib/Syntax nodes while we integrate | |||
| /// the infrastructure into the compiler. | |||
| class SyntaxASTMap final { | |||
| friend class LegacyASTTransformer; | |||
| friend class swift::syntax::LegacyASTTransformer; | |||
There was a problem hiding this comment.
The right way to fix this might be to just use friend LegacyASTTransformer without the class. Or that might not work, in which case this is okay.
There was a problem hiding this comment.
That gives 'C++ requires a type specifier for all declarations' and 'friends can only be classes or functions' when compiling using clang-cl and with -fms-compatibility-version=19. I think we're stuck with the explicit namespacing, unfortunately.
|
The IRGen changes seem fine to me. |
|
@swift-ci Please smoke test |
|
Missing As for adding a test case for it: I can try to do that, but I'm not sure exactly what I'd be testing or how I'd be testing it; I haven't looked too much into the Swift testing infrastructure yet. |
|
I don't think there's any good reason we can't use swiftcc on Windows — it's implemented in the x86 backend, and I'd be surprised if somehow being a Windows target interfered with that. Of course it might not be implemented for x86-32, but that matters to us, too. |
|
At the moment using swiftcc on Windows is blocked by https://reviews.llvm.org/D31372; in short, the Microsoft mangling doesn't support the Swift calling convention. There's a workaround suggested there by @compnerd, but I haven't been able to figure out how to get it to work. |
|
I'll take that up on that thread. |
|
Is there anything remaining blocking this pull request or could it be merged in? |
|
I'd wait a bit to see if John has any further comments, but it looks simple enough to me. Let's do a quick retest just in case, though. @swift-ci Please smoke test |
| @@ -432,7 +432,8 @@ class ModuleFile : public LazyMemberLoader { | |||
|
|
|||
| /// Emits one last diagnostic, logs the error, and then aborts for the stack | |||
| /// trace. | |||
| void fatal(llvm::Error error) LLVM_ATTRIBUTE_NORETURN; | |||
| LLVM_ATTRIBUTE_NORETURN | |||
| void fatal(llvm::Error error); | |||
| @@ -392,7 +392,7 @@ IRGenModule::IRGenModule(IRGenerator &irgen, | |||
| AtomicBoolAlign = Alignment(ClangASTContext->getTypeSize(atomicBoolTy)); | |||
| } | |||
|
|
|||
| IsSwiftErrorInRegister = | |||
| IsSwiftErrorInRegister = UseSwiftCC && | |||
There was a problem hiding this comment.
Why was the additional UseSwiftCC needed again?
There was a problem hiding this comment.
To summarise my earlier answer: both UseSwiftCC and IsSwiftErrorInRegister are checked when deciding whether to emit the swifterror attribute, but only the latter is checked later in the code, so there’s a mismatch - it expects there to be attributes present that weren’t added.
There was a problem hiding this comment.
This is the least disruptive way to fix that particular issue, but the correct solution may be to change one of the problematic functions to match - which to change depends on whether IsSwiftErrorInRegister should require UseSwiftCC or not.
There was a problem hiding this comment.
That being a workaround is reasonable, though we should at least know where exactly it shows up and how to reproduce it so that we can track fixing it. I think it is worth filing an SR for that issue.
There was a problem hiding this comment.
So the exact locations are line 163 of GenCall.cpp, where it has
if (!UseSwiftCC || !this->IsSwiftErrorInRegister)
as opposed to line 2204 of GenCall.cpp and line 3369 of IRGenSIL.cpp, which check
if (IGM.IsSwiftErrorInRegister)
only.
The comments before lines 163 and 2204 of GenCall.cpp more-or-less match, so one of those conditions is incorrect; my guess would be that IsSwiftErrorInRegister should be dependent on UseSwiftCC all of the time, and then we're just left with a duplicated test in line 163 of GenCall.cpp.
I won't have a chance to try a native Windows build for a while and give you the exact error message, but that's the reasoning.
|
Since this has been open a while and I’m seeing other (semi-)duplicate PRs (#12475), can we get this merged in? The last commit is just unifying the conditionals around IsSwiftErrorInRegister. |
|
@troughton, could you please rebase this? |
|
Sorry, I wasn't aware of the existence of this. I was trying to cross-compile Swift for Windows with clang-cl, and submitted #12475 and #12479 to fix issues I ran into, which are addressed by this PR as well. I'm wondering if the additional changes you required were a result of using MSVC rather than clang-cl, or if other code has changed in the meantime that makes those changes unnecessary. |
|
I think the only remaining changes from this that haven’t been separately addressed are the |
This pull request contains a few changes to AST, Serialization, and IRGen to enable compiling on Windows against the MSVC toolchain.
The
LegacyASTTransformerchange to explicit namespacing is somewhat odd, but I was only able to get the compiler to recognise the declaration as being the same as the true definition by doing this; otherwise, it incorrectly infers thatLegacyASTTransformeris inswift::rather thanswift::syntax::In IRGen, the change to the condition for DLLImport linkage is directly taken from an assertion within Verifier.cpp in LLVM.