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

Toolchain: Update LLVM to 18.1.3 #23960

Merged
merged 8 commits into from
Apr 18, 2024
Merged

Conversation

DanShaders
Copy link
Contributor

@DanShaders DanShaders commented Apr 14, 2024

TODO:

Depends on #23947

CC @spholz @BertalanD @ADKaster

@DanShaders DanShaders force-pushed the llvm18 branch 5 times, most recently from b4b9a39 to df80a05 Compare April 15, 2024 06:35
BertalanD and others added 6 commits April 15, 2024 20:48
Doing so causes a function type mismatch, which makes the test crash
when built with a new enough version of UBSan.
The original approach failed to link with LLVM 18 because of the changed
symbol mangling.
These don't cause compilation to fail but they still crash crashd.
Small position independent code model (which we end up using after this
change) is suitable for us since the kernel is not expected to grow more
than 2Gb in size. This might be a bit risky since this model is not
mentioned anywhere except for System V ABI document but experiments show
that the kernel compiled with this change works just fine.
@DanShaders DanShaders marked this pull request as ready for review April 17, 2024 03:02
@github-actions github-actions bot added the 👀 pr-needs-review PR needs review from a maintainer or community member label Apr 17, 2024
Clang toolchain. Then, using the `llvm-ifs` tool, these libraries need to be converted
into a stripped-down stub form. To do that, run the following command:
First, you need to compile the LLVM toolchain and the SerenityOS's LibC. This will be a bit awkward
(but possible with enough time spend modifying CMake files) until (unless) we solve the dependency
Copy link
Member

Choose a reason for hiding this comment

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

wanna explain briefly what to mess with in the cmake files? not a requirement, but this reads like "fuck you, not my problem".

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 is really just "I don't care how you get your libc, just get it". I think what has to be done is the following:

  • compile clang without runtimes
  • make llvm not set -Wl,-z,defs for runtime
  • make empty libc.so stub
  • compile runtime
  • compile Serenity
  • generate proper libc.so stub
  • undo -Wl,-z,defs patch

Or you can cheat and first compile Serenity using GCC toolchain and grab libc from there.

Copy link
Member

Choose a reason for hiding this comment

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

(I mostly meant giving a better description in the document than ":shrug: get a libc build")

Copy link
Member

Choose a reason for hiding this comment

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

I kinda don't like that we're committing object files into the repo.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Introducing backdoor payload in stub libraries that are neither linked in nor executed. (Well, theoretically I can execute code in them during CMake configure checks that try to run executables but that would require providing correct interpreter and runtime paths somehow)

More seriously, I think the ideal solution would be to have a LibC stub source that looks like

extern "C" {
void fopen() {}
void fclose() {}
...
}

and then we can compile it with -nostdlib in between two stages of llvm compilation to get an actual stub. At this point, we would probably also want to ensure in CI that it is always up-to-date. But this PR is already too big for what it does so let's just commit binary files.

Copy link
Member

Choose a reason for hiding this comment

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

Not really concerned with the security aspect, more so that these lovely files just feel wrong to have.

not too strongly opinionated on it tho, we already have them in master so :yakshrug:

LLVM toolchain is about to get updated to 18.1.3, which would desync
these patches.
Copy link
Member

@ADKaster ADKaster 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. @BertalanD any final thoughts? or @implicitfield ?

Comment on lines +3 to +6
if (CMAKE_VERSION VERSION_EQUAL "3.29.0" OR CMAKE_VERSION VERSION_EQUAL "3.29.1")
message(FATAL_ERROR "CMake versions 3.29.0 and 3.29.1 are known to not install LLVM correctly. "
"Please either downgrade CMake or update it to 3.29.2+.")
endif()
Copy link
Member

Choose a reason for hiding this comment

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

I found that the Serenity build itself had trouble with these cmake versions. we should ban them in either Serenity/CMakeLists.txt or Meta/serenity.sh in a follow-up.

@BertalanD
Copy link
Member

lg2m, and thanks for picking this up

For Ali's comment about the bootstrapping docs, I'd appreciate a link in the docs to the discussion here, but I don't think we need to go into more detail.

Apart from bumping the toolchain Clang's and port's version, this commit
completely overhauls the way LLVM toolchain is built.

First, it gets rid of a complicated two-stage process of first compiling
clang and compiler-rt builtins and then building libunwind, libc++abi,
and libc++ -- it is possible to create a complete cross-compilation
toolchain in a single CMake invocation with a modern LLVM. Moreover, the
old method was inherently unsupported and subtly broken.

Next, it utilizes full potential of the Stubs "framework". Now we are
even able to compile Clang with -Wl,-z,defs which makes one of the
patches obsolete and the whole installation less error-prone. Note that
it comes at a cost of complicating the bootstrap process on a completely
novel architecture but this hopefully won't happen often.

Lastly, it fixes handling of the -no*lib* family of flags in the
Serenity LLVM driver and correctly uses -nostartfiles in conjunction
with stubs to make necessary CMake configure-time checks succeed.
@ADKaster ADKaster merged commit fa1eef8 into SerenityOS:master Apr 18, 2024
14 checks passed
@github-actions github-actions bot removed the 👀 pr-needs-review PR needs review from a maintainer or community member label Apr 18, 2024
@DanShaders DanShaders deleted the llvm18 branch April 21, 2024 23:31
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