Skip to content

[cxx-interop] Detect IndirectFields nested in non-importable anon-union. #37287

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 1 commit into from
May 14, 2021
Merged

[cxx-interop] Detect IndirectFields nested in non-importable anon-union. #37287

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
14 changes: 14 additions & 0 deletions lib/ClangImporter/ImportDecl.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3516,6 +3516,20 @@ namespace {
Impl.ImportedDecls.end())
continue;

// If we encounter an IndirectFieldDecl, ensure that its parent is
// importable before attempting to import it because they are dependent
// when it comes to getter/setter generation.
if (const auto *ind = dyn_cast<clang::IndirectFieldDecl>(nd)) {
const clang::CXXRecordDecl *parentUnion =
(ind->getChainingSize() >= 2)
? dyn_cast<clang::CXXRecordDecl>(
ind->getAnonField()->getParent())
: nullptr;
if (parentUnion && parentUnion->isAnonymousStructOrUnion() &&
parentUnion->isUnion() && !isCxxRecordImportable(parentUnion))
continue;
}

auto member = Impl.importDecl(nd, getActiveSwiftVersion());
if (!member) {
if (!isa<clang::TypeDecl>(nd) && !isa<clang::FunctionDecl>(nd)) {
Expand Down
16 changes: 16 additions & 0 deletions test/Interop/Cxx/union/Inputs/anonymous-union-partly-invalid.h
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
@class C;
@interface C
{}
@end

struct S {
union {
C *t;
Copy link
Contributor

Choose a reason for hiding this comment

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

From the other patch:

I believe because it thinks the destructor is deleted at

https://github.com/apple/swift/blob/13449004a069f7d39d841c675c07d7d6debf0f4a/lib/ClangImporter/ImportDecl.cpp#L3352

in isCxxRecordImportable

I'm a bit confused. Where is the destructor deleted?

If that's the problem I think we actually need to fix this differently: figure out why clang thinks the destructor is deleted here (or maybe I'm just missing something 😁). Then we should try to reproduce this assertion again using a "differently invalid" type.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually I think it's because the destructor for an anonymous union here is not deleted (sorry read the condition inverted), and since I dont think theres no way to do a ~() = delete; I can't try this out otherwise for an anonymous union. I think the reason for this is that in this similar instance:

@interface C
@end

union {
  C *t;
  char c;
};

The anon union contains an arc managed pointer so the destructor can't be trivial but I am not certain of this.

Copy link
Contributor

Choose a reason for hiding this comment

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

If the destructor is not deleted, then why can't we import it? I want to make sure we completely understand the problem here.

Copy link
Contributor

Choose a reason for hiding this comment

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

Here's an idea about why this might be failing, it comes from the observation that a similar C++ program doesn't fail:

struct C {
  C(const C&) = delete;
};

struct S {
  union {
    OC *t;
    char c;
  };
  S(const S &s) {}
  ~S() { }
  int f() { return 42; }
};

S *getSPtr();

In the above program, C isn't able to be imported, but its pointer type is still imported as OpaquePointer so we get:

struct S {
  struct __Unnamed_union___Anonymous_field0 {
    var t: OpaquePointer!
    var c: CChar
    init()
    init(t: OpaquePointer!)
    init(c: CChar)
  }
  var __Anonymous_field0: S.__Unnamed_union___Anonymous_field0
  var t: OpaquePointer!
  var c: CChar
  mutating func f() -> Int32
}
func getSPtr() -> UnsafeMutablePointer<S>!

However, as I understand it (and this might not be correct), this is not the case for ObjC types: if they can't be imported, their type is nullptr. This might be the reason it's only happening for ObjC.

However, I'm still not sure we know the answer to the question: why is isCxxRecordImportable returning false?

char c;
};
S(const S &s) {}
~S() { }
Copy link
Contributor

Choose a reason for hiding this comment

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

Additionally, it's odd that this doesn't fail when the special members are gone, this contributes to my thinking that we don't know the whole story here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah yeah I agreed. I thought there was something going on where the special members of the containing struct affected the anon-union's. If it is ok with you to land this so I can be unblocked, I will continue investigating in the background if that is ok?

int f() { return 42; }
};

S *getSPtr();
4 changes: 4 additions & 0 deletions test/Interop/Cxx/union/Inputs/module.modulemap
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
module AnonymousUnionPartlyInvalid {
header "anonymous-union-partly-invalid.h"
requires cplusplus
}
8 changes: 8 additions & 0 deletions test/Interop/Cxx/union/anonymous-union-partly-invalid-module-interface.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
// RUN: %target-swift-ide-test -print-module -module-to-print=AnonymousUnionPartlyInvalid -I %S/Inputs -source-filename=x -enable-cxx-interop -enable-objc-interop | %FileCheck %s

// CHECK: class C {
// CHECK-NEXT: }
// CHECK-NEXT: struct S {
// CHECK-NEXT: mutating func f() -> Int32
// CHECK-NEXT: }
// CHECK-NEXT: func getSPtr() -> UnsafeMutablePointer<S>!
13 changes: 13 additions & 0 deletions test/Interop/Cxx/union/anonymous-union-partly-invalid.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
// RUN: %swift -I %S/Inputs -enable-cxx-interop -enable-objc-interop -emit-ir %s | %FileCheck %s

import AnonymousUnionPartlyInvalid

let sPtr = getSPtr()
let a = sPtr![0].f()

// CHECK: i32 @main
// CHECK-NEXT: entry:
// CHECK-NEXT: bitcast
// CHECK-NEXT: call %struct.S
// CHECK-NEXT: ptrtoint %struct.S