Merge pull request #82725 from Xazax-hun/dependence-on-sharedobj-is-unsafe

[cxx-interop] Lifetime dependence on a class is unsafe

Author: Xazax-hun

lib/ClangImporter/ImportDecl.cpp

@@ -63,6 +63,7 @@
 #include "clang/AST/Decl.h"
 #include "clang/AST/DeclCXX.h"
 #include "clang/AST/DeclObjCCommon.h"
+#include "clang/AST/DeclTemplate.h"
 #include "clang/AST/Expr.h"
 #include "clang/AST/PrettyPrinter.h"
 #include "clang/AST/RecordLayout.h"
@@ -4194,6 +4195,20 @@ namespace {
       return false;
     }
 
+    static bool canTypeMutateBuffer(clang::QualType ty) {
+      // FIXME: better way to detect if a type can mutate the underlying buffer.
+      if (const auto *rd = ty->getAsRecordDecl()) {
+        if (rd->isInStdNamespace() && rd->getName() == "span")
+          return !cast<clang::ClassTemplateSpecializationDecl>(rd)
+                      ->getTemplateArgs()
+                      .get(0)
+                      .getAsType()
+                      .isConstQualified();
+      }
+      // Conservatively assume most types can mutate the underlying buffer.
+      return true;
+    }
+
     void addLifetimeDependencies(const clang::FunctionDecl *decl,
                                  AbstractFunctionDecl *result) {
       if (decl->getTemplatedKind() == clang::FunctionDecl::TK_FunctionTemplate)
@@ -4205,12 +4220,23 @@ namespace {
           !isa<clang::CXXMethodDecl, clang::ObjCMethodDecl>(decl))
         return;
 
+      bool hasSkippedLifetimeAnnotation = false;
       auto isEscapable = [this](clang::QualType ty) {
         return evaluateOrDefault(
                    Impl.SwiftContext.evaluator,
                    ClangTypeEscapability({ty.getTypePtr(), &Impl}),
                    CxxEscapability::Unknown) != CxxEscapability::NonEscapable;
       };
+      auto importedAsClass = [this](clang::QualType ty, bool forSelf) {
+        if (!forSelf) {
+          if (ty->getPointeeType().isNull())
+            return false;
+          ty = ty->getPointeeType();
+        }
+        if (const auto *rd = ty->getAsRecordDecl())
+          return recordHasReferenceSemantics(rd);
+        return false;
+      };
 
       auto swiftParams = result->getParameters();
       bool hasSelf =
@@ -4237,6 +4263,7 @@ namespace {
       }
 
       auto retType = decl->getReturnType();
+      bool retMayMutateBuffer = canTypeMutateBuffer(retType);
       auto warnForEscapableReturnType = [&] {
         if (isEscapableAnnotatedType(retType.getTypePtr())) {
           Impl.addImportDiagnostic(
@@ -4252,8 +4279,11 @@ namespace {
       SmallBitVector scopedLifetimeParamIndicesForReturn(dependencyVecSize);
       SmallBitVector paramHasAnnotation(dependencyVecSize);
       std::map<unsigned, SmallBitVector> inheritedArgDependences;
-      auto processLifetimeBound = [&](unsigned idx, clang::QualType ty) {
+      auto processLifetimeBound = [&](unsigned idx, clang::QualType ty,
+                                      bool forSelf = false) {
         warnForEscapableReturnType();
+        if (retMayMutateBuffer && importedAsClass(ty, forSelf))
+          hasSkippedLifetimeAnnotation = true;
         paramHasAnnotation[idx] = true;
         if (isEscapable(ty))
           scopedLifetimeParamIndicesForReturn[idx] = true;
@@ -4302,7 +4332,8 @@ namespace {
       if (getImplicitObjectParamAnnotation<clang::LifetimeBoundAttr>(decl))
         processLifetimeBound(
             result->getSelfIndex(),
-            cast<clang::CXXMethodDecl>(decl)->getThisType()->getPointeeType());
+            cast<clang::CXXMethodDecl>(decl)->getThisType()->getPointeeType(),
+            /*forSelf=*/true);
       if (auto attr =
               getImplicitObjectParamAnnotation<clang::LifetimeCaptureByAttr>(
                   decl))
@@ -4352,16 +4383,21 @@ namespace {
             Impl.SwiftContext.AllocateCopy(lifetimeDependencies));
       }
 
-      for (auto [idx, param] : llvm::enumerate(decl->parameters())) {
-        if (isEscapable(param->getType()))
-          continue;
-        if (param->hasAttr<clang::NoEscapeAttr>() || paramHasAnnotation[idx])
-          continue;
-        // We have a nonescapable parameter that does not have its lifetime
-        // annotated nor is it marked noescape.
+      if (hasSkippedLifetimeAnnotation) {
         auto attr = new (ASTContext) UnsafeAttr(/*implicit=*/true);
         result->getAttrs().add(attr);
-        break;
+      } else {
+        for (auto [idx, param] : llvm::enumerate(decl->parameters())) {
+          if (isEscapable(param->getType()))
+            continue;
+          if (param->hasAttr<clang::NoEscapeAttr>() || paramHasAnnotation[idx])
+            continue;
+          // We have a nonescapable parameter that does not have its lifetime
+          // annotated nor is it marked noescape.
+          auto attr = new (ASTContext) UnsafeAttr(/*implicit=*/true);
+          result->getAttrs().add(attr);
+          break;
+        }
       }
 
       Impl.diagnoseTargetDirectly(decl);
@@ -9549,13 +9585,22 @@ void ClangImporter::Implementation::swiftify(AbstractFunctionDecl *MappedDecl) {
       SIW_DBG("  Found bounds info '" << clang::QualType(CAT, 0) << "' on return value\n");
       attachMacro = true;
     }
+    auto requiresExclusiveClassDependency = [](ParamDecl *fromParam,
+                                               clang::QualType toType) {
+      return fromParam->getInterfaceType()->isAnyClassReferenceType() &&
+             SwiftDeclConverter::canTypeMutateBuffer(toType);
+    };
     bool returnHasLifetimeInfo = false;
     if (SwiftDeclConverter::getImplicitObjectParamAnnotation<
             clang::LifetimeBoundAttr>(ClangDecl)) {
       SIW_DBG("  Found lifetimebound attribute on implicit 'this'\n");
-      printer.printLifetimeboundReturn(SwiftifyInfoPrinter::SELF_PARAM_INDEX,
-                                       true);
-      returnHasLifetimeInfo = true;
+      if (!requiresExclusiveClassDependency(
+              MappedDecl->getImplicitSelfDecl(/*createIfNeeded*/ true),
+              ClangDecl->getReturnType())) {
+        printer.printLifetimeboundReturn(SwiftifyInfoPrinter::SELF_PARAM_INDEX,
+                                         true);
+        returnHasLifetimeInfo = true;
+      }
     }
 
     bool isClangInstanceMethod =
@@ -9612,14 +9657,18 @@ void ClangImporter::Implementation::swiftify(AbstractFunctionDecl *MappedDecl) {
       if (clangParam->hasAttr<clang::LifetimeBoundAttr>()) {
         SIW_DBG("  Found lifetimebound attribute on parameter '"
                         << *clangParam << "'\n");
-        // If this parameter has bounds info we will tranform it into a Span,
-        // so then it will no longer be Escapable.
-        bool willBeEscapable = swiftParamTy->isEscapable() &&
-            (!paramHasBoundsInfo ||
-             mappedIndex == SwiftifyInfoPrinter::SELF_PARAM_INDEX);
-        printer.printLifetimeboundReturn(mappedIndex, willBeEscapable);
-        paramHasLifetimeInfo = true;
-        returnHasLifetimeInfo = true;
+        if (!requiresExclusiveClassDependency(swiftParam,
+                                              ClangDecl->getReturnType())) {
+          // If this parameter has bounds info we will tranform it into a Span,
+          // so then it will no longer be Escapable.
+          bool willBeEscapable =
+              swiftParamTy->isEscapable() &&
+              (!paramHasBoundsInfo ||
+               mappedIndex == SwiftifyInfoPrinter::SELF_PARAM_INDEX);
+          printer.printLifetimeboundReturn(mappedIndex, willBeEscapable);
+          paramHasLifetimeInfo = true;
+          returnHasLifetimeInfo = true;
+        }
       }
       if (paramIsStdSpan && paramHasLifetimeInfo) {
         SIW_DBG("  Found both std::span and lifetime info "

test/Interop/Cxx/class/safe-interop-mode.swift

@@ -64,10 +64,14 @@ View safeFunc(View v1 [[clang::noescape]], View v2 [[clang::lifetimebound]]);
 void unsafeFunc(View v1 [[clang::noescape]], View v2);
 
 class SharedObject {
+public:
+  View getView() const [[clang::lifetimebound]];
 private:
   int *p;
 } SWIFT_SHARED_REFERENCE(retainSharedObject, releaseSharedObject);
 
+View getViewFromSharedObject(SharedObject* p [[clang::lifetimebound]]);
+
 inline void retainSharedObject(SharedObject *) {}
 inline void releaseSharedObject(SharedObject *) {}
 
@@ -161,6 +165,10 @@ func useUnsafeLifetimeAnnotated(v: View) {
 func useSharedReference(frt: SharedObject, x: OwnedData) {
   let _ = frt
   x.takeSharedObject(frt)
+  // expected-warning@+1{{expression uses unsafe constructs but is not marked with 'unsafe'}}
+  let _ = frt.getView() // expected-note{{reference to unsafe instance method 'getView()'}}
+  // expected-warning@+1{{expression uses unsafe constructs but is not marked with 'unsafe'}}
+  let _ = getViewFromSharedObject(frt) // expected-note{{reference to unsafe global function 'getViewFromSharedObject'}}
 }
 
 @available(SwiftStdlib 5.8, *)

test/Interop/Cxx/stdlib/Inputs/std-span.h

@@ -65,6 +65,7 @@ inline SpanOfInt initSpan(int arr[], size_t size) {
 struct DependsOnSelf {
   std::vector<int> v;
   __attribute__((swift_name("get()")))
+  __attribute__((swift_attr("@safe")))
   ConstSpanOfInt get() const [[clang::lifetimebound]] { return ConstSpanOfInt(v.data(), v.size()); }
 };
 
@@ -181,4 +182,20 @@ inline void mutableKeyword(SpanOfInt copy [[clang::noescape]]) {}
 inline void spanWithoutTypeAlias(std::span<const int> s [[clang::noescape]]) {}
 inline void mutableSpanWithoutTypeAlias(std::span<int> s [[clang::noescape]]) {}
 
+#define IMMORTAL_FRT                                                           \
+  __attribute__((swift_attr("import_reference")))                              \
+  __attribute__((swift_attr("retain:immortal")))                               \
+  __attribute__((swift_attr("release:immortal")))
+
+struct IMMORTAL_FRT DependsOnSelfFRT {
+  std::vector<int> v;
+  __attribute__((swift_name("get()"))) ConstSpanOfInt get() const
+      [[clang::lifetimebound]] {
+    return ConstSpanOfInt(v.data(), v.size());
+  }
+  SpanOfInt getMutable() [[clang::lifetimebound]] {
+    return SpanOfInt(v.data(), v.size());
+  }
+};
+
 #endif // TEST_INTEROP_CXX_STDLIB_INPUTS_STD_SPAN_H

test/Interop/Cxx/stdlib/std-span-interface.swift

@@ -38,6 +38,17 @@ import CxxStdlib
 // CHECK-NEXT:   mutating func otherTemplatedType2(_ copy: ConstSpanOfInt, _: UnsafeMutablePointer<S<CInt>>!)
 // CHECK-NEXT: }
 
+// CHECK: class DependsOnSelfFRT {
+// CHECK-NEXT:   init()
+// CHECK-NEXT:   /// This is an auto-generated wrapper for safer interop
+// CHECK-NEXT:   @available(visionOS 1.0, tvOS 12.2, watchOS 5.2, iOS 12.2, macOS 10.14.4, *)
+// CHECK-NEXT:   @_lifetime(borrow self)
+// CHECK-NEXT:   @_alwaysEmitIntoClient @_disfavoredOverload public borrowing func get() -> Span<CInt>
+// CHECK-NEXT:   borrowing func get() -> ConstSpanOfInt
+// CHECK-NEXT:   borrowing func {{(__)?}}getMutable{{(Unsafe)?}}() -> SpanOfInt
+// CHECK-NEXT:   var v: std.{{.*}}vector<CInt, std.{{.*}}allocator<CInt>>
+// CHECK-NEXT: }
+
 // CHECK:      /// This is an auto-generated wrapper for safer interop
 // CHECK-NEXT: @available(visionOS 1.0, tvOS 12.2, watchOS 5.2, iOS 12.2, macOS 10.14.4, *)
 // CHECK-NEXT: @_lifetime(s: copy s)