This is an archive of the discontinued LLVM Phabricator instance.

[RFC] Remove support for building libc++ with `LLVM_ENABLE_PROJECTS`
ClosedPublic

Authored by Ericson2314 on Aug 20 2022, 6:54 PM.

Details

Summary

This has been officially deprecated since D112724, meaning the
deprecation warning is present in released 14 and 15.

This makes me think that now, shortly after the 15 release is branched,
is a good time to pull the trigger.

Diff Detail

Event Timeline

Ericson2314 created this revision.Aug 20 2022, 6:54 PM
Herald added a project: Restricted Project. · View Herald TranscriptAug 20 2022, 6:54 PM
Ericson2314 requested review of this revision.Aug 20 2022, 6:54 PM
Herald added projects: Restricted Project, Restricted Project, Restricted Project, Restricted Project. · View Herald TranscriptAug 20 2022, 6:54 PM
Herald added a reviewer: Restricted Project. · View Herald Transcript
Herald added subscribers: llvm-commits, libcxx-commits, Restricted Project and 2 others. · View Herald Transcript
phosek accepted this revision.Aug 20 2022, 7:40 PM

LGTM

compiler-rt/lib/sanitizer_common/symbolizer/scripts/build_symbolizer.sh
103

I'd consider renaming this to RUNTIMES.

Do the rename @phosek suggested

lkail added a subscriber: lkail.Aug 20 2022, 9:13 PM
This revision was not accepted when it landed; it landed in state Needs Review.Aug 21 2022, 5:11 AM
This revision was automatically updated to reflect the committed changes.
Ericson2314 marked an inline comment as done.
Ericson2314 retitled this revision from [RFC] Remove support for building C++ with `LLVM_ENABLE_PROJECTS` to [RFC] Remove support for building libc++ with `LLVM_ENABLE_PROJECTS`.Aug 21 2022, 5:12 AM

Ah I see email about sphinx jobs defined out of tree :/ I will take a look at that, see if it is easy to fix.

In 952f90b72b3546d6b6b038d410f07ce520c59b48 I relented and made it a non-fatal error until the remaining jobs are figured out.

MaskRay added inline comments.Aug 21 2022, 11:21 AM
clang/lib/Driver/ToolChains/Linux.cpp
310

@mgorny FYI, this has been removed.

@vitalybuka -DLLVM_ENABLE_PROJECTS='libcxx;libcxxabi' in zorg/buildbot/builders/sanitizers/buildbot_functions.sh needs adjustment.

vitalybuka added a comment.EditedAug 21 2022, 5:59 PM

@vitalybuka -DLLVM_ENABLE_PROJECTS='libcxx;libcxxabi' in zorg/buildbot/builders/sanitizers/buildbot_functions.sh needs adjustment.

If I do -DLLVM_ENABLE_RUNTIMES='libcxx;libcxxabi' now it builds parts of LLVM
problem is that with -DLLVM_USE_SANITIZER=Memory, it builds llvm instrumented as well, but we have no instrumented libc++, so we have false report

previously with DLLVM_ENABLE_PROJECTS='libcxx;libcxxabi' no LLVM tools, like tbl-gen where built

Fails like this: https://lab.llvm.org/buildbot/#/builders/74/builds/12976/steps/10/logs/stdio

cmake -GNinja -DCMAKE_BUILD_TYPE=Release -DLLVM_ENABLE_ASSERTIONS=ON -DLLVM_ENABLE_PER_TARGET_RUNTIME_DIR=OFF -DCMAKE_C_COMPILER=/b/sanitizer-x86_64-linux-bootstrap-msan/build/llvm_build0/bin/clang -DCMAKE_CXX_COMPILER=/b/sanitizer-x86_64-linux-bootstrap-msan/build/llvm_build0/bin/clang++ -DLLVM_USE_LINKER=lld '-DLLVM_ENABLE_RUNTIMES=libcxx;libcxxabi' -DCMAKE_BUILD_TYPE=Release -DLLVM_USE_SANITIZER=Memory '-DCMAKE_C_FLAGS=-fsanitize=memory -fsanitize-memory-use-after-dtor -fsanitize-memory-param-retval ' '-DCMAKE_CXX_FLAGS=-fsanitize=memory -fsanitize-memory-use-after-dtor -fsanitize-memory-param-retval ' /b/sanitizer-x86_64-linux-bootstrap-msan/build/llvm-project/llvm

ninja cxx cxxabi

Worked with PROJECTS

@vitalybuka -DLLVM_ENABLE_PROJECTS='libcxx;libcxxabi' in zorg/buildbot/builders/sanitizers/buildbot_functions.sh needs adjustment.

If I do -DLLVM_ENABLE_RUNTIMES='libcxx;libcxxabi' now it builds parts of LLVM
problem is that with -DLLVM_USE_SANITIZER=Memory, it builds llvm instrumented as well, but we have no instrumented libc++, so we have false report

previously with DLLVM_ENABLE_PROJECTS='libcxx;libcxxabi' no LLVM tools, like tbl-gen where built

Fails like this: https://lab.llvm.org/buildbot/#/builders/74/builds/12976/steps/10/logs/stdio

cmake -GNinja -DCMAKE_BUILD_TYPE=Release -DLLVM_ENABLE_ASSERTIONS=ON -DLLVM_ENABLE_PER_TARGET_RUNTIME_DIR=OFF -DCMAKE_C_COMPILER=/b/sanitizer-x86_64-linux-bootstrap-msan/build/llvm_build0/bin/clang -DCMAKE_CXX_COMPILER=/b/sanitizer-x86_64-linux-bootstrap-msan/build/llvm_build0/bin/clang++ -DLLVM_USE_LINKER=lld '-DLLVM_ENABLE_RUNTIMES=libcxx;libcxxabi' -DCMAKE_BUILD_TYPE=Release -DLLVM_USE_SANITIZER=Memory '-DCMAKE_C_FLAGS=-fsanitize=memory -fsanitize-memory-use-after-dtor -fsanitize-memory-param-retval ' '-DCMAKE_CXX_FLAGS=-fsanitize=memory -fsanitize-memory-use-after-dtor -fsanitize-memory-param-retval ' /b/sanitizer-x86_64-linux-bootstrap-msan/build/llvm-project/llvm

ninja cxx cxxabi

Worked with PROJECTS

I guess now I need to use runtimes/ for that?

Ah I see email about sphinx jobs defined out of tree :/ I will take a look at that, see if it is easy to fix.

This still hasn't been fixed, so none of the documentation is being updated on the website: https://lab.llvm.org/buildbot/#/builders/89/builds/31614 -- if it's not easy, please revert until we have a path forward.

I did a "soft revert" in rG952f90b72b3546d6b6b038d410f07ce520c59b48 which makes it a non-fatal error so everything keeps on working, but I can do a real revert too if that is preferred.

Ericson2314 added a comment.EditedAug 22 2022, 7:58 AM

https://reviews.llvm.org/rZORG3a209ca6c1b9 This is what I had started doing. but I am not sure it is a very good solution. I think I might write a discourse post with my thoughts.

I did a "soft revert" in rG952f90b72b3546d6b6b038d410f07ce520c59b48 which makes it a non-fatal error so everything keeps on working, but I can do a real revert too if that is preferred.

It doesn't seem to have helped the sphinx publish bot: https://lab.llvm.org/buildbot/#/builders/89/builds/31676

CMake Error at /home/buildbot/as-worker-4/publish-sphinx-docs/llvm-project/libcxxabi/CMakeLists.txt:138 (message):
  LIBCXXABI_LIBCXX_INCLUDES= is not a valid directory.  Please provide the
  path to where the libc++ headers have been installed.

Please don't change libc++ things like that without my approval first. This is a transition I've been working on for 2+ years, I had a local patch for it waiting to be published, and this patch is incomplete in several ways. I greatly appreciate the effort and wanting to chime into this transition, however concretely this was rushed in.

Please don't change libc++ things like that without my approval first. This is a transition I've been working on for 2+ years, I had a local patch for it waiting to be published, and this patch is incomplete in several ways. I greatly appreciate the effort and wanting to chime into this transition, however concretely this was rushed in.

FWIW, I was also pretty surprised to see something with [RFC] in the title be proposed on a Saturday night and landed the next morning. While we had community agreement on the original deprecation, I think we usually would then follow up with an RFC (on Discourse) proposing to remove the deprecated functionality so people have more notice before the changes landed. (Just something to keep in mind for the future.)

For clarity, @ldionne are you requesting that these changes be reverted due to being incomplete, or do you prefer this be fixed forward?

For clarity, @ldionne are you requesting that these changes be reverted due to being incomplete, or do you prefer this be fixed forward?

Sorry, I left no action item on my end after my first comment. I'm wrapping my head around this change and the other changes that followed D132298 and D132411 first, and I'll post here what I think we should do. I've also seen some internal fallout of this change and I suspect I'm not alone.

Like I said, I greatly appreciate that folks took the lead to push this removal forward, however it should have been under review for more than 24h on a week-end. Concretely, what I planned on doing was turn the message(WARNING into message(ERROR as a dye test without changing anything else, and then turn it back on to message(WARNING to give folks a bit of time to fix their up/downstream CI systems (to avoid them being red for too long).

I'll post here again when I've wrapped my head around this.

This sucks, but I'm going to revert this series of patches. This is getting somewhat out of hands and I feel that we're rushing to fix all kinds of issues in various directions. I'm going to revert the following patches, in this order:

  1. 952f90b72b35 [CMake] Weaken 176db3b3ab25ff8a9b2405f50ef5a8bd9304a6d5
  2. e6a0800532bb [libcxxabi][cmake] Allow building without libcxx again
  3. 176db3b3ab25 [RFC] Remove support for building C++ with LLVM_ENABLE_PROJECTS

Otherwise:

  • I also think we don't want to move forward with D132411 -- it's going against this transition.
  • https://reviews.llvm.org/rZORG3a209ca6c1b9 sounds like a good change we'd need to make no matter what, so that's great.
  • D132454 seems like a good change regardless of this series of patches.

I think that's the whole set of changes/reviews associated to this in the past few days. If I've missed anything, please let me know ASAP.

This will have been extremely useful as a way to shake out places where the deprecated LLVM_ENABLE_PROJECTS build is still being used for libc++/libc++abi. However, we should take the time to fix these places first before landing the actual breaking change, otherwise things become too unstable and we're rushed into making potentially poor decisions.

Concretely, as an action item to move this transition forward, I will:

  1. Grep for places where we still use LLVM_ENABLE_PROJECTS for libc++ and libc++abi in the LLVM monorepo
  2. Do the same in the llvm-zorg repo
  3. Do the same in our downstream bots (if other folks see this because of downstream breakage in their forks, be proactive and do this too, or you will get broken in the future!)
  4. Re-land a patch that turns the WARNING into a FATAL_ERROR, and just that. Watch for more breakage and try fixing it iteratively.
  5. Once we have FATAL_ERROR in place and things are green, we can land the rest of this patch (plus a few other things I had locally) to actually remove support for LLVM_ENABLE_PROJECTS with libc++/libc++abi.

This sucks, but I'm going to revert this series of patches. This is getting somewhat out of hands and I feel that we're rushing to fix all kinds of issues in various directions. I'm going to revert the following patches, in this order:

  1. 952f90b72b35 [CMake] Weaken 176db3b3ab25ff8a9b2405f50ef5a8bd9304a6d5
  2. e6a0800532bb [libcxxabi][cmake] Allow building without libcxx again
  3. 176db3b3ab25 [RFC] Remove support for building C++ with LLVM_ENABLE_PROJECTS

Otherwise:

  • I also think we don't want to move forward with D132411 -- it's going against this transition.
  • https://reviews.llvm.org/rZORG3a209ca6c1b9 sounds like a good change we'd need to make no matter what, so that's great.
  • D132454 seems like a good change regardless of this series of patches.

I think that's the whole set of changes/reviews associated to this in the past few days. If I've missed anything, please let me know ASAP.

This will have been extremely useful as a way to shake out places where the deprecated LLVM_ENABLE_PROJECTS build is still being used for libc++/libc++abi. However, we should take the time to fix these places first before landing the actual breaking change, otherwise things become too unstable and we're rushed into making potentially poor decisions.

Concretely, as an action item to move this transition forward, I will:

  1. Grep for places where we still use LLVM_ENABLE_PROJECTS for libc++ and libc++abi in the LLVM monorepo
  2. Do the same in the llvm-zorg repo
  3. Do the same in our downstream bots (if other folks see this because of downstream breakage in their forks, be proactive and do this too, or you will get broken in the future!)
  4. Re-land a patch that turns the WARNING into a FATAL_ERROR, and just that. Watch for more breakage and try fixing it iteratively.
  5. Once we have FATAL_ERROR in place and things are green, we can land the rest of this patch (plus a few other things I had locally) to actually remove support for LLVM_ENABLE_PROJECTS with libc++/libc++abi.

Thank you @ldionne -- I think this plan of action makes sense to me. The disruption from reverts is unfortunate, but I think it's better to take a clean run at this once we have everything lined up and ready in the other projects.

That sounds good to me too.

Sorry I did unquestionably jump the gun here --- I suppose I was surprised to get a fairly simple approval when I put on the [RFC], and thought "hmm, maybe this isn't so controversial as I feared because indeed the deprecation cycle has been so thorough", but then again the simple approval was probably predicated on the [RFC] indicating I would wait long!

I also think we don't want to move forward with D132411 -- it's going against this transition.

I would like to discuss some of this sort of thing so more, but better to do that on the the discourse than my shoddy commits that need reverting!

I also think we don't want to move forward with D132411 -- it's going against this transition.

I would like to discuss some of this sort of thing so more, but better to do that on the the discourse than my shoddy commits that need reverting!

Sure, I'd be curious to understand why that's needed.

After some thinking, I created the following stack of patches: D132478 => D132479 => D132480.

The last patch is basically a reboot of this patch, i.e. it's the one that will make folks who use LLVM_ENABLE_PROJECTS=libcxx start failing. However, I am also trying to keep LLVM_ENABLE_PROJECTS and LLVM_ENABLE_RUNTIMES consistent, i.e. it should be possible to use LLVM_ENABLE_PROJECTS=all and LLVM_ENABLE_RUNTIMES=all without ever being broken by any of our changes (except libcxx, libcxxabi and libunwind would suddenly start building with the bootstrapping build instead of the normal runtimes build).