[libcxx-commits] [PATCH] D57107: [libunwind] Support building hermetic static library

Fri Jan 25 01:07:18 PST 2019

mstorsjo added inline comments.

================
Comment at: libunwind/src/CMakeLists.txt:134
+      _LIBUNWIND_DISABLE_VISIBILITY_ANNOTATIONS
+      _LIBCPP_DISABLE_VISIBILITY_ANNOTATIONS
+    FLAGS ${UNWIND_STATIC_OBJECTS_FLAGS})
----------------
phosek wrote:
> ldionne wrote:
> > We need to define this when building libunwind? This circular dependency looks like a smell.
> Unfortunately yes because libunwind uses `operator new` (https://github.com/llvm/llvm-project/blob/master/libunwind/src/libunwind.cpp#L125) which definition comes from libc++ and without this define, we get a conflicting declaration error (the implicit declaration provided by the compiler has hidden visibility due to `-fvisibility-global-new-delete-hidden` but that one that comes from libc++ has default visibility).
I was under the impression that even if libunwind requires C++ headers to be available, it shouldn't actually require linking to any C++ library (or that this was the design/intention at least); I regularly build it in this setup where my libunwind doesn't link against libc++, as that indeed is a nasty circular dependency. (I only have the checked out libcxx available for libunwind to find headers from.)

This seems to be one of few cases where it's actually invoked as a normal `operator new`. The other cases that do exist use placement new for invoking the constructor, but avoiding a call to any Standard C++ library function. Would it be better to just adjust this call as well, e.g. into malloc + placement new, to avoid the libcxx linking dependency altogether?

This code seems to be within `#ifdef UNW_REMOTE`, and I don't see anything within libunwind actually even setting that define, so how are you ending up running into it? (That explains why I haven't noticed.)

Repository:
  rUNW libunwind

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

https://reviews.llvm.org/D57107