Skip to content

llvm 13.0.0#85898

Closed
carlocab wants to merge 15 commits intoHomebrew:masterfrom
carlocab:llvm-13
Closed

llvm 13.0.0#85898
carlocab wants to merge 15 commits intoHomebrew:masterfrom
carlocab:llvm-13

Conversation

@carlocab
Copy link
Copy Markdown
Member

  • Have you followed the guidelines for contributing?
  • Have you ensured that your commits follow the commit style guide?
  • 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 --build-from-source <formula>)? If this is a new formula, does it pass brew audit --new <formula>?

@carlocab carlocab added prerelease-testing Pull request from upstream, testing a pre-release with homebrew dependencies CI-long-timeout [DEPRECATED] Use longer GitHub Actions CI timeout. CI-no-fail-fast Continue CI tests despite failing GitHub Actions matrix builds. labels Sep 25, 2021
@BrewTestBot BrewTestBot added the automerge-skip `brew pr-automerge` will skip this pull request label Sep 25, 2021
@carlocab
Copy link
Copy Markdown
Member Author

GCC 5 patch will need updating. CC @danielnachun

@Bo98
Copy link
Copy Markdown
Member

Bo98 commented Sep 25, 2021

The patch shouldn't be necessary in LLVM 13 since it's supposed to have been fixed upstream.

@carlocab
Copy link
Copy Markdown
Member Author

The patch shouldn't be necessary in LLVM 13 since it's supposed to have been fixed upstream.

LLDB still fails to build, so it seems this is not yet fixed. The fix referenced at the start of the bug report isn't complete.

@Bo98
Copy link
Copy Markdown
Member

Bo98 commented Sep 26, 2021

The fix referenced at the start of the bug report isn't complete.

Ok I didn't notice the additional part. The additional patch to NativeRegisterContextLinux_x86_64::GetSyscallData might still be needed but the rest of our patch, which was backporting an existing commit, should no longer be needed.

