Skip to content
This repository was archived by the owner on Mar 28, 2020. It is now read-only.

Fix building specific Swift files in swift-clang with MSVC #45

Merged
merged 3 commits into from
Dec 1, 2016
Merged

Fix building specific Swift files in swift-clang with MSVC #45

merged 3 commits into from
Dec 1, 2016

Conversation

hughbe
Copy link
Collaborator

@hughbe hughbe commented Nov 18, 2016

Previous PR #44 was nuked by GitHub

Commit 1:

  • MSVC complains: specific_attr_begin is not a member of swift::Decl
  • The fix is to extract the default parameter and specifiy it for each call of the method

Commit 2:

MSVC implodes compiling Clang, owing to the definition of llvm::None:

namespace llvm {
/// \brief A simple null object to allow implicit construction of Optional<T>
/// and similar types without having to spell out the specialization's name.
enum class NoneType { None };
const NoneType None = None;
}

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

It throws the following strange, obfuscated error:

Error: Unhandled exception at 0x00007FFFF9669633 (ntdll.dll) in cl.exe: 0xC0000005: Access violation writing location 0x00000036C7A00E90.

[1/665] Building CXX object tools\clang\lib\APINotes\CMakeFiles\clangAPINotes.dir\APINotesYAMLCompiler.cpp.obj
FAILED: tools/clang/lib/APINotes/CMakeFiles/clangAPINotes.dir/APINotesYAMLCompiler.cpp.obj
C:\PROGRA~2\MICROS~1.0\VC\bin\amd64\cl.exe   /nologo /TP -DCLANG_ENABLE_ARCMT -DCLANG_ENABLE_OBJC_REWRITER -DCLANG_ENABLE_STATIC_ANALYZER -DGTEST_HAS_RTTI=0 -DUNICODE -D_CRT_NONSTDC_NO_DEPRECATE -D_CRT_NONSTDC_NO_WARNINGS -D_CRT_SECURE_NO_DEPRECATE -D_CRT_SECURE_NO_WARNINGS -D_GNU_SOURCE -D_HAS_EXCEPTIONS=0 -D_SCL_SECURE_NO_DEPRECATE -D_SCL_SECURE_NO_WARNINGS -D_UNICODE -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS -Itools\clang\lib\APINotes -IC:\Users\hbellamy\Documents\GitHub\llvm\tools\clang\lib\APINotes -IC:\Users\hbellamy\Documents\GitHub\llvm\tools\clang\include -Itools\clang\include -Iinclude -IC:\Users\hbellamy\Documents\GitHub\llvm\include /W4 -wd4141 -wd4146 -wd4180 -wd4244 -wd4258 -wd4267 -wd4291 -wd4345 -wd4351 -wd4355 -wd4456 -wd4457 -wd4458 -wd4459 -wd4503 -wd4624 -wd4722 -wd4800 -wd4100 -wd4127 -wd4512 -wd4505 -wd4610 -wd4510 -wd4702 -wd4245 -wd4706 -wd4310 -wd4701 -wd4703 -wd4389 -wd4611 -wd4805 -wd4204 -wd4577 -wd4091 -wd4592 -wd4319 -wd4324 -w14062 -we4238 /Zc:inline /Zc:strictStrings /Oi /Zc:rvalueCast -O2   -UNDEBUG  /EHs-c- /GR- /showIncludes /Fotools\clang\lib\APINotes\CMakeFiles\clangAPINotes.dir\APINotesYAMLCompiler.cpp.obj /Fdtools\clang\lib\APINotes\CMakeFiles\clangAPINotes.dir\ /FS -c C:\Users\hbellamy\Documents\GitHub\llvm\tools\clang\lib\APINotes\APINotesYAMLCompiler.cpp
Internal Compiler Error in C:\PROGRA~2\MICROS~1.0\VC\bin\amd64\cl.exe.  You will be prompted to send an error report to Microsoft later.

The fix is to give MSVC some more information. However, it still complains if we use llvm::None, so we have to use llvm::NoneType::None. I assume the problem is that MSVC gets horribly confused at this line: const NoneType None = None;

Commit 3

  • MSVC cannot resolve the operator.
  • The fix is to add a static cast to unsigned: this has been done in other places in the file, and was missing in this case, presumably as an oversight

/cc @DougGregor (this time, these files are swift-specific and no upstream version exists in Clang)

@hyp @jrose-apple

@@ -180,7 +184,7 @@ static void ProcessAPINotes(Sema &S, Decl *D,
return UnavailableAttr::CreateImplicit(S.Context,
CopyString(S.Context,
info.UnavailableMsg));
});
}, getAttrIterator<UnavailableAttr>);
Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry, I still think this would be handled better by overloading handleAPINotedAttribute.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Don't be sorry. I didn't understand what you meant, thought that was what you wanted.

You're right - I've updated the PR

Copy link
Contributor

Choose a reason for hiding this comment

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

Didn't get pushed?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Well just my luck, git has it in for me

@hughbe
Copy link
Collaborator Author

hughbe commented Nov 19, 2016

For reference, I've opened https://connect.microsoft.com/VisualStudio/feedback/details/3112067/msvc-fails-to-compile-code-that-compiles-in-clang-and-gcc to discuss MSVC's failings in the area of lambdas with generic parameters implicitly converting into another object inside a default parameter of a generic method. That is a mouthful!

@jrose-apple
Copy link
Contributor

@swift-ci Please test

@jrose-apple
Copy link
Contributor

@swift-ci Please test

@hughbe
Copy link
Collaborator Author

hughbe commented Nov 29, 2016

Hey, could you retrigger the CI - Clang had a syntax error that MSVC ignored

@jrose-apple
Copy link
Contributor

Ah, will do.

@swift-ci Please test

@jrose-apple
Copy link
Contributor

Hey, @shahmishal, looks like we need the SHA-checking on the Clang bots too.

Mac Linux

@shahmishal
Copy link
Member

@jrose-apple Thanks will update the PR jobs.

@jrose-apple
Copy link
Contributor

Re-running anyway because the failures were unrelated.

@swift-ci Please test

@hughbe
Copy link
Collaborator Author

hughbe commented Dec 1, 2016

Linux failure seems unrelated - lldb compile errors. Could you trigger the Linux tests (I don't have permission)
Thanks

@jrose-apple
Copy link
Contributor

@swift-ci Please test Linux platform

@jrose-apple
Copy link
Contributor

Ah, @shahmishal, until the swift-3.1-branch of Swift and LLDB stabilizes, we should probably continue testing LLVM/Clang PRs against their master branches.

@jrose-apple
Copy link
Contributor

I'm going to merge this anyway in the meantime. We got past compiling Swift on Linux, and these should not cause any functional changes.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants