[C++ interop] Broaden support for the 'swift_private' attribute. #85461
Conversation
junov-google
requested review from
Xazax-hun,
beccadax,
egorzhdan,
ian-twilightcoder,
j-hui and
susmonteiro
as code owners
|
Hi reviewers, this is my first time contributing to this project, so please be vigilant as I may have overlooked important details that I'm not familiar with. The location in the code where I added the attribute check (ClangImporter::Implementation::importDeclImpl) works correctly, but I am not 100% confident about whether it's the best place to do this check. |
|
|
||
| // Don't import this decl if it has `__attribute__((swift_private))` | ||
| if (ClangDecl->hasAttr<clang::SwiftPrivateAttr>()) | ||
| return nullptr; |
There was a problem hiding this comment.
I'm concerned that short-circuiting importDecl() like this will interfere with the importation of field decls, which are needed by imported records to infer properties like layout and Sendable conformance.
There was a problem hiding this comment.
Sorry, I guess that feedback is not actually very constructive/instructive on what actually needs to be done. I don't know myself either because I haven't investigated this thoroughly myself yet. But this is certainly a bug that needs to be fixed, so thank you for raising this issue and working on it!
There's some references to SwiftPrivateAttr in ImportName.cpp that suggests this attribute also causes names to be mangled with leading __. But I'm not clear on when that happens vs when the attribute causes a decl to not be imported altogether. I think that should be clarified.
To address my earlier point about record layouts, I think your PR should also include some executable tests that make sure we are generating code based on invalid layout information.
And—this is more of a nit—if we are skipping the per-decl visitor methods entirely if we encounter SwiftPrivateAttr in importDeclImpl(), I think we should also remove the equivalent code from there, e.g., VisitNamespaceDecl().
There was a problem hiding this comment.
In my latest push, I added an executable test that validates that this change does not affect memory layout. I also cleaned-up VisitNamespaceDecl.
I am still working on the Sendable conformance thing...
There was a problem hiding this comment.
The inference of Sendable conformance, which you alluded to in your initial comment, seems to be generally broken when importing via C++ interop. Basically, C++ structs and classes are always assumed to be Sendable. This is how I am testing it:
func checkSendable<T: Sendable>(_: T.Type) {}
let _ = checkSendable(NonSendableType.self)
This does not generate any compiler errors if NonSendableType was imported from C++, regardless of what the it does in C++. I tried structs with mutable state, with raw pointers to mutable objects, with methods that access global variable or thread-local storage, etc. Nothing I do in C++ will cause checkSendable(NonSendableType.self) to generate a failure or warning related to lack of Sendable conformance. The only way I found to trip it is by explicitly making the type non-Sendable in swift (post import) like this:
@available(*, unavailable)
extension NonSendableType : @retroactive Sendable {}
Motivation: It is often desirable to explicitly hide C++ declaraitons from swift either because some entry-points are known to be unsafe to call from swift or to suppress "unavailable (cannot import)" messages. Before this change, it was possible to exclude declarationis by using '#if !defined(__swift__)`, but that approach is often unsafe because it can lead to undetected ODR violations (undetected because linkers do not check ODR by default), which can lead to memory corruption. A better approach is to use an annotation that excludes declarations from being visible to swift without affecting class definitions to avoid divergent v-tables and object layouts. `__attribute__((swift_private))` does this, but that attribute was not being checked on most types of C++ declarations. Description of changes: * Added a check for the swift_private attribute in `ClangImporter::Implementation::importDeclImpl` in order to exclude declarations from import. * Added test coverage for use of the swift_private attribute on methods, fields, functions and classes in test ClangImporter/cxx_interop.swift
junov-google
force-pushed
the
junov/cxx_interop_swift_private
branch
from
090e849 to
03b0a67
Compare
|
@swift-ci please test |
4 participants
This change broadens support for the
swift_privateattribute so that it works as expected on all typesof imported C++ declarations.
Discussion: https://forums.swift.org/t/something-equivalent-to-ns-swift-unavailable-but-for-c-interop/83149
Description of changes:
ClangImporter::Implementation::importDeclImplin order to exclude declarations from being imported.