But the error doesn't appear to be complaining about that but rather ThreadPlanStack instead. From first glance, I'm guessing this one is more on the C++ stdlib side rather than the compiler side (though that doesn't change too much from our angle).

@carlocab
Copy link
Copy Markdown
Member Author

Some of these look like they might need something other than a revision bump, but let's see.

Julia will fail to build with Xcode 13. JuliaLang/julia@4dea1c4 needs backporting.

@carlocab carlocab added the CI-linux-self-hosted Build on Linux self-hosted runner label Sep 29, 2021
@carlocab
Copy link
Copy Markdown
Member Author

spirv-llvm-translator has a hard requirement on LLVM 12 (i.e. it won't accept anything newer or older). Some of the others I've switched may not be absolutely necessary, but some are.

@carlocab carlocab force-pushed the llvm-13 branch 3 times, most recently from 0f06fc1 to 7f640ec Compare September 29, 2021 11:24
@carlocab
Copy link
Copy Markdown
Member Author

carlocab commented Sep 30, 2021

In file included from /tmp/llvm-20210929-83167-i3pu42/llvm-project-13.0.0rc4.src/compiler-rt/lib/builtins/absvdi2.c:13:
In file included from /tmp/llvm-20210929-83167-i3pu42/llvm-project-13.0.0rc4.src/compiler-rt/lib/builtins/int_lib.h:93:
In file included from /tmp/llvm-20210929-83167-i3pu42/llvm-project-13.0.0rc4.src/llvm/build/lib/clang/13.0.0/include/limits.h:21:
In file included from /usr/include/limits.h:25:
/usr/include/features.h:367:12: fatal error: 'sys/cdefs.h' file not found
#  include <sys/cdefs.h>
           ^~~~~~~~~~~~~
1 error generated.
CMakeFiles/clang_rt.builtins-i386.dir/build.make:75: recipe for target 'CMakeFiles/clang_rt.builtins-i386.dir/absvdi2.c.o' failed
make[5]: *** [CMakeFiles/clang_rt.builtins-i386.dir/absvdi2.c.o] Error 1
make[5]: Leaving directory '/tmp/llvm-20210929-83167-i3pu42/llvm-project-13.0.0rc4.src/llvm/build/runtimes/builtins-bins'
CMakeFiles/Makefile2:439: recipe for target 'CMakeFiles/clang_rt.builtins-i386.dir/all' failed
make[4]: *** [CMakeFiles/clang_rt.builtins-i386.dir/all] Error 2
make[4]: Leaving directory '/tmp/llvm-20210929-83167-i3pu42/llvm-project-13.0.0rc4.src/llvm/build/runtimes/builtins-bins'
Makefile:135: recipe for target 'all' failed
make[3]: *** [all] Error 2

Ideas, anyone?

sys/cdefs.h appears to be in /usr/include/x86_64-linux-gnu in our Docker container, but that path is likely not portable. I suppose adding it to the include path doesn't hurt? CC @danielnachun @iMichka

@danielnachun
Copy link
Copy Markdown
Contributor

I think sys/cdefs.h is a part of glibc, so I do find it odd that it can't find those headers. Nonetheless I think adding that include path is probably fine. LLVM is relocatable so almost nobody will be building it from source.

@carlocab carlocab force-pushed the llvm-13 branch 3 times, most recently from e9a0a32 to 0215433 Compare September 30, 2021 16:18
@carlocab carlocab force-pushed the llvm-13 branch 2 times, most recently from 6962090 to 6563163 Compare October 2, 2021 06:51
@carlocab
Copy link
Copy Markdown
Member Author

carlocab commented Oct 2, 2021

LLVM 13 is out. Still haven't worked out how to build it on Linux. Going to try Homebrew GCC.

@carlocab carlocab changed the title llvm 13.0.0-rc4 llvm 13.0.0 Oct 2, 2021
Also, replace workaround with setting the appropriate `RPATH` instead of
copying `libLLVM`.
Also, update the patches to use the merged ones from klee/klee#1389.
Also, skip the `dlopen` tests on Linux, as they're not suitable when
Julia depends on a keg-only LLVM.
@danielnachun
Copy link
Copy Markdown
Contributor

@chandlerc the error @carlocab posted above is new as of LLVM 13, and prevents us from building with GCC 5. Can you take a look? It is probably related to this commit llvm/llvm-project@6eb576d, though there were two others before it that were added after the LLVM 12 release.

@carlocab carlocab mentioned this pull request Oct 7, 2021
6 tasks
@carlocab
Copy link
Copy Markdown
Member Author

carlocab commented Oct 7, 2021

All builds are green. This should be ready to go very soon.

@carlocab
Copy link
Copy Markdown
Member Author

carlocab commented Oct 7, 2021

Merging, since #80227 is waiting on this one.

@BrewTestBot

This comment has been minimized.

@BrewTestBot

This comment has been minimized.

@BrewTestBot
Copy link
Copy Markdown
Contributor

:shipit: @carlocab has triggered a merge.

@carlocab carlocab deleted the llvm-13 branch October 7, 2021 14:53
@carlocab
Copy link
Copy Markdown
Member Author

carlocab commented Oct 7, 2021

@chandlerc the error @carlocab posted above is new as of LLVM 13, and prevents us from building with GCC 5. Can you take a look? It is probably related to this commit llvm/llvm-project@6eb576d, though there were two others before it that were added after the LLVM 12 release.

@danielnachun note that I was using the patch here: Homebrew/formula-patches#398. This is just the remaining hunk from the patch we used for LLVM 12 that isn't upstream yet.

Patching the source just to build with the host GCC is a pain. I think we can just give up on patching and build LLDB with the just-built Clang instead. Or package LLDB as a separate formula (which we can build with GCC or LLVM as appropriate). The latter would allow us to drop Python as a runtime dependency of this formula.

@danielnachun
Copy link
Copy Markdown
Contributor

I agree as it seems that LLVM (or at least LLDB) isn’t being tested with GCC 5 despite officially supporting it. Given how demanding LLVM is in general of the host compiler I think using brewed GCC may be a better choice. The biggest pain for Linux users with using having brewed GCC as a runtime for LLVM is that the GCC bottle is incorrectly made non-relocatable on Linux. However this will be fixed by Homebrew/brew#11899, which I will reopen and prioritize finishing. From a maintainer perspective, the main issue is C++ runtime dependencies of LLVM needing to be built with brewed GCC. I suspect a large chunk of that is through mesa, which requires brewed GCC regardless of LLVM. If the set of remaining runtime dependents of LLVM which are not through Mesa is small, this won’t be too much work to fix them.

@carlocab
Copy link
Copy Markdown
Member Author

carlocab commented Oct 7, 2021

Or, another alternative:

Build LLVM with a slightly older GCC (7?), make it a build-only dependency, and then add a Requirement that checks that the user has GCC runtime libraries that are new enough, which can presumably be satisfied by brew install gcc (or even brew install gcc@7). Would that work?

@carlocab carlocab mentioned this pull request Oct 8, 2021
6 tasks
@chrmoritz chrmoritz mentioned this pull request Oct 8, 2021
@danielnachun
Copy link
Copy Markdown
Contributor

Or, another alternative:

Build LLVM with a slightly older GCC (7?), make it a build-only dependency, and then add a Requirement that checks that the user has GCC runtime libraries that are new enough, which can presumably be satisfied by brew install gcc (or even brew install gcc@7). Would that work?

It's probably true that this would help to reduce the number of users who have to installed brewed gcc as a runtime dependency, but like I said above that will be less painful when gcc is a relocatable bottle which will be done soon, so it would just save some disk space.

However I think the above solution still requires us to update all LLVM C++ dependents so they know to build with the newer GCC, as merely having some version of brewed GCC in the dependency tree does not mean it actually gets used to compile. Note that this is necessary even if the formula is built with clang, because it still needs to prepend the newer libstdc++ the LDFLAGS so that it doesn't try to use the system one which is too old. Using some version of GCC between the host GCC in CI and newest version also creates more work in terms of keeping track of which version should be used. @iMichka, @sjackman and I all had a pretty strong consenus that we wanted to limit the use of older versions of brewed GCC to the few cases where neither the host GCC nor the newest version of GCC would work, and that has been extremely rare.

These issues are making me wonder if in the long run on Linux it will make more sense for us to just start using brewed GCC for C++ on Linux. On macOS using the host toolchain to build C++ binaries isn't too bad because Apple updates it for us regularly. On Linux that's not the case, and it is the source of a lot of our difficulties with C++. The fact that some users who happen to have a new enough host version of libstdc++ would end up having to use an extra 225MB to install another copy of GCC to me is not worth all the trouble of trying to use the host GCC for C++, given the complexities of both the compiler itself and runtime library. Using the host GCC for C code still makes sense, as libc, the C compiler and the C language itself is much more conservative in terms of changes.

These are just my thoughts at the moment and I'm happy to discuss this in more depth (though probably in a different place than this closed PR).

carlocab added a commit to carlocab/homebrew-core that referenced this pull request Oct 13, 2021
This is because of the `fails_with :gcc`, whose behaviour changed with
Homebrew/homebrew-test-bot#675. Note that this didn't come up in Homebrew#85898.
carlocab added a commit to carlocab/homebrew-core that referenced this pull request Oct 15, 2021
This removes a workaround from Homebrew#85898.
@carlocab carlocab mentioned this pull request Oct 15, 2021
6 tasks
@github-actions github-actions bot added the outdated PR was locked due to age label Nov 9, 2021
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 9, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

automerge-skip `brew pr-automerge` will skip this pull request CI-linux-self-hosted Build on Linux self-hosted runner CI-long-timeout [DEPRECATED] Use longer GitHub Actions CI timeout. CI-no-fail-fast Continue CI tests despite failing GitHub Actions matrix builds. outdated PR was locked due to age ready to merge PR can be merged once CI is green

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants