From de531403c5ddc857265a40266981651aacb790d0 Mon Sep 17 00:00:00 2001 From: "Henrik G. Olsson" Date: Wed, 3 Dec 2025 17:32:44 -0800 Subject: [PATCH 1/2] [Swiftify] don't add macro when required type is unavailable A Swift module only imports a type definition once. This means that some clang modules may have function signatures imported with `UnsafePointer` even if `qux` is only forward declared in that module (which would normally be imported as `OpaquePointer`), because some Swift file in the module imported the clang module with the definition, so ClangImporter sees the definition. The macro expansion only imports the imports of the clang module it belongs to however, so namelookup for `qux` fails. This patch detects when this will result in an error and avoids attaching the macro in those cases. rdar://165864358 --- lib/ClangImporter/SwiftifyDecl.cpp | 51 +++++- .../conflicting-opaqueness.swift | 148 ++++++++++++++++++ 2 files changed, 195 insertions(+), 4 deletions(-) create mode 100644 test/Interop/C/swiftify-import/conflicting-opaqueness.swift diff --git a/lib/ClangImporter/SwiftifyDecl.cpp b/lib/ClangImporter/SwiftifyDecl.cpp index 9a7770117092f..2b332fa5cffab 100644 --- a/lib/ClangImporter/SwiftifyDecl.cpp +++ b/lib/ClangImporter/SwiftifyDecl.cpp @@ -24,8 +24,10 @@ #include "clang/AST/ASTContext.h" #include "clang/AST/Attr.h" #include "clang/AST/Decl.h" +#include "clang/AST/RecursiveASTVisitor.h" #include "clang/AST/StmtVisitor.h" #include "clang/AST/Type.h" +#include "clang/Basic/Module.h" using namespace swift; using namespace importer; @@ -271,14 +273,15 @@ struct CountedByExpressionValidator }; -// Don't try to transform any Swift types that _SwiftifyImport doesn't know how -// to handle. -static bool SwiftifiableCountedByPointerType(Type swiftType) { +static bool IsConcretePointerType(Type swiftType) { Type nonnullType = swiftType->lookThroughSingleOptionalType(); PointerTypeKind PTK; return nonnullType->getAnyPointerElementType(PTK) && (PTK == PTK_UnsafePointer || PTK == PTK_UnsafeMutablePointer); } + +// Don't try to transform any Swift types that _SwiftifyImport doesn't know how +// to handle. static bool SwiftifiableSizedByPointerType(const clang::ASTContext &ctx, Type swiftType, const clang::CountAttributedType *CAT) { @@ -311,7 +314,7 @@ static bool SwiftifiableCAT(const clang::ASTContext &ctx, return CAT && CountedByExpressionValidator().Visit(CAT->getCountExpr()) && (CAT->isCountInBytes() ? SwiftifiableSizedByPointerType(ctx, swiftType, CAT) - : SwiftifiableCountedByPointerType(swiftType)); + : IsConcretePointerType(swiftType)); } // Searches for template instantiations that are not behind type aliases. @@ -331,6 +334,36 @@ struct UnaliasedInstantiationVisitor } }; +struct ForwardDeclaredConcreteTypeVisitor + : clang::RecursiveASTVisitor { + bool hasForwardDeclaredConcreteType = false; + clang::Module *Owner; + + ForwardDeclaredConcreteTypeVisitor(clang::Module *Owner) : Owner(Owner) {}; + + bool VisitRecordType(clang::RecordType *RT) { + const clang::RecordDecl *RD = RT->getDecl()->getDefinition(); + ASSERT(RD && "pointer to concrete type without type definition?"); + const clang::Module *M = RD->getOwningModule(); + if (!Owner->isModuleVisible(M)) { + hasForwardDeclaredConcreteType = true; + DLOG("Imported signature contains concrete type not available in clang module, skipping"); + LLVM_DEBUG(DUMP(RD)); + return false; + } + return true; + } + + bool IsIncompatibleImport(Type SwiftTy, clang::QualType ClangTy) { + DLOG_SCOPE("Checking compatibility of type: " << ClangTy << "\n"); + if (!IsConcretePointerType(SwiftTy)) { + return false; + } + TraverseType(ClangTy); + return hasForwardDeclaredConcreteType; + } +}; + // until CountAttributedType::getAttributeName lands in our LLVM branch static StringRef getAttributeName(const clang::CountAttributedType *CAT) { switch (CAT->getKind()) { @@ -375,6 +408,8 @@ static bool swiftifyImpl(ClangImporter::Implementation &Self, clang::ASTContext &clangASTContext = Self.getClangASTContext(); + ForwardDeclaredConcreteTypeVisitor CheckForwardDecls(ClangDecl->getOwningModule()); + // We only attach the macro if it will produce an overload. Any __counted_by // will produce an overload, since UnsafeBufferPointer is still an improvement // over UnsafePointer, but std::span will only produce an overload if it also @@ -389,6 +424,10 @@ static bool swiftifyImpl(ClangImporter::Implementation &Self, else ABORT("Unexpected AbstractFunctionDecl subclass."); clang::QualType clangReturnTy = ClangDecl->getReturnType(); + + if (CheckForwardDecls.IsIncompatibleImport(swiftReturnTy, clangReturnTy)) + return false; + bool returnIsStdSpan = printer.registerStdSpanTypeMapping( swiftReturnTy, clangReturnTy); auto *CAT = clangReturnTy->getAs(); @@ -440,6 +479,10 @@ static bool swiftifyImpl(ClangImporter::Implementation &Self, } ASSERT(swiftParam); Type swiftParamTy = swiftParam->getInterfaceType(); + + if (CheckForwardDecls.IsIncompatibleImport(swiftParamTy, clangParamTy)) + return false; + bool paramHasBoundsInfo = false; auto *CAT = clangParamTy->getAs(); if (CAT && mappedIndex == SwiftifyInfoPrinter::SELF_PARAM_INDEX) { diff --git a/test/Interop/C/swiftify-import/conflicting-opaqueness.swift b/test/Interop/C/swiftify-import/conflicting-opaqueness.swift new file mode 100644 index 0000000000000..a000c541e74d1 --- /dev/null +++ b/test/Interop/C/swiftify-import/conflicting-opaqueness.swift @@ -0,0 +1,148 @@ +// REQUIRES: swift_feature_SafeInteropWrappers + +// RUN: %empty-directory(%t) +// RUN: split-file %s %t + +// RUN: %target-swift-frontend -emit-module -plugin-path %swift-plugin-dir -o %t/test.swiftmodule %t/test.swift -I %t -enable-experimental-feature SafeInteropWrappers -strict-memory-safety \ +// RUN: -verify -verify-additional-file %t%{fs-sep}foo.h -verify-additional-file %t%{fs-sep}bar.h -verify-additional-file %t%{fs-sep}baz.h -verify-additional-file %t%{fs-sep}qux.h -Rmacro-expansions + +// Macro expansions in foo.h do not have access to the definition of `struct qux`, +// so don't attach macro on functions that use contain that type in foo.h. +// bar.h imports the type, so attach the macro. +// baz.h imports bar.h which re-exports the type, so attach the macro there too. + +//--- foo.h +#pragma once + +#define __counted_by(x) __attribute__((__counted_by__(x))) + +struct qux; +// expected-note@+1{{'foo' declared here}} +void foo(struct qux *x, int * __counted_by(len) p, int len); +// expected-note@+1{{'fooReturn' declared here}} +struct qux * fooReturn(int * __counted_by(len) p, int len); + + +//--- bar.h +#pragma once +#include "qux.h" + +#define __counted_by(x) __attribute__((__counted_by__(x))) + +// expected-expansion@+10:59{{ +// expected-remark@1{{macro content: |/// This is an auto-generated wrapper for safer interop|}} +// expected-remark@2{{macro content: |@_alwaysEmitIntoClient @_disfavoredOverload public func bar(_ x: UnsafeMutablePointer!, _ p: UnsafeMutableBufferPointer) {|}} +// expected-remark@3{{macro content: | let len = Int32(exactly: p.count)!|}} +// expected-remark@4{{macro content: | return unsafe bar(x, p.baseAddress!, len)|}} +// expected-remark@5{{macro content: |}|}} +// }} +// expected-expansion@+3:6{{ +// expected-note@1 5{{in expansion of macro '_SwiftifyImport' on global function 'bar' here}} +// }} +void bar(struct qux *x, int * __counted_by(len) p, int len); +// expected-expansion@+10:58{{ +// expected-remark@1{{macro content: |/// This is an auto-generated wrapper for safer interop|}} +// expected-remark@2{{macro content: |@_alwaysEmitIntoClient @_disfavoredOverload public func barReturn(_ p: UnsafeMutableBufferPointer) -> UnsafeMutablePointer! {|}} +// expected-remark@3{{macro content: | let len = Int32(exactly: p.count)!|}} +// expected-remark@4{{macro content: | return unsafe barReturn(p.baseAddress!, len)|}} +// expected-remark@5{{macro content: |}|}} +// }} +// expected-expansion@+3:14{{ +// expected-note@1 5{{in expansion of macro '_SwiftifyImport' on global function 'barReturn' here}} +// }} +struct qux * barReturn(int * __counted_by(len) p, int len); + + +//--- baz.h +#pragma once + +#include "bar.h" + +struct qux; +// expected-expansion@+10:59{{ +// expected-remark@1{{macro content: |/// This is an auto-generated wrapper for safer interop|}} +// expected-remark@2{{macro content: |@_alwaysEmitIntoClient @_disfavoredOverload public func baz(_ x: UnsafeMutablePointer!, _ p: UnsafeMutableBufferPointer) {|}} +// expected-remark@3{{macro content: | let len = Int32(exactly: p.count)!|}} +// expected-remark@4{{macro content: | return unsafe baz(x, p.baseAddress!, len)|}} +// expected-remark@5{{macro content: |}|}} +// }} +// expected-expansion@+3:6{{ +// expected-note@1 5{{in expansion of macro '_SwiftifyImport' on global function 'baz' here}} +// }} +void baz(struct qux *x, int * __counted_by(len) p, int len); +// expected-expansion@+10:58{{ +// expected-remark@1{{macro content: |/// This is an auto-generated wrapper for safer interop|}} +// expected-remark@2{{macro content: |@_alwaysEmitIntoClient @_disfavoredOverload public func bazReturn(_ p: UnsafeMutableBufferPointer) -> UnsafeMutablePointer! {|}} +// expected-remark@3{{macro content: | let len = Int32(exactly: p.count)!|}} +// expected-remark@4{{macro content: | return unsafe bazReturn(p.baseAddress!, len)|}} +// expected-remark@5{{macro content: |}|}} +// }} +// expected-expansion@+3:14{{ +// expected-note@1 5{{in expansion of macro '_SwiftifyImport' on global function 'bazReturn' here}} +// }} +struct qux * bazReturn(int * __counted_by(len) p, int len); + + +//--- qux.h +#pragma once + +struct qux { int placeholder; }; + + +//--- test.swift +import Foo +import Bar +import Baz + +func callFoo(_ x: UnsafeMutablePointer, _ p: UnsafeMutablePointer, _ len: CInt) { + unsafe foo(x, p, len) + let _: UnsafeMutablePointer = unsafe fooReturn(p, len) +} + +func callFoo2(_ x: UnsafeMutablePointer, _ p: UnsafeMutableBufferPointer) { + // expected-error@+2{{missing argument for parameter #3 in call}} + // expected-error@+1{{cannot convert value of type 'UnsafeMutableBufferPointer' (aka 'UnsafeMutableBufferPointer') to expected argument type 'UnsafeMutablePointer'}} + unsafe foo(x, p) + // expected-error@+2{{missing argument for parameter #2 in call}} + // expected-error@+1{{cannot convert value of type 'UnsafeMutableBufferPointer' (aka 'UnsafeMutableBufferPointer') to expected argument type 'UnsafeMutablePointer'}} + let _: UnsafeMutablePointer = unsafe fooReturn(p) +} + + +func callBar(_ x: UnsafeMutablePointer, _ p: UnsafeMutablePointer, _ len: CInt) { + unsafe bar(x, p, len) + let _: UnsafeMutablePointer = unsafe barReturn(p, len) +} + +func callBar2(_ x: UnsafeMutablePointer, _ p: UnsafeMutableBufferPointer) { + unsafe bar(x, p) + let _: UnsafeMutablePointer = unsafe barReturn(p) +} + + +func callBaz(_ x: UnsafeMutablePointer, _ p: UnsafeMutablePointer, _ len: CInt) { + unsafe baz(x, p, len) + let _: UnsafeMutablePointer = unsafe bazReturn(p, len) +} + +func callBaz2(_ x: UnsafeMutablePointer, _ p: UnsafeMutableBufferPointer) { + unsafe baz(x, p) + let _: UnsafeMutablePointer = unsafe bazReturn(p) +} + + +//--- module.modulemap +module Foo { + header "foo.h" +} +module Bar { + header "bar.h" + export qux +} +module Baz { + header "baz.h" + export Bar +} +module qux { + header "qux.h" +} From 1e2c9de1b9be290dd7f781d345d05cd18a7d0b79 Mon Sep 17 00:00:00 2001 From: "Henrik G. Olsson" Date: Tue, 16 Dec 2025 14:19:08 -0800 Subject: [PATCH 2/2] [Swiftify] fix missing owning module Implicit decls don't have an owning module, since multiple modules could instantiate them on demand. Since no implicit functions currently have any need for safe wrappers we can simply detect this and early exit. For templated functions, we need to fetch the owning module from the instantiation. --- lib/ClangImporter/SwiftifyDecl.cpp | 25 ++++++++++++++++++++++--- 1 file changed, 22 insertions(+), 3 deletions(-) diff --git a/lib/ClangImporter/SwiftifyDecl.cpp b/lib/ClangImporter/SwiftifyDecl.cpp index 2b332fa5cffab..54c59c177cad7 100644 --- a/lib/ClangImporter/SwiftifyDecl.cpp +++ b/lib/ClangImporter/SwiftifyDecl.cpp @@ -24,6 +24,7 @@ #include "clang/AST/ASTContext.h" #include "clang/AST/Attr.h" #include "clang/AST/Decl.h" +#include "clang/AST/DeclObjC.h" #include "clang/AST/RecursiveASTVisitor.h" #include "clang/AST/StmtVisitor.h" #include "clang/AST/Type.h" @@ -337,9 +338,9 @@ struct UnaliasedInstantiationVisitor struct ForwardDeclaredConcreteTypeVisitor : clang::RecursiveASTVisitor { bool hasForwardDeclaredConcreteType = false; - clang::Module *Owner; + const clang::Module *Owner; - ForwardDeclaredConcreteTypeVisitor(clang::Module *Owner) : Owner(Owner) {}; + ForwardDeclaredConcreteTypeVisitor(const clang::Module *Owner) : Owner(Owner) { ASSERT(Owner); }; bool VisitRecordType(clang::RecordType *RT) { const clang::RecordDecl *RD = RT->getDecl()->getDefinition(); @@ -393,6 +394,13 @@ static size_t getNumParams(const clang::FunctionDecl* D) { } } // namespace +static const clang::Decl *getTemplateInstantiation(const clang::FunctionDecl *D) { + return D->getTemplateInstantiationPattern(); +} +static const clang::Decl *getTemplateInstantiation(const clang::ObjCMethodDecl *) { + return nullptr; +} + template static bool swiftifyImpl(ClangImporter::Implementation &Self, SwiftifyInfoFunctionPrinter &printer, @@ -406,9 +414,20 @@ static bool swiftifyImpl(ClangImporter::Implementation &Self, ClangDecl->getAccess() == clang::AS_private) return false; + if (ClangDecl->isImplicit()) { + DLOG("implicit functions lack lifetime and bounds info"); + return false; + } + clang::ASTContext &clangASTContext = Self.getClangASTContext(); - ForwardDeclaredConcreteTypeVisitor CheckForwardDecls(ClangDecl->getOwningModule()); + const clang::Module *OwningModule = nullptr; + if (const auto *Instance = getTemplateInstantiation(ClangDecl)) { + OwningModule = Instance->getOwningModule(); + } else { + OwningModule = ClangDecl->getOwningModule(); + } + ForwardDeclaredConcreteTypeVisitor CheckForwardDecls(OwningModule); // We only attach the macro if it will produce an overload. Any __counted_by // will produce an overload, since UnsafeBufferPointer is still an improvement