-
Notifications
You must be signed in to change notification settings - Fork 1.5k
Begin building libc++ and libc++abi runtimes on demand #6424
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
base: trunk
Are you sure you want to change the base?
Conversation
f86c0f6 to
0ff37f2
Compare
| # We can inject custom logic at the end of the site configuration with | ||
| # the ABI defines. This can custom set (or re-set) any of the relevant | ||
| # configuration defines. Note that while this is sorted here in the | ||
| # BUILD file, it is expanded at the _end_ of the configuration header | ||
| # and so overrides the other configuration settings. | ||
| "_LIBCPP_ABI_DEFINES": "\n".join([ |
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.
This seems like a fairly large chunk of C++ code to be including in a build rule. Would it make sense to move the contents of _LIBCPP_ABI_DEFINES out into a separate header file and add a #include here instead?
(Admittedly that only helps a little, as the remaining cmake defines are equally large.)
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'm a bit torn about this...
On one hand, separating out seems much cleaner as the C++ code is no longer embedded in a BUILD file.
On the other hand, there are complex interactions between this C++ code and the defines here, and moving those further apart seems unfortunate.
But seems worth exploring in a follow-up. I've left a TODO to document that this should be revisited and I'll prep a PR we can look at concretely.
45c95f4 to
d3d3fc6
Compare
chandlerc
left a comment
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 for review, merging!
Will send follow-ups to tidy up TODOs and the next steps.
| # We can inject custom logic at the end of the site configuration with | ||
| # the ABI defines. This can custom set (or re-set) any of the relevant | ||
| # configuration defines. Note that while this is sorted here in the | ||
| # BUILD file, it is expanded at the _end_ of the configuration header | ||
| # and so overrides the other configuration settings. | ||
| "_LIBCPP_ABI_DEFINES": "\n".join([ |
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'm a bit torn about this...
On one hand, separating out seems much cleaner as the C++ code is no longer embedded in a BUILD file.
On the other hand, there are complex interactions between this C++ code and the defines here, and moving those further apart seems unfortunate.
But seems worth exploring in a follow-up. I've left a TODO to document that this should be revisited and I'll prep a PR we can look at concretely.
This builds on the previous work to flesh out more on-demand runtimes building. It adds building of the `libc++.a` archive runtime. A number of changes are required for this to work: - The runtimes build infrastructure needs to support building sources from multiple parts of LLVM rather than a single part. We do this by lifting the root of the runtimes source paths up a level to a common runtimes tree, and installing the runtimes sources below this directory. - Both libc++ and libc++abi runtimes sources need to be installed, and we even need to install some interesting parts of llvm-libc that are used in the build of libc++. - We need to generate the site configuration header file for libc++ from the CMake template. This includes both setting up a set of platform-independent defines and introducing some basic Bazel support for processing the CMake template itself. Doing all of this also exposed some missing features and limitations of the runtimes building infrastructure that are addressed here. One note is that all of this just adds libc++ to the explicit `build-runtimes` command for testing. It doesn't yet trigger automatically building these prior to linking, or configuring any of the other subcommands to automatically use these runtimes. All of that will come in follow-up PRs. Also, this makes the `clang_runtimes_test` ... _very_ slow in our default build configuration. Compiling libc++, even with many threads on a large Linux server requires up to 50 seconds. I'm open to any suggestions on how to handle this, including disabling the test in non-optimized builds. I have some ideas to speed this up, but fundamentally building libc++ is... not cheap. I did look at some of the existing Bazel tools to process the CMake template, but they all seemed significantly more complex than what we need and didn't have broad adoption. Given that, it seemed slightly better to just roll our own given the simple format. The second new LLVM patch is currently under review upstream and so hopefully temporary: llvm/llvm-project#169155
d3d3fc6 to
6dcb30e
Compare
This builds on the previous work to flesh out more on-demand runtimes
building. It adds building of the
libc++.aarchive runtime.A number of changes are required for this to work:
The runtimes build infrastructure needs to support building sources
from multiple parts of LLVM rather than a single part. We do this by
lifting the root of the runtimes source paths up a level to a common
runtimes tree, and installing the runtimes sources below this
directory.
Both libc++ and libc++abi runtimes sources need to be installed, and
we even need to install some interesting parts of llvm-libc that are
used in the build of libc++.
We need to generate the site configuration header file for libc++ from
the CMake template. This includes both setting up a set of
platform-independent defines and introducing some basic Bazel support
for processing the CMake template itself.
Doing all of this also exposed some missing features and limitations of
the runtimes building infrastructure that are addressed here.
One note is that all of this just adds libc++ to the explicit
build-runtimescommand for testing. It doesn't yet triggerautomatically building these prior to linking, or configuring any of the
other subcommands to automatically use these runtimes. All of that will
come in follow-up PRs.
Also, this makes the
clang_runtimes_test... very slow in ourdefault build configuration. Compiling libc++, even with many threads on
a large Linux server requires up to 50 seconds. I'm open to any
suggestions on how to handle this, including disabling the test in
non-optimized builds. I have some ideas to speed this up, but
fundamentally building libc++ is... not cheap.
I did look at some of the existing Bazel tools to process the CMake
template, but they all seemed significantly more complex than what we
need and didn't have broad adoption. Given that, it seemed slightly
better to just roll our own given the simple format.
Two of the new LLVM patch are currently under review upstream and so
hopefully temporary: