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

Martin Storsjö via Phabricator via libcxx-commits libcxx-commits at lists.llvm.org
Fri Jan 25 09:59:51 PST 2019


mstorsjo added subscribers: EricWF, mclow.lists.
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:
> mstorsjo wrote:
> > mstorsjo wrote:
> > > 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.)
> > That particular function you linked, unw_create_addr_space_for_task, seems to even lack a return statement, so I'm rather sure that this function isn't in use. IIRC the remote unwinding part of libunwind isn't fully implemented, not built by default, and probably bitrotting.
> I looked into the remote unwinding recently and the current implementation is unfinished and doesn't work (and I don't think it ever worked).  
> 
> I'd be fine changing this to placement `new` to eliminate this dependency, but maybe it'd be better to remove this code altogether? Do you have any preference?
I don't mind removing the unused and incomplete remote unwinding code, but that's up to the code owners, @mclow.lists, @EricWF and @ldionne.


Repository:
  rUNW libunwind

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

https://reviews.llvm.org/D57107





More information about the libcxx-commits mailing list