Introduce SWIFT_EXTERN_RUNTIME_EXPORT for Windows #6820
Conversation
|
@swift-ci Please smoke test |
| @@ -216,6 +216,7 @@ | |||
| // But some of the runtime entries are invoked directly from | |||
| // the foundation. Therefore they should be visible. | |||
| #define SWIFT_RT_ENTRY_VISIBILITY SWIFT_RUNTIME_EXPORT | |||
| #define SWIFT_EXTERN_RT_ENTRY_VISIBILITY SWIFT_EXTERN_RUNTIME_EXPORT | |||
There was a problem hiding this comment.
I think this should be SWIFT_RT_EXTERN_ENTRY_VISIBILITY at the very least.
Is there ever a case we wouldnt want the extern "C" on the interface?
There was a problem hiding this comment.
Casting.cpp
- https://github.com/apple/swift/blob/master/stdlib/public/runtime/Casting.cpp#L266
- https://github.com/apple/swift/blob/master/stdlib/public/runtime/Casting.cpp#L2423
As well as in Enum.cpp
- https://github.com/apple/swift/blob/master/stdlib/public/runtime/Enum.cpp#L203
- https://github.com/apple/swift/blob/master/stdlib/public/runtime/Enum.cpp#L2
Heap.cpp
- https://github.com/apple/swift/blob/master/stdlib/public/runtime/Heap.cpp#L25
- https://github.com/apple/swift/blob/master/stdlib/public/runtime/Heap.cpp#L34
As well as HeapObject.cpp
- https://github.com/apple/swift/blob/master/stdlib/public/runtime/Heap.cpp#L34
- https://github.com/apple/swift/blob/master/stdlib/public/runtime/HeapObject.cpp#L307
- https://github.com/apple/swift/blob/master/stdlib/public/runtime/HeapObject.cpp#L330
- https://github.com/apple/swift/blob/master/stdlib/public/runtime/HeapObject.cpp#L386
- https://github.com/apple/swift/blob/master/stdlib/public/runtime/HeapObject.cpp#L402
- https://github.com/apple/swift/blob/master/stdlib/public/runtime/HeapObject.cpp#L410
- https://github.com/apple/swift/blob/master/stdlib/public/runtime/HeapObject.cpp#L415
- https://github.com/apple/swift/blob/master/stdlib/public/runtime/HeapObject.cpp#L432
- https://github.com/apple/swift/blob/master/stdlib/public/runtime/HeapObject.cpp#L463
- https://github.com/apple/swift/blob/master/stdlib/public/runtime/HeapObject.cpp#L475
- https://github.com/apple/swift/blob/master/stdlib/public/runtime/HeapObject.cpp#L605
As well as Metadata.cpp
- https://github.com/apple/swift/blob/master/stdlib/public/runtime/Metadata.cpp#L230
- https://github.com/apple/swift/blob/master/stdlib/public/runtime/Metadata.cpp#L2364
As well as SwiftObject.mm
There was a problem hiding this comment.
Judging by the number of these, I say we keep these two separate? I'll rename to SWIFT_RT...
There was a problem hiding this comment.
Every export from the Swift runtime ought to be extern "C".
There was a problem hiding this comment.
Hmm, interesting only cpp files have export without extern - maybe this is an oversight.
Do we make the cpp files use extern C, or simply remove them - not sure if the headers are already annotated as extern C
There was a problem hiding this comment.
I think a better way to make intentions explicit there would be to have a SWIFT_CALLED_VIA_SILGEN_NAME marker. That would incorporate extern "C" and potentially be useful for keeping the C++ side and the Swift side in sync.
There was a problem hiding this comment.
…but IMO that ought to be a job for a follow-up PR.
There was a problem hiding this comment.
Yeah I like that. I'll submit a PR once I get the other extern c stuff merged :)
Cheers
There was a problem hiding this comment.
…but IMO that ought to be a job for a follow-up PR.
Took the words out of my mouth ;)
There was a problem hiding this comment.
Great minds think alike, obviously.
| #else | ||
| #define SWIFT_EXTERN_RUNTIME_EXPORT SWIFT_RUNTIME_EXPORT extern "C" | ||
| #endif | ||
|
|
There was a problem hiding this comment.
This should be unnecessary. Use the form from _MSC_VER clause always.
|
Okay, I've updated this PR to do the following:
I've tested on Windows, everything works, but I'll trigger on Linux/OSX (for objc only methods) before the review I'm attaching a summary of my work as well - it details examples where we used one of these macros without extern "C". Most of these will be cleaned up in a future PR SWIFT_RT_ENTRY_VISIBILITY.txt |
|
@swift-ci please smoke test |
1 similar comment
|
@swift-ci please smoke test |
|
@swift-ci Please smoke test |
|
@hughbe Sorry, for now only those with commit access can trigger CI. |
|
I do have commit access! |
|
Oh, well then CI was just being flaky. |
|
@swift-ci please test |
|
Build failed |
|
Build failed |
|
@compnerd Mind clicking the big green button? :) |
|
Yeah, given the follow up work and the changes now, LG. |
|
Thanks, Hugh. |
IRGen includes Metdata.h. This means that Metadata.h has to be able to be compiled with MSVC, even though its part of the runtime.
The correct syntax for dll exporting an extern C symbol is
extern "C" __declspec(dllexport_) ...However, current we do
__declspec(dllexport) ... extern "C"This means that we get several hundred MSVC errors compiling IRGen.
The fix is to introduce a macro,
SWIFT_EXTERN_RUNTIME_EXPORTthat fixes thedeclspecerrors.