This is an archive of the discontinued LLVM Phabricator instance.

[libunwind] Only include cet.h if __CET__ defined
ClosedPublic

Authored by nikic on Feb 14 2022, 1:37 AM.

Details

Reviewers
xiongji90
MaskRay
Group Reviewers
Restricted Project
Commits
rG2d2ef384b2f6: [libunwind] Only include cet.h if __CET__ defined
Summary

We should not assume that the cet.h header exists just because we're on x86 linux. Only include it if __CET__ is defined. This makes the code more similar to what compiler-rt does in https://github.com/llvm/llvm-project/blob/ee423d93ead39e94c2970b3cc7ef6e6faa75d10b/compiler-rt/lib/builtins/assembly.h#L17 (though that one also has a __has_include() check -- I've not found that to be necessary).

Diff Detail

Event Timeline

nikic created this revision.Feb 14 2022, 1:37 AM
nikic requested review of this revision.Feb 14 2022, 1:37 AM
MaskRay accepted this revision.Feb 14 2022, 3:59 PM
MaskRay added a subscriber: MaskRay.

Thanks!

though that one also has a __has_include() check -- I've not found that to be necessary).

It's not necessary because libunwind requires to be built with fresh Clang, which definitely includes cet.h in the resource directory when __CET__ is defined.
compiler-rt may be built with LLVM_ENABLE_PROJECTS with slightly older GCC.

This revision is now accepted and ready to land.Feb 14 2022, 3:59 PM
MaskRay retitled this revision from [compiler-rt] Only include cet.h if __CET__ defined to [libunwind] Only include cet.h if __CET__ defined.Feb 14 2022, 4:00 PM
xiongji90 accepted this revision.Feb 14 2022, 6:01 PM

Thanks!

though that one also has a __has_include() check -- I've not found that to be necessary).

It's not necessary because libunwind requires to be built with fresh Clang, which definitely includes cet.h in the resource directory when __CET__ is defined.
compiler-rt may be built with LLVM_ENABLE_PROJECTS with slightly older GCC.

Uh, really? I couldn't find docs on this, and the llvm-dev discussion on the topic (https://discourse.llvm.org/t/compiler-support-in-libc/57751/1) was only about raising toolchain requirements for libc++. Requiring a recent toolchain for libc++ makes a lot of sense, but not so much for libunwind, which does not have any tight integration with C++ (and is in fact used as a runtime for other languages, which is why I would find it very surprising if building libunwind also requires you to build clang first).

Thanks!

though that one also has a __has_include() check -- I've not found that to be necessary).

It's not necessary because libunwind requires to be built with fresh Clang, which definitely includes cet.h in the resource directory when __CET__ is defined.
compiler-rt may be built with LLVM_ENABLE_PROJECTS with slightly older GCC.

Uh, really? I couldn't find docs on this, and the llvm-dev discussion on the topic (https://discourse.llvm.org/t/compiler-support-in-libc/57751/1) was only about raising toolchain requirements for libc++. Requiring a recent toolchain for libc++ makes a lot of sense, but not so much for libunwind, which does not have any tight integration with C++ (and is in fact used as a runtime for other languages, which is why I would find it very surprising if building libunwind also requires you to build clang first).

https://reviews.llvm.org/D119351 : "Note that in the near future, libcxx, libcxxabi and libunwind will stop supporting being built with LLVM_ENABLE_PROJECTS altogether."

It's not necessary because libunwind requires to be built with fresh Clang, which definitely includes cet.h in the resource directory when __CET__ is defined.
compiler-rt may be built with LLVM_ENABLE_PROJECTS with slightly older GCC.

Uh, really? I couldn't find docs on this, and the llvm-dev discussion on the topic (https://discourse.llvm.org/t/compiler-support-in-libc/57751/1) was only about raising toolchain requirements for libc++. Requiring a recent toolchain for libc++ makes a lot of sense, but not so much for libunwind, which does not have any tight integration with C++ (and is in fact used as a runtime for other languages, which is why I would find it very surprising if building libunwind also requires you to build clang first).

A couple issues are being conflated here. First, libc++ wants to require a fairly new compiler, either one of the last two stable releases of Clang, or the latest GCC. This doesn't extend to other runtime libs, as far as I know.

libunwind, libc++abi and libc++ aren't meant to be built via LLVM_ENABLE_PROJECTS any longer, but that's not about compiler support, but instead about reducing the number of build system combinations that have to be supported.

You can still build libunwind, libc++abi and libc++ with any compiler of your choice; point CMake at llvm-project/runtimes and pass LLVM_ENABLE_RUNTIMES=libunwind, this will build only libunwind, with whatever compiler you configure CMake to use. You can also pass LLVM_ENABLE_RUNTIMES when pointing CMake at llvm-project/llvm, which then would build those runtimes with the newly-built Clang .

nikic added a comment.Feb 15 2022, 1:39 AM

Thanks!

though that one also has a __has_include() check -- I've not found that to be necessary).

It's not necessary because libunwind requires to be built with fresh Clang, which definitely includes cet.h in the resource directory when __CET__ is defined.
compiler-rt may be built with LLVM_ENABLE_PROJECTS with slightly older GCC.

Uh, really? I couldn't find docs on this, and the llvm-dev discussion on the topic (https://discourse.llvm.org/t/compiler-support-in-libc/57751/1) was only about raising toolchain requirements for libc++. Requiring a recent toolchain for libc++ makes a lot of sense, but not so much for libunwind, which does not have any tight integration with C++ (and is in fact used as a runtime for other languages, which is why I would find it very surprising if building libunwind also requires you to build clang first).

https://reviews.llvm.org/D119351 : "Note that in the near future, libcxx, libcxxabi and libunwind will stop supporting being built with LLVM_ENABLE_PROJECTS altogether."

How is that related? Even without LLVM_ENABLE_PROJECTS, runtimes still support two build modes, one rooted at llvm/ (the bootstrap build) and one rooted at runtimes/ (the "default" build). The latter does not use a bootstrapped clang.

This revision was automatically updated to reflect the committed changes.
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptFeb 16 2022, 12:47 AM

Do we need this in the release/14.x branch?

Do we need this in the release/14.x branch?

Probably, so other people don't need to patch this as well. I opened https://github.com/llvm/llvm-project/issues/54028.