Fix errors and warnings building swift/shims on Windows using MSVC #6138

hughbe · 2016-12-08T12:03:28Z

No description provided.

jrose-apple · 2016-12-08T18:38:02Z

stdlib/public/SwiftShims/LibcShims.h

-__attribute__((__pure__)) SWIFT_RUNTIME_STDLIB_INTERFACE __swift_size_t
+#if defined(__clang__) || defined(__GNUC__)
+__attribute__((__pure__))
+#endif


We usually prefer pulling the attributes themselves into defines, e.g. SWIFT_RUNTIME_ATTRIBUTE_PURE.

jrose-apple · 2016-12-08T18:41:24Z

stdlib/public/SwiftShims/SwiftStddef.h

@@ -13,7 +13,12 @@
 #ifndef SWIFT_STDLIB_SHIMS_SWIFT_STDDEF_H
 #define SWIFT_STDLIB_SHIMS_SWIFT_STDDEF_H

+#if !defined(__clang__)
+#include <stdint.h>
+typedef size_t __swift_size_t;


The purpose of these headers is to get system-equivalent types without importing any system headers, for ordering reasons. If MSVC doesn't have something like __SIZE_TYPE__, it's better just to use an appropriate raw type, and maybe we can static_assert somewhere else that it's the same.

I'm not sure that's the point of these headers (or perhaps not the main one). For example, there's a comment in SwiftStdint.h that says:

// stdint.h is provided by Clang, but it dispatches to libc's stdint.h. As a
// result, using stdint.h here would pull in Darwin module (which includes
// libc). This creates a dependency cycle, so we can't use stdint.h in

I don't think we have this problem with MSVC (confirmed) and perhaps even Clang on Win32(unconfirmed)

I'm not convinced that things will be any better here if MSVC is ever modularized, but okay.

jrose-apple · 2016-12-08T18:42:38Z

stdlib/public/SwiftShims/RefCount.h

    __atomic_fetch_add(&refCount, RC_ONE, __ATOMIC_RELAXED);
+#else
+    std::atomic<uint32_t> stdRefCount = ATOMIC_VAR_INIT(refCount);


Wait, this doesn't make any sense. You need to increment the real refCount, not a local variable.

@gparker42, @jckarter, is there any problem with just making the real refcount field a std::atomic?

I'm fairly sure there might be some bugs in this, but I wanted to get the thing building. I think the issue you mentioned is correct - perhaps we then need to set refCount to the value of stdRefCount here.
Either way, take a look at some of the TODOs I've sprinkled in the file. I'm not sure what the fix is for the issue that there is no such std::atomic_sub_fetch_explicit even though there is a __atomic_sub_fetch intrinsic with Clang/GCC

Storing back into refCount just wouldn't be atomic.

jrose-apple · 2016-12-08T18:43:20Z

stdlib/public/SwiftShims/SwiftStdint.h

@@ -13,6 +13,24 @@
 #ifndef SWIFT_STDLIB_SHIMS_SWIFT_STDINT_H
 #define SWIFT_STDLIB_SHIMS_SWIFT_STDINT_H

+#if !defined(__clang__)
+#include <stdint.h>


Same thing as above, we can't do this because it pulls in system modules.

jrose-apple · 2016-12-08T18:44:08Z

stdlib/public/SwiftShims/Visibility.h

@@ -18,6 +18,10 @@
 #ifndef SWIFT_STDLIB_SHIMS_VISIBILITY_H
 #define SWIFT_STDLIB_SHIMS_VISIBILITY_H

+#if !defined(__GNUC__)


You can do this in general using !defined(__has_feature), as seen on the very next line. Clang treats it as defined. (And GCC doesn't have __has_feature either AFAIK.)

jckarter · 2016-12-08T20:57:07Z

I'm not sure this does any good—AFAIK, Shims are only built as part of the runtime (which must build with Clang) and are imported by the standard library (which uses swiftc's embedded Clang).

