Skip to content

[cxx-interop] Emit -enable-experimental-cxx-interop flag into the module interface #60725

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
Aug 30, 2022

Conversation

egorzhdan
Copy link
Contributor

This makes sure that when Swift is generating a .swiftinterface file for a Swift module with a dependency on C++ module, -enable-experimental-cxx-interop is emitted under // swift-module-flags:.

The module interface might refer to C++ symbols which are not available in Swift without C++ interop enabled. This caused a build error for Swift LLVM bindings during verify-module-interface stage: Swift tried to import the module interface and couldn't find C++ stdlib headers because C++ interop was disabled.

<module-includes>:1:9: note: in file included from <module-includes>:1:
#import "Support/ConvertUTF.h"
        ^
/Volumes/Projects/swift/llvm-project/llvm/include/llvm/Support/ConvertUTF.h:92:10: error: 'cstddef' file not found
#include <cstddef>
         ^
Sources/LLVM/LLVM_Utils.swiftinterface:4:19: error: could not build Objective-C module 'LLVM_Utils'
@_exported import LLVM_Utils
                  ^
Sources/LLVM/LLVM_Utils.swiftinterface:1:1: error: failed to verify module interface of 'LLVM_Utils' due to the errors above; the textual interface may be broken by project issues or a compiler bug
// swift-interface-format-version: 1.0
^
<unknown>:0: error: verify-module-interface command failed with exit code 1 (use -v to see invocation)

@egorzhdan egorzhdan added the c++ interop Feature: Interoperability with C++ label Aug 23, 2022
@@ -1,4 +1,4 @@
module Namespace {
module Namespaces {
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 renamed the module to avoid name collision: both the namespace and its containing module were called Namespace. This is a general Swift issue that can be worked around by passing -module-interface-preserve-types-as-written flag.

@egorzhdan
Copy link
Contributor Author

@swift-ci please smoke test

@hyp
Copy link
Contributor

hyp commented Aug 23, 2022

Why do the Swift LLVM bindings need a module interface file in the first place? Are they going to be ABI stable and require library evolution because of that?

@egorzhdan
Copy link
Contributor Author

@hyp good question, I think we don't really need library evolution for the bindings, at least for now. So this change shouldn't be a requirement for the bindings to work, but users might hit this error in other projects.

@bnbarham bnbarham requested a review from xymus August 24, 2022 21:29
@bnbarham
Copy link
Contributor

Should this be emitted in the module interface, or should we pass it along when building modules from their interface when the original command has it enabled?

If I don't have experimental C++ interop enabled and I depend on a module that requires it, I think I'd expect building that module to fail rather than to build with interop 🤔?

Any thoughts @xymus?

@xymus
Copy link
Contributor

xymus commented Aug 24, 2022

Printing the flag in the swiftinterface makes sense to keep it self contained and buildable/verifiable on its own. Making the swiftinterface build options depend on its clients would require a different cache hash, I don't think it's worth it here.

@@ -0,0 +1,9 @@
// RUN: %empty-directory(%t)
// RUN: %target-build-swift %s -I %S/Inputs -enable-library-evolution -swift-version 5 -Xfrontend -enable-experimental-cxx-interop %S/Inputs/namespace-extension-lib.swift -emit-module -emit-library -emit-module-interface -emit-module-interface-path %t/UsesCxxStruct.swiftinterface -static -module-name UsesCxxStruct
// RUN: %FileCheck --input-file=%t/UsesCxxStruct.swiftinterface %s
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be interesting to verify that the generated swiftinterface can be parsed. There are macros for that, here's a sample usage:

// RUN: %target-swift-emit-module-interface(%t/Original.swiftinterface) %t/Original.swift
// RUN: %target-swift-typecheck-module-from-interface(%t/Original.swiftinterface)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, I didn't know about these macros. Added target-swift-typecheck-module-from-interface as well.

…odule interface

This makes sure that when Swift is generating a `.swiftinterface` file for a Swift module with a dependency on C++ module, `-enable-experimental-cxx-interop` is emitted under `// swift-module-flags:`.

The module interface might refer to C++ symbols which are not available in Swift without C++ interop enabled. This caused a build error for Swift LLVM bindings during `verify-module-interface` stage: Swift tried to import the module interface and couldn't find C++ stdlib headers because C++ interop was disabled.
@egorzhdan egorzhdan force-pushed the egorzhdan/cxx-module-interface-flag branch from 6f6cedd to 0c35771 Compare August 26, 2022 12:48
@egorzhdan
Copy link
Contributor Author

@swift-ci please smoke test

@egorzhdan
Copy link
Contributor Author

@swift-ci please smoke test macOS

Copy link
Contributor

@xymus xymus left a comment

Choose a reason for hiding this comment

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

Thanks for adding the swiftinterface verification!

You may want to keep an eye on references to anything put in the bridging header like namespace members and templates. I would expected them to be printed under the module name __ObjC and break the swiftinterface.

@egorzhdan
Copy link
Contributor Author

You may want to keep an eye on references to anything put in the bridging header like namespace members and templates. I would expected them to be printed under the module name __ObjC and break the swiftinterface.

Hmm, I think we have logic in the module interface printer tailored for namespaces that makes sure they namespaces' members are printed in the module which they are declared in.
We should probably have tests for this, I'll add some in another PR. Thanks for bringing it up!

@egorzhdan egorzhdan merged commit 032d50d into main Aug 30, 2022
@egorzhdan egorzhdan deleted the egorzhdan/cxx-module-interface-flag branch August 30, 2022 15:44
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.

4 participants