[cxx-interop] Flip the switch: only import safe APIs. #59624

zoecarver · 2022-06-22T00:02:52Z

This patch adds is the first of many to flip the switch and stop eagerly importing C++ APIs. We will now only import safe APIs or APIs that are annotated by the user. I will follow this patch up with improved diagnostics, testing, and documentation.

zoecarver · 2022-06-22T00:03:23Z

lib/ClangImporter/ImportDecl.cpp

+
+    // Recursively checks that there are no pointers, user-provided copy
+    // constructors, or destructors in any fields or base classes.
+    static bool isSufficientlyTrivial(const clang::CXXRecordDecl *decl) {


We may want to cache this.

zoecarver · 2022-06-22T00:03:49Z

lib/ClangImporter/ImportDecl.cpp

+        if (bridgingInfo.getClangName() != "begin" &&
+            bridgingInfo.getClangName() != "end" &&
+            !decl->isOverloadedOperator() && !decl->isStatic() &&
+            !hasUnsafeAPIAttr(decl) && !hasImportAsRefAttr(decl->getParent()))


add tests for all of these

zoecarver · 2022-06-22T00:05:28Z

test/Interop/Cxx/class/inheritance/Inputs/functions.h

-  inline const char *inNonTrivial() const { return "NonTrivial::inNonTrivial"; }
-  inline const char *inNonTrivialWithArgs(int a, int b) const {
+  inline const char *inNonTrivial() const
+      __attribute__((swift_attr("import_unsafe_api"))) {


Review note: I was originally going to update all of these tests to be safe APIs, but I decided that should happen in a follow up PR. These are testing very specific things, so we should upgrade them slowly/incrementally to ensure we don't lose test coverage and regress.

egorzhdan · 2022-06-22T10:44:09Z

This essentially makes the enable_unsafe_eager_importing flag viral in a sense that if one of your Swift dependencies uses this flag, you also have to use it even if you don't reference any C++ APIs at all, otherwise you'll get SIL deserialization errors: the decls referenced by that dependency won't be found when compiling the current module.

This makes me think that ClangImporter might not be the right place for this logic. I think a better approach would be to import all APIs in ClangImporter, and then reject unsafe calls during type checking. Let's discuss offline :)

zoecarver · 2022-07-12T01:36:51Z

test/Interop/Cxx/stdlib/use-std-iterator.swift

@@ -1,37 +0,0 @@
-// RUN: %target-run-simple-swift(-I %S/Inputs -Xfrontend -enable-experimental-cxx-interop)


This is not something that we want people to be able to do anymore. The correct way to do this is with the stdlib overlay, which we already have tests for.

zoecarver · 2022-07-15T20:37:47Z

@swift-ci please test

zoecarver · 2022-07-18T03:07:26Z

@swift-ci please test

hyp · 2022-07-18T10:46:45Z

docs/CppInteroperability/UserManual.md

@@ -0,0 +1,37 @@
+# C++ Interop User Manual


This is user manual lacks introduction and basic context to be able to understand the sections below. Is it meant to be a manual for forward interop, or bidirectional whole interop? How is the user supposed to read it and understand what categories are without some sort of basic introduction.

egorzhdan · 2022-07-18T11:29:17Z

lib/ClangImporter/ClangImporter.cpp

+    return CxxRecordSemanticsKind::Owned;
+  }
+
+  if (hasIteratorAPIAttr(decl)) {


I don't think we should require the user to annotate all iterators with an attribute. Swift can detect that the type is an iterator statically without an attribute by doing clangRecordDecl->lookup(getClangASTContext().Idents.get("iterator_category")) – if there is a subtype called iterator_category, then this type is an iterator (see swift::conformToCxxIteratorIfNeeded from #60001).

If the iterator type doesn't define iterator_category, then the user can add an attribute, but I don't think we should require attributes for something we can correctly deduce statically.

As discussed offline: we will use both to determine what's an iterator.

egorzhdan · 2022-07-18T11:33:52Z

stdlib/public/Cxx/std/String.swift

+    let count = string.count
+    self.init(count, value_type(0))
+    var string = string
+    string.withUTF8 {


Thanks for fixing this! 👍

I thought this would be a small perf improvement but actually seems to not work. We can investigate in a separate follow up patch.

egorzhdan · 2022-07-18T11:41:13Z

lib/ClangImporter/ClangImporter.cpp

+       !copyAssignOperatorIsDefaulted(decl)) ||
+      (decl->hasUserDeclaredDestructor() &&
+       !decl->getDestructor()->isDefaulted()))
+    return false;


I'm not sure I understand why a type with a custom copy constructor is unsafe. I think this is probably too restrictive, let's discuss.

hyp · 2022-07-18T13:44:49Z

docs/CppInteroperability/UserManual.md

+The Swift compiler enforces certain API rules, not to ensure a completely safe C++ API interface, but to prevent especially unsafe patterns that will likely lead to bugs. Many of these unsafe patterns stem from the subtly different lifetime semantics in C++ and Swift and aim to prevent memory projections of owned types. The currently enforced rules are as follows:
+
+* Unsafe lifetime types and un-importable types are not allowed to be used in any contexts.
+* Unsafe pointer types are not allowed to represent unsafe memory projections from owned types. Note: unsafe pointer types *are* allowed to represent memory projections from reference types. Note: the Swift compiler assumes that global and static functions do not return projections.


Should you clarify that unsafe pointers are allowed to represent memory projections from immortal reference types here?

We don't have the idea of immortal vs other foreign reference types. Above it clarifies that foreign reference types must be immortal or manually managed, so I think it's fine. This states our current support, not our future plans.

zoecarver · 2022-07-18T17:01:18Z

test/Interop/Cxx/class/invalid-class-errors.swift

+
+import Test
+
+// CHECK: note: record 'X' is not defined (incomplete)


@NuriAmari thanks again for building all the infrastructure for these diagnostics. It made it super easy (one line) to add diagnostics for all these other cases.

My pleasure, glad it's being put to good use 👍

zoecarver · 2022-07-18T18:33:31Z

@swift-ci please test.

zoecarver · 2022-07-18T19:56:56Z

@swift-ci please test.

This makes "owned" the default kind for records that have custom copy constructors and no pointer-members.

…nter types. Previously types containing pointers would mark all C++ methods as unsafe.

zoecarver · 2022-07-18T21:17:01Z

@swift-ci please test.

zoecarver · 2022-07-19T00:07:07Z

@swift-ci please test.

zoecarver · 2022-07-19T02:27:26Z

@swift-ci please test.

zoecarver · 2022-07-19T13:33:56Z

@swift-ci please test.

For some reason this isn't working, so I reverted to the previous approach. We can try to re-visit this in the future for a potential perf improvement.

zoecarver · 2022-07-19T13:46:51Z

@swift-ci please test.

zoecarver · 2022-07-19T17:18:28Z

@swift-ci please smoke test linux.

zoecarver · 2022-07-19T18:20:47Z

@swift-ci please smoke test linux.

zoecarver · 2022-07-19T19:13:15Z

@swift-ci please smoke test linux.

zoecarver · 2022-07-19T19:53:21Z

@swift-ci please smoke test linux.

zoecarver · 2022-07-19T20:39:21Z

@swift-ci please smoke test.

zoecarver · 2022-07-19T20:39:29Z

@swift-ci please test.

zoecarver added the c++ interop Feature: Interoperability with C++ label Jun 22, 2022

zoecarver requested a review from egorzhdan June 22, 2022 00:03

zoecarver commented Jun 22, 2022

View reviewed changes

zoecarver requested a review from hyp June 22, 2022 00:07

zoecarver force-pushed the disable-eager-import branch from bc301b4 to 44cb846 Compare July 1, 2022 01:16

zoecarver force-pushed the disable-eager-import branch from 44cb846 to 0198e0f Compare July 11, 2022 17:47

zoecarver commented Jul 12, 2022

View reviewed changes

zoecarver force-pushed the disable-eager-import branch from ea20131 to 494ff2d Compare July 15, 2022 20:36

zoecarver marked this pull request as ready for review July 15, 2022 20:37

zoecarver changed the title ~~[cxx-interop] Flip the switch: only import safe APIs (pt. 1)~~ [cxx-interop] Flip the switch: only import safe APIs. Jul 15, 2022

zoecarver force-pushed the disable-eager-import branch 2 times, most recently from abd1fd9 to f32c529 Compare July 15, 2022 21:49

hyp reviewed Jul 18, 2022

View reviewed changes

egorzhdan reviewed Jul 18, 2022

View reviewed changes

hyp reviewed Jul 18, 2022

View reviewed changes

zoecarver commented Jul 18, 2022

View reviewed changes

zoecarver added 5 commits July 18, 2022 17:15

[cxx-interop] Flip the switch: only import safe APIs.

6acffbb

[nfc][cxx-interop] Add diagnostics when something cannot be imported.

9d1d031

[nfc] Clang format changes.

ca5fa9a

[cxx-interop] Remove UnsafeLifetimeOperation record semantic kind.

3498ff9

This makes "owned" the default kind for records that have custom copy constructors and no pointer-members.

[cxx-interop] Simplify logic in IsSafeUseOfCxxDecl; fix bug for poi…

09384be

…nter types. Previously types containing pointers would mark all C++ methods as unsafe.

zoecarver force-pushed the disable-eager-import branch from 781850c to 228a886 Compare July 18, 2022 21:16

Mark a few API uses in Swift Compiler Sources as unsafe.

12933bf

zoecarver force-pushed the disable-eager-import branch from 228a886 to 12933bf Compare July 19, 2022 00:06

[libswift] Update SWIFT_IMPORT_REFERENCE to use import_reference.

ba2eb7b

zoecarver added 2 commits July 19, 2022 09:24

[cxx-interop] Don't add incomplete records to lookup table.

79c68f8

[nfc][cxx-interop] Fix test failures and diagnostic names.

e3563ef

[cxx-interop][overlay] Revert memcpy string init approach.

d5e88d9

For some reason this isn't working, so I reverted to the previous approach. We can try to re-visit this in the future for a potential perf improvement.

zoecarver force-pushed the disable-eager-import branch from 07a97f8 to e62ce9b Compare July 19, 2022 19:13

zoecarver force-pushed the disable-eager-import branch from e62ce9b to 20ffe0b Compare July 19, 2022 19:53

[cxx-interop] Fix predicate to find copy assignment operators.

ba7e6e4

zoecarver force-pushed the disable-eager-import branch from 20ffe0b to ba7e6e4 Compare July 19, 2022 20:39

zoecarver merged commit 33335f9 into swiftlang:main Jul 20, 2022

		@@ -1,37 +0,0 @@
		// RUN: %target-run-simple-swift(-I %S/Inputs -Xfrontend -enable-experimental-cxx-interop)


		import Test

		// CHECK: note: record 'X' is not defined (incomplete)

[cxx-interop] Flip the switch: only import safe APIs. #59624

[cxx-interop] Flip the switch: only import safe APIs. #59624

Uh oh!

Conversation

zoecarver commented Jun 22, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zoecarver Jun 22, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

egorzhdan commented Jun 22, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zoecarver commented Jul 15, 2022

Uh oh!

zoecarver commented Jul 18, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zoecarver commented Jul 18, 2022

Uh oh!

zoecarver commented Jul 18, 2022

Uh oh!

zoecarver commented Jul 18, 2022

Uh oh!

zoecarver commented Jul 19, 2022

Uh oh!

zoecarver commented Jul 19, 2022

Uh oh!

zoecarver commented Jul 19, 2022

Uh oh!

zoecarver commented Jul 19, 2022

Uh oh!

zoecarver commented Jul 19, 2022

Uh oh!

zoecarver commented Jul 19, 2022

Uh oh!

zoecarver commented Jul 19, 2022

Uh oh!

zoecarver commented Jul 19, 2022

Uh oh!

zoecarver commented Jul 19, 2022

Uh oh!

zoecarver commented Jul 19, 2022

Uh oh!

Uh oh!

zoecarver commented Jun 22, 2022 •

edited

Loading

zoecarver Jun 22, 2022 •

edited

Loading