-
Notifications
You must be signed in to change notification settings - Fork 10.6k
Objc interop #12702
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Objc interop #12702
Conversation
|
@swift-ci Please smoke test |
dc27eb5 to
676d296
Compare
|
@swift-ci please smoke test |
|
@swift-ci please smoke test |
|
@swift-ci please smoke test |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems like a good change. Some comments, though…
(Also, there should be tests that show that both modes work on Linux now. I care less about both modes working on Darwin.)
lib/ClangImporter/ClangImporter.cpp
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems suspicious to me. Checking the implementation of clang::ObjCRuntime, iOS 2 is treated very differently from, say, iOS 7 (Swift's earliest iOS deployment target). Can you at least leave a note to re-inspect this later?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can update this to ios-7.0, I missed that version specifications are allowed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure if that's the right answer either, but it's probably good enough for now.
lib/ClangImporter/ClangImporter.cpp
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This part does only belong in the Darwin section, because the compiler version numbers are only set in official Apple builds. I'd rather not have people start using that for other purposes at this point in time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cc @compnerd - this is part of your change
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jrose-apple sure, thats easy enough to move. However, that does seem useful to have in general. Any reason to not always have the swift compiler version made available?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't set this value at all for open source builds. (This is the "900.0.30" thing, not "4.0.2".)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We barely even use it at Apple; there's a chance we might take it out altogether.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jrose-apple SGTM, I'll remove that
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
EnableObjCInterop won't be on for these invocations, though, right?. They're all using -target and there's no argument for -enable-objc-interop in swift-ide-test.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hrm, I'm not super familiar with the code yet, but it looks to me like EnableObjCInterop is actually defaulted to true in LangOptions.h. A few of the tools default it to isOSDarwin() (Frontend/CompilerInvocation.cpp, sil-llvm-gen, sil-opt), but swift-ide-test doesn't modify the value. Not sure the best approach here, since swift-ide-test doesn't look like it plays much with flags currently.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting. I feel like swift-ide-test should follow the other tools to defaulting it based on the target triple and allowing that to be overridden.
c4f7530 to
8ca2803
Compare
|
@swift-ci please smoke test |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please respect both options in all modes. The target just chooses the default.
(I know this has the same effect assuming people don't pass both flags, but it's not a good way to write it, in that I didn't realize that immediately.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe that this already handles all the cases except the case where both flags are set simultaneously. I can handle it more explicitly though if that's preferred.
Current behavior:
darwin + disable = disabled
darwin + enable = enabled (by virtue of disable being unset and falling back to the default)
non-darwin + enable = enabled
non-darwin + disable = disabled (by virtue of enable being unset and falling back to the default)
Not sure what we'd want to do in the case where somebody sets both (or if we even need to handle that).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(disregard my comment, looks like we passed each other while typing)
|
@swift-ci please smoke test |
Select the language for the clang importer based on the `-enable-objc-interop` flag rather than target. This paves the road to enabling ObjC tests on non-Darwin targets as well as building on Darwin without ObjC interop.
This allows objc interop to be tested on non-darwin platforms. The iOS runtime is used because the abi is the same as the macOS abi, but without fragility concerns.
This adds a -disable-objc-interop flag to allow objc-interop to be disabled, and an -enable-objc-interop flag to allow objc-interop to be enabled. Objc interop will default to enabled on darwin, and disabled on other platforms.
|
@swift-ci please smoke test |
| // TODO: Investigate whether 7.0 is a suitable default version. | ||
| if (!triple.isOSDarwin()) | ||
| invocationArgStrs.insert(invocationArgStrs.end(), | ||
| {"-fobjc-runtime=ios-7.0"}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will let us make some progress, but eventually you'll need to decide which Objective-C runtime(s) to target outside of Darwin. For example, Clang supports GNUstep, and one could imagine surfacing Clang's understanding of the Objective-C runtime so that Swift could build on top of it (rather than duplicating it).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't have much time to work on it, but I'm happy to provide advice to anyone who wants to add support for the GNUstep runtime (and add any missing runtime features that Swift needs).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Until we get GNUStep to the point where the object layout works in a compatible manner (or change the swift object layout), I think that not permitting control over the runtime is quite reasonable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are Swift's requirements documented anywhere?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think so. Off the top of my head two of the biggest requirements are:
- Swift wants to generate ObjC class structures (at compile time and at runtime) with additional Swift data prepended and appended.
- Swift wants control of a bit in the ObjC class structure in order to mark ObjC classes that are also Swift classes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The second requirement is easy - we have a flags field and can reserve a bit for Swift to use.
The first is a bit harder because the clang generates these structures without providing hooks to add fields. The structure is defined here.
This will change in the next version of the runtime, and the current prototype for the new ABI would make this more difficult, because it puts all of the class structures in a section and the loader in the runtime expects them to be contiguous in memory, so adding other fields would mean that the runtime would need to be aware of the structure.
I'm also rearranging some of the fields to split the structure into ones that are likely to be modified at run time and ones that aren't, based on feedback from Microsoft that the runtime generates more CoW faults at start than they'd like - I'm not sure where the Swift fields would like to go, but if it's on the immutable part then that's probably easier, because that will be reachable via a pointer to the start from the mutable part, and that can easily be a pointer to the middle of a larger structure.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me at this point. Thanks, Francis! (and sorry for the delay)
This PR includes the changes made by @compnerd in #12657 to support objc interop, in addition to a patch to allow objc interop testing on non-darwin platforms.