Port swift/basic to Windows using MSVC #5949

hughbe · 2016-11-28T17:09:47Z

The UUID stuff is the heart of this PR, and relies on linking rpcrt4.lib, found in #5904. We can merge these PRs in any order, however.
The only limitation is that we can't generate a UUID based on the current time, but this isn't terrible.

compnerd · 2016-11-28T17:14:56Z

include/swift/Basic/type_traits.h

@@ -29,7 +29,7 @@ namespace swift {
 /// is not intended to be specialized.
 template<typename T>
 struct IsTriviallyCopyable {
-#if _LIBCPP_VERSION
+#if _LIBCPP_VERSION || defined(_MSC_VER)


What if you are using msvcprt with clang-cl?

Right, that was my concern. Would || (defined(_MSC_VER) && !defined(__clang__) work? I dunno how to detect clang-cl!

Maybe its a version thing - we should check _MSC_VER >= X?

The value for _MSC_VER will be the same across clang and clang-cl. What exactly are you trying to protect against?

Well if we don't have this check, I get a fatal error saying "not implemented" at compile time. We need this, but need to figure out how to detect msvcprt

Okay - I've addressed this by changing it to:

#if _LIBCPP_VERSION || (defined(_MSC_VER) && !defined(__clang__))

This works, since this works __has_feature(is_trivially_copyable) with clang-cl

compnerd · 2016-11-28T17:15:16Z

include/swift/Basic/type_traits.h

@@ -53,7 +53,7 @@ struct IsTriviallyConstructible {

 template<typename T>
 struct IsTriviallyDestructible {
-#if _LIBCPP_VERSION
+#if _LIBCPP_VERSION || (defined(_MSC_VER) && defined(_WIN32))


Why the additional _WIN32 check?

no idea, I'll remove when we get a better idea about the other stuff

compnerd · 2016-11-28T17:17:15Z

lib/Basic/SourceLoc.cpp

+  // _attribute__((used) is not defined in MSVC
+#if !defined(_MSC_VER)
+  __attribute__((used))
+#endif


Hmm, I wonder if there is a better way to handle this. Something like using a __pragma(warning(supress:????)))). In that case, we could have a SWIFT_USED macro.

This is the only place __attribute((used)) is used in the entire library I think, so I'm not sure its worth the effort!

What does LLVM_ATTRIBUTE_USED do?

compnerd · 2016-11-28T17:17:51Z

lib/Basic/UUID.cpp

+// WIN32 doesn't natively support <uuid/uuid.h>. Instead, we use Win32 APIs
+// class
+#if defined(_WIN32)
+#include <rpc.h>


Might be nice to #define WIN32_LEAN_AND_MEAN here.

compnerd · 2016-11-28T17:18:45Z

lib/Basic/Version.cpp

 #endif
 #if __has_include("SwiftRevision.inc")
-# include "SwiftRevision.inc"
+#include "SwiftRevision.inc"
+#endif


Why the whitespace changes?

cleanliness and consistency

The old way is meant to show #if nesting.

right - will revert.

jckarter · 2016-11-28T18:26:58Z

I believe that the only place we use UUIDs in the compiler is to print "opened archetypes" in SIL in a way that lets them be reliably parsed back as unique types. We could probably use a better uniquing scheme in SIL printing now that we have better tracking of opened type uses in SIL, and kill the UUID dependency.

hughbe · 2016-11-28T22:00:23Z

Interesting @jckarter... I've opened SR-3285 to track this. I've filed it as a starter bug. I'm hideously busy atm due so I hope you don't mind that I'm not addressing that in this PR?

jckarter · 2016-11-28T22:06:59Z

@hughbe Sure, no problem.

compnerd · 2016-11-29T01:57:28Z

@jckarter what exactly did you have in mind for replacing the naming scheme with? It sounds simple enough that I might be able to take that on.

jrose-apple · 2016-11-29T17:19:02Z

include/swift/Basic/ImmutablePointerSet.h

@@ -112,7 +112,8 @@ template <typename T> class ImmutablePointerSet : public llvm::FoldingSetNode {
    return *LowerBound == Ptr;
  }

-  using iterator = typename decltype(Data)::iterator;
+  // Ideally we would use decltype(Data)::iterator, but MSVC reports an error


Nitpick: comments should end with periods. (Although I don't really think you need this one; the alternate formulation isn't so bad that anyone would be tempted to change it.)

Removed the comment as suggested.

jrose-apple · 2016-11-29T17:19:42Z

lib/Basic/SourceLoc.cpp

@@ -24,7 +24,9 @@ void SourceManager::verifyAllBuffers() const {
  };

  // FIXME: This depends on the buffer IDs chosen by llvm::SourceMgr.
-  __attribute__((used)) static char arbitraryTotal = 0;
+  // _attribute__((used) is not defined in MSVC


No need for the comment here.

Removed, thanks

jrose-apple · 2016-11-29T17:20:29Z

lib/Basic/UUID.cpp

+// convert the memory of a 128 bit unsigned char array to a ::UUID.
+// We also can't expose <rpc.h> in any headers, as it imports <windows.h> which
+// defines macros that conflict with many definitions (e.g. ERROR, std::max)
+union Conversion {


memcpy already takes a void *, so you don't need this.

I'm not sure I can entirely remove this. We use this conversion union in methods such as UUIDCompare and UUIDToString that require a UUID passed. I have removed all the conversions from memcpy though as you suggested.

Either ::UUID is a POD type, in which case you can just memcpy into one directly, or it's not, in which case the union isn't safe either.

I had no idea you can just memcpy like that - you learn something new everyday:

This works, and the file builds, so we're good:

#include <rpc.h> int main() { ::UUID uuid1; unsigned char data[16] = { 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16 }; memcpy(&uuid1, data, 16); // uuid1 = {04030201-0605-0807-090A-0B0C0D0E0F10} return 0; }

jrose-apple · 2016-11-29T17:22:03Z

lib/Basic/Version.cpp

@@ -43,6 +43,12 @@
  SWIFT_MAKE_VERSION_STRING(SWIFT_VERSION_MAJOR, SWIFT_VERSION_MINOR)
 #endif

+// MSVC doesn't support __has_include


Hm. This isn't 100% correct, then, because these files won't exist when the source root isn't a Git or SVN repo. Maybe we should just have CMake generate empty files in those cases?

Strange - I see lines in the build log on Windows saying "Generating LLVMRevision.inc" etc. etc. for each file. Seems like CMake already does this and has already generated these files by the time Version.cpp is built.

It only does that if you're in a checkout of a Git or SVN repo.

Ah I understand now. I've modified lib/Basic/CMakeLists.txt file to generate an empty file if these files are not found. I've made it a seperate commit because its not necessarily Windows/MSVC specific.

jrose-apple · 2016-11-30T17:29:51Z

include/swift/Basic/TaskQueue.h

@@ -57,8 +57,10 @@ class TaskQueue {

  // TODO: remove once -Wdocumentation stops warning for \param, \returns on
  // std::function (<rdar://problem/15665132>).
+#if !defined(_MSC_VER)


These shouldn't be necessary now, since we decided to allow unknown pragmas.

(If we did have them, we should check for Clang instead of for !MSVC.)

I removed the ifs

jrose-apple · 2016-11-30T17:32:09Z

lib/Basic/UUID.cpp

+// convert the memory of a 128 bit unsigned char array to a ::UUID.
+// We also can't expose <rpc.h> in any headers, as it imports <windows.h> which
+// defines macros that conflict with many definitions (e.g. ERROR, std::max)
+union Conversion {


Either ::UUID is a POD type, in which case you can just memcpy into one directly, or it's not, in which case the union isn't safe either.

jrose-apple · 2016-12-02T17:09:55Z

lib/Basic/CMakeLists.txt

+  set_source_files_properties("${revision_inc}"
+    PROPERTIES GENERATED TRUE
+               HEADER_FILE_ONLY TRUE)
+  set(${revision_inc_var} ${revision_inc} PARENT_SCOPE)
  endif()


Stray endif() here?

jrose-apple · 2016-12-02T17:16:08Z

@swift-ci Please smoke test and merge

hughbe · 2016-12-02T21:13:52Z

Also this one didn't merge although tests passed

jrose-apple · 2016-12-02T21:22:02Z

sigh

hughbe · 2016-12-15T16:12:36Z

include/swift/Basic/ImmutablePointerSet.h

@@ -112,7 +112,7 @@ template <typename T> class ImmutablePointerSet : public llvm::FoldingSetNode {
    return *LowerBound == Ptr;
  }

-  using iterator = typename decltype(Data)::iterator;
+  using iterator = typename ArrayRef<PtrTy>::iterator;


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

hughbe · 2016-12-15T16:13:18Z

include/swift/Basic/EncodedSequence.h

+    static const size_t Size = (sizeof(void*) - 1) / sizeof(Chunk);
+    static const size_t ActualSize = max<size_t>(Size, 1);
+
+    using MapBase = PrefixMap<Chunk, ValueType, ActualSize>;


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

hughbe · 2016-12-15T16:14:48Z

include/swift/Basic/EncodedSequence.h

    MapBase TheMap;

  public:
-    using SequenceIterator = EncodedSequence::iterator;
+    using SequenceIterator = typename EncodedSequence::iterator;


This seems to be a Clang deficiency rather than an MSVC deficiency

Agreed, typename ought to be required by the standard. (Assuming EncodedSequence is dependent, of course.)

compnerd reviewed Nov 28, 2016

View reviewed changes

jrose-apple reviewed Nov 29, 2016

View reviewed changes

jrose-apple reviewed Nov 30, 2016

View reviewed changes

Port swift/basic to Windows

56dfb08

jrose-apple reviewed Dec 2, 2016

View reviewed changes

Generate empty *Revision.inc files during the build process

50e94af

jrose-apple approved these changes Dec 2, 2016

View reviewed changes

jrose-apple merged commit a184057 into swiftlang:master Dec 2, 2016

hughbe deleted the basic-win-msvc branch December 9, 2016 14:11

hughbe commented Dec 15, 2016

View reviewed changes

hughbe mentioned this pull request Dec 22, 2016

Fix linking libuuid on Windows #6460

Merged

Port swift/basic to Windows using MSVC #5949

Port swift/basic to Windows using MSVC #5949

Conversation

hughbe commented Nov 28, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hughbe Nov 30, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jckarter commented Nov 28, 2016

hughbe commented Nov 28, 2016

jckarter commented Nov 28, 2016

compnerd commented Nov 29, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hughbe Nov 30, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hughbe Dec 2, 2016 • edited Loading

Choose a reason for hiding this comment

jrose-apple commented Dec 2, 2016

hughbe commented Dec 2, 2016 • edited Loading

jrose-apple commented Dec 2, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jckarter Dec 15, 2016 • edited Loading

Choose a reason for hiding this comment

hughbe commented Nov 28, 2016 •

edited

Loading

hughbe Nov 30, 2016 •

edited

Loading

hughbe Nov 30, 2016 •

edited

Loading

hughbe Dec 2, 2016 •

edited

Loading

hughbe commented Dec 2, 2016 •

edited

Loading

jckarter Dec 15, 2016 •

edited

Loading