[libcxx-commits] [PATCH] D93003: [libunwind] LIBUNWIND_HERMETIC_STATIC_LIBRARY fixes

[Ryan Prichard via Phabricator via libcxx-commits]

rprichard added a comment.

I assume the `LIBUNWIND_HERMETIC_STATIC_LIBRARY` setting isn't used on Darwin -- it leaves unw_* symbols as Extern and not PrivateExtern. Maybe the CMake file could reject that flag on Darwin instead of trying to make it work.

AFAICT, the existing unw_* aliases aren't weak on macOS. For aliases to C++ functions, the `__attribute__((weak_import))` didn't seem to do anything when I tested it. I think the declared unw_* functions aren't referenced in the TU that declares them, so the compiler doesn't do anything with the declaration. When I experimented, if I *did* reference the function, then the compiler outputted a .weak_reference directive. However, AFAIK, the assembler ignores the .weak_reference directive and uses the weakdef/weakref flags from the target of the alias instead (i.e. the __unw_* function).

For unw_getcontext specifically, assembly.h's `WEAK_SYMBOL(aliasname) SEPARATOR` omitted the SYMBOL_NAME macro on Darwin, so I think the .weak_reference would have referred to the wrong symbol. For arm64 iphoneos, I saw this preprocessed output:

  .globl _unw_getcontext %% .weak_reference unw_getcontext %% _unw_getcontext = ___unw_getcontext

Adding SYMBOL_NAME doesn't make unw_getcontext weak.

I'm not sure if the aliases can actually be marked weak on Darwin, so instead, I removed the code making them weak. Windows already leaves the symbols non-weak. I'm not very familiar with Mach-O, though, so maybe I'm wrong here. In particular, maybe there's some configuration where it works (e.g. where the assembler behaves differently).



================
Comment at: libunwind/CMakeLists.txt:333
+  add_definitions(-D_LIBUNWIND_HERMETIC_STATIC_LIBRARY)
 endif()
 
----------------
This code block effectively duplicates the `LIBUNWIND_HERMETIC_STATIC_LIBRARY` block in libunwind/src/CMakeLists.txt, assuming that the -fvisibility flags are ignored for Windows. It's on by default, though, whereas the `LIBUNWIND_HERMETIC_STATIC_LIBRARY` option is off by default. Maybe this code block should be removed, but I suppose that would break current users, quietly.


================
Comment at: libunwind/src/CMakeLists.txt:163
 
   if(LIBUNWIND_HERMETIC_STATIC_LIBRARY)
     append_flags_if_supported(UNWIND_STATIC_LIBRARY_FLAGS -fvisibility=hidden)
----------------
e.g. Maybe this could be: `if(LIBUNWIND_HERMETIC_STATIC_LIBRARY OR WIN32)`

Or maybe we can default LIBUNWIND_HERMETIC_STATIC_LIBRARY to ON for WIN32? I'm not sure what's possible with CMake.


================
Comment at: libunwind/src/CMakeLists.txt:165
     append_flags_if_supported(UNWIND_STATIC_LIBRARY_FLAGS -fvisibility=hidden)
     append_flags_if_supported(UNWIND_STATIC_LIBRARY_FLAGS -fvisibility-global-new-delete-hidden)
     target_compile_options(unwind_static PRIVATE ${UNWIND_STATIC_LIBRARY_FLAGS})
----------------
I think this `-fvisibility-global-new-delete-hidden` flag has no effect, but if it did have an effect, it could break the NDK's libc++_shared.so when it's linked with libunwind.a.

i.e. AFAIK, libunwind doesn't define or use the global new/delete operators. (D57251 switched to placement new.) If libunwind merely referenced new/delete, then this flag would make the undefined symbol reference hidden, and then linking libunwind.a into the NDK's libc++_shared.so could make the new/delete definitions hidden (because undef-hidden + def-default ==> def-hidden).

It was added with D57107.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93003