Skip to content

[cxx-interop] Use VFS to inject modulemap into libstdc++ installation #58843

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 2 commits into from
May 13, 2022

Conversation

egorzhdan
Copy link
Contributor

@egorzhdan egorzhdan commented May 11, 2022

Previously the modulemap for the C++ stdlib on Linux was provided via -fmodule-map-file= Clang argument pointing to the modulemap file within the Swift toolchain. The modulemap file could not reference the stdlib headers directly, since the exact stdlib include directory varies across Linux versions (it generally looks like /usr/include/c++/{gcc_version}). So the modulemap file instead referenced a local header, which #include <>-ed the stdlib headers, relying on Clang include resolution.

Unfortunately this did not work properly in the presence of another C++ module which included the stdlib headers: sometimes decls from the stdlib were hijacked by the other module, and were not treated as a part of the stdlib by Clang. This caused compile errors in Swift.

This change uses LLVM VFS to inject the modulemap file into the libstdc++ directory. The modulemap file is now able to reference the stdlib headers directly, which fixes the issue.

Credits to Rintaro Ishizaki for proposing a similar idea for SwiftGlibc back in 2016.

@egorzhdan egorzhdan force-pushed the egorzhdan/libstdcxx-vfs branch 3 times, most recently from bfe466b to 9a17417 Compare May 13, 2022 16:42
@egorzhdan
Copy link
Contributor Author

@swift-ci please smoke test

@egorzhdan
Copy link
Contributor Author

@rintaro this is largely based on your commit back from 2016 that never got merged (rintaro@1dc920b).
Thanks for proposing this idea and working on it!

@egorzhdan egorzhdan marked this pull request as ready for review May 13, 2022 16:44
@egorzhdan egorzhdan requested review from rintaro, zoecarver and hyp May 13, 2022 16:44
@egorzhdan egorzhdan added the c++ interop Feature: Interoperability with C++ label May 13, 2022
@egorzhdan
Copy link
Contributor Author

This is a requirement for #58699.

@egorzhdan egorzhdan force-pushed the egorzhdan/libstdcxx-vfs branch from 9a17417 to ff6b4c4 Compare May 13, 2022 16:52
@egorzhdan
Copy link
Contributor Author

@swift-ci please smoke test

@rintaro
Copy link
Member

rintaro commented May 13, 2022

Thank you for resurrecting this idea :)

Copy link
Contributor

@zoecarver zoecarver left a comment

Choose a reason for hiding this comment

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

Looks good to me. Thanks, Egor. Ship it when you feel ready!

@@ -1,79 +1,79 @@
#include <algorithm>
Copy link
Contributor

Choose a reason for hiding this comment

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

Why did these have to change? Is it ever going to be a problem for users if they use angle brackets in their includes?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It'll work fine with angle brackets as well. I just wanted to make it more clear that both the modulemap & the header are now located in the same directory as the stdlib headers, so there's no need to use angle brackets.

egorzhdan added 2 commits May 13, 2022 22:21
Previously the modulemap for the C++ stdlib on Linux was provided via `-fmodule-map-file=` Clang argument pointing to the modulemap file within the Swift toolchain. The modulemap file could not reference the stdlib headers directly, since the exact stdlib include directory varies across Linux versions (it generally looks like `/usr/include/c++/{gcc_version}`). So the modulemap file instead referenced a local header, which `#include <>`-ed the stdlib headers, relying on Clang include resolution.

Unfortunately this did not work properly in the presence of another C++ module which included the stdlib headers: sometimes decls from the stdlib were hijacked by the other module, and were not treated as a part of the stdlib by Clang. This caused compile errors in Swift.

This change uses LLVM VFS to inject the modulemap file into the libstdc++ directory. The modulemap file is now able to reference the stdlib headers directly, which fixes the issue.

Credits to Rintaro Ishizaki for proposing a similar idea for SwiftGlibc back in 2016.
Previously this header was sometimes getting hijacked by the first header to include it. This caused Swift compiler failures on Linux when using any LLVM header that uses `assert`.

Now `cassert` is referenced directly from a modulemap, which fixes the issue.
@egorzhdan egorzhdan force-pushed the egorzhdan/libstdcxx-vfs branch from ff6b4c4 to 20650ea Compare May 13, 2022 21:22
@egorzhdan
Copy link
Contributor Author

@swift-ci please smoke test

@egorzhdan egorzhdan merged commit 1ef02ef into main May 13, 2022
@egorzhdan egorzhdan deleted the egorzhdan/libstdcxx-vfs branch May 13, 2022 23:40
@aschwaighofer
Copy link
Contributor

aschwaighofer commented May 14, 2022

Hi @egorzhdan,

Can you fix the line

/home/build-user/swift/lib/ClangImporter/ClangImporter.cpp:869:8: error: too few template arguments for class template 'SmallVector'
static SmallVector<std::pair<std::string, std::string>>
       ^
/home/build-user/swift/include/swift/Basic/LLVM.h:47:43: note: template is declared here
  template <typename T, unsigned N> class SmallVector;
  ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~       ^
/home/build-user/swift/lib/ClangImporter/ClangImporter.cpp:926:3: error: too few template arguments for class template 'SmallVector'
  SmallVector<std::pair<std::string, std::string>> result;
  ^
/home/build-user/swift/include/swift/Basic/LLVM.h:47:43: note: template is declared here
  template <typename T, unsigned N> class SmallVector;
  ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~       ^
2 errors generated.

SmallVector without a on stack storage template parameter causes a compile failure on some linux configurations:

https://ci.swift.org/view/Dashboard/job/oss-swift-package-centos-7/480/

cxxStdlibDirs.push_back(Path(iter->path()));
}

SmallVector<std::pair<std::string, std::string>> result;
Copy link
Contributor

Choose a reason for hiding this comment

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

^ Here

@aschwaighofer
Copy link
Contributor

@egorzhdan
I put up a fix here: #58922

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