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

susmonteiro · 2025-04-03T15:13:43Z

From libc++ 20, std::string has 3 fields marked with [[no_unique_address]]. This attribute is not supported in Swift and was causing a runtime crash when printing a std::string in Swift. In this patch, we prevent Swift from importing such fields.

rdar://147263490

susmonteiro · 2025-04-03T15:14:28Z

@swift-ci please smoke test

egorzhdan

Thanks! I think the main thing missing here is a few more tests.

lib/ClangImporter/ImportDecl.cpp

egorzhdan · 2025-04-03T15:25:22Z

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

@@ -490,4 +490,11 @@ StdStringTestSuite.test("pass as a default argument") {
 }
 #endif

+StdStringTestSuite.test("print std::string") {
+    let cxxString: std.string = "Hello"
+    print(cxxString)


This ensures that we don't crash, but I wonder if we can also verify that the right string got printed. One way to do that would be creating a new test file, containing the same code snippet, building and running it, then checking the output.

egorzhdan · 2025-04-03T15:27:37Z

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

+    let swiftString = String(cxxString)
+    print(swiftString)
+}
+


Could you please also add a test that ensures that the error message is emitted when someone tries to access a no_unique_address field from Swift?
test/Interop/Cxx/class would probably be the best place for such test

Also done. Thanks for the suggestions!

Xazax-hun

Might be interesting to sync up with John McCall on this. When he reviewed my PR to fix a crash in codegen (#79076) I had the impression that he would prefer to keep this information in the compiler instead of skipping this.

Swift already supports 0-sized types, empty structs are zero sized. So in case the root cause of the problem is that we might want to look into mimicking what happens for zero sized Swift types.

All this being said I am not against not importing them.

Xazax-hun · 2025-04-03T15:26:42Z

lib/ClangImporter/ImportDecl.cpp

@@ -4383,6 +4383,15 @@ namespace {
      std::optional<ImportedName> correctSwiftName;
      ImportedName importedName;

+      if (decl->hasAttr<clang::NoUniqueAddressAttr>()) {


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.

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.

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.

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.

Sounds good!

egorzhdan

LGTM, thanks! Just one minor comment.

egorzhdan · 2025-04-04T14:39:54Z

test/Interop/Cxx/class/member-variables-typechecker.swift


 import MemberVariables
+import CxxStdlib


Do we actually need this import?

No, we don't. Thanks

susmonteiro · 2025-04-04T15:18:59Z

@swift-ci please smoke test

susmonteiro · 2025-04-07T12:33:49Z

@swift-ci please smoke test

…ique_address]]

susmonteiro · 2025-04-07T15:10:15Z

@swift-ci please smoke test

susmonteiro requested review from zoecarver, egorzhdan, Xazax-hun, j-hui, fahadnayyar, hnrklssn, beccadax and ian-twilightcoder as code owners April 3, 2025 15:13

susmonteiro added the c++ interop Feature: Interoperability with C++ label Apr 3, 2025

egorzhdan reviewed Apr 3, 2025

View reviewed changes

Xazax-hun reviewed Apr 3, 2025

View reviewed changes

susmonteiro closed this Apr 3, 2025

susmonteiro force-pushed the susmonteiro/skip-no-unique-address-attribute branch from 279584d to 7418b25 Compare April 3, 2025 16:31

susmonteiro reopened this Apr 4, 2025

susmonteiro force-pushed the susmonteiro/skip-no-unique-address-attribute branch from c3eb234 to 4be78c2 Compare April 4, 2025 12:39

egorzhdan approved these changes Apr 4, 2025

View reviewed changes

susmonteiro force-pushed the susmonteiro/skip-no-unique-address-attribute branch from 4be78c2 to f4435ee Compare April 4, 2025 15:12

fahadnayyar approved these changes Apr 6, 2025

View reviewed changes

susmonteiro force-pushed the susmonteiro/skip-no-unique-address-attribute branch from f4435ee to 56be6f8 Compare April 7, 2025 12:33

[cxx-interop] Prevent Swift from importing fields marked with [[no_un…

fbd4e66

…ique_address]]

susmonteiro force-pushed the susmonteiro/skip-no-unique-address-attribute branch from 56be6f8 to fbd4e66 Compare April 7, 2025 15:09

susmonteiro closed this Apr 23, 2025

susmonteiro deleted the susmonteiro/skip-no-unique-address-attribute branch May 28, 2025 17:21

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

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

Uh oh!

Conversation

susmonteiro commented Apr 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

susmonteiro commented Apr 3, 2025

Uh oh!

egorzhdan left a comment

Choose a reason for hiding this comment

Uh oh!

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!

Xazax-hun left a comment

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!

egorzhdan left a comment

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!

susmonteiro commented Apr 4, 2025

Uh oh!

susmonteiro commented Apr 7, 2025

Uh oh!

susmonteiro commented Apr 7, 2025

Uh oh!

Uh oh!

susmonteiro commented Apr 3, 2025 •

edited

Loading