Skip to content

Windows [AST], [Serialization] and [IRGen] Fixes #11111

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.

Sign up for GitHub

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

Closed
wants to merge 4 commits into from
Closed

Windows [AST], [Serialization] and [IRGen] Fixes #11111

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
f6c515d
Minor fixes for Windows compilation
troughton Jul 21, 2017
413724f
Only set DLLImport on decls for ExternalLinkage
troughton Jul 19, 2017
492bbf4
IsSwiftErrorInRegister requires UseSwiftCC
troughton Jul 21, 2017
ff061d0
Remove duplicated if condition in addSwiftErrorAttributes
troughton Sep 21, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion include/swift/AST/SyntaxASTMap.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

llvm::DenseMap<RC<syntax::SyntaxData>, ASTNode> SyntaxMap;
public:

Expand Down
3 changes: 2 additions & 1 deletion include/swift/Serialization/ModuleFile.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wrote this patch too. 👍


ASTContext &getContext() const {
assert(FileContext && "no associated context yet");
Expand Down
2 changes: 1 addition & 1 deletion lib/IRGen/GenCall.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -170,7 +170,7 @@ void IRGenModule::addSwiftErrorAttributes(llvm::AttributeSet &attrs,
// We create a shadow stack location of the swifterror parameter for the
// debugger on such platforms and so we can't mark the parameter with a
// swifterror attribute.
if (!UseSwiftCC || !this->IsSwiftErrorInRegister)
if (!this->IsSwiftErrorInRegister)
return;

static const llvm::Attribute::AttrKind attrKinds[] = {
Expand Down
4 changes: 2 additions & 2 deletions lib/IRGen/IRGenModule.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -392,7 +392,7 @@ IRGenModule::IRGenModule(IRGenerator &irgen,
AtomicBoolAlign = Alignment(ClangASTContext->getTypeSize(atomicBoolTy));
}

IsSwiftErrorInRegister =
IsSwiftErrorInRegister = UseSwiftCC &&
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why was the additional UseSwiftCC needed again?

Copy link
Contributor Author

@troughton troughton Sep 20, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@troughton troughton Sep 20, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@troughton troughton Sep 20, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

clang::CodeGen::swiftcall::isSwiftErrorLoweredInRegister(
ClangCodeGen->CGM());
}
Expand Down Expand Up @@ -453,7 +453,7 @@ llvm::Constant *swift::getRuntimeFn(llvm::Module &Module,
fn->setCallingConv(cc);

if (::useDllStorage(llvm::Triple(Module.getTargetTriple())) &&
(fn->getLinkage() == llvm::GlobalValue::ExternalLinkage ||
((fn->isDeclaration() && fn->getLinkage() == llvm::GlobalValue::ExternalLinkage) ||
fn->getLinkage() == llvm::GlobalValue::AvailableExternallyLinkage))
fn->setDLLStorageClass(llvm::GlobalValue::DLLImportStorageClass);

Expand Down