[libunwind] Install the headers by default

[ldionne]

Summary

Just like we install libc++ and libc++abi headers by default when we
install the library, it makes sense to install the libunwind headers
by default when we build libunwind. In the current state of things,
there is an increased risk that folks are using older (previously
installed) libunwind headers along with a recent libunwind dylib,
which is not ideal.

[ldionne]

The runtimes CI passed.

[barannikov88]

clang's unwind.h also gets installed and is found first when included from a source file (and it includes_next the libunwind's version, but only for apple platforms).
Not sure I understand the logic, may it be somehow related to the fact that clang driver supports different unwind libraries?
Either way, installing libunwind's header by default does not update clang's version, so the issue persists.

[ldionne]

In D135663#4187545, @barannikov88 wrote:

clang's unwind.h also gets installed and is found first when included from a source file (and it includes_next the libunwind's version, but only for apple platforms).
Not sure I understand the logic, may it be somehow related to the fact that clang driver supports different unwind libraries?
Either way, installing libunwind's header by default does not update clang's version, so the issue persists.

I think https://github.com/llvm/llvm-project/commit/032d422 is relevant here. It seems to be intentional that Clang doesn't use unwind.h outside of Apple.

[barannikov88]

In D135663#4332468, @ldionne wrote:

In D135663#4187545, @barannikov88 wrote:

clang's unwind.h also gets installed and is found first when included from a source file (and it includes_next the libunwind's version, but only for apple platforms).
Not sure I understand the logic, may it be somehow related to the fact that clang driver supports different unwind libraries?
Either way, installing libunwind's header by default does not update clang's version, so the issue persists.

I think https://github.com/llvm/llvm-project/commit/032d422 is relevant here. It seems to be intentional that Clang doesn't use unwind.h outside of Apple.

If I read it correctly, it says that clang's unwind.h supersedes the system unwind.h,
and that this has been proven for all targets except Apple (and that's why it includes_next
the system header on Apple platforms).
With the current state of things, the installed unwind.h (from libunwind) can only affect Apple targets.
Other targets will use clang's unwind.h, which is not synced with libunwind's unwind.h, potentially causing bugs.

The commit also says that there are no incompatibilities. May be it was true at that time,
but the headers currently diverge. Even _Unwind_Exception definitions are different:

clang's:

typedef uintptr_t _Unwind_Word __attribute__((__mode__(__unwind_word__)));
...
struct _Unwind_Exception {
  _Unwind_Exception_Class exception_class;
  _Unwind_Exception_Cleanup_Fn exception_cleanup;
#if !defined (__USING_SJLJ_EXCEPTIONS__) && defined (__SEH__)
  _Unwind_Word private_[6];
#else
  _Unwind_Word private_1;
  _Unwind_Word private_2;
#endif
  /* The Itanium ABI requires that _Unwind_Exception objects are "double-word
   * aligned".  GCC has interpreted this to mean "use the maximum useful
   * alignment for the target"; so do we. */
} __attribute__((__aligned__));

libunwind's:

struct _Unwind_Exception {
  _Unwind_Exception_Class exception_class;
  void (*exception_cleanup)(_Unwind_Reason_Code reason,
                            _Unwind_Exception *exc);
#if defined(__SEH__) && !defined(__USING_SJLJ_EXCEPTIONS__)
  uintptr_t private_[6];
#else
  uintptr_t private_1; // non-zero means forced unwind
  uintptr_t private_2; // holds sp that phase1 found for phase2 to use
#endif
#if __SIZEOF_POINTER__ == 4
  // The implementation of _Unwind_Exception uses an attribute mode on the
  // above fields which has the side effect of causing this whole struct to
  // round up to 32 bytes in size (48 with SEH). To be more explicit, we add
  // pad fields added for binary compatibility.
  uint32_t reserved[3];
#endif
  // The Itanium ABI requires that _Unwind_Exception objects are "double-word
  // aligned".  GCC has interpreted this to mean "use the maximum useful
  // alignment for the target"; so do we.
} __attribute__((__aligned__));

IMO clang's unwind.h should be as good as deleted. However, @aaron.ballman suggested
on discord that it might be there for use with -ffreestanding. Can that make sense?

[ldionne]

In D135663#4332558, @barannikov88 wrote:

The commit also says that there are no incompatibilities. May be it was true at that time,
but the headers currently diverge. Even _Unwind_Exception definitions are different:

Hmm, yeah, I guess that was unavoidable and it's pretty bad.

IMO clang's unwind.h should be as good as deleted.

FWIW that would also be my immediate reaction -- I'm not sure why it would make much sense for Clang to provide the header associated to another library.

However, @aaron.ballman suggested on discord that it might be there for use with -ffreestanding. Can that make sense?

I am not sure why a freestanding implementation would require <unwind.h>, but if it does, then my reflex would be to say that it should get it from its unwinder library (aka libunwind or other). In my mind, freestanding is not the same as "no runtime libraries". If you are in freestanding and you still want exceptions, you'll need some unwind library to support that no matter what.

So naively, I'd be fine with removing the Clang version, but I wouldn't be surprised if there's a bunch of reasons why this can't work. If you want, you could create a review and we can have a discussion and ping the right people on it.

[barannikov88]

In D135663#4332576, @ldionne wrote:

I am not sure why a freestanding implementation would require <unwind.h>, but if it does, then my reflex would be to say that it should get it from its unwinder library (aka libunwind or other). In my mind, freestanding is not the same as "no runtime libraries". If you are in freestanding and you still want exceptions, you'll need some unwind library to support that no matter what.

I agree.

So naively, I'd be fine with removing the Clang version, but I wouldn't be surprised if there's a bunch of reasons why this can't work. If you want, you could create a review and we can have a discussion and ping the right people on it.

I'll try not to forget to create a review when I have more time.

[aaron.ballman]

In D135663#4332576, @ldionne wrote:

In D135663#4332558, @barannikov88 wrote:

The commit also says that there are no incompatibilities. May be it was true at that time,
but the headers currently diverge. Even _Unwind_Exception definitions are different:

Hmm, yeah, I guess that was unavoidable and it's pretty bad.

IMO clang's unwind.h should be as good as deleted.

FWIW that would also be my immediate reaction -- I'm not sure why it would make much sense for Clang to provide the header associated to another library.

However, @aaron.ballman suggested on discord that it might be there for use with -ffreestanding. Can that make sense?

I am not sure why a freestanding implementation would require <unwind.h>, but if it does, then my reflex would be to say that it should get it from its unwinder library (aka libunwind or other). In my mind, freestanding is not the same as "no runtime libraries". If you are in freestanding and you still want exceptions, you'll need some unwind library to support that no matter what.

So naively, I'd be fine with removing the Clang version, but I wouldn't be surprised if there's a bunch of reasons why this can't work. If you want, you could create a review and we can have a discussion and ping the right people on it.

Ah, sorry for introducing confusion. IIRC, I was speaking more generally about why we provide headers and then include_next to the system from them (we do that sometimes so we can provide macros in freestanding but then defer to the system for hosted). I don't know if we have any freestanding needs for <unwind.h>. When we discussed freestanding recently, I think we were heading towards consensus that we expect users to provide a freestanding-compatible libc implementation like we already expect them to do for hosted; the compiler headers should only be producing simple macros or calls to intrinsics. So that matches what @ldionne says about freestanding not meaning "no runtime library".

libunwind/CMakeLists.txt

 option(LIBUNWIND_ENABLE_THREADS "Build libunwind with threading support." ON)
 option(LIBUNWIND_WEAK_PTHREAD_LIB "Use weak references to refer to pthread functions." OFF)
 option(LIBUNWIND_USE_COMPILER_RT "Use compiler-rt instead of libgcc" OFF)
 option(LIBUNWIND_INCLUDE_DOCS "Build the libunwind documentation." ${LLVM_INCLUDE_DOCS})
 option(LIBUNWIND_INCLUDE_TESTS "Build the libunwind tests." ${LLVM_INCLUDE_TESTS})
 option(LIBUNWIND_IS_BAREMETAL "Build libunwind for baremetal targets." OFF)
 option(LIBUNWIND_USE_FRAME_HEADER_CACHE "Cache frame headers for unwinding. Requires locking dl_iterate_phdr." OFF)
 option(LIBUNWIND_REMEMBER_HEAP_ALLOC "Use heap instead of the stack for .cfi_remember_state." OFF)
-option(LIBUNWIND_INSTALL_HEADERS "Install the libunwind headers." OFF)
+option(LIBUNWIND_INSTALL_HEADERS "Install the libunwind headers." ON)
 
 set(LIBUNWIND_LIBDIR_SUFFIX "${LLVM_LIBDIR_SUFFIX}" CACHE STRING
     "Define suffix of library directory name (32/64)")
 option(LIBUNWIND_INSTALL_LIBRARY "Install the libunwind library." ON)
 cmake_dependent_option(LIBUNWIND_INSTALL_STATIC_LIBRARY
   "Install the static libunwind library." ON
   "LIBUNWIND_ENABLE_STATIC;LIBUNWIND_INSTALL_LIBRARY" OFF)
 cmake_dependent_option(LIBUNWIND_INSTALL_SHARED_LIBRARY