-
-
Notifications
You must be signed in to change notification settings - Fork 3.2k
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
Conversation
b4b9a39
to
df80a05
Compare
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.
Ports/zig/patches/0001-Support-Add-support-for-building-LLVM-on-SerenityOS.patch
Show resolved
Hide resolved
Toolchain/Patches/llvm/0005-libcxx-Add-support-for-SerenityOS.patch
Outdated
Show resolved
Hide resolved
Toolchain/Stubs/README.md
Outdated
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 |
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.
wanna explain briefly what to mess with in the cmake files? not a requirement, but this reads like "fuck you, not my problem".
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.
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.
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 mostly meant giving a better description in the document than ":shrug: get a libc build")
Toolchain/Stubs/aarch64/libc.so
Outdated
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 kinda don't like that we're committing object files into the repo.
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.
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.
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.
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.
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.
Looks good to me. @BertalanD any final thoughts? or @implicitfield ?
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() |
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 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.
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.
TODO:
Wait for spholz to patch the linker scriptomp.h
in install directory when building[email protected]
withopenmp
as runtime with[email protected]
llvm/llvm-project#86744, update cmake to >= 3.29.2 actions/runner-images#9680Toolchain/Patches/llvm/ReadMe.md
Depends on #23947CC @spholz @BertalanD @ADKaster