Skip to content

[cxx-interop] Prevent Swift from importing fields marked with [[no_unique_address]] #80497

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

Closed
wants to merge 1 commit into from
+51 −4
Closed

[cxx-interop] Prevent Swift from importing fields marked with [[no_unique_address]] #80497

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
3 changes: 3 additions & 0 deletions include/swift/AST/DiagnosticsClangImporter.def
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -373,5 +373,8 @@ NOTE(ptr_to_nonescapable,none,
"pointer to non-escapable type %0 cannot be imported",
(const clang::Type*))

WARNING(unsupported_attribute, none, "Swift doesn't support fields marked with %0",
(StringRef))

#define UNDEFINE_DIAGNOSTIC_MACROS
#include "DefineDiagnosticMacros.h"
10 changes: 10 additions & 0 deletions lib/ClangImporter/ImportDecl.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4383,6 +4383,16 @@ namespace {
std::optional<ImportedName> correctSwiftName;
ImportedName importedName;

if (decl->hasAttr<clang::NoUniqueAddressAttr>()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is NoUniqueAddressAttr the source of the problem or is it something else e.g., the field having 0 as its size? In case it is the latter we might want to make this workaround more targeted.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think the problem is that we are incorrectly handling fields with the no_unique_address attribute, and consequently the swift runtime tries to access fields of std::string that were not imported, resulting in a runtime crash. We eventually want to correctly handle this attribute, but the impact of the bug required a quicker fix.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, let me elaborate a bit. According to CppReference:

Makes this member subobject potentially-overlapping, i.e., allows this member to be overlapped with other non-static data members or base class subobjects of its class. This means that if the member has an empty class type (e.g. stateless allocator), the compiler may optimize it to occupy no space, just like if it were an empty base. If the member is not empty, any tail padding in it may be also reused to store other data members.

So this attribute can have two effects:

  • Empty sized fields can get optimized away
  • Non-empty types' tail padding can be reused

If we know exactly which one of this is problematic, we could make this check more targeted. E.g., if we only have trouble with empty fields we can check for the size instead of the presence of the attribute, so we end up rejecting less code.

That being said, I am OK with landing this as is and doing the rest of the investigation as a follow-up.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This makes sense to me. I can for sure investigate this further and, if possible, refine this workaround. However, if you agree, I would merge this patch and open a new one when we have a better fix.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sounds good!

// TODO: rdar://148437848 Swift doesn't support fields marked with [[no_unique_address]]

Impl.addImportDiagnostic(
decl,
Diagnostic(diag::unsupported_attribute, "[[no_unique_address]]"),
decl->getLocation());
return nullptr;
}

std::tie(importedName, correctSwiftName) = importFullName(decl);
if (!importedName) {
return nullptr;
Expand Down
10 changes: 10 additions & 0 deletions test/Interop/Cxx/class/Inputs/member-variables.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,16 @@ struct Empty {
int getNum() const { return 42; }
};

struct HasNoUniqueAddressField {
MyClass simpleField;
#if __is_target_os(windows)
[[msvc::no_unique_address]]
#else
[[no_unique_address]]
#endif
MyClass noUniqueAddressField; // expected-warning {{Swift doesn't support fields marked with [[no_unique_address]]}}
};

struct HasZeroSizedField {
int a;
[[no_unique_address]] Empty b;
Expand Down
11 changes: 8 additions & 3 deletions test/Interop/Cxx/class/member-variables-typechecker.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,11 @@
// RUN: %target-typecheck-verify-swift -I %S/Inputs -enable-experimental-cxx-interop
// RUN: %target-typecheck-verify-swift -I %S/Inputs -enable-experimental-cxx-interop -verify-additional-file %S/Inputs/member-variables.h

import MemberVariables

var s = MyClass()
s.const_member = 42 // expected-error {{cannot assign to property: 'const_member' setter is inaccessible}}
var s1 = MyClass()
s1.const_member = 42 // expected-error {{cannot assign to property: 'const_member' setter is inaccessible}}

// TODO: rdar://148437848 Swift doesn't support fields marked with [[no_unique_address]]
var s2 = HasNoUniqueAddressField()
_ = s2.simpleField.const_member
_ = s2.noUniqueAddressField.const_member // expected-error {{value of type 'HasNoUniqueAddressField' has no member 'noUniqueAddressField'}}
3 changes: 2 additions & 1 deletion test/Interop/Cxx/class/zero-sized-field.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,8 @@ FieldsTestSuite.test("Zero sized field") {
expectEqual(s2.c, 7)
expectEqual(s2.c, s2.get_c())
expectEqual(takesZeroSizedInCpp(s2), 5)
expectEqual(s.b.getNum(), 42)
// TODO: rdar://148437848 Swift doesn't support fields marked with [[no_unique_address]]
// expectEqual(s.b.getNum(), 42)
}

runAllTests()
18 changes: 18 additions & 0 deletions test/Interop/Cxx/stdlib/print-std-string.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
// RUN: %empty-directory(%t)
// RUN: %target-build-swift %s -o %t/a.out -Xfrontend -enable-experimental-cxx-interop
// RUN: %target-codesign %t/a.out
// RUN: %target-run %t/a.out | %FileCheck %s

// REQUIRES: executable_test

import CxxStdlib

func printString(s: std.string) {
print(s)
let swiftString = String(s)
print(swiftString)
}

printString(s: "Hello")
// CHECK: basic_string<CChar, std.__1.char_traits<CChar>, std.__1.allocator<CChar>>()
// CHECK: Hello