Port swift/remote to Windows and MSVC #6025

hughbe · 2016-12-02T12:06:20Z

Don't rely on dlfcn.h, use Win32 APIs instead. This requires adding a CPP file to avoid importing <windows.h> in a header.
Fix Error C2248 'anonymous-namespace'::RemoteASTContextImpl::fail': cannot access private member declared in class 'anonymous-namespace'::RemoteASTContextImpl' by making a private method protected.
Fix error C2872: 'ReflectionContext': ambiguous symbol by qualifying the namespace of ReflectionContext
Unhandled control path warnings - have to use std::cerr and std::abort as we get linker errors when we use llvm_unreachable

compnerd · 2016-12-03T20:49:58Z

include/swift/Remote/MetadataReader.h

@@ -756,6 +757,9 @@ class MetadataReader {
      return BuiltOpaque;
    }
    }
+
+    std::cerr << "fatal error: " << "Unhandled MetadataKind in switch." << "\n";
+    std::abort();


Is this better handled with a covered switch and llvm_unreachable?

We get linker errors compiling swiftCore.dll if we use llvm_unreachable. The errors are unresolved external llvm_unreachable_internal. This is because we can't/don't link any LLVM libraries from the stdlib.

Ah right. I think that having a swift_unreachable is going to be cleaner. We could model it after llvm_unreachable.

compnerd · 2016-12-03T20:54:45Z

lib/RemoteAST/InProcessMemoryReader.cpp

+
+RemoteAddress InProcessMemoryReader::getSymbolAddress(const std::string &name) {
+#if defined(_WIN32)
+    HMODULE module = GetModuleHandle(0);


Yes, 0 is equivalent, but please use NULL as the documentation states.

Right - fixed, thanks

compnerd · 2016-12-03T20:55:30Z

stdlib/public/SwiftRemoteMirror/SwiftRemoteMirror.cpp

@@ -175,6 +175,9 @@ swift_layout_kind_t getTypeInfoKind(const TypeInfo &TI) {
    }
  }
  }
+
+  std::cerr << "fatal error: " << "Unhandled TypeInfoKind in switch." << "\n";
+  std::abort();


Covered switch + llvm_unreachable?

compnerd · 2016-12-08T03:02:50Z

include/swift/Remote/MetadataReader.h

@@ -756,6 +757,9 @@ class MetadataReader {
      return BuiltOpaque;
    }
    }
+
+    std::cerr << "fatal error: " << "Unhandled MetadataKind in switch." << "\n";
+    std::abort();


Ah right. I think that having a swift_unreachable is going to be cleaner. We could model it after llvm_unreachable.

compnerd · 2016-12-08T03:04:56Z

lib/RemoteAST/InProcessMemoryReader.cpp

+#if defined(_WIN32)
+    HMODULE module = GetModuleHandle(NULL);
+    auto pointer = GetProcAddress(module, name.c_str());
+    FreeLibrary(module);


GetModuleHandle does not increment reference count for the module. When you are invoking FreeLibrary here, you are decrementing the refcount on the module. This can cause the main module to hit a reference count of 0, and be unloaded! Am I missing something?

The GetModuleHandle function returns a handle to a mapped module without incrementing its reference count. However, if this handle is passed to the FreeLibrary function, the reference count of the mapped module will be decremented. Therefore, do not pass a handle returned by GetModuleHandle to the FreeLibrary function. Doing so can cause a DLL module to be unmapped prematurely.

According to the docs - my bad

hughbe · 2016-12-08T10:50:47Z

Thanks @compnerd. I introduced a swift_unreachable API. I made it a static method as there's no real benefit to it being a macro, I think. Also, I stuck to C-only APIs (assert and abort) to avoid Swift compilation errors - we still get a customized message and a stack trace so that's fine.

compnerd

LGTM

compnerd · 2016-12-09T03:06:12Z

include/swift/Basic/Unreachable.h

