[libcxx-commits] [PATCH] D135663: [libunwind] Install the headers by default

[Sergei Barannikov via Phabricator via libcxx-commits]

barannikov88 added a subscriber: aaron.ballman.
barannikov88 added a comment.

In D135663#4332468 <https://reviews.llvm.org/D135663#4332468>, @ldionne wrote:

> In D135663#4187545 <https://reviews.llvm.org/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?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D135663/new/

https://reviews.llvm.org/D135663