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

llvm: Collapse runtimes and projects being configured. #78176

Closed
wants to merge 1 commit into from

Conversation

chandlerc
Copy link
Contributor

Using the separate lists here isn't the documented way of building LLVM,
nor what is used by the upstream packagers. It results in different
directory layouts on different Linux distributions that don't work as
well with Clang and other instructions (and don't match the layout on
Mac). It also caused some other problems when porting to linuxbrew that
resulted in disabling things there.

Switching to the simpler all-projects configuration, which is the
documented and recommended way of building LLVM, these problems go away
and the predictable and usable. This causes the tests to pass, etc.

Everything also continues to work on Mac, and this really seems to be
the canonical and preferred way to build LLVM. I have built LLVM from
source with this on my Mac and ensured the tests pass and the resulting
toolchain works.

I also have tested a version of this in Linuxbrew on both a Debian
testing and Ubuntu 16.04 (the distro baseline where historical problems
occurred) and it works well and passes all tests.

If anyone hits issues with LLVM builds in Homebrew, I'm happy to help
debug and help maintain the LLVM build here. I'm a very long time LLVM
developer and user with lots of experience with the build system.


  • Have you followed the guidelines for contributing?
  • Have you checked that there aren't other open pull requests for the same formula update/change?
  • Have you built your formula locally with brew install --build-from-source <formula>, where <formula> is the name of the formula you're submitting?
  • Is your test running fine brew test <formula>, where <formula> is the name of the formula you're submitting?
  • Does your build pass brew audit --strict <formula> (after doing brew install <formula>)?

Using the separate lists here isn't the documented way of building LLVM,
nor what is used by the upstream packagers. It results in different
directory layouts on different Linux distributions that don't work as
well with Clang and other instructions (and don't match the layout on
Mac). It also caused some other problems when porting to linuxbrew that
resulted in disabling things there.

Switching to the simpler all-projects configuration, which is the
documented and recommended way of building LLVM, these problems go away
and the predictable and usable. This causes the tests to pass, etc.

Everything also continues to work on Mac, and this really seems to be
the canonical and preferred way to build LLVM. I have built LLVM from
source with this on my Mac and ensured the tests pass and the resulting
toolchain works.

I also have tested a version of this in Linuxbrew on both a Debian
testing and Ubuntu 16.04 (the distro baseline where historical problems
occurred) and it works well and passes all tests.

If anyone hits issues with LLVM builds in Homebrew, I'm happy to help
debug and help maintain the LLVM build here. I'm a very long time LLVM
developer and user with lots of experience with the build system.
@BrewTestBot BrewTestBot added the python Python use is a significant feature of the PR or issue label May 27, 2021
chandlerc added a commit to chandlerc/linuxbrew-core that referenced this pull request May 27, 2021
Fixes #13332 (again, it has broken and then been fixed a few times in
the interim from what I can tell).

This is essentially the combination of reverting #23059 and applying the
general improvement I have just sent out in
Homebrew/homebrew-core#78176.

The issue motivating #23059 (#23065) seem to have been at least in part
caused by the use of the runtimes configuration approach which is not
the recommended way to build LLVM.

Moreover, even if you don't hit #23065, the prior structure of
configuring libc++ produced a very different layout of files on some
Linux distributions that doesn't match what Clang and LLVM themselves
expect, nor any of the Homebrew or Linuxbrew tests.

With this PR applied, LLVM-12 installs cleanly from source for me on
both a Debian testing machine and an Ubuntu 16.04 machine. Provided the
16.04 machine has a recent enough GCC toolchain installed (LLVM-12 built
binaries can't be linked with GCCs prior to GCC-7), all of `brew test
llvm@12` stuff passes now. And Ubuntu 16.04 is the same baseline as the
distribution in #23065 (sadly I couldn't directly test Linux Mint as
I couldn't find a VM with it installed).

Note that this doesn't add any option for *using* libc++, it simply
builds and includes libc++ as part of the install LLVM and Clang
toolchain.

In addition to the testing on Debian Testing and Ubuntu, I have also
tested the Homebrew/homebrew-core#78176 on my
Mac and it works correctly there as well.

If anyone hits issues with LLVM builds in Homebrew, I'm happy to help
debug and help maintain the LLVM build here. I'm a very long time LLVM
developer and user with lots of experience with the build system.

Also let me know in what order these PRs should land and if one should
be rebased on the other at some point. The Linuxbrew side is the one
that I'm most urgently interested in fixing because at the moment
Linuxbrew installed LLVM-12 doesn't provide libc++ which I need and is
a regression from LLVM-11. But just let me know what the best process is
here.
@chandlerc
Copy link
Contributor Author

Note that a roll-up PR including this for Linuxbrew is here: https://github.com/Homebrew/linuxbrew-core/pull/23339

I'm happy to land these in whatever order makes the most sense. The Linuxbrew case is the pressing one as libc++ is actually broken there w/o the fix, but it seemed good to align the formulas in both cases.

@carlocab
Copy link
Member

Using the separate lists here isn't the documented way of building LLVM

Switching to the simpler all-projects configuration, which is the
documented and recommended way of building LLVM

I'm not convinced this is true. See, for example, LLVM's example for building a distribution of LLVM:

#Enable LLVM projects and runtimes
set(LLVM_ENABLE_PROJECTS "clang;clang-tools-extra;lld" CACHE STRING "")
set(LLVM_ENABLE_RUNTIMES "compiler-rt;libcxx;libcxxabi" CACHE STRING "")

See also https://llvm.org/docs/BuildingADistribution.html#relevant-cmake-options:

When building a distribution that includes LLVM runtime projects (i.e. libcxx, compiler-rt, libcxxabi, libunwind…), it is important to build those projects with the just-built compiler.

So it seems to me that what we put in LLVM_ENABLE_RUNTIMES is exactly what is recommended.

@carlocab carlocab requested a review from Bo98 May 27, 2021 14:38
Copy link
Member

@carlocab carlocab left a comment

Choose a reason for hiding this comment

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

See comment above.

@Bo98
Copy link
Member

Bo98 commented May 27, 2021

It results in different directory layouts

Can you be more specific here? What is the before & after difference here?

The big difference between LLVM_ENABLE_PROJECTS and LLVM_ENABLE_RUNTIMES is that the latter uses the just-built compiler while the former uses the host compiler. You can maybe get away with either for libcxx but you'll probably have issues with compiler-rt.

To me it depends a bit on how you want to use libc++. On macOS, we only support using libc++ via the Clang provided in the LLVM formula - we don't support using the host compiler (you can just use the system provided libc++ instead) - so it makes sense for the just-built compiler to be handling libc++.

There's some discussion in the mailing list which seems to align with my thoughts: https://lists.llvm.org/pipermail/llvm-dev/2020-April/140757.html

@Bo98
Copy link
Member

Bo98 commented May 27, 2021

The difference you are seeing might be a result of Homebrew's compiler shims. LLVM_ENABLE_RUNTIMES will bypass the shims due to the nature of how it works, and I know Linux is a lot more reliant on the shims (this can be both a pro and a con) which is why LLVM_ENABLE_PROJECTS might make things work. But it's not anything that can't be fixed through other CMake options, if you know what the issue is. I know rpath is a common one - is it that?

@chandlerc
Copy link
Contributor Author

I understand that the distribution building instructions mention these options, but I want to be clear that the vast majority of LLVM developers don't use them. =] I'm one of the developers. The example script also does a 2-stage bootstrap which changes things pretty significantly.

I've read the email thread, and I'm excited about this eventually being a reliable way to build LLVM. But I do not think the project is there yet, and the fact that the main documentation hasn't been updated nor have the defaults if you check out LLVM been updated is IMO a pretty clear sign that this is a much less well tested and trodden path.

For now, things work reliably if you don't use this somewhat advanced feature. But they break in some edge cases if you do.

The current config even has a runtime in the normal projects list already: openmp.

As for the specific problem, on Linux it assumes a Debian distribution style multi-arch layout of the built artifacts. This differs from MacOS and all other distributions. I have no idea how to even fix this because the behavior of these variables isn't fully documented yet and IMO isn't as mature.

I'm fine if you want this reported to the community so it improves in the future, but today, there are real problems fixed by using the same instructions as are on the main getting started documentation:
https://llvm.org/docs/GettingStarted.html#local-llvm-configuration

@chandlerc
Copy link
Contributor Author

The difference you are seeing might be a result of Homebrew's compiler shims. LLVM_ENABLE_RUNTIMES will bypass the shims due to the nature of how it works, and I know Linux is a lot more reliant on the shims (this can be both a pro and a con) which is why LLVM_ENABLE_PROJECTS might make things work. But it's not anything that can't be fixed through other CMake options, if you know what the issue is. I know rpath is a common one - is it that?

Just to be clear, this was not the issue. It tries to do a Debian multiarch style installation but does so in a way that doesn't work correctly.

And this is quite broken. The LLVM libraries are installed without a multiarch split, but the runtime libraries are installed with the split. So neither can be used independently.

I could try to find options to fix them, but I would much rather use the simple and boring approach that matches the defaults, the getting started docs, and also already behaves correctly here.

@chandlerc
Copy link
Contributor Author

Also, I'm not as familiar with Mac as with Linux, but happy to try to figure out the failures in the CI and fix them. But I'd like to make sure that folks are OK with the rough approach in parallel.

@carlocab
Copy link
Member

carlocab commented May 27, 2021

This has also been used successfully by thousands, if not tens of thousands, of users for over a year, so I'm not sure it's as unreliable or untested as you suggest it is.

openmp is specified as a project rather than a runtime on purpose, since we want to be able to use it with the host compiler instead of the one that comes with the llvm formula. Though, we do have a libomp formula now, so perhaps we maybe should move it to the runtimes array instead.

I could try to find options to fix them, but I would much rather use the simple and boring approach that matches the defaults, the getting started docs, and also already behaves correctly here.

I'd want to know what this looks like.

Also, I'm not as familiar with Mac as with Linux, but happy to try to figure out the failures in the CI and fix them.

This should be fixed by adding -DCMAKE_INSTALL_RPATH=#{rpath} to the args array. That's what this line is for:

args << "-DRUNTIMES_CMAKE_ARGS=-DCMAKE_INSTALL_RPATH=@loader_path/../lib"

but this line no longer has any effect with the changes you've made.

@carlocab
Copy link
Member

The example script also does a 2-stage bootstrap which changes things pretty significantly.

This is actually also something I'm interested in doing because I think it could help with #77975.

@carlocab
Copy link
Member

I guess alternatively we could scope putting everything in LLVM_ENABLE_PROJECTS to Linux, as this is where it's causing problems.

@chandlerc
Copy link
Contributor Author

The example script also does a 2-stage bootstrap which changes things pretty significantly.

This is actually also something I'm interested in doing because I think it could help with #77975.

FWIW, I'd love to see this happen in general.

My only hesitance is that my first and foremost goal is to make libc++ at least usable on Linux, where today it isn't.

Anyways, will dig into the other things in the thread later today.

@chandlerc
Copy link
Contributor Author

openmp is specified as a project rather than a runtime on purpose, since we want to be able to use it with the host compiler instead of the one that comes with the llvm formula. Though, we do have a libomp formula now, so perhaps we maybe should move it to the runtimes array instead.

Not sure why it would be unable to be used if in the runtimes? AFAIK, it should have the same ABI and DSO dependencies either way, but maybe there is something I'm not aware of -- I'm much more familiar with libc++ and Clang than LLVM's openmp build.

I could try to find options to fix them, but I would much rather use the simple and boring approach that matches the defaults, the getting started docs, and also already behaves correctly here.

I'd want to know what this looks like.

Ok, I'll go digging. I'm .... not especially optimistic.

Also, I'm not as familiar with Mac as with Linux, but happy to try to figure out the failures in the CI and fix them.

This should be fixed by adding -DCMAKE_INSTALL_RPATH=#{rpath} to the args array. That's what this line is for:

args << "-DRUNTIMES_CMAKE_ARGS=-DCMAKE_INSTALL_RPATH=@loader_path/../lib"

but this line no longer has any effect with the changes you've made.

Good catch. FWIW, I think we could make this part work with either approach. That actually seems desirable to me so that things like the libclang's dylib are found correctly. But for now I'll just try to find what undocumented parts of CMake can fix the ...RUNTIMES behavior on Linux as that's the thing that is actually completely broken today....

@carlocab
Copy link
Member

Getting libc++ working on Linux has been on my to-do list for a while too. Thanks for looking into this. 🙂

TBH I don't mind just putting everything in LLVM_ENABLE_PROJECTS on Linux, as that seems like a straightforward enough fix without resorting to undocumented hackery.

I'm also warming up to doing the same on macOS, but I'm a little wary of making big changes here. Some fixes I've applied recently turned out to have broken things for some people (it seems they might've been relying on broken behaviour).

@Bo98
Copy link
Member

Bo98 commented May 28, 2021

The current config even has a runtime in the normal projects list already: openmp.

This was only introduced into LLVM_ENABLE_RUNTIMES=all somewhat recently for their GPU offloader (https://openmp.llvm.org/docs/SupportAndFAQ.html#q-how-to-build-an-openmp-gpu-offload-capable-compiler). We do not support GPU offloaders at this time and so do not need to treat OpenMP as a runtime.

the fact that the main documentation hasn't been updated

I argue the distribution documentation is the main documentation for us - we are distributors. It's even listed first: https://llvm.org/docs/UserGuides.html#llvm-builds-and-distributions.

I want to be clear that the vast majority of LLVM developers don't use them

Probably because it's not really needed for 2-stage builds since your runtime libraries will be compiled with the stage 1 Clang anyway rather than the host one.

I'll be really interested to see an example of someone using compiler-rt compiled under a host GCC. This is the real untested territory for me.

There's perhaps an argument we should be using a 2-stage build here. The benefits are reduced on macOS but there's still perhaps some benefit - this needs investigating more at some point and indeed should not be something we tackle right this moment.

It tries to do a Debian multiarch style installation but does so in a way that doesn't work correctly.

Is that all? That'll be because LLVM_ENABLE_PER_TARGET_RUNTIME_DIR defaults to ON for runtimes, except on AIX (and on macOS it is a no-op).

-DRUNTIMES_CMAKE_ARGS=-DLLVM_ENABLE_PER_TARGET_RUNTIME_DIR=OFF probably works (though I've not tested myself).

@chandlerc
Copy link
Contributor Author

I want to be clear that the vast majority of LLVM developers don't use them

Probably because it's not really needed for 2-stage builds since your runtime libraries will be compiled with the stage 1 Clang anyway rather than the host one.

Most devs don't do 2-stage builds most of the time FWIW. Too slow, and too few benefits.

I'll be really interested to see an example of someone using compiler-rt compiled under a host GCC. This is the real untested territory for me.

This was moderately common for me for a long time (several years).

More recently, I've routinely used a compiler-rt built from a prior version of Clang, no 2-stage bootstrap.

There's perhaps an argument we should be using a 2-stage build here. The benefits are reduced on macOS but there's still perhaps some benefit - this needs investigating more at some point and indeed should not be something we tackle right this moment.

FWIW, if you want the benefits of compiling things with the just-built-compiler, I think 2-stage builds are a far superior option to the RUNTIMES / PROJECTS split. It's simpler and gives more broad benefits IMO. Maybe the split is useful just to simplify the 2-stage build.

And while developers on LLVM rarely benefit from a 2-stage build, for a distribution I actually think it makes lots of sense.

It tries to do a Debian multiarch style installation but does so in a way that doesn't work correctly.

Is that all? That'll be because LLVM_ENABLE_PER_TARGET_RUNTIME_DIR defaults to ON for runtimes, except on AIX.

-DRUNTIMES_CMAKE_ARGS=-DLLVM_ENABLE_PER_TARGET_RUNTIME_DIR=OFF probably works (though I've not tested myself).

Nice, saves me the searching. I'll try this out and report back.

@chandlerc
Copy link
Contributor Author

The Linux issues have been directly fixed, abandoning this. Sorry for the noise and thanks for the help on the other PRs.

@chandlerc chandlerc closed this Jun 6, 2021
@chandlerc chandlerc deleted the fix-libcxx branch June 6, 2021 10:33
@carlocab
Copy link
Member

carlocab commented Jun 6, 2021

No apologies necessary. Thanks very much for the time you put into this.

@github-actions github-actions bot added the outdated PR was locked due to age label Jul 7, 2021
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 7, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
outdated PR was locked due to age python Python use is a significant feature of the PR or issue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants