[cxx-interop] Synthesized derived-to-base field getter should copy out a retainable FRT value

This matches the semantics of accessing the same field from the base class

Author: hyp

lib/ClangImporter/ClangImporter.cpp

@@ -4846,6 +4846,11 @@ static clang::CXXMethodDecl *synthesizeCxxBaseMethod(
   newMethod->setImplicit();
   newMethod->setImplicitlyInline();
   newMethod->setAccess(clang::AccessSpecifier::AS_public);
+  if (method->hasAttr<clang::CFReturnsRetainedAttr>()) {
+    // Return an FRT field at +1 if the base method also follows this
+    // convention.
+    newMethod->addAttr(clang::CFReturnsRetainedAttr::CreateImplicit(clangCtx));
+  }
 
   llvm::SmallVector<clang::ParmVarDecl *, 4> params;
   for (size_t i = 0; i < method->getNumParams(); ++i) {
@@ -5047,7 +5052,8 @@ synthesizeBaseClassMethodBody(AbstractFunctionDecl *afd, void *context) {
 // to the base class while the field is accessed.
 static clang::CXXMethodDecl *synthesizeCxxBaseGetterAccessorMethod(
     ClangImporter &impl, const clang::CXXRecordDecl *derivedClass,
-    const clang::CXXRecordDecl *baseClass, const clang::FieldDecl *field) {
+    const clang::CXXRecordDecl *baseClass, const clang::FieldDecl *field,
+    ValueDecl *retainOperationFn) {
   auto &clangCtx = impl.getClangASTContext();
   auto &clangSema = impl.getClangSema();
 
@@ -5078,51 +5084,95 @@ static clang::CXXMethodDecl *synthesizeCxxBaseGetterAccessorMethod(
   newMethod->setImplicit();
   newMethod->setImplicitlyInline();
   newMethod->setAccess(clang::AccessSpecifier::AS_public);
+  if (retainOperationFn) {
+    // Return an FRT field at +1.
+    newMethod->addAttr(clang::CFReturnsRetainedAttr::CreateImplicit(clangCtx));
+  }
 
   // Create a new Clang diagnostic pool to capture any diagnostics
   // emitted during the construction of the method.
   clang::sema::DelayedDiagnosticPool diagPool{
       clangSema.DelayedDiagnostics.getCurrentPool()};
   auto diagState = clangSema.DelayedDiagnostics.push(diagPool);
 
+  // Returns the expression that accesses the base field from derived type.
+  auto createFieldAccess = [&]() -> clang::Expr * {
+    auto *thisExpr = new (clangCtx)
+        clang::CXXThisExpr(clang::SourceLocation(), newMethod->getThisType(),
+                           /*IsImplicit=*/false);
+    clang::QualType baseClassPtr = clangCtx.getRecordType(baseClass);
+    baseClassPtr.addConst();
+    baseClassPtr = clangCtx.getPointerType(baseClassPtr);
+
+    clang::CastKind Kind;
+    clang::CXXCastPath Path;
+    clangSema.CheckPointerConversion(thisExpr, baseClassPtr, Kind, Path,
+                                     /*IgnoreBaseAccess=*/false,
+                                     /*Diagnose=*/true);
+    auto conv = clangSema.ImpCastExprToType(thisExpr, baseClassPtr, Kind,
+                                            clang::VK_PRValue, &Path);
+    if (!conv.isUsable())
+      return nullptr;
+    auto memberExpr = clangSema.BuildMemberExpr(
+        conv.get(), /*isArrow=*/true, clang::SourceLocation(),
+        clang::NestedNameSpecifierLoc(), clang::SourceLocation(),
+        const_cast<clang::FieldDecl *>(field),
+        clang::DeclAccessPair::make(const_cast<clang::FieldDecl *>(field),
+                                    clang::AS_public),
+        /*HadMultipleCandidates=*/false,
+        clang::DeclarationNameInfo(field->getDeclName(),
+                                   clang::SourceLocation()),
+        returnType, clang::VK_LValue, clang::OK_Ordinary);
+    auto returnCast = clangSema.ImpCastExprToType(
+        memberExpr, returnType, clang::CK_LValueToRValue, clang::VK_PRValue);
+    if (!returnCast.isUsable())
+      return nullptr;
+    return returnCast.get();
+  };
+
+  llvm::SmallVector<clang::Stmt *, 2> body;
+  if (retainOperationFn) {
+    // Check if the returned value needs to be retained. This might occur if the
+    // field getter is returning a shared reference type using, as it needs to
+    // perform the retain to match the expected @owned convention.
+    auto *retainClangFn =
+        dyn_cast<clang::FunctionDecl>(retainOperationFn->getClangDecl());
+    if (!retainClangFn) {
+      return nullptr;
+    }
+    auto *fnRef = new (clangCtx) clang::DeclRefExpr(
+        clangCtx, const_cast<clang::FunctionDecl *>(retainClangFn), false,
+        retainClangFn->getType(), clang::ExprValueKind::VK_LValue,
+        clang::SourceLocation());
+    auto fieldExpr = createFieldAccess();
+    if (!fieldExpr)
+      return nullptr;
+    auto retainCall = clangSema.BuildResolvedCallExpr(
+        fnRef, const_cast<clang::FunctionDecl *>(retainClangFn),
+        clang::SourceLocation(), {fieldExpr}, clang::SourceLocation());
+    if (!retainCall.isUsable())
+      return nullptr;
+    body.push_back(retainCall.get());
+  }
+
   // Construct the method's body.
-  auto *thisExpr = new (clangCtx) clang::CXXThisExpr(
-      clang::SourceLocation(), newMethod->getThisType(), /*IsImplicit=*/false);
-  clang::QualType baseClassPtr = clangCtx.getRecordType(baseClass);
-  baseClassPtr.addConst();
-  baseClassPtr = clangCtx.getPointerType(baseClassPtr);
-
-  clang::CastKind Kind;
-  clang::CXXCastPath Path;
-  clangSema.CheckPointerConversion(thisExpr, baseClassPtr, Kind, Path,
-                                   /*IgnoreBaseAccess=*/false,
-                                   /*Diagnose=*/true);
-  auto conv = clangSema.ImpCastExprToType(thisExpr, baseClassPtr, Kind,
-                                          clang::VK_PRValue, &Path);
-  if (!conv.isUsable())
-    return nullptr;
-  auto memberExpr = clangSema.BuildMemberExpr(
-      conv.get(), /*isArrow=*/true, clang::SourceLocation(),
-      clang::NestedNameSpecifierLoc(), clang::SourceLocation(),
-      const_cast<clang::FieldDecl *>(field),
-      clang::DeclAccessPair::make(const_cast<clang::FieldDecl *>(field),
-                                  clang::AS_public),
-      /*HadMultipleCandidates=*/false,
-      clang::DeclarationNameInfo(field->getDeclName(), clang::SourceLocation()),
-      returnType, clang::VK_LValue, clang::OK_Ordinary);
-  auto returnCast = clangSema.ImpCastExprToType(
-      memberExpr, returnType, clang::CK_LValueToRValue, clang::VK_PRValue);
-  if (!returnCast.isUsable())
+  auto fieldExpr = createFieldAccess();
+  if (!fieldExpr)
     return nullptr;
   auto returnStmt = clang::ReturnStmt::Create(clangCtx, clang::SourceLocation(),
-                                              returnCast.get(), nullptr);
+                                              fieldExpr, nullptr);
+  body.push_back(returnStmt);
 
   // Check if there were any Clang errors during the construction
   // of the method body.
   clangSema.DelayedDiagnostics.popWithoutEmitting(diagState);
   if (!diagPool.empty())
     return nullptr;
-  newMethod->setBody(returnStmt);
+  newMethod->setBody(body.size() > 1
+                         ? clang::CompoundStmt::Create(
+                               clangCtx, body, clang::FPOptionsOverride(),
+                               clang::SourceLocation(), clang::SourceLocation())
+                         : body[0]);
   return newMethod;
 }
 
@@ -5163,12 +5213,28 @@ synthesizeBaseClassFieldGetterBody(AbstractFunctionDecl *afd, void *context) {
                   RemoveReference,
         /*forceConstQualifier=*/true);
   } else if (auto *fd = dyn_cast_or_null<clang::FieldDecl>(baseClangDecl)) {
+    ValueDecl *retainOperationFn = nullptr;
+    // Check if this field getter is returning a retainable FRT.
+    if (getterDecl->getResultInterfaceType()->isForeignReferenceType()) {
+      auto retainOperation = evaluateOrDefault(
+          ctx.evaluator,
+          CustomRefCountingOperation({getterDecl->getResultInterfaceType()
+                                          ->lookThroughAllOptionalTypes()
+                                          ->getClassOrBoundGenericClass(),
+                                      CustomRefCountingOperationKind::retain}),
+          {});
+      if (retainOperation.kind ==
+          CustomRefCountingOperationResult::foundOperation) {
+        retainOperationFn = retainOperation.operation;
+      }
+    }
     // Field getter is represented through a generated
     // C++ method call that returns the value of the base field.
     baseGetterCxxMethod = synthesizeCxxBaseGetterAccessorMethod(
         *static_cast<ClangImporter *>(ctx.getClangModuleLoader()),
         cast<clang::CXXRecordDecl>(derivedStruct->getClangDecl()),
-        cast<clang::CXXRecordDecl>(baseStruct->getClangDecl()), fd);
+        cast<clang::CXXRecordDecl>(baseStruct->getClangDecl()), fd,
+        retainOperationFn);
   }
 
   if (!baseGetterCxxMethod) {

lib/SIL/IR/SILFunctionType.cpp

@@ -3621,6 +3621,10 @@ class CXXMethodConventions : public CFunctionTypeConventions {
       // possible to make it easy for LLVM to optimize away the thunk.
       return ResultConvention::Indirect;
     }
+    if (TheDecl->hasAttr<clang::CFReturnsRetainedAttr>() &&
+        resultTL.getLoweredType().isForeignReferenceType()) {
+      return ResultConvention::Owned;
+    }
     return CFunctionTypeConventions::getResult(resultTL);
   }
   static bool classof(const Conventions *C) {

test/Interop/Cxx/foreign-reference/Inputs/derived-field-getter-returns-owned-frt.h

@@ -0,0 +1,75 @@
+#pragma once
+
+#define FRT                                       \
+  __attribute__((swift_attr("import_reference"))) \
+  __attribute__((swift_attr("retain:immortal")))  \
+  __attribute__((swift_attr("release:immortal")))
+
+int &getLiveRefCountedCounter() {
+    static int counter = 0;
+    return counter;
+}
+
+class RefCounted {
+public:
+    RefCounted() { getLiveRefCountedCounter()++; }
+    ~RefCounted() {
+        getLiveRefCountedCounter()--;
+    }
+
+    void retain() {
+        ++refCount;
+    }
+    void release() {
+        --refCount;
+        if (refCount == 0)
+            delete this;
+    }
+
+    int testVal = 1;
+private:
+    int refCount = 1;
+}   __attribute__((swift_attr("import_reference")))
+__attribute__((swift_attr("retain:retainRefCounted")))
+__attribute__((swift_attr("release:releaseRefCounted")));
+
+RefCounted * _Nonnull createRefCounted() {
+    return new RefCounted;
+}
+
+void retainRefCounted(RefCounted *r) {
+    if (r)
+        r->retain();
+}
+void releaseRefCounted(RefCounted *r) {
+    if (r)
+        r->release();
+}
+
+class BaseFieldFRT {
+public:
+    BaseFieldFRT(): value(new RefCounted) {}
+    BaseFieldFRT(const BaseFieldFRT &other): value(other.value) {
+        value->retain();
+    }
+    ~BaseFieldFRT() {
+        value->release();
+    }
+
+    RefCounted * _Nonnull value;
+};
+
+class DerivedFieldFRT : public BaseFieldFRT {
+};
+
+class NonEmptyBase {
+public:
+    int getY() const {
+        return y;
+    }
+private:
+    int y = 11;
+};
+
+class DerivedDerivedFieldFRT : public NonEmptyBase, public DerivedFieldFRT {
+};

test/Interop/Cxx/foreign-reference/Inputs/module.modulemap

@@ -37,3 +37,9 @@ module MemberInheritance {
   header "member-inheritance.h"
   requires cplusplus
 }
+
+module DerivedFieldGetterReturnsOwnedFRT {
+  header "derived-field-getter-returns-owned-frt.h"
+  requires cplusplus
+}
+

test/Interop/Cxx/foreign-reference/derived-field-getter-retainable-frt-irgen.swift

@@ -0,0 +1,16 @@
+// RUN: %target-swift-emit-irgen -I %S/Inputs -enable-experimental-cxx-interop %s -validate-tbd-against-ir=none -Xcc -fignore-exceptions | %FileCheck %s
+
+import DerivedFieldGetterReturnsOwnedFRT
+
+func testGetX() -> CInt {
+    let derived = DerivedFieldFRT()
+    return derived.value.testVal
+}
+
+let _ = testGetX()
+
+
+// CHECK: define {{.*}}linkonce_odr{{.*}} ptr @{{.*}}__synthesizedBaseGetterAccessor_{{.*}}(ptr {{.*}} %[[THIS_PTR:.*]])
+// CHECK: %[[VALUE_PTR_PTR:.*]] = getelementptr inbounds %class.BaseFieldFRT, ptr %{{.*}}, i32 0, i32 0
+// CHECK: %[[VALUE_PTR:.*]] = load ptr, ptr %[[VALUE_PTR_PTR]], align 8
+// CHECK: call void @{{.*}}retainRefCounted{{.*}}(ptr noundef %[[VALUE_PTR]])

test/Interop/Cxx/foreign-reference/derived-field-getter-retainable-frt-silgen.swift

@@ -0,0 +1,12 @@
+// RUN: %target-swift-emit-sil -I %S/Inputs -enable-experimental-cxx-interop %s -validate-tbd-against-ir=none -Xcc -fignore-exceptions | %FileCheck %s
+
+import DerivedFieldGetterReturnsOwnedFRT
+
+func testGetX() -> CInt {
+    let derived = DerivedFieldFRT()
+    return derived.value.testVal
+}
+
+let _ = testGetX()
+
+// CHECK:  function_ref @{{.*}}__synthesizedBaseGetterAccessor_{{.*}} : $@convention(cxx_method) (@in_guaranteed DerivedFieldFRT) -> @owned RefCounted

test/Interop/Cxx/foreign-reference/derived-field-getter-returns-owned-frt.swift

@@ -0,0 +1,70 @@
+// RUN: %target-run-simple-swift(-I %S/Inputs/ -Xfrontend -enable-experimental-cxx-interop -Xcc -fignore-exceptions -Xfrontend -disable-availability-checking -O)
+// RUN: %target-run-simple-swift(-I %S/Inputs/ -Xfrontend -enable-experimental-cxx-interop -Xcc -fignore-exceptions -Xfrontend -disable-availability-checking)
+//
+// REQUIRES: executable_test
+
+import StdlibUnittest
+import DerivedFieldGetterReturnsOwnedFRT
+
+var FunctionsTestSuite = TestSuite("Calling functions in base foreign reference classes")
+
+FunctionsTestSuite.test("base member FRT field accessing shared reference FRT") {
+    let refScope = {
+        let frt: RefCounted
+        do {
+            let base = BaseFieldFRT()
+            frt = base.value
+        }
+        expectEqual(getLiveRefCountedCounter().pointee, 1)
+        return frt.testVal
+    }
+    let p = refScope()
+    expectEqual(p, 1)
+    expectEqual(getLiveRefCountedCounter().pointee, 0)
+}
+
+FunctionsTestSuite.test("derived-to-base member FRT field accessing shared reference FRT") {
+    let refScope = {
+        let frt: RefCounted
+        do {
+            let derived = DerivedFieldFRT()
+            frt = derived.value
+        }
+        expectEqual(getLiveRefCountedCounter().pointee, 1)
+        return frt.testVal
+    }
+    let p = refScope()
+    expectEqual(p, 1)
+    expectEqual(getLiveRefCountedCounter().pointee, 0)
+}
+
+FunctionsTestSuite.test("derivedDerived-to-derived-to-base member FRT field accessing shared reference FRT") {
+    let refScope = {
+        let frt: RefCounted
+        do {
+            let derived = DerivedDerivedFieldFRT()
+            frt = derived.value
+        }
+        expectEqual(getLiveRefCountedCounter().pointee, 1)
+        return frt.testVal
+    }
+    let p = refScope()
+    expectEqual(p, 1)
+    expectEqual(getLiveRefCountedCounter().pointee, 0)
+}
+
+FunctionsTestSuite.test("base member FRT field setting shared reference FRT") {
+    let frt = createRefCounted()
+    var base = BaseFieldFRT()
+    base.value = frt
+    expectEqual(getLiveRefCountedCounter().pointee, 1)
+}
+
+FunctionsTestSuite.test("derived member FRT field setting shared reference FRT") {
+    let frt = createRefCounted()
+    var base = DerivedFieldFRT()
+    base.value = frt
+    expectEqual(getLiveRefCountedCounter().pointee, 1)
+}
+
+runAllTests()