+
+SWIFT_ATTRIBUTE_NORETURN
+inline static void swift_unreachable(const char* msg) {
+  assert(false && msg);


Don't you get an unused variable warning when building release? (_NDEBUG).

I think so, haven't done a release build - I've fixed this

compnerd · 2016-12-09T03:06:55Z

lib/RemoteAST/InProcessMemoryReader.cpp

+RemoteAddress InProcessMemoryReader::getSymbolAddress(const std::string &name) {
+#if defined(_WIN32)
+    HMODULE module = GetModuleHandle(NULL);
+    auto pointer = GetProcAddress(module, name.c_str());


Could've inlined the GetModuleHandle.

Sure, fixed

compnerd · 2016-12-10T01:05:42Z

include/swift/Remote/MetadataReader.h


+#include <iostream>


Don't think that iostream is needed any more.

Correct, fixed thanks

compnerd · 2016-12-10T17:16:49Z

@swift-ci please test and merge

hughbe · 2016-12-10T18:04:17Z

@compnerd sorry for the churn, tests passed but this didnt merge

compnerd · 2016-12-10T19:24:12Z

@swift-ci Please smoke test OS X platform

compnerd · 2016-12-10T19:24:30Z

@hughbe going to be a bit more paranoid and run the smoke tests on macOS before merging.

jrose-apple · 2016-12-12T21:13:05Z

include/swift/Basic/Unreachable.h

+#endif
+
+SWIFT_ATTRIBUTE_NORETURN
+inline static void swift_unreachable(const char* msg) {


Ah, please don't name this swift_unreachable—we still want people to prefer llvm_unreachable when possible. How about swift_runtime_unreachable?

…if there isn't already such a thing somewhere in the real runtime, in which case we should use that.

Could we just use llvm_unreachable with a runtime-local implementation of llvm_unreachable_internal for debug builds? Unreachable branches should be optimized as really unreachable in release builds of the runtime.

That could be even better. I'm honestly fairly unhappy about us having runtime things in include/swift/Basic/, but maybe I should be unhappy about having runtime libraries in lib/.

I was thinking we'd just reimplement llvm_unreachable_internal locally inside the runtime. We already have to provide a reimplementation of some LLVM hashing stuff to be able to use DenseMap inside the runtime.

Oh, I didn't realize we'd actually bitten the bullet and put DenseMap in. Should we just link llvmSupport and let the linker figure it out? ;-)

@jrose-apple, while you propose that (probably) in jest, that would help with another issue that recently came up. The ABI breaking validation for LLVM uses a value in llvmSuppprt. We ended up having to add another build option to disable it and force disable for swift.

It's worth a shot. If linking llvmSupport into the runtime doesn't affect the binary size too much, it'd certainly be preferable to cherry-picking certain definitions we need.

hughbe · 2016-12-16T11:49:57Z

stdlib/public/SwiftRemoteMirror/SwiftRemoteMirror.cpp

@@ -20,7 +21,7 @@ using namespace swift::reflection;
 using namespace swift::remote;

 using NativeReflectionContext
-  = ReflectionContext<External<RuntimeTarget<sizeof(uintptr_t)>>>;
+  = swift::reflection::ReflectionContext<External<RuntimeTarget<sizeof(uintptr_t)>>>;


Duplicate of https://connect.microsoft.com/VisualStudio/feedback/details/3116636/msvc-reports-ambiguous-symbol-error-for-friend-class-declaration-in-an-anonymous-namespace

compnerd reviewed Dec 3, 2016

View reviewed changes

compnerd requested changes Dec 8, 2016

View reviewed changes

This was referenced Dec 8, 2016

Fix warnings building swift/ABI with MSVC #6031

Merged

Fix errors and warnings building swift/reflection on Windows with MSVC #6026

Merged

compnerd approved these changes Dec 9, 2016

View reviewed changes

compnerd reviewed Dec 10, 2016

View reviewed changes

Fix errors and warnings building swift/remote on Windows using MSVC

53744b9

compnerd merged commit ac6edab into swiftlang:master Dec 10, 2016

hughbe deleted the remote-msvc branch December 11, 2016 09:52

jrose-apple reviewed Dec 12, 2016

View reviewed changes

hughbe commented Dec 16, 2016

View reviewed changes


		#include <iostream>

Port swift/remote to Windows and MSVC #6025

Port swift/remote to Windows and MSVC #6025

Uh oh!

Conversation

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

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!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hughbe commented Dec 8, 2016

Uh oh!

compnerd left a comment

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!

compnerd commented Dec 10, 2016

Uh oh!

hughbe commented Dec 10, 2016

Uh oh!

compnerd commented Dec 10, 2016

Uh oh!

compnerd commented Dec 10, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jckarter Dec 12, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

Uh oh!

hughbe Dec 8, 2016 •

edited

Loading

jckarter Dec 12, 2016 •

edited

Loading