hughbe · 2016-12-08T21:14:53Z

@jckarter I think you're right - I'll update this PR to remove the non required parts

jrose-apple · 2016-12-08T21:23:38Z

Is the runtime actually built with Clang when building for MSVC yet?

hughbe · 2016-12-08T21:27:26Z

@jrose-apple not yet publically, but once my Windows port is merged, I'll fix it up for Clang.

Quick question @jckarter @jrose-apple: I've almost managed to get the entire swift project building using clang-cl and will send a PR to the CMake project once my MSVC/Windows port is merged. I assume that if we can build the runtime with clang, then we can build the runtime with clang-cl?

The reason I'm preferring clang-cl right now is because CMake seems to be able to identify the compiler clang-cl better than clang on Windows, which may take a bunch of work and perhaps even changes to CMake itself.

hughbe · 2016-12-08T21:28:09Z

I've also updated the PR to avoid a warning if __has_feature is not defined, and correctly define ssize_t on Win32

jrose-apple · 2016-12-08T22:01:22Z

I was asking specifically about the runtime. Our CMake already has an option to try to build the runtime library specifically with the just-built Clang, rather than the compiler on the host system (which probably wouldn't have the necessary Swift extensions). I believe @bob-wilson just re-enabled that by default if you do a clean build, but I could be mistaken.

hughbe · 2016-12-08T22:03:37Z

Ah right I see, my bad for misunderstanding

compnerd · 2016-12-09T05:23:47Z

Im not sure I see the point of the two separate commits, but LGTM.

compnerd · 2016-12-10T22:20:49Z

@swift-ci please test and merge

compnerd · 2016-12-11T00:58:30Z

@swift-ci please smoke test OS X platform

compnerd · 2016-12-11T03:20:10Z

@hughbe actually the changes required to build the runtime with clang is near zero. The runtime for windows is currently built with clang on Linux and Darwin.

hughbe · 2016-12-11T09:27:18Z

@compnerd I know! Some small windows specific stuff but very little
(Also this needs merging!)

compnerd · 2016-12-11T22:19:00Z

Actually, I think that @jrose-apple's comments have been addressed. The diff currently looks entirely trivial and correct, so Im going to go ahead and merge this.

hughbe changed the title ~~Fix errors and warnings building libSwiftStdlibShims on Windows using MSVC~~ Fix errors and warnings building swift/shims on Windows using MSVC Dec 8, 2016

jrose-apple reviewed Dec 8, 2016

View reviewed changes

Fix warning building with compiler without __has_feature

87012b8

Fix condition in the runtime from _MSC_VER to _WIN32

60e1cd1

compnerd merged commit ad99326 into swiftlang:master Dec 11, 2016

hughbe deleted the shims-msvc branch December 11, 2016 22:34

Fix errors and warnings building swift/shims on Windows using MSVC #6138

Fix errors and warnings building swift/shims on Windows using MSVC #6138

Uh oh!

Conversation

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

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!

jckarter commented Dec 8, 2016

Uh oh!

hughbe commented Dec 8, 2016

Uh oh!

jrose-apple commented Dec 8, 2016

Uh oh!

hughbe commented Dec 8, 2016

Uh oh!

hughbe commented Dec 8, 2016

Uh oh!

jrose-apple commented Dec 8, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hughbe commented Dec 8, 2016

Uh oh!

compnerd commented Dec 9, 2016

Uh oh!

compnerd commented Dec 10, 2016

Uh oh!

compnerd commented Dec 11, 2016

Uh oh!

compnerd commented Dec 11, 2016

Uh oh!

hughbe commented Dec 11, 2016

Uh oh!

compnerd commented Dec 11, 2016

Uh oh!

Uh oh!

hughbe commented Dec 8, 2016 •

edited

Loading

jrose-apple commented Dec 8, 2016 •

edited

Loading