Skip to content

[cxx-interop] Emit type metadata for foreign types more often #76011

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 1 commit into from
Oct 22, 2024

Conversation

Xazax-hun
Copy link
Contributor

@Xazax-hun Xazax-hun commented Aug 21, 2024

Metadata for foreign types are emitted lazily, when SILGen generates a reference to it. Unfortunately, C++ reverse interop can also introduce references to such metadata in the generated header when types are used as generic arguments. This adds a type visitor to make note of the type metadata use for those generic arguments in public APIs when C++ interop is enabled. Fixes #75593

rdar://132925256

@Xazax-hun Xazax-hun added the c++ interop Feature: Interoperability with C++ label Aug 21, 2024
@Xazax-hun
Copy link
Contributor Author

@swift-ci please smoke test

@Xazax-hun Xazax-hun force-pushed the gaborh/emit-more-metadata branch from 623e284 to 1a968d7 Compare August 22, 2024 11:08
@Xazax-hun
Copy link
Contributor Author

@swift-ci please smoke test

@Xazax-hun Xazax-hun force-pushed the gaborh/emit-more-metadata branch from 1a968d7 to afc6195 Compare August 23, 2024 14:04
@Xazax-hun
Copy link
Contributor Author

Looks like emitting type metadata for foreing types on Windows is not supported at the moment.

@swift-ci please smoke test

@Xazax-hun Xazax-hun marked this pull request as ready for review August 23, 2024 19:57
Copy link
Contributor

@ADKaster ADKaster left a comment

Choose a reason for hiding this comment

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

surface level from me, thanks for looking at my reported issue!

Metadata for foreign types are emitted lazily, when SILGen generates a
reference to it. Unfortunately, C++ reverse interop can also introduce
references to such metadata in the generated header when types are used
as generic arguments. This adds a type visitor to make note of the type
metadata use for those generic arguments in public APIs when C++ interop
is enabled.

rdar://132925256
@Xazax-hun Xazax-hun force-pushed the gaborh/emit-more-metadata branch from afc6195 to cdbe999 Compare August 30, 2024 13:45
@Xazax-hun
Copy link
Contributor Author

@swift-ci please smoke test

@@ -2487,6 +2538,12 @@ void IRGenModule::emitSILFunction(SILFunction *f) {
f->isAvailableExternally())
return;

// Type metadata for foreign references is not yet supported on Windows. Bug #76168.
Copy link
Member

Choose a reason for hiding this comment

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

Is there a reason that this is not supported on Windows?

Copy link
Contributor

Choose a reason for hiding this comment

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

My understanding is that on Windows we end up requiring a metadata completion function for C++ foreign reference types. That logic just isn't implemented yet for C++ types.

swift/lib/IRGen/GenMeta.cpp

Lines 6636 to 6639 in bd313de

void emitInitializeMetadata(IRGenFunction &IGF, llvm::Value *metadata,
MetadataDependencyCollector *collector) {
llvm_unreachable("Not implemented for foreign reference types.");
}

@Xazax-hun
Copy link
Contributor Author

@swift-ci please smoke test

@Xazax-hun
Copy link
Contributor Author

Only rerunning tests because the last completion was a long time ago (to make sure it does not conflict with anything added since then).

@Xazax-hun
Copy link
Contributor Author

@swift-ci please smoke test windows

@Xazax-hun Xazax-hun merged commit 266a622 into main Oct 22, 2024
3 checks passed
@Xazax-hun Xazax-hun deleted the gaborh/emit-more-metadata branch October 22, 2024 07:18
@rjmccall
Copy link
Contributor

This is acceptable as a short-term workaround, but in the long term we don't want to be doing this. It is an important goal of C++ interop to be able to work with an arbitrary existing Swift module, which means we must not require the module to be emitted in a special way. Maybe we need some ability to emit a supporting .o when emitting a C++ interop header.

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.

Interop: Compiler and C++ Bridging header disagree on ABI of Optional<CxxValueType>
7 participants