Skip to content
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

[cxx-interop] Support importing value types exported from Swift #76777

Closed
wants to merge 1 commit into from

Conversation

Xazax-hun
Copy link
Contributor

@Xazax-hun Xazax-hun commented Sep 30, 2024

Whenever a Swift type is exported to C++, we create a corresponding type on the
C++ side. However, we did not support importing value types back into Swift.
This PR addresses this problem.

Unfortunately, we cannot just set the C++ type as the ClangNode of
Swift's type. Types with ClangNodes are using the foreign
representation, but in this case we want to use the native type whenever
possible (e.g., for codegen). To work this around, this PR updates
ClangTypeConverter to make sure we have the correct Clang types when we
emit function calls to C++ APIs. It also updates the importer to not
attempt to import types that are annotated with ExternalSourceSymbolAttr
attributes as Swift types.

rdar://136964764

@Xazax-hun Xazax-hun added the c++ interop Feature: Interoperability with C++ label Sep 30, 2024
@Xazax-hun
Copy link
Contributor Author

This should the first address half of #76024 . I plan to address the rest in a follow-up PR as doing the same for Optional seems to pose some unique challenges.

@Xazax-hun
Copy link
Contributor Author

@swift-ci please smoke test

lib/AST/ClangTypeConverter.cpp Outdated Show resolved Hide resolved
lib/AST/ClangTypeConverter.h Show resolved Hide resolved
lib/ClangImporter/ImportDecl.cpp Outdated Show resolved Hide resolved
decl->getIdentifier() && decl->getName() == "String") {
auto swiftDecl = Impl.SwiftContext.getStringDecl();
Impl.SwiftContext.registerExportedClangDecl(swiftDecl,
decl->getCanonicalDecl());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this state change on import going to be susceptible to ordering issues? For example, could we end up querying the ClangTypeConverter for Swift's String without having seen the C++ swift::String? It might be more robust if ClangTypeConverter::convert were able to call into the Clang importer to ask "Do you have a mapping for this type?".

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this should not happen now. We populate this map when we import the declarations and query it during lowering. Currently, ClangTypeConverter::convert should handle missing mappings gracefully. But I can move the code around if you think it would be more straightforward that way.

@rjmccall
Copy link
Contributor

This seems like something we want to generalize, so we should set up that generalization now instead of hard-coding namespaces and identifiers.

IIRC, there are two cases in Swift-to-C++ export: some types can be directly exported, and some types need a wrapper that indirects the storage. We should design this so that the importer imports both into Swift as their native Swift type. Directly-exported types can stay as their native type in SIL. Wrapper types should be different types in SIL, and we should just implicitly bridge back and forth with the native type, like we do with Objective-C. We can start with just the directly-exported types, of course.

I think we should use attributes for this. It should be straightforward to put an attribute on the exported type that declares that it's a direct export of a Swift type. The wrapper-export thing would be nice to generalize.

@Xazax-hun Xazax-hun force-pushed the gaborh/import-swift-string branch from fb3b479 to 4732b28 Compare October 1, 2024 11:21
@Xazax-hun
Copy link
Contributor Author

I think we should use attributes for this.

It turns out we already annotate exported types with the ExternalSourceSymbolAttr attribute. I changed this patch to rely on this attribute instead of hardcoding the types. Let me know what you think.

@Xazax-hun Xazax-hun changed the title [cxx-interop] Support importing swift::String cxx-interop] Support importing value types exported from Swift Oct 1, 2024
@Xazax-hun Xazax-hun changed the title cxx-interop] Support importing value types exported from Swift [cxx-interop] Support importing value types exported from Swift Oct 1, 2024
@Xazax-hun
Copy link
Contributor Author

@swift-ci please smoke test

@Xazax-hun Xazax-hun force-pushed the gaborh/import-swift-string branch from 4732b28 to 1ee1ffb Compare October 1, 2024 13:21
@Xazax-hun
Copy link
Contributor Author

@swift-ci please smoke test

@rjmccall
Copy link
Contributor

rjmccall commented Oct 1, 2024

I think we should use attributes for this.

It turns out we already annotate exported types with the ExternalSourceSymbolAttr attribute. I changed this patch to rely on this attribute instead of hardcoding the types. Let me know what you think.

If there's an existing attribute that exactly matches what we want, great, but we should understand what this attribute is meant for before layering assumptions on top of it. In particular, which type gets annotated with this attribute for wrapped types?

@Xazax-hun
Copy link
Contributor Author

Xazax-hun commented Oct 2, 2024

In particular, which type gets annotated with this attribute for wrapped types?

We add this annotation to all of the types in the interop header when exporting Swift types to C++. For example:

class SWIFT_SYMBOL("s:SS") String final {
 ...
 private:
   swift::_impl::OpaqueStorage _storage;
}

I suspect that this is what you meant by a wrapper that indirects the storage.

Wrapper types should be different types in SIL

Could you elaborate on this? What type would you expect to see in the SIL in this case and what lowered type would you expect? With this patch, we would use the native Swift type in the SIL, and would lower it to the C++ wrapper type when we call a C++ foreign function. My understanding was that this should be OK since they are layout compatible, but let me know if this is wrong.

Directly-exported types can stay as their native type in SIL

By directly exported types, do you mean foreign types that have a clang node? Or @objc types? Or is there something else?

@rjmccall
Copy link
Contributor

rjmccall commented Oct 2, 2024

If we're going to import something from C++ and then use it in SIL interchangeably with that Swift type, it's important that it actually have the exact same ABI in C++ and Swift. So if we generate a type in C++ that indirects the storage — and yeah, what you're describing above is exactly what I'm thinking about, although I'm a little surprised we do it for String — then okay, we can still round-trip it into Swift, but we need to treat it as a different type at the SIL layer and then bridge between those types in SILGen.

If we generate a type in C++ that exactly matches the Swift type in its ABI (which is what I mean by "directly-exported"), we can round-trip it back into both Swift and SIL in the way you're suggesting. If we generate a type in C++ that just has a pointer or an opaque value buffer in it or something, we can round-trip it into Swift in the importer, but SIL needs to model it as a different type. So the immediate questions are (1) are there actually any types that get directly exported in this way? and (2) do we already have the information that we would need in order to distinguish these two cases?

@Xazax-hun Xazax-hun force-pushed the gaborh/import-swift-string branch from 1ee1ffb to 85c358d Compare October 3, 2024 10:09
@Xazax-hun
Copy link
Contributor Author

Xazax-hun commented Oct 3, 2024

Thanks for the explanation!

Indeed, I dug a bit deeper, and there are two kinds of exported classes. One with an opaque pointer, like Optional:

template<class T_0_0>
class SWIFT_SYMBOL("s:Sq") Optional final {
 ...
   swift::_impl::OpaqueStorage _storage;
};

And one with an inline buffer like String (I got lost in the generated header, so I was actually wrong about the representation for String, sorry for the confusion):

class SWIFT_SYMBOL("s:SS") String final {
...
  alignas(8) char _storage[16];
};

When we print these classes, we check whether the printed Swift type has a statically known layout by checking if it has FixedTypeInfo as its type information. If it does, we use the inline buffer, otherwise we use the OpaqueStorage.

Based on my initial testing, round-tripping String worked just fine, but round-tripping Optional did not. I updated this patch to only allow round-tripping value types when their representations are using the inline buffer that should be compatible with the layout of the Swift type.

I plan to resolve the OpaqueStorage case in a follow-up PR.

@Xazax-hun
Copy link
Contributor Author

@swift-ci please smoke test

@@ -7717,26 +7722,44 @@ ValueDecl *
CxxRecordAsSwiftType::evaluate(Evaluator &evaluator,
CxxRecordSemanticsDescriptor desc) const {
auto cxxDecl = dyn_cast<clang::CXXRecordDecl>(desc.decl);
if (!cxxDecl)
// TODO: support specializations like swift::Optional<int>
if (!cxxDecl || isa<clang::ClassTemplateSpecializationDecl>(cxxDecl))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you need the template specialization check here? Optional gets ruled out by your storage check below, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

True, the storage check rules out swift::Optional as of today. But not necessarily in the future, if/when swift::Optional<TrivialType> has a different representation.

}
}
if (isOpaqueLayout)
return nullptr;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't love this attempt to pattern-match exactly what we produce in export, especially because I know that export needs to be producing different patterns than this in some cases, like when a struct contains floats. But if you're going to do it, you should really do it properly: check that there's exactly one field, check that the field has the name you expect, and check that its type is exactly a constant-size array of char.

Copy link
Contributor Author

@Xazax-hun Xazax-hun Oct 4, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed, I am also not the fan of pattern matching on the contents of the record. I looked into an alternative approach, but it required significantly more plumbing (need to heap allocate an IRABIDetailsProvider somewhere and plumb through a pointer a couple of calls). I figured since we plan to support round-tripping for all types, we might prefer the simpler check for a temporary solution. (Also, not matching on some type here is relatively benign, we end up not importing it.) Let me know if you prefer the other approach even for a temporary workaround. In the meantime I made the pattern matching stricter as you suggested.

Whenever a Swift type is exported to C++, we create a corresponding type on the
C++ side. However, we did not support importing value types back into Swift.
This PR addresses this problem.

Unfortunately, we cannot just set the C++ type as the ClangNode of
Swift's  type. Types with ClangNodes are using the foreign
representation, but in this case we want to use the native type whenever
possible (e.g., for codegen). To work this around, this PR updates
ClangTypeConverter to make sure we have the correct Clang types when we
emit function calls to C++ APIs. It also updates the importer to not
attempt to import types that are annotated with ExternalSourceSymbolAttr
attributes as Swift types.

rdar://136964764
@Xazax-hun Xazax-hun force-pushed the gaborh/import-swift-string branch from 85c358d to 599e01e Compare October 4, 2024 09:09
@Xazax-hun
Copy link
Contributor Author

@swift-ci please smoke test

@Xazax-hun
Copy link
Contributor Author

After some discussions about this feature we decided to go with another approach. I will close this PR and open a new one hopefully soon.

@Xazax-hun Xazax-hun closed this Oct 16, 2024
@Xazax-hun Xazax-hun deleted the gaborh/import-swift-string branch November 21, 2024 12:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ interop Feature: Interoperability with C++
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants