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.

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

Conversation

ahatanaka
Copy link
Contributor

Use the C function pointer convention instead of the block convention.

rdar://122977380

lowering a closure to a C++ function pointer

Use the C function pointer convention instead of the block convention.

rdar://122977380
@ahatanaka
Copy link
Contributor Author

@swift-ci please test

@egorzhdan egorzhdan added the c++ interop Feature: Interoperability with C++ label Apr 15, 2024
@ahatanaka ahatanaka requested a review from fahadnayyar April 15, 2024 17:08
Copy link
Contributor

@egorzhdan egorzhdan left a comment

Choose a reason for hiding this comment

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

Thanks Akira!
I have just a couple of comments regarding tests.

@@ -141,3 +141,8 @@ module CopyMoveAssignment {
header "copy-move-assignment.h"
requires cplusplus
}

module NonTrivialClasses {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we call this module Closures, since closures are the special thing in this test module?

@@ -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

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!

- Rename module.
- Fix check string in test
- Add execution test
@ahatanaka
Copy link
Contributor Author

@swift-ci please test

@ahatanaka
Copy link
Contributor Author

@swift-ci please test

Copy link
Contributor

@egorzhdan egorzhdan left a comment

Choose a reason for hiding this comment

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

LGTM!

@ahatanaka
Copy link
Contributor Author

@swift-ci please test

@ahatanaka ahatanaka merged commit d76dbb1 into main Apr 16, 2024
@ahatanaka ahatanaka deleted the closure-to-cxx-function-pointer branch April 16, 2024 22:18
ahatanaka added a commit that referenced this pull request Apr 23, 2024
…ring a closure to a C++ function pointer (#73039)

Use the C function pointer convention instead of the block convention.

rdar://122977380
(cherry picked from commit d76dbb1)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ interop Feature: Interoperability with C++
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants