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

[stdlib] Get rid of the single bridging header for ELF platforms like linux #66665

Closed
wants to merge 3 commits into from

Conversation

finagolfin
Copy link
Contributor

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.

header "SwiftGlibc.h"
% if CMAKE_SDK not in ["ANDROID"]:
header "stdc-predef.h"
header "features.h"
Copy link
Contributor Author

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"
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 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"
Copy link
Contributor Author

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"
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 don't see either of these in any Ubuntu packages, nor in Bionic.

header "spawn.h"
header "strings.h"
% if CMAKE_SDK not in ["LINUX", "ANDROID"]:
header "sys/event.h"
Copy link
Contributor Author

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"
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 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.

@egorzhdan
Copy link
Collaborator

@swift-ci please smoke test

@finagolfin
Copy link
Contributor Author

The linux CI failed because it couldn't find sys/file.h, which glibc places in /usr/include/x86_64-linux-gnu/ instead. That worked before because the headers from SwiftGlibc.h were found using clang's includes, but if we move them back to the modulemap, I guess we'll need something like the prior solution when these headers were in the modulemap, ie explicitly specifying this triple directory for platforms that need it.

What do you think, @egorzhdan, should I add that triple prefix for linux so we can run the CI again?

@finagolfin
Copy link
Contributor Author

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.

@finagolfin
Copy link
Contributor Author

@ktoso, if you're still up, would you run the linux CI on this?

@ktoso
Copy link
Member

ktoso commented Jun 16, 2023

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.

@ktoso
Copy link
Member

ktoso commented Jun 16, 2023

@swift-ci please smoke test Linux

@finagolfin
Copy link
Contributor Author

Hmm, now it chokes on the linux CI when trying to build CxxStdlib:

===== /usr/lib/gcc/x86_64-linux-gnu/9/../../../../include/c++/9/cmath:662:12 =====
 662 |   { return __builtin_signbit(__x); }
     |            ^ note: '__builtin_signbit' declared here
===== /usr/lib/gcc/x86_64-linux-gnu/9/../../../../include/c++/9/complex:1850:14 =====
1850 |       return std::signbit(__x) ? __type(3.1415926535897932384626433832795029L)
     |              ^ error: no member named '__builtin_signbit' in namespace 'std'; did you mean simply '__builtin_signbit'?

=== /home/build-user/swift/stdlib/public/Cxx/std/std.swift:13:19 ===
12 | 
13 | @_exported import CxxStdlib // Clang module
   |                   ^ error: could not build C module 'std'
14 | @_exported import Cxx // Swift module

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.

@kateinoigakukun
Copy link
Member

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?

@finagolfin
Copy link
Contributor Author

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.

@finagolfin
Copy link
Contributor Author

@egorzhdan, please run the CI again on this, wondering if anything has changed.

@kateinoigakukun
Copy link
Member

@swift-ci Please smoke test linux

@finagolfin
Copy link
Contributor Author

Still fails when building CxxStdlib:

/usr/lib/gcc/x86_64-linux-gnu/9/../../../../include/c++/9/cmath:42:10: error: cyclic dependency in module 'std': std -> SwiftGlibc -> std

@egorzhdan, any idea what's still missing?

@finagolfin
Copy link
Contributor Author

@egorzhdan, any idea if this should work now?

@finagolfin
Copy link
Contributor Author

@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.

@kateinoigakukun
Copy link
Member

@swift-ci Please smoke test Linux Platform

@finagolfin
Copy link
Contributor Author

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:

<module-includes>:1:10: note: in file included from <module-includes>:1:
#include "TSCclibc.h"
         ^
/home/build-user/swift-tools-support-core/Sources/TSCclibc/include/TSCclibc.h:2:10: note: in file included from /home/build-user/swift-tools-support-core/Sources/TSCclibc/include/TSCclibc.h:2:
#include <sys/inotify.h>
         ^
/usr/include/x86_64-linux-gnu/sys/inotify.h:24:10: note: in file included from /usr/include/x86_64-linux-gnu/sys/inotify.h:24:
#include <bits/inotify.h>
         ^
/usr/include/x86_64-linux-gnu/bits/inotify.h:25:5: error: redefinition of enumerator 'IN_CLOEXEC'
    IN_CLOEXEC = 02000000,
    ^
/usr/include/x86_64-linux-gnu/bits/inotify.h:26:20: note: expanded from macro 'IN_CLOEXEC'
#define IN_CLOEXEC IN_CLOEXEC
                   ^
/usr/include/x86_64-linux-gnu/bits/inotify.h:25:5: note: previous definition is here
    IN_CLOEXEC = 02000000,
    ^

@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 SwiftGlibc.h.gyb header you added a couple years ago in #32404?

@ian-twilightcoder
Copy link
Contributor

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 -fbuiltin-headers-in-system-modules when it adds these module maps, but that doesn't allow layering with libc++ (and it's kind of a hack).

@finagolfin
Copy link
Contributor Author

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.

@finagolfin finagolfin closed this Mar 20, 2024
@finagolfin finagolfin deleted the libc branch March 26, 2024 06:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants