[libcxx-commits] [PATCH] D119538: [libcxxabi] [test] Depend on unwind only if available

[Louis Dionne via Phabricator via libcxx-commits]

ldionne added inline comments.
Herald added a project: All.


================
Comment at: libcxxabi/test/CMakeLists.txt:66
   list(APPEND LIBCXXABI_TEST_DEPS cxx)
-  if (LIBCXXABI_USE_LLVM_UNWINDER)
+  if (LIBCXXABI_USE_LLVM_UNWINDER AND TARGET unwind)
     list(APPEND LIBCXXABI_TEST_DEPS unwind)
----------------
emaste wrote:
> ldionne wrote:
> > emaste wrote:
> > > mgorny wrote:
> > > > ldionne wrote:
> > > > > emaste wrote:
> > > > > > mgorny wrote:
> > > > > > > ldionne wrote:
> > > > > > > > mgorny wrote:
> > > > > > > > > ldionne wrote:
> > > > > > > > > > Do I understand correctly that you're passing `LIBCXXABI_USE_LLVM_UNWINDER` but not including `libunwind` in `LLVM_ENABLE_RUNTIMES`? Is there a reason for doing that?
> > > > > > > > > Yes, I'm linking to system-installed libunwind.
> > > > > > > > Why are you specifying `LIBCXXABI_USE_LLVM_UNWINDER` then? That's the part I don't understand:
> > > > > > > > 
> > > > > > > > ```
> > > > > > > > option(LIBCXXABI_USE_LLVM_UNWINDER "Build and use the LLVM unwinder.")
> > > > > > > > ```
> > > > > > > > 
> > > > > > > > You specifically don't want to do that, right?
> > > > > > > > 
> > > > > > > Well, the option enables some conditional code that I presume should be enabled when linking to LLVM libunwind, doesn't it?
> > > > > > It enables cases like
> > > > > > ```
> > > > > > #if !defined(LIBCXXABI_USE_LLVM_UNWINDER)
> > > > > >     // Copy the address of _Unwind_Control_Block to r12 so that
> > > > > >     // _Unwind_GetLanguageSpecificData() and _Unwind_GetRegionStart() can
> > > > > >     // return correct address.
> > > > > >     _Unwind_SetGR(context, REG_UCB, reinterpret_cast<uint32_t>(unwind_exception));
> > > > > > #endif
> > > > > > ```
> > > > > > 
> > > > > > In FreeBSD we have the same case @mgorny mentions, there is a system-provided built-in llvm libunwind. I haven't looked into the implication of **not** defining `LIBCXXABI_USE_LLVM_UNWINDER` but using llvm libunwind.
> > > > > I see, thanks for the explanation. So IIUC this means you're also building against a LLVM-provided `unwind.h` header, right? In the longer term, what would you think if `libunwind` provided a `_LIBUNWIND_VERSION` macro in its header that you can query for (like we do for libc++ and libc++abi)?
> > > > Our systems do have `unwind.h` from llvm-libunwind or "nongnu" libunwind but at this point I'm not really sure whether it is picked up by the compiler vs the one supplied along with the compiler.
> > > > 
> > > > I don't really mind some extra macro but I don't really see why that would be a question for me. I'm merely packaging it, not writing software against libunwind.
> > > It was a bit of a mess in FreeBSD; we do now install LLVM's unwind.h as of https://reviews.freebsd.org/D34065. Prior to this change we had a copy of HP libunwind unwind.h in the tree but it wasn't installed.
> > > 
> > > Also agree with @mgorny, I think a `_LIBUNWIND_VERSION` is reasonable but it doesn't really matter to me per se.
> > Sorry, I didn't explain properly. My point is that if you install the correct `unwind.h` header and it has a `_LIBUNWIND_VERSION` macro defined in it, this code in libc++abi could simply query `if defined(_LIBUNWIND_VERSION)` instead of having to be told that it's using the LLVM libunwind by means of setting `LIBCXXABI_USE_LLVM_UNWINDER`. That would solve the problem that this patch tries to solve in a cleaner way IMO.
> Ah, yes.
See D121015


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119538