-
Notifications
You must be signed in to change notification settings - Fork 10.8k
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
[compiler-rt] Allow building builtins.a without a libc #86737
[compiler-rt] Allow building builtins.a without a libc #86737
Conversation
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.
Thanks, I've definitely done this in the past and we should continue to keep this working. I wonder if we could add a basic "cross compile the builtins for baremetal" GitHub action?
Commit message nit: we're no longer checking for a libs header but rather one that is commonly provided by the compiler. pthread_mutex_lock is also only needed for the atomics helper, all other builtins should not require it |
Hmm - FWIW, for doing this kind of bootstrap, it's already possible by building the builtins library on its own, standalone from the rest of compiler-rt, by pointing cmake at |
compiler-rt/cmake/config-ix.cmake
Outdated
set(SIMPLE_SOURCE ${CMAKE_BINARY_DIR}${CMAKE_FILES_DIRECTORY}/simple.cc) | ||
file(WRITE ${SIMPLE_SOURCE} "#include <stdlib.h>\n#include <stdio.h>\nint main(void) { printf(\"hello, world\"); }\n") | ||
file(WRITE ${SIMPLE_SOURCE} "#include <stdint.h>\nint main(void) { return 0; }\n") |
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.
Do we need to pull in any headers at all? Why not just use int main(void) { return 0; }
?
Yes, this scenario is supported by the bootstrapping build which uses |
CMAKE_C_COMPILER_WORKS=TRUE doesn't help, sadly. I've tried a lot of cmake cross compilation hacks lately without much luck. I think cmake deprecated and then disabled CMAKE_C_COMPILER_WORKS at some point. I think in principle cross compilation can work, but our scripts have a lot of ad hoc compiler and run style checks like this one which vary in the extent to which they pass cmake flags around. There's a CheckAtomic or similar elsewhere in runtimes which has failed a lot as I try to find some path through our scripts. I'll drop the stdint.h part, not sure it adds anything. Compiling compiler-rt builtins by itself is a really useful first step in standing up a sysroot. |
62fa1a4
to
57c41c9
Compare
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.
Bit late, but LGTM.
This patch broke our clang-armv8-lld-2stage buildbot: https://lab.llvm.org/buildbot/#/builders/178/builds/7117. Can you please take a look at it? Let me know if you need any help to investigate it. |
This reverts commit 8669225. Reverting due to buildbot failures.
Reverted to get the buildbot passing again. |
I'm pretty sure the buildbot break is unrelated to this. The sanitizer source that choked on an arm specific GNU header is unrelated to a cmake test file looking for stdio. I see the arm bot is presently green, going to reland this. |
So much for that theory, failed the same way. Is your buildbot choosing whether to try to build sanitizers based on whether it can built builtins, such that spuriously failing to build builtins without this patch is "success" but successfully building builtins means it tries to build the sanitizers, which it can't, and that failure to build is not ok? If you don't need sanitizers (presumably true, since it looks like your bot doesn't build them with or without this patch?), can you pass the sanitizers=off flag to cmake? |
I'd like to reopen this PR to track it, but there is no button next to the comment where it should be, and the checkbox option silently does nothing. I don't really want to leave the bot broken, but I also don't want @luporl to miss this as I'd like them to change their buildbot. I think my least-bad option is revert this, hack around it locally and spawn an issue to decrease the chance of me forgetting about the local patch. Opened #87206, reverted the patch, asked on discourse about opening a merged patch and apparently it's something github can't do. |
compiler-rt may depend on libc (memset etc). Likewise a libc built by clang may depend on compiler-rt builtins.
This circular dependency doesn't matter much once they're both compiled. The easy compilation order to build both from source is:
This patch relaxes the cmake sanity check to pass without requiring a libc library. That allows the above sequence to work. Otherwise one needs to build a static libc, then use that to pass the compiler-rt cmake check, then build a normal libc.