Skip to content

Toolchain+Ports: Update LLVM to 16.0.3#18522

Merged
awesomekling merged 10 commits intoSerenityOS:masterfrom
implicitfield:LLVM
Jun 27, 2023
Merged

Toolchain+Ports: Update LLVM to 16.0.3#18522
awesomekling merged 10 commits intoSerenityOS:masterfrom
implicitfield:LLVM

Conversation

@implicitfield
Copy link
Copy Markdown
Contributor

@implicitfield implicitfield commented Apr 26, 2023

Notes:

In addition, the following changes were made to reconcile conflicts between LibC and libc++:

  • Install LibC into the sysroot, as that's the only way to get the include order right with libc++.
  • Never allow #include <LibC/, as that breaks the new include order.
  • All targets that directly or indirectly depend on LibC's headers but which don't link against LibC must depend on install_libc_headers to ensure that those headers have been (re)installed into the sysroot.
  • Including any other complex number header than AK/Complex.h is now prohibited, as all of those will point to libc++ when compiling in C++ mode.
  • A bunch of fixes to facilitate the aforementioned changes.

@BertalanD BertalanD self-requested a review April 26, 2023 17:13
@BertalanD
Copy link
Copy Markdown
Member

Hi.

Thank you for doing this. We have discussed the situation regarding the libc++ include checks on Discord, would you mind joining the discussion in #clang-toolchain?

@implicitfield
Copy link
Copy Markdown
Contributor Author

would you mind joining the discussion in #clang-toolchain?

Unfortunately, it looks like Discord won't let me verify my account in a feasible way, so this isn't possible for me at the moment.

@implicitfield implicitfield marked this pull request as ready for review April 29, 2023 21:33
@github-actions github-actions bot added the 👀 pr-needs-review PR needs review from a maintainer or community member label Apr 29, 2023
@implicitfield
Copy link
Copy Markdown
Contributor Author

Fixing the libc++ include situation was a lot more involved than I expected. I believe I've resolved it in the most correct way possible, by installing LibC into the sysroot like most systems do, and by fixing all the issues I could find arising from that.

@implicitfield
Copy link
Copy Markdown
Contributor Author

implicitfield commented Apr 29, 2023

Tests are still failing to build, drafting this again for now.

@implicitfield implicitfield marked this pull request as draft April 29, 2023 21:57
@github-actions github-actions bot removed the 👀 pr-needs-review PR needs review from a maintainer or community member label Apr 29, 2023
@implicitfield
Copy link
Copy Markdown
Contributor Author

implicitfield commented Apr 30, 2023

Looks like a bunch of platforms don't actually include a functional elf.h. I guess Serenity's elf header will have to do even in Lagom builds, as a part of LibELF.

@implicitfield implicitfield marked this pull request as ready for review May 2, 2023 11:35
@implicitfield implicitfield requested a review from trflynn89 as a code owner May 2, 2023 11:35
@github-actions github-actions bot added the 👀 pr-needs-review PR needs review from a maintainer or community member label May 2, 2023
@implicitfield
Copy link
Copy Markdown
Contributor Author

Updated to LLVM 16.0.3, which was released a few days ago.

@implicitfield implicitfield changed the title Toolchain+Ports: Update LLVM to 16.0.2 Toolchain+Ports: Update LLVM to 16.0.3 May 7, 2023
string(REPLACE "../" "" subdirectory "${directory}")
file(MAKE_DIRECTORY "${CMAKE_STAGING_PREFIX}/${CMAKE_INSTALL_INCLUDEDIR}/${subdirectory}")
add_custom_command(
TARGET install_libc_headers
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

if I'm reading this right, this will create a distinct custom command target for each header in LibC? that sounds like... a lot of commands. Is there a way we can do this in one command? I feel like this will be kind of slow.

At the very least, we should make sure that the custom prebuild command has a dependency on the input file location, and that the custom target install_libc_headers has a dependency on the destination file location.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

i.e., have you tested that updating any LibC header will cause this action to re-run?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Updating any LibC header should indeed cause this to rerun. I believe copy_if_different only works with individual files and directories, so we'd need to move all of LibC's headers into a subdirectory to reduce the amount of commands.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Right... install(FILES) can work on directories with a FILES_MATCHING pattern, not sure about file(COPY) at configure time or any of the things we can do in the build stage rather than the install or configure stage.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Install doesn't seem to quite work in this case, since there doesn't seem to be a way to wire it with a custom target, unless I'm missing something? Seems like CMake doesn't support this usecase particularly well.

implicitfield and others added 10 commits June 26, 2023 20:01
libc++ disallows including LibC's complex.h in C++ mode. This means that
a C++ file cannot expect LibC's complex.h to be included, and thus
cannot use c-prefixed complex number functions. As a result,
complex.cpp is broken when libc++ has a higher include priority
than LibC.

A check for __cplusplus has been added to complex.h to warn users of
toolchains that don't use libc++.
LibC's complex.h should not be used in C++ code, and libc++'s version
implementation does not follow the Serenity C++ style.
This is needed to avoid including LibC headers in Lagom builds.
This is needed to avoid including LibC headers in Lagom builds.
Unfortunately, we cannot rely on the build machine to provide a
fully POSIX-compatible ELF header for Lagom builds, so we have to
use our own.
Once LibC is installed to the sysroot and its conflicts with libc++
are resolved, including LibC headers in such a way will cause errors
with a modern LLVM-based toolchain.
Since https://reviews.llvm.org/D131441, libc++ must be included before
LibC. As clang includes libc++ as one of the system includes, LibC
must be included after those, and the only correct way to do that is
to install LibC's headers into the sysroot.

Targets that don't link with LibC yet require its headers for one
reason or another must add install_libc_headers as a dependency to
ensure that the correct headers have been (re)installed into the
sysroot.

LibC/stddef.h has been dropped since the built-in stddef.h receives
a higher include priority.

In addition, string.h and wchar.h must
define __CORRECT_ISO_CPP_STRING_H_PROTO and
_LIBCPP_WCHAR_H_HAS_CONST_OVERLOADS respectively in order to tell
libc++ to not try to define methods implemented by LibC.
All uses of this were removed in c4707ed.
@ADKaster ADKaster added ✅ pr-maintainer-approved-but-awaiting-ci PR has been approved by a maintainer and can be merged after CI has passed and removed 👀 pr-needs-review PR needs review from a maintainer or community member labels Jun 27, 2023
@awesomekling awesomekling merged commit aa0ed4a into SerenityOS:master Jun 27, 2023
@github-actions github-actions bot removed the ✅ pr-maintainer-approved-but-awaiting-ci PR has been approved by a maintainer and can be merged after CI has passed label Jun 27, 2023
@implicitfield implicitfield deleted the LLVM branch June 27, 2023 18:30
emilytrau added a commit to emilytrau/nixpkgs that referenced this pull request Jun 28, 2023
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.

6 participants