Skip to content
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

[cxx-interop] Move included stdlib headers from libstdcxx.h to the modulemap #60722

Merged
merged 1 commit into from
Aug 24, 2022

Conversation

egorzhdan
Copy link
Contributor

By referencing the C++ stdlib header directly from the modulemap, we make sure that the header is treated as a part of the stdlib, not a part of the first clang module to include it.

Addresses the issue described in https://forums.swift.org/t/llvm-fails-to-build-with-modules/59700/8.

@egorzhdan egorzhdan added the c++ interop Feature: Interoperability with C++ label Aug 23, 2022
@egorzhdan
Copy link
Contributor Author

@swift-ci please smoke test

@egorzhdan
Copy link
Contributor Author

@swift-ci please smoke test

@hyp
Copy link
Contributor

hyp commented Aug 23, 2022

This looks incorrect - Why do you want to do this only for those two headers? Shouldn't all the headers referenced in libstdcxx.h be in the module map?

@hyp
Copy link
Contributor

hyp commented Aug 23, 2022

You can also try making libstdcxx.h an umbrella header, since VFS places it into the libstdcxx directory.

@egorzhdan
Copy link
Contributor Author

Yeap, you're right, we should try to refer to the stdlib headers in a consistent way. I moved these two headers only for now to make sure this doesn't cause too much fallout – moving all headers into the modulemap at once could produce a lot of compiler errors which would be hard to investigate.

I tried making libstdcxx.h an umbrella header in the past, the problem is that it causes clang to treat libstdc++ headers as different submodules, and that causes "duplicate definition" errors for types like size_t which are defined in multiple headers (behind something like #if SIZE_T_DEFINED).

@hyp
Copy link
Contributor

hyp commented Aug 23, 2022

Ok, in that case the header map should be generated dynamically in the compiler, and all the headers should be referenced in the module map to avoid issues like this. I don't see how this could cause additional fallout - at this point you're just playing whack a mole with the errors reported on the forums (there could be more), so wouldn't it make sense to address this correctly?

@egorzhdan
Copy link
Contributor Author

A change like this could cause new modularization errors if, for example, there is a C++ stdlib header that is not directly referenced from the modulemap, but is #include-d from a user project. This happened when I tried moving tuple only (without functional) to the modulemap in this PR.

at this point you're just playing whack a mole with the errors reported on the forums (there could be more), so wouldn't it make sense to address this correctly?

I agree, I'm going to move more headers to the modulemap before merging this.

I wonder if it's possible to move the optional headers (those under #if __has_include) to the modulemap without having to generate the modulemap in the compiler. I guess it isn't, unless we inject empty headers into libstdc++ via VFS.

…modulemap

By referencing a C++ stdlib header directly from the modulemap, we make sure that the header is treated as a part of the stdlib, not a part of the first clang module to include it.

Addresses the issue described in https://forums.swift.org/t/llvm-fails-to-build-with-modules/59700/8.
@egorzhdan egorzhdan marked this pull request as draft August 23, 2022 15:48
@egorzhdan
Copy link
Contributor Author

@swift-ci please smoke test

@egorzhdan egorzhdan changed the title [cxx-interop] Move tuple from libstdc++ header to the modulemap [cxx-interop] Move included stdlib headers from libstdcxx.h to the modulemap Aug 24, 2022
@egorzhdan egorzhdan marked this pull request as ready for review August 24, 2022 10:18
@egorzhdan
Copy link
Contributor Author

I will move the rest of the headers (those that are under #if __has_include) to the modulemap in a separate PR.

@egorzhdan egorzhdan merged commit 1549ef2 into main Aug 24, 2022
@egorzhdan egorzhdan deleted the egorzhdan/libstdcxx-headers branch August 24, 2022 14:21
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.

None yet

2 participants