Skip to content

[SILGen] Fix a bug where the wrong convention was being used for lowering a closure to a C++ function pointer #73039

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.

Sign up for GitHub

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

Merged
merged 4 commits into from
Apr 16, 2024
Merged

[SILGen] Fix a bug where the wrong convention was being used for lowering a closure to a C++ function pointer #73039

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
d18413a
[SILGen] Fix a bug where the wrong convention was being used for
ahatanaka Apr 12, 2024
6fc2afa
Address review comments
ahatanaka Apr 15, 2024
9f36cfc
Fix import
ahatanaka Apr 16, 2024
d496799
Fix typo and declare copy constructor in test
ahatanaka Apr 16, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 8 additions & 2 deletions lib/SIL/IR/SILFunctionType.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3798,8 +3798,14 @@ static CanSILFunctionType getSILFunctionTypeForAbstractCFunction(
TypeConverter &TC, AbstractionPattern origType,
CanAnyFunctionType substType, SILExtInfoBuilder extInfoBuilder,
std::optional<SILDeclRef> constant) {
if (origType.isClangType()) {
auto clangType = origType.getClangType();
const clang::Type *clangType = nullptr;

if (origType.isClangType())
clangType = origType.getClangType();
else
clangType = extInfoBuilder.getClangTypeInfo().getType();

if (clangType) {
const clang::FunctionType *fnType;
if (auto blockPtr = clangType->getAs<clang::BlockPointerType>()) {
fnType = blockPtr->getPointeeType()->castAs<clang::FunctionType>();
Expand Down
15 changes: 15 additions & 0 deletions test/Interop/Cxx/class/Inputs/closure.h
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
#ifndef __CLOSURE__
#define __CLOSURE__

struct NonTrivial {
NonTrivial() { p = new int(123); }
~NonTrivial() { delete p; }
NonTrivial(const NonTrivial &other);
int *p;
};

void cfunc2(void (*fp)(NonTrivial)) {
(*fp)(NonTrivial());
}

#endif // __CLOSURE__
5 changes: 5 additions & 0 deletions test/Interop/Cxx/class/Inputs/module.modulemap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -141,3 +141,8 @@ module CopyMoveAssignment {
header "copy-move-assignment.h"
requires cplusplus
}

module Closure {
header "closure.h"
requires cplusplus
}
18 changes: 18 additions & 0 deletions test/Interop/Cxx/class/closure-thunk-executable.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
// RUN: %target-run-simple-swift(-I %S/Inputs/ -Xfrontend -cxx-interoperability-mode=default)
//
// REQUIRES: executable_test

// The test is enabled only on windows until https://github.com/apple/swift/pull/73019
// is fixed.
// REQUIRES: OS=windows-msvc

import StdlibUnittest
import Closure

var ClosureTestSuite = TestSuite("Closure")

ClosureTestSuite.test("ConvertToFunctionPointer") {
cfunc2({N in})
}

runAllTests()
21 changes: 21 additions & 0 deletions test/Interop/Cxx/class/closure-thunk.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
// RUN: %target-swiftxx-frontend -I %S/Inputs -emit-silgen %s | %FileCheck %s
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you please also add an execution test? That would help to make sure we don't crash in IRGen or at runtime.

Copy link
Contributor Author

@ahatanaka ahatanaka Apr 15, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I can add an execution test, but IRGen will still be incorrect until we fix #73019.

Notice that the parameter of the thunk in the test case is @in NonTrivial, which means the destructor is called in the thunk. Calling cfunc2 would result in a double-free crash if NonTrivial was defined as follows:

struct NonTrivial {
  NonTrivial() { p = new int(123); }
  ~NonTrivial() { delete p; }
  int *p = nullptr;
};

void cfunc2(void (*fp)(NonTrivial)) {
  NonTrivial t;
  (*fp)(t);
}

Copy link
Contributor

Choose a reason for hiding this comment

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

I see, thank you!


import Closure

// CHECK: sil [ossa] @$s4main20testClosureToFuncPtryyF : $@convention(thin) () -> () {
// CHECK: %[[V0:.*]] = function_ref @$s4main20testClosureToFuncPtryyFySo10NonTrivialVcfU_To : $@convention(c) (@in NonTrivial) -> ()
// CHECK: %[[V1:.*]] = enum $Optional<@convention(c) (@in NonTrivial) -> ()>, #Optional.some!enumelt, %[[V0]] : $@convention(c) (@in NonTrivial) -> ()
// CHECK: %[[V2:.*]] = function_ref @{{.*}}cfunc2{{.*}}NonTrivial{{.*}} : $@convention(c) (Optional<@convention(c) (@in NonTrivial) -> ()>) -> ()
// CHECK: %[[V3:.*]] = apply %[[V2]](%[[V1]]) : $@convention(c) (Optional<@convention(c) (@in NonTrivial) -> ()>) -> ()

// CHECK: sil private [ossa] @$s4main20testClosureToFuncPtryyFySo10NonTrivialVcfU_ : $@convention(thin) (@in_guaranteed NonTrivial) -> () {

// CHECK: sil private [thunk] [ossa] @$s4main20testClosureToFuncPtryyFySo10NonTrivialVcfU_To : $@convention(c) (@in NonTrivial) -> () {
// CHECK: bb0(%[[V0:.*]] : $*NonTrivial):
// CHECK: %[[V1:.*]] = function_ref @$s4main20testClosureToFuncPtryyFySo10NonTrivialVcfU_ : $@convention(thin) (@in_guaranteed NonTrivial) -> ()
// CHECK: %[[V2:.*]] = apply %[[V1]](%[[V0]]) : $@convention(thin) (@in_guaranteed NonTrivial) -> ()
// CHECK: destroy_addr %[[V0]] : $*NonTrivial

public func testClosureToFuncPtr() {
cfunc2({N in})
}