Make Objective-C interoperability configurable in the runtime #40843
Conversation
|
Corresponding LLDB patch is in swiftlang/llvm-project#3789 |
|
This patch changes the class hierarchy to TargetAnyClassMetadataObjCInterop inherits from TargetAnyClassMetadata because most of the implementation is the same. This choice makes TargetClassMetadata a bit tricky. In this patch I went with templating the parent class. |
| @@ -540,7 +580,7 @@ struct TargetMetadata { | |||
|
|
|||
| #if SWIFT_OBJC_INTEROP | |||
There was a problem hiding this comment.
This is great! As a followup, can we eliminate some or all of the #if SWIFT_OBJC_INTEROP conditional compilations in these headers? (It would make sense for runtime implementation to still use #if SWIFT_OBJC_INTEROP, to implement operations that fundamentally rely on the ObjC runtime, but it'd be cool to make as much functionality as possible dependent only on the template.)
There was a problem hiding this comment.
One thing that wasn't clear to me is how much of the code depends on definitions in objc system headers, but I'm happy to eliminate as much of the conditionals as possible.
adrian-prantl
force-pushed
the
87179578
branch
from
d2f1ca3 to
88bc9c7
Compare
In order to be able to debug, for example, a Linux process from a macOS host, we
need to be able to initialize a ReflectionContext without Objective-C
interoperability. This patch turns ObjCInterOp into another template trait, so
it's possible to instantiate a non-ObjC MetadataReader on a system built with
ObjC-interop (but not vice versa).
This patch changes the class hierarchy to
TargetMetadata<Runtime>
|
TargetHeapMetadata<Runtime>
|
TargetAnyClassMetadata<Runtime>
/ \
/ TargetAnyClassMetadataObjCInterop<Runtime>
/ \
TargetClassMetadata<Runtime, TargetAnyClassMetadata<Runtime>> \
\
TargetClassMetadata<Runtime, TargetAnyClassMetadataObjCInterop<Runtime>>
TargetAnyClassMetadataObjCInterop inherits from TargetAnyClassMetadata because
most of the implementation is the same. This choice makes TargetClassMetadata a
bit tricky. In this patch I went with templating the parent class.
rdar://87179578
adrian-prantl
force-pushed
the
87179578
branch
from
88bc9c7 to
e84c6cc
Compare
adrian-prantl
changed the title
|
Removing the [WIP] tag. This patch is ready for review now and passes Swift and LLDB tests on the Mac. |
|
@swift-ci test |
|
test with swiftlang/llvm-project#3789 |
|
Build failed |
|
Build failed |
|
Swift-CI test failures are unrelated. |
| @@ -120,14 +150,44 @@ struct ExternalPointer { | |||
| StoredPointer PointerValue; | |||
| }; | |||
|
|
|||
| /// An external process's runtime target, which may be a different architecture. | |||
| template <typename Runtime> struct WithObjCInterop { | |||
There was a problem hiding this comment.
This seems a little awkward, especially with the nesting at the point of use. Would it be reasonable to make ObjC interop another parameter of RuntimeTarget instead?
There was a problem hiding this comment.
It's possible to sink this into a
template <unsigned PointerSize, bool WithObjCInterOp> struct RuntimeTarget
the downside is that since we need to conditionally define TargetAnyClassMetadata, we'd end up with 4 template specializations:
template <> struct RuntimeTarget<4, false>
template <> struct RuntimeTarget<4, true>
template <> struct RuntimeTarget<8, false>
template <> struct RuntimeTarget<8, true>
which take roughly the same amount of lines as the current definition with its reexporting using declarations. I like the separate nested template a little more since it doesn't duplicate any definitions and it would scale better if we have to add more dimensions, but I don't feel very strongly about it.
There was a problem hiding this comment.
I was thinking it could be a partial specialization like template <bool WithObjCInterOp> struct RuntimeTarget<4>, then a little more magic to resolve TargetAnyClassMetadata based on the remaining template parameter. Not entirely sure if that's possible, though. If not then I agree with you that the separate nested template is better.
There was a problem hiding this comment.
I'm not a very experienced template metaprogrammer, but in my experiments it looked like template specialization and using separate nested templates (like in the patch) are the only ways to make a using type declaration configurable. When I discovered std::conditional I thought looked like a good fit, but I could not actually get it to work without instantiation failures deep in the ClassMetadata hierarchy.
There was a problem hiding this comment.
std::conditional ought to work, but this stuff is really difficult to debug if you don't get it right.
Given all of that, let's go ahead and keep it as-is, and then if something ever comes up where we really want it to be in RuntimeTarget we can try again then.
|
test with swiftlang/llvm-project#3789 |
|
Build failed |
|
Build failed |
In order to be able to debug, for example, a Linux process from a macOS host, we need to be able to initialize a ReflectionContext without Objective-C interoperability. This patch turns ObjCInterOp into another template trait, so it's possible to instantiate a non-ObjC MetadataReader on a system built with ObjC-interop (but not vice versa).
Posting this early to get feedback on my amateurish C++ templating.
rdar://87179578