-
Notifications
You must be signed in to change notification settings - Fork 10.4k
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
[stdlib] Get rid of the single bridging header for ELF platforms like linux #66665
Conversation
header "SwiftGlibc.h" | ||
% if CMAKE_SDK not in ["ANDROID"]: | ||
header "stdc-predef.h" | ||
header "features.h" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All these headers were removed for Android because they don't exist in Bionic, except for features.h
, which is only in Bionic for glibc compatibility and is unnecessary plus it causes build issues in C++ Interop.
header "inttypes.h" | ||
header "iso646.h" | ||
% if CMAKE_SDK not in ["LINUX", "ANDROID"]: | ||
header "libutil.h" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I checked all these headers in glibc too, and both Bionic and glibc don't have this. I only see a bsd/libutil.h
from the libbsd-dev
Ubuntu package, which I don't think the compiler finds.
If this header is wanted on linux, I can change this to bsd/libutil.h
instead, so that the compiler will find it.
header "tgmath.h" | ||
header "time.h" | ||
% if CMAKE_SDK in ["OPENBSD"]: | ||
header "util.h" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@3405691582 added this for his OpenBSD port, and neither glibc nor Bionic have it.
header "arpa/inet.h" | ||
% if CMAKE_SDK not in ["LINUX", "ANDROID"]: | ||
header "bsd/ifaddrs.h" | ||
header "bsd/pty.h" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
header "spawn.h" | ||
header "strings.h" | ||
% if CMAKE_SDK not in ["LINUX", "ANDROID"]: | ||
header "sys/event.h" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only one Ubuntu package, libkqueue-dev
, installs this, but in a different directory so the compiler won't find it even if it happens to be installed. It isn't in Bionic either.
% if CMAKE_SDK not in ["ANDROID"]: | ||
header "ulimit.h" | ||
header "utmpx.h" | ||
header "wordexp.h" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried to keep the ordering of headers the same throughout, in case that matters, but shifted these last two. I can keep them separately above if the order matters.
@swift-ci please smoke test |
The linux CI failed because it couldn't find What do you think, @egorzhdan, should I add that triple prefix for linux so we can run the CI again? |
Alright, I added another commit just as a hack for the linux CI. Please run the linux CI again and we can see how well this works. If it does, I will turn this into a CMake variable later. |
@ktoso, if you're still up, would you run the linux CI on this? |
Middle of the day for me, so sure — I can kick that off. No idea if changes make sense tho, someone else will have to review. |
@swift-ci please smoke test Linux |
Hmm, now it chokes on the linux CI when trying to build CxxStdlib:
The good news is that it was able to build the glibc module and the Swift stdlib for linux just fine before that. The bad news is that it has problems with C++, only when building itself now. @egorzhdan, maybe you know how to deal with these builtins, let me know if I should try something. |
I'm not sure I fully understand the problem here, but can I ask how this approach solves the first problem "SwiftGlibc is missing definitions in some submodules" in the forum post? |
Egor said last year that this approach should fix both those problems and an additional one he laid out, but I'm still seeing those cyclic dependency issues with libc++ on Android after this pull. I don't deal with this stuff, so I can't answer your question, but Egor did offer to go into it in more detail then. Maybe he will now. |
@egorzhdan, please run the CI again on this, wondering if anything has changed. |
@swift-ci Please smoke test linux |
Still fails when building CxxStdlib:
@egorzhdan, any idea what's still missing? |
@egorzhdan, any idea if this should work now? |
@kateinoigakukun, I notice you use this for the WASI libc after removing the builtin headers, 35120a1, so I did the same here and rebased. Please run the CI and let's see if it works now. |
@swift-ci Please smoke test Linux Platform |
Oh nice, this passes the full compiler validation suite now and builds all the corelibs fine, only failing with a header collision on the linux CI when building TSC for SwiftPM with CMake:
@egorzhdan, what do you think: should I try to fix that last issue and get this in now? @martinboehme, what do you think about removing the |
Love this, SwiftGlibc.h is bad news - it doesn't actually modularize anything! The libc++ headers end up eating a lot of the Glibc headers and it's random which module ultimately claims them. I don't know if we want to tackle it in this PR, but the C standard library headers should all get their own top level modules so that they can layer properly with the clang and libc++ headers. It only works at all right now because Swift is passing |
Closing in favor of the more extensive Bionic changes in #72161, after which others can follow suit by modularizing Glibc and the BSD libc's also. |
As discussed before with @egorzhdan, this removes the single libc header added in #32404 and moves to the same config as the libstdc++ modulemap. This is just intended for test and discussion right now, so we can see if it works and refine it.
I built the stdlib natively on Android AArch64 using this pull and it mostly worked, but around 70 C++ Interop tests from the compiler validation suite fail along with 1 IRGen test related to C Interop.
I cannot build the stdlib on linux right now with the latest trunk snapshot, as the Swift compiler crashes, and I don't want to debug why it fails for me on Ubuntu 20.04 but not on the CI.
@egorzhdan, please run the linux CI on this and let's see if any tests fail.
@3405691582 and @mhjacobson, you'll want to try this on the BSDs and let me know if we should disable any of these headers on there, plus what tests fail for you.