[stdlib/msvc] Runtime with MSVC library #1918

tinysun212 · 2016-03-29T03:12:32Z

What's in this pull request?

Resolved bug number: (SR-)

Before merging this pull request to apple/swift repository:

Test pull request on Swift continuous integration.

Triggering Swift CI

The swift-ci is triggered by writing a comment on this PR addressed to the GitHub user @swift-ci. Different tests will run depending on the specific comment that you use. The currently available comments are:

Smoke Testing

Platform	Comment
All supported platforms	@swift-ci Please smoke test
OS X platform	@swift-ci Please smoke test OS X platform
Linux platform	@swift-ci Please smoke test Linux platform

Validation Testing

Platform	Comment
All supported platforms	@swift-ci Please test
OS X platform	@swift-ci Please test OS X platform
Linux platform	@swift-ci Please test Linux platform

Note: Only members of the Apple organization can trigger swift-ci.

This patch is for libswiftCore.lib, linking with the library set of Visual Studio 2015. Clang with the option -fms-extension is used to build this port.

tkremenek · 2016-03-29T03:48:39Z

@swift-ci smoke test

tkremenek · 2016-03-29T03:49:11Z

Patch LGTM.

@gribozavr Can you review?

gribozavr · 2016-03-29T04:38:51Z

stdlib/public/stubs/Assert.cpp

@@ -89,9 +121,20 @@ _swift_stdlib_reportUnimplementedInitializer(const char *className,
                                             intptr_t initNameLength,
                                             uint32_t flags) {
  char *log;
+#if defined(_MSC_VER)
+  int len =
+      _scprintf("fatal error: use of unimplemented "


Please define a shim for asprintf() instead of duplicating the code.

tinysun212 · 2016-03-29T05:38:27Z

Testing OS X Platform is failed. Is this my fault?

gribozavr · 2016-03-29T05:50:25Z

@tinysun212 Currently master is passing tests on OS X, so it might be related. Let's retest to be sure:

@swift-ci Please test

gribozavr · 2016-03-30T03:19:37Z

stdlib/public/runtime/Metadata.cpp

@@ -27,9 +27,15 @@
 #include <condition_variable>
 #include <new>
 #include <cctype>
+#if defined(_MSC_VER)
+#include <windows.h>


#define NOMINMAX please.

tinysun212 · 2016-03-30T07:56:37Z

stdlib/public/runtime/Casting.cpp

+#include <shared_mutex>
+#else
+#include <pthread.h>
+#endif
 #include <type_traits>


I think I should expand Mutex.h/Mutex.cpp and use it here.

Please review the newly opened PR #1950. It adds read/write lock support as well is a little better factored to allow for alternate platforms to provide their own implementations if they don't have pthreads.

tinysun212 · 2016-04-02T09:46:28Z

(mistake in rebasing.. I dropped this branch and restored.)
I added MutexWin32.cpp/h and revoked the Casting.cpp which is using read/write lock. Later, Mutex.h should be patched to include MutexWin32.h.

gribozavr · 2016-04-02T10:05:10Z

How does this work? I'm not seeing any code using ConditionPlatformHelper.

I thought that you would modify include/swift/Runtime/Mutex.h and add #ifs for Windows in class Mutex etc.

shawnce · 2016-04-02T11:37:35Z

He is depending on my open PR #1950 which has the pthread equivalent
platform helper code.

On Sat, Apr 2, 2016 at 3:06 AM Dmitri Gribenko [email protected]
wrote:

How does this work? I'm not seeing any code using ConditionPlatformHelper.

I thought that you would modify include/swift/Runtime/Mutex.h and add #ifs
for Windows in class Mutex etc.

—
You are receiving this because you commented.

Reply to this email directly or view it on GitHub
#1918 (comment)

tinysun212 · 2016-04-02T20:54:57Z

Yes, this PR should be merged with one more commit (Mutex.h patching) after #1950 merged. I'm waiting it is merged or other reviews.

shawnce · 2016-04-03T22:42:31Z

stdlib/public/runtime/MutexWin32.cpp

+
+void ConditionPlatformHelper::wait(CONDITION_VARIABLE &condition,
+								   SRWLOCK &mutex) {
+  SleepConditionVariableSRW(&condition, &mutex, INFINITE, 0);


Looks like this is one of the few functions that return an error...

If the function succeeds, the return value is nonzero.
If the function fails, the return value is zero. To get extended error information, call GetLastError.

...consider testing for an error and calling fatalError like is done in MutexPThread.cpp.

gribozavr · 2016-05-30T05:08:22Z

stdlib/public/stubs/Stubs.cpp

+  if (LinePtr == nullptr)
+    return -1;
+
+  int Capacity = 0;


Probably does not matter in practice, but Capacity should probably be declared as ssize_t.

gribozavr · 2016-05-30T05:22:40Z

@tinysun212 Please consider submitting approved parts as a separate pull request. This will make it much easier to iterate on the other parts.

tinysun212 · 2016-05-30T05:56:02Z

@gribozavr Thanks for your reviews, I'll make separate PR containing only approved parts.

jckarter · 2016-06-02T22:49:34Z

stdlib/public/runtime/CMakeLists.txt

  set(swift_runtime_port_sources
+      MutexWin32.cpp
      CygwinPort.cpp)


Is CygwinPort.cpp intended to be part of the MSVC build? Should it be renamed Win32.cpp or something more general instead?

Yes, it also used for the MSVC build. I agree it to have a new name, but leaved for another PR or iteration.

gribozavr · 2016-06-03T07:11:15Z

@tinysun212 Thank you, I merged #2829. Would be happy to continue to iterate on the rest!

This patch is for libswiftCore.lib, linking with the library set of Visual Studio 2015. Clang with the option -fms-extension is used to build.

tinysun212 · 2016-06-03T20:51:20Z

@gribozavr, I rebased without any patch for the convenience. Your reviews will be applied in several commits.

gribozavr · 2016-06-03T20:54:01Z

stdlib/public/runtime/Reflection.mm

@@ -24,6 +24,9 @@
 #include <cstring>
 #include <new>
 #include <string>
+#if !defined(_MSC_VER)
+#include <dlfcn.h>
+#endif


You shouldn't need this :)

tinysun212 · 2016-06-03T21:58:27Z

@gribozavr, @jckarter, I'll touch all your previous reviews one by one, but it takes some time. You can know if it is touched from my comment for your review.

gribozavr · 2016-06-03T22:01:36Z

@tinysun212 Just let us know when you're ready!

jckarter · 2016-06-06T15:24:20Z

@gribozavr _read and _write are more-or-less equivalent to the POSIX APIs. Microsoft consistently marks non-standard APIs in their C runtime with a leading underscore.

tinysun212 · 2016-06-09T21:18:47Z

In stdlib/public/stubs/LibcShims.cpp, _read/_write/_close should be used for MSVC. The patch was omitted for some reason.

tinysun212 · 2016-06-09T22:22:21Z

@gribozavr, this PR is ready for review.

gribozavr · 2016-06-09T22:58:27Z

stdlib/public/runtime/Errors.cpp

@@ -50,7 +60,7 @@ enum: uint32_t {

 using namespace swift;

-#if !defined(__CYGWIN__) && !defined(__ANDROID__)
+#if SWIFT_SUPPORTS_BACKTRACE_REPORTING


Thank you, this is much cleaner now!

gribozavr · 2016-06-11T10:05:25Z

All changes LGTM.

gribozavr · 2016-06-11T10:05:28Z

@swift-ci Please test and merge

[pull] swiftwasm from main

tkremenek assigned gribozavr Mar 29, 2016

gribozavr reviewed Mar 29, 2016
View reviewed changes

gribozavr reviewed Mar 30, 2016
View reviewed changes

tinysun212 force-pushed the pr-stdlib-msvc-1 branch from d3315ce to 1f9fe14 Compare March 30, 2016 07:46

tinysun212 reviewed Mar 30, 2016
View reviewed changes

tinysun212 force-pushed the pr-stdlib-msvc-1 branch from 1f9fe14 to 9aafb90 Compare March 30, 2016 08:09

shawnce mentioned this pull request Mar 30, 2016

[runtime] enhanced and refactored recently added Mutex abstraction #1950

Merged

1 task

tinysun212 closed this Apr 2, 2016

tinysun212 deleted the pr-stdlib-msvc-1 branch April 2, 2016 09:00

tinysun212 restored the pr-stdlib-msvc-1 branch April 2, 2016 09:05

tinysun212 reopened this Apr 2, 2016

tinysun212 force-pushed the pr-stdlib-msvc-1 branch from 5dc18b6 to 1045c7d Compare April 2, 2016 09:39

shawnce reviewed Apr 3, 2016
View reviewed changes

gribozavr reviewed May 30, 2016
View reviewed changes

tinysun212 mentioned this pull request Jun 1, 2016

[stdlib/msvc] Runtime with MSVC library (Part) #2829

Merged

1 task

jckarter reviewed Jun 2, 2016
View reviewed changes

stdlib/msvc: Runtime with MSVC library

198441b

This patch is for libswiftCore.lib, linking with the library set of Visual Studio 2015. Clang with the option -fms-extension is used to build.

tinysun212 force-pushed the pr-stdlib-msvc-1 branch from a92e146 to 198441b Compare June 3, 2016 20:38

gribozavr reviewed Jun 3, 2016
View reviewed changes

Modified for some reviews

e6ecc9a

tinysun212 force-pushed the pr-stdlib-msvc-1 branch from 0a10fc3 to e6ecc9a Compare June 9, 2016 21:27

gribozavr reviewed Jun 9, 2016
View reviewed changes

swift-ci merged commit 91ffcd7 into swiftlang:master Jun 11, 2016

tinysun212 deleted the pr-stdlib-msvc-1 branch October 21, 2018 00:09

MaxDesiatov pushed a commit that referenced this pull request Oct 19, 2020

Merge pull request #1918 from swiftwasm/main

ea7d6e8

[pull] swiftwasm from main

[stdlib/msvc] Runtime with MSVC library #1918

[stdlib/msvc] Runtime with MSVC library #1918

Uh oh!

Conversation

tinysun212 commented Mar 29, 2016

What's in this pull request?

Resolved bug number: (SR-)

Uh oh!

tkremenek commented Mar 29, 2016

Uh oh!

tkremenek commented Mar 29, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tinysun212 commented Mar 29, 2016

Uh oh!

gribozavr commented Mar 29, 2016

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!

tinysun212 commented Apr 2, 2016

Uh oh!

gribozavr commented Apr 2, 2016

Uh oh!

shawnce commented Apr 2, 2016

Uh oh!

tinysun212 commented Apr 2, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gribozavr commented May 30, 2016

Uh oh!

tinysun212 commented May 30, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gribozavr commented Jun 3, 2016

Uh oh!

tinysun212 commented Jun 3, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tinysun212 commented Jun 3, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gribozavr commented Jun 3, 2016

Uh oh!

jckarter commented Jun 6, 2016

Uh oh!

tinysun212 commented Jun 9, 2016

Uh oh!

tinysun212 commented Jun 9, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gribozavr commented Jun 11, 2016

Uh oh!

gribozavr commented Jun 11, 2016

Uh oh!

Uh oh!

tinysun212 commented Jun 3, 2016 •

edited

